Re: [PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Tomi Valkeinen]

Hi,

On 14/01/2025 13:24, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> > Move the bridge pre_enable call before crtc enable, and the bridge
> > post_disable call after the crtc disable.
> >
> > The sequence of enable after this patch will look like:
> >
> > 	bridge[n]_pre_enable
> > 	...
> > 	bridge[1]_pre_enable
> >
> > 	crtc_enable
> > 	encoder_enable
> >
> > 	bridge[1]_enable
> > 	...
> > 	bridge[n]_enable
> >
> > And, the disable sequence for the display pipeline will look like:
> >
> > 	bridge[n]_disable
> > 	...
> > 	bridge[1]_disable
> >
> > 	encoder_disable
> > 	crtc_disable
> >
> > 	bridge[1]_post_disable
> > 	...
> > 	bridge[n]_post_disable
> >
> > The definition of bridge pre_enable hook says that,
> > "The display pipe (i.e. clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called".
> >
> > Since CRTC is also a source feeding the bridge, it should not be enabled
> > before the bridges in the pipeline are pre_enabled. Fix that by
> > re-ordering the sequence of bridge pre_enable and bridge post_disable.
>
> The patch contains both refactoring of the corresponding functions and
> changing of the order. Please split it into two separate commits.
>
> > Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
> > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
> >   1 file changed, 181 insertions(+), 119 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 5186d2114a50..ad6290a4a528 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -74,6 +74,12 @@
> >    * also shares the &struct drm_plane_helper_funcs function table with the plane
> >    * helpers.
> >    */
> > +
> > +enum bridge_chain_operation_type {
> > +	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
> > +	DRM_BRIDGE_ENABLE_OR_DISABLE,
> > +};
> > +
>
> I have mixed feelings towards this approach. I doubt that it actually
> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
> post_disable' arguments?

If my memory serves, I suggested the enum. I don't like it too much 
either. But neither do I like the boolean that much, as this is not a 
yes/no, on/off case... Then again, maybe boolean is fine here, as the 
"off" case is the "normal/default" case so it's still ok-ish.

But this doesn't matter much, I think. It's internal, and can be 
trivially adjusted later.

 Tomi