| Message ID | 20240511153051.1355825-7-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
On Sat, May 11, 2024 at 09:00:50PM +0530, Aradhya Bhatia wrote: > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 4baca0d9107b..40f93230abb2 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -206,6 +206,20 @@ struct drm_bridge_funcs { > */ > void (*post_disable)(struct drm_bridge *bridge); > > + /** > + * @late_disable: > + * > + * This callback should disable the bridge. It is called right after the > + * preceding element in the display pipe is disabled. If the preceding > + * element is a bridge this means it's called after that bridge's > + * @atomic_post_disable. If the preceding element is a &drm_crtc it's > + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable > + * hook. > + * > + * The @late_disable callback is optional. > + */ > + void (*late_disable)(struct drm_bridge *bridge); > + > /** > * @mode_set: > * > @@ -235,6 +249,26 @@ struct drm_bridge_funcs { > void (*mode_set)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > const struct drm_display_mode *adjusted_mode); > + > + /** > + * @early_enable: > + * > + * This callback should enable the bridge. It is called right before > + * the preceding element in the display pipe is enabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's @early_enable. If the preceding element is a &drm_crtc it's > + * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable > + * hook. > + * > + * The display pipe (i.e. clocks and timing signals) feeding this bridge > + * will not yet be running when this callback is called. The bridge can > + * enable the display link feeding the next bridge in the chain (if > + * there is one) when this callback is called. > + * > + * The @early_enable callback is optional. > + */ > + void (*early_enable)(struct drm_bridge *bridge); > + You don't need the legacy option here, just go straight for the atomic one. > /** > * @pre_enable: > * > @@ -285,6 +319,26 @@ struct drm_bridge_funcs { > */ > void (*enable)(struct drm_bridge *bridge); > > + /** > + * @atomic_early_enable: > + * > + * This callback should enable the bridge. It is called right before > + * the preceding element in the display pipe is enabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's @atomic_early_enable. If the preceding element is a > + * &drm_crtc it's called right before the crtc's > + * &drm_crtc_helper_funcs.atomic_enable hook. > + * > + * The display pipe (i.e. clocks and timing signals) feeding this bridge > + * will not yet be running when this callback is called. The bridge can > + * enable the display link feeding the next bridge in the chain (if > + * there is one) when this callback is called. > + * > + * The @early_enable callback is optional. > + */ > + void (*atomic_early_enable)(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state); > + > /** > * @atomic_pre_enable: > * > @@ -361,6 +415,21 @@ struct drm_bridge_funcs { > void (*atomic_post_disable)(struct drm_bridge *bridge, > struct drm_bridge_state *old_bridge_state); > > + /** > + * @atomic_late_disable: > + * > + * This callback should disable the bridge. It is called right after the > + * preceding element in the display pipe is disabled. If the preceding > + * element is a bridge this means it's called after that bridge's > + * @atomic_late_disable. If the preceding element is a &drm_crtc it's > + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable > + * hook. > + * > + * The @atomic_late_disable callback is optional. > + */ > + void (*atomic_late_disable)(struct drm_bridge *bridge, > + struct drm_bridge_state *old_bridge_state); > + But more importantly, I don't quite get the use case you're trying to solve here. If I got the rest of your series, the Cadence DSI bridge needs to be powered up before its source is started. You can't use atomic_enable or atomic_pre_enable because it would start the source before the DSI bridge. Is that correct? If it is, then how is it different from what drm_atomic_bridge_chain_pre_enable is doing? The assumption there is that it starts enabling bridges last to first, to it should be enabled before anything starts. The whole bridge enabling order code starts to be a bit of a mess, so it would be great if you could list all the order variations we have currently, and why none work for cdns-dsi. Maxime
Hi Maxime, Thank you for reviewing the patches! On 16/05/24 13:52, Maxime Ripard wrote: > On Sat, May 11, 2024 at 09:00:50PM +0530, Aradhya Bhatia wrote: >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index 4baca0d9107b..40f93230abb2 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -206,6 +206,20 @@ struct drm_bridge_funcs { >> */ >> void (*post_disable)(struct drm_bridge *bridge); >> >> + /** >> + * @late_disable: >> + * >> + * This callback should disable the bridge. It is called right after the >> + * preceding element in the display pipe is disabled. If the preceding >> + * element is a bridge this means it's called after that bridge's >> + * @atomic_post_disable. If the preceding element is a &drm_crtc it's >> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable >> + * hook. >> + * >> + * The @late_disable callback is optional. >> + */ >> + void (*late_disable)(struct drm_bridge *bridge); >> + >> /** >> * @mode_set: >> * >> @@ -235,6 +249,26 @@ struct drm_bridge_funcs { >> void (*mode_set)(struct drm_bridge *bridge, >> const struct drm_display_mode *mode, >> const struct drm_display_mode *adjusted_mode); >> + >> + /** >> + * @early_enable: >> + * >> + * This callback should enable the bridge. It is called right before >> + * the preceding element in the display pipe is enabled. If the >> + * preceding element is a bridge this means it's called before that >> + * bridge's @early_enable. If the preceding element is a &drm_crtc it's >> + * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable >> + * hook. >> + * >> + * The display pipe (i.e. clocks and timing signals) feeding this bridge >> + * will not yet be running when this callback is called. The bridge can >> + * enable the display link feeding the next bridge in the chain (if >> + * there is one) when this callback is called. >> + * >> + * The @early_enable callback is optional. >> + */ >> + void (*early_enable)(struct drm_bridge *bridge); >> + > > You don't need the legacy option here, just go straight for the atomic one. Got it! I can remove these in v2. > >> /** >> * @pre_enable: >> * >> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { >> */ >> void (*enable)(struct drm_bridge *bridge); >> >> + /** >> + * @atomic_early_enable: >> + * >> + * This callback should enable the bridge. It is called right before >> + * the preceding element in the display pipe is enabled. If the >> + * preceding element is a bridge this means it's called before that >> + * bridge's @atomic_early_enable. If the preceding element is a >> + * &drm_crtc it's called right before the crtc's >> + * &drm_crtc_helper_funcs.atomic_enable hook. >> + * >> + * The display pipe (i.e. clocks and timing signals) feeding this bridge >> + * will not yet be running when this callback is called. The bridge can >> + * enable the display link feeding the next bridge in the chain (if >> + * there is one) when this callback is called. >> + * >> + * The @early_enable callback is optional. >> + */ >> + void (*atomic_early_enable)(struct drm_bridge *bridge, >> + struct drm_bridge_state *old_bridge_state); >> + >> /** >> * @atomic_pre_enable: >> * >> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { >> void (*atomic_post_disable)(struct drm_bridge *bridge, >> struct drm_bridge_state *old_bridge_state); >> >> + /** >> + * @atomic_late_disable: >> + * >> + * This callback should disable the bridge. It is called right after the >> + * preceding element in the display pipe is disabled. If the preceding >> + * element is a bridge this means it's called after that bridge's >> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's >> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable >> + * hook. >> + * >> + * The @atomic_late_disable callback is optional. >> + */ >> + void (*atomic_late_disable)(struct drm_bridge *bridge, >> + struct drm_bridge_state *old_bridge_state); >> + > > But more importantly, I don't quite get the use case you're trying to > solve here. > > If I got the rest of your series, the Cadence DSI bridge needs to be > powered up before its source is started. You can't use atomic_enable or > atomic_pre_enable because it would start the source before the DSI > bridge. Is that correct? > That's right. I cannot use bridge_atomic_pre_enable / bridge_atomic_enable here. But that's because my source is CRTC, which gets enabled via crtc_atomic_enable. > If it is, then how is it different from what > drm_atomic_bridge_chain_pre_enable is doing? The assumption there is > that it starts enabling bridges last to first, to it should be enabled > before anything starts. > > The whole bridge enabling order code starts to be a bit of a mess, so it > would be great if you could list all the order variations we have > currently, and why none work for cdns-dsi. > Of course! I can elaborate on the order. Without my patches (and given there isn't any bridge setting the "pre_enable_prev_first" flag) the order of enable for any single display chain, looks like this - crtc_enable bridge[n]_pre_enable --- bridge[1]_pre_enable encoder_enable bridge[1]_enable --- bridge[n]_enable The tidss enables at the crtc_enable level, and hence is the first entity with stream on. cdns-dsi doesn't stand a chance with bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no bridge call happening before crtc currently. That's where the early_enable APIs come into picture. They are being called before the crtc is even enabled, helping cdns-dsi to be enabled before tidss. The order then looks like - bridge[1]_early_enable --- bridge[n]_early_enable crtc_enable --- (the order is same from here on) Regards Aradhya
Hi, On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote: > >> /** > >> * @pre_enable: > >> * > >> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { > >> */ > >> void (*enable)(struct drm_bridge *bridge); > >> > >> + /** > >> + * @atomic_early_enable: > >> + * > >> + * This callback should enable the bridge. It is called right before > >> + * the preceding element in the display pipe is enabled. If the > >> + * preceding element is a bridge this means it's called before that > >> + * bridge's @atomic_early_enable. If the preceding element is a > >> + * &drm_crtc it's called right before the crtc's > >> + * &drm_crtc_helper_funcs.atomic_enable hook. > >> + * > >> + * The display pipe (i.e. clocks and timing signals) feeding this bridge > >> + * will not yet be running when this callback is called. The bridge can > >> + * enable the display link feeding the next bridge in the chain (if > >> + * there is one) when this callback is called. > >> + * > >> + * The @early_enable callback is optional. > >> + */ > >> + void (*atomic_early_enable)(struct drm_bridge *bridge, > >> + struct drm_bridge_state *old_bridge_state); > >> + > >> /** > >> * @atomic_pre_enable: > >> * > >> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { > >> void (*atomic_post_disable)(struct drm_bridge *bridge, > >> struct drm_bridge_state *old_bridge_state); > >> > >> + /** > >> + * @atomic_late_disable: > >> + * > >> + * This callback should disable the bridge. It is called right after the > >> + * preceding element in the display pipe is disabled. If the preceding > >> + * element is a bridge this means it's called after that bridge's > >> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's > >> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable > >> + * hook. > >> + * > >> + * The @atomic_late_disable callback is optional. > >> + */ > >> + void (*atomic_late_disable)(struct drm_bridge *bridge, > >> + struct drm_bridge_state *old_bridge_state); > >> + > > > > But more importantly, I don't quite get the use case you're trying to > > solve here. > > > > If I got the rest of your series, the Cadence DSI bridge needs to be > > powered up before its source is started. You can't use atomic_enable or > > atomic_pre_enable because it would start the source before the DSI > > bridge. Is that correct? > > > > That's right. I cannot use bridge_atomic_pre_enable / > bridge_atomic_enable here. But that's because my source is CRTC, which > gets enabled via crtc_atomic_enable. > > > > If it is, then how is it different from what > > drm_atomic_bridge_chain_pre_enable is doing? The assumption there is > > that it starts enabling bridges last to first, to it should be enabled > > before anything starts. > > > > The whole bridge enabling order code starts to be a bit of a mess, so it > > would be great if you could list all the order variations we have > > currently, and why none work for cdns-dsi. > > > > Of course! I can elaborate on the order. > > Without my patches (and given there isn't any bridge setting the > "pre_enable_prev_first" flag) the order of enable for any single display > chain, looks like this - > > crtc_enable > > bridge[n]_pre_enable > --- > bridge[1]_pre_enable > > encoder_enable > > bridge[1]_enable > --- > bridge[n]_enable > > The tidss enables at the crtc_enable level, and hence is the first > entity with stream on. cdns-dsi doesn't stand a chance with > bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no > bridge call happening before crtc currently. Thanks for filling the blanks :) I assume that since cdns-dsi is a bridge, and it only has a simple encoder implementation, for it to receive some video signal we need to enable the CRTC before the bridge. If so, I think that's the original intent between the bridge pre_enable. The original documentation had: pre_enable: this contains things needed to be done for the bridge before this contains things needed to be done for the bridge before this contains things needed to be done for the bridge before. and the current one has: The display pipe (i.e. clocks and timing signals) feeding this bridge will not yet be running when this callback is called. The bridge must not enable the display link feeding the next bridge in the chain (if there is one) when this callback is called. I would say the CRTC is such a source, even more so now that the encoder is usually transparent, so I think we should instead move the crtc enable call after the bridge pre_enable. Would that work? Maxime
Hi Maxime, On 21/05/24 18:45, Maxime Ripard wrote: > Hi, > > On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote: >>>> /** >>>> * @pre_enable: >>>> * >>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { >>>> */ >>>> void (*enable)(struct drm_bridge *bridge); >>>> >>>> + /** >>>> + * @atomic_early_enable: >>>> + * >>>> + * This callback should enable the bridge. It is called right before >>>> + * the preceding element in the display pipe is enabled. If the >>>> + * preceding element is a bridge this means it's called before that >>>> + * bridge's @atomic_early_enable. If the preceding element is a >>>> + * &drm_crtc it's called right before the crtc's >>>> + * &drm_crtc_helper_funcs.atomic_enable hook. >>>> + * >>>> + * The display pipe (i.e. clocks and timing signals) feeding this bridge >>>> + * will not yet be running when this callback is called. The bridge can >>>> + * enable the display link feeding the next bridge in the chain (if >>>> + * there is one) when this callback is called. >>>> + * >>>> + * The @early_enable callback is optional. >>>> + */ >>>> + void (*atomic_early_enable)(struct drm_bridge *bridge, >>>> + struct drm_bridge_state *old_bridge_state); >>>> + >>>> /** >>>> * @atomic_pre_enable: >>>> * >>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { >>>> void (*atomic_post_disable)(struct drm_bridge *bridge, >>>> struct drm_bridge_state *old_bridge_state); >>>> >>>> + /** >>>> + * @atomic_late_disable: >>>> + * >>>> + * This callback should disable the bridge. It is called right after the >>>> + * preceding element in the display pipe is disabled. If the preceding >>>> + * element is a bridge this means it's called after that bridge's >>>> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's >>>> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable >>>> + * hook. >>>> + * >>>> + * The @atomic_late_disable callback is optional. >>>> + */ >>>> + void (*atomic_late_disable)(struct drm_bridge *bridge, >>>> + struct drm_bridge_state *old_bridge_state); >>>> + >>> >>> But more importantly, I don't quite get the use case you're trying to >>> solve here. >>> >>> If I got the rest of your series, the Cadence DSI bridge needs to be >>> powered up before its source is started. You can't use atomic_enable or >>> atomic_pre_enable because it would start the source before the DSI >>> bridge. Is that correct? >>> >> >> That's right. I cannot use bridge_atomic_pre_enable / >> bridge_atomic_enable here. But that's because my source is CRTC, which >> gets enabled via crtc_atomic_enable. >> >> >>> If it is, then how is it different from what >>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is >>> that it starts enabling bridges last to first, to it should be enabled >>> before anything starts. >>> >>> The whole bridge enabling order code starts to be a bit of a mess, so it >>> would be great if you could list all the order variations we have >>> currently, and why none work for cdns-dsi. >>> >> >> Of course! I can elaborate on the order. >> >> Without my patches (and given there isn't any bridge setting the >> "pre_enable_prev_first" flag) the order of enable for any single display >> chain, looks like this - >> >> crtc_enable >> >> bridge[n]_pre_enable >> --- >> bridge[1]_pre_enable >> >> encoder_enable >> >> bridge[1]_enable >> --- >> bridge[n]_enable >> >> The tidss enables at the crtc_enable level, and hence is the first >> entity with stream on. cdns-dsi doesn't stand a chance with >> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no >> bridge call happening before crtc currently. > > Thanks for filling the blanks :) > > I assume that since cdns-dsi is a bridge, and it only has a simple > encoder implementation, for it to receive some video signal we need to > enable the CRTC before the bridge. > > If so, I think that's the original intent between the bridge pre_enable. > The original documentation had: > > pre_enable: this contains things needed to be done for the bridge > before this contains things needed to be done for the bridge before > this contains things needed to be done for the bridge before. > > and the current one has: > > The display pipe (i.e. clocks and timing signals) feeding this bridge > will not yet be running when this callback is called. The bridge must > not enable the display link feeding the next bridge in the chain (if > there is one) when this callback is called. > > I would say the CRTC is such a source, even more so now that the encoder > is usually transparent, so I think we should instead move the crtc > enable call after the bridge pre_enable. Hmm, if I understand you right, the newer sequence of calls will look like this, bridge[n]_pre_enable --- bridge[1]_pre_enable crtc_enable encoder_enable bridge[1]_enable --- bridge[n]_enable I do agree with this. This makes sense. CRTC is indeed such a source, and should ideally be enabled after the bridges are pre_enabled. > > Would that work? > So, this could potentially work, yes. The cdns-dsi would get pre_enabled after all bridges after cdns-dsi are pre_enabled. But over a quick test with BBAI64 + RPi Panel, I don't see any issue. However, the one concern that I have right now, is about breaking any existing (albeit faulty) implementation which relies on CRTC being enabled before the bridges are pre_enabled. =) Regards Aradhya
On Fri, May 24, 2024 at 04:38:13PM GMT, Aradhya Bhatia wrote: > Hi Maxime, > > On 21/05/24 18:45, Maxime Ripard wrote: > > Hi, > > > > On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote: > >>>> /** > >>>> * @pre_enable: > >>>> * > >>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { > >>>> */ > >>>> void (*enable)(struct drm_bridge *bridge); > >>>> > >>>> + /** > >>>> + * @atomic_early_enable: > >>>> + * > >>>> + * This callback should enable the bridge. It is called right before > >>>> + * the preceding element in the display pipe is enabled. If the > >>>> + * preceding element is a bridge this means it's called before that > >>>> + * bridge's @atomic_early_enable. If the preceding element is a > >>>> + * &drm_crtc it's called right before the crtc's > >>>> + * &drm_crtc_helper_funcs.atomic_enable hook. > >>>> + * > >>>> + * The display pipe (i.e. clocks and timing signals) feeding this bridge > >>>> + * will not yet be running when this callback is called. The bridge can > >>>> + * enable the display link feeding the next bridge in the chain (if > >>>> + * there is one) when this callback is called. > >>>> + * > >>>> + * The @early_enable callback is optional. > >>>> + */ > >>>> + void (*atomic_early_enable)(struct drm_bridge *bridge, > >>>> + struct drm_bridge_state *old_bridge_state); > >>>> + > >>>> /** > >>>> * @atomic_pre_enable: > >>>> * > >>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { > >>>> void (*atomic_post_disable)(struct drm_bridge *bridge, > >>>> struct drm_bridge_state *old_bridge_state); > >>>> > >>>> + /** > >>>> + * @atomic_late_disable: > >>>> + * > >>>> + * This callback should disable the bridge. It is called right after the > >>>> + * preceding element in the display pipe is disabled. If the preceding > >>>> + * element is a bridge this means it's called after that bridge's > >>>> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's > >>>> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable > >>>> + * hook. > >>>> + * > >>>> + * The @atomic_late_disable callback is optional. > >>>> + */ > >>>> + void (*atomic_late_disable)(struct drm_bridge *bridge, > >>>> + struct drm_bridge_state *old_bridge_state); > >>>> + > >>> > >>> But more importantly, I don't quite get the use case you're trying to > >>> solve here. > >>> > >>> If I got the rest of your series, the Cadence DSI bridge needs to be > >>> powered up before its source is started. You can't use atomic_enable or > >>> atomic_pre_enable because it would start the source before the DSI > >>> bridge. Is that correct? > >>> > >> > >> That's right. I cannot use bridge_atomic_pre_enable / > >> bridge_atomic_enable here. But that's because my source is CRTC, which > >> gets enabled via crtc_atomic_enable. > >> > >> > >>> If it is, then how is it different from what > >>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is > >>> that it starts enabling bridges last to first, to it should be enabled > >>> before anything starts. > >>> > >>> The whole bridge enabling order code starts to be a bit of a mess, so it > >>> would be great if you could list all the order variations we have > >>> currently, and why none work for cdns-dsi. > >>> > >> > >> Of course! I can elaborate on the order. > >> > >> Without my patches (and given there isn't any bridge setting the > >> "pre_enable_prev_first" flag) the order of enable for any single display > >> chain, looks like this - > >> > >> crtc_enable > >> > >> bridge[n]_pre_enable > >> --- > >> bridge[1]_pre_enable > >> > >> encoder_enable > >> > >> bridge[1]_enable > >> --- > >> bridge[n]_enable > >> > >> The tidss enables at the crtc_enable level, and hence is the first > >> entity with stream on. cdns-dsi doesn't stand a chance with > >> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no > >> bridge call happening before crtc currently. > > > > Thanks for filling the blanks :) > > > > I assume that since cdns-dsi is a bridge, and it only has a simple > > encoder implementation, for it to receive some video signal we need to > > enable the CRTC before the bridge. > > > > If so, I think that's the original intent between the bridge pre_enable. > > The original documentation had: > > > > pre_enable: this contains things needed to be done for the bridge > > before this contains things needed to be done for the bridge before > > this contains things needed to be done for the bridge before. > > > > and the current one has: > > > > The display pipe (i.e. clocks and timing signals) feeding this bridge > > will not yet be running when this callback is called. The bridge must > > not enable the display link feeding the next bridge in the chain (if > > there is one) when this callback is called. > > > > I would say the CRTC is such a source, even more so now that the encoder > > is usually transparent, so I think we should instead move the crtc > > enable call after the bridge pre_enable. > > Hmm, if I understand you right, the newer sequence of calls will look > like this, > > bridge[n]_pre_enable > --- > bridge[1]_pre_enable > > crtc_enable > encoder_enable > > bridge[1]_enable > --- > bridge[n]_enable Yes :) > I do agree with this. This makes sense. CRTC is indeed such a source, > and should ideally be enabled after the bridges are pre_enabled. > > > > > Would that work? > > > > So, this could potentially work, yes. The cdns-dsi would get pre_enabled > after all bridges after cdns-dsi are pre_enabled. But over a quick test > with BBAI64 + RPi Panel, I don't see any issue. > > However, the one concern that I have right now, is about breaking any > existing (albeit faulty) implementation which relies on CRTC being > enabled before the bridges are pre_enabled. =) I don't think it'll be a big deal. If there was a proper encoder driver, it was probably gating the signal until it's enabled. If there isn't, then yeah it might disrupt things, but it mostly means that the driver wasn't properly split between pre_enable and enable. So I think it's worth trying, and we'll see the outcome. Maxime
Hi Maxime, On 28/05/24 17:13, Maxime Ripard wrote: > On Fri, May 24, 2024 at 04:38:13PM GMT, Aradhya Bhatia wrote: >> Hi Maxime, >> >> On 21/05/24 18:45, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote: >>>>>> /** >>>>>> * @pre_enable: >>>>>> * >>>>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs { >>>>>> */ >>>>>> void (*enable)(struct drm_bridge *bridge); >>>>>> >>>>>> + /** >>>>>> + * @atomic_early_enable: >>>>>> + * >>>>>> + * This callback should enable the bridge. It is called right before >>>>>> + * the preceding element in the display pipe is enabled. If the >>>>>> + * preceding element is a bridge this means it's called before that >>>>>> + * bridge's @atomic_early_enable. If the preceding element is a >>>>>> + * &drm_crtc it's called right before the crtc's >>>>>> + * &drm_crtc_helper_funcs.atomic_enable hook. >>>>>> + * >>>>>> + * The display pipe (i.e. clocks and timing signals) feeding this bridge >>>>>> + * will not yet be running when this callback is called. The bridge can >>>>>> + * enable the display link feeding the next bridge in the chain (if >>>>>> + * there is one) when this callback is called. >>>>>> + * >>>>>> + * The @early_enable callback is optional. >>>>>> + */ >>>>>> + void (*atomic_early_enable)(struct drm_bridge *bridge, >>>>>> + struct drm_bridge_state *old_bridge_state); >>>>>> + >>>>>> /** >>>>>> * @atomic_pre_enable: >>>>>> * >>>>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs { >>>>>> void (*atomic_post_disable)(struct drm_bridge *bridge, >>>>>> struct drm_bridge_state *old_bridge_state); >>>>>> >>>>>> + /** >>>>>> + * @atomic_late_disable: >>>>>> + * >>>>>> + * This callback should disable the bridge. It is called right after the >>>>>> + * preceding element in the display pipe is disabled. If the preceding >>>>>> + * element is a bridge this means it's called after that bridge's >>>>>> + * @atomic_late_disable. If the preceding element is a &drm_crtc it's >>>>>> + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable >>>>>> + * hook. >>>>>> + * >>>>>> + * The @atomic_late_disable callback is optional. >>>>>> + */ >>>>>> + void (*atomic_late_disable)(struct drm_bridge *bridge, >>>>>> + struct drm_bridge_state *old_bridge_state); >>>>>> + >>>>> >>>>> But more importantly, I don't quite get the use case you're trying to >>>>> solve here. >>>>> >>>>> If I got the rest of your series, the Cadence DSI bridge needs to be >>>>> powered up before its source is started. You can't use atomic_enable or >>>>> atomic_pre_enable because it would start the source before the DSI >>>>> bridge. Is that correct? >>>>> >>>> >>>> That's right. I cannot use bridge_atomic_pre_enable / >>>> bridge_atomic_enable here. But that's because my source is CRTC, which >>>> gets enabled via crtc_atomic_enable. >>>> >>>> >>>>> If it is, then how is it different from what >>>>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is >>>>> that it starts enabling bridges last to first, to it should be enabled >>>>> before anything starts. >>>>> >>>>> The whole bridge enabling order code starts to be a bit of a mess, so it >>>>> would be great if you could list all the order variations we have >>>>> currently, and why none work for cdns-dsi. >>>>> >>>> >>>> Of course! I can elaborate on the order. >>>> >>>> Without my patches (and given there isn't any bridge setting the >>>> "pre_enable_prev_first" flag) the order of enable for any single display >>>> chain, looks like this - >>>> >>>> crtc_enable >>>> >>>> bridge[n]_pre_enable >>>> --- >>>> bridge[1]_pre_enable >>>> >>>> encoder_enable >>>> >>>> bridge[1]_enable >>>> --- >>>> bridge[n]_enable >>>> >>>> The tidss enables at the crtc_enable level, and hence is the first >>>> entity with stream on. cdns-dsi doesn't stand a chance with >>>> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no >>>> bridge call happening before crtc currently. >>> >>> Thanks for filling the blanks :) >>> >>> I assume that since cdns-dsi is a bridge, and it only has a simple >>> encoder implementation, for it to receive some video signal we need to >>> enable the CRTC before the bridge. >>> >>> If so, I think that's the original intent between the bridge pre_enable. >>> The original documentation had: >>> >>> pre_enable: this contains things needed to be done for the bridge >>> before this contains things needed to be done for the bridge before >>> this contains things needed to be done for the bridge before. >>> >>> and the current one has: >>> >>> The display pipe (i.e. clocks and timing signals) feeding this bridge >>> will not yet be running when this callback is called. The bridge must >>> not enable the display link feeding the next bridge in the chain (if >>> there is one) when this callback is called. >>> >>> I would say the CRTC is such a source, even more so now that the encoder >>> is usually transparent, so I think we should instead move the crtc >>> enable call after the bridge pre_enable. >> >> Hmm, if I understand you right, the newer sequence of calls will look >> like this, >> >> bridge[n]_pre_enable >> --- >> bridge[1]_pre_enable >> >> crtc_enable >> encoder_enable >> >> bridge[1]_enable >> --- >> bridge[n]_enable > > Yes :) > >> I do agree with this. This makes sense. CRTC is indeed such a source, >> and should ideally be enabled after the bridges are pre_enabled. >> >>> >>> Would that work? >>> >> >> So, this could potentially work, yes. The cdns-dsi would get pre_enabled >> after all bridges after cdns-dsi are pre_enabled. But over a quick test >> with BBAI64 + RPi Panel, I don't see any issue. >> >> However, the one concern that I have right now, is about breaking any >> existing (albeit faulty) implementation which relies on CRTC being >> enabled before the bridges are pre_enabled. =) > > I don't think it'll be a big deal. If there was a proper encoder driver, > it was probably gating the signal until it's enabled. If there isn't, > then yeah it might disrupt things, but it mostly means that the driver > wasn't properly split between pre_enable and enable. > > So I think it's worth trying, and we'll see the outcome. > Alright! =) Have made the changes as per your suggestions in v2. Thanks! Regards Aradhya
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fb97b51b38f1..396bb83e4296 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1234,6 +1234,49 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (ret == 0) drm_crtc_vblank_put(crtc); } + + for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, + new_conn_state, i) { + struct drm_encoder *encoder; + struct drm_bridge *bridge; + + /* + * Shut down everything that's in the changeset and currently + * still on. So need to check the old, saved state. + */ + if (!old_conn_state->crtc) + continue; + + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc); + + if (new_conn_state->crtc) + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, + new_conn_state->crtc); + else + new_crtc_state = NULL; + + if (!crtc_needs_disable(old_crtc_state, new_crtc_state) || + !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state)) + continue; + + encoder = old_conn_state->best_encoder; + + /* We shouldn't get this far if we didn't previously have + * an encoder.. but WARN_ON() rather than explode. + */ + if (WARN_ON(!encoder)) + continue; + + drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); + + /* + * Each encoder has at most one connector (since we always steal + * it away), so we won't call disable hooks twice. + */ + bridge = drm_bridge_chain_get_first_bridge(encoder); + drm_atomic_bridge_chain_late_disable(bridge, old_state); + } } /** @@ -1469,6 +1512,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, struct drm_connector_state *new_conn_state; int i; + for_each_new_connector_in_state(old_state, connector, new_conn_state, i) { + struct drm_encoder *encoder; + struct drm_bridge *bridge; + + if (!new_conn_state->best_encoder) + continue; + + if (!new_conn_state->crtc->state->active || + !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state)) + continue; + + encoder = new_conn_state->best_encoder; + + drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); + + /* + * Each encoder has at most one connector (since we always steal + * it away), so we won't call enable hooks twice. + */ + bridge = drm_bridge_chain_get_first_bridge(encoder); + drm_atomic_bridge_chain_early_enable(bridge, old_state); + } + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs; diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 28abe9aa99ca..b9070f6b3554 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -623,6 +623,50 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_disable); +/** + * drm_atomic_bridge_chain_late_disable - disables all bridges in the encoder + * chain after crtc is disabled. + * @bridge: bridge control structure + * @old_state: old atomic state + * + * Calls &drm_bridge_funcs.atomic_late_disable (falls back on + * &drm_bridge_funcs.late_disable) op for all the bridges in the + * encoder chain, starting from the last bridge to the first. These are called + * after calling &drm_crtc_helper_funcs.atomic_disable. + * + * Note: the bridge passed should be the one closest to the encoder + */ +void drm_atomic_bridge_chain_late_disable(struct drm_bridge *bridge, + struct drm_atomic_state *old_state) +{ + struct drm_encoder *encoder; + struct drm_bridge *iter; + + if (!bridge) + return; + + encoder = bridge->encoder; + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { + if (iter->funcs->atomic_late_disable) { + struct drm_bridge_state *old_bridge_state; + + old_bridge_state = + drm_atomic_get_old_bridge_state(old_state, + iter); + if (WARN_ON(!old_bridge_state)) + return; + + iter->funcs->atomic_late_disable(iter, old_bridge_state); + } else if (iter->funcs->late_disable) { + iter->funcs->late_disable(iter); + } + + if (iter == bridge) + break; + } +} +EXPORT_SYMBOL(drm_atomic_bridge_chain_late_disable); + static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { @@ -728,6 +772,46 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable); +/** + * drm_atomic_bridge_chain_early_enable - enables all bridges in the encoder + * chain before it's crtc is enabled + * @bridge: bridge control structure + * @old_state: old atomic state + * + * Calls &drm_bridge_funcs.atomic_early_enable (falls back on + * &drm_bridge_funcs.early_enable) op for all the bridges in the + * encoder chain, starting from the first bridge to the last. These are called + * before even the &drm_crtc_helper_funcs.atomic_enable is called. + * + * Note: the bridge passed should be the one closest to the encoder. + */ +void drm_atomic_bridge_chain_early_enable(struct drm_bridge *bridge, + struct drm_atomic_state *old_state) +{ + struct drm_encoder *encoder; + + if (!bridge) + return; + + encoder = bridge->encoder; + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { + if (bridge->funcs->atomic_early_enable) { + struct drm_bridge_state *old_bridge_state; + + old_bridge_state = + drm_atomic_get_old_bridge_state(old_state, + bridge); + if (WARN_ON(!old_bridge_state)) + return; + + bridge->funcs->atomic_early_enable(bridge, old_bridge_state); + } else if (bridge->funcs->early_enable) { + bridge->funcs->early_enable(bridge); + } + } +} +EXPORT_SYMBOL(drm_atomic_bridge_chain_early_enable); + static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 4baca0d9107b..40f93230abb2 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -206,6 +206,20 @@ struct drm_bridge_funcs { */ void (*post_disable)(struct drm_bridge *bridge); + /** + * @late_disable: + * + * This callback should disable the bridge. It is called right after the + * preceding element in the display pipe is disabled. If the preceding + * element is a bridge this means it's called after that bridge's + * @atomic_post_disable. If the preceding element is a &drm_crtc it's + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable + * hook. + * + * The @late_disable callback is optional. + */ + void (*late_disable)(struct drm_bridge *bridge); + /** * @mode_set: * @@ -235,6 +249,26 @@ struct drm_bridge_funcs { void (*mode_set)(struct drm_bridge *bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode); + + /** + * @early_enable: + * + * This callback should enable the bridge. It is called right before + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called before that + * bridge's @early_enable. If the preceding element is a &drm_crtc it's + * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable + * hook. + * + * The display pipe (i.e. clocks and timing signals) feeding this bridge + * will not yet be running when this callback is called. The bridge can + * enable the display link feeding the next bridge in the chain (if + * there is one) when this callback is called. + * + * The @early_enable callback is optional. + */ + void (*early_enable)(struct drm_bridge *bridge); + /** * @pre_enable: * @@ -285,6 +319,26 @@ struct drm_bridge_funcs { */ void (*enable)(struct drm_bridge *bridge); + /** + * @atomic_early_enable: + * + * This callback should enable the bridge. It is called right before + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called before that + * bridge's @atomic_early_enable. If the preceding element is a + * &drm_crtc it's called right before the crtc's + * &drm_crtc_helper_funcs.atomic_enable hook. + * + * The display pipe (i.e. clocks and timing signals) feeding this bridge + * will not yet be running when this callback is called. The bridge can + * enable the display link feeding the next bridge in the chain (if + * there is one) when this callback is called. + * + * The @early_enable callback is optional. + */ + void (*atomic_early_enable)(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state); + /** * @atomic_pre_enable: * @@ -361,6 +415,21 @@ struct drm_bridge_funcs { void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state); + /** + * @atomic_late_disable: + * + * This callback should disable the bridge. It is called right after the + * preceding element in the display pipe is disabled. If the preceding + * element is a bridge this means it's called after that bridge's + * @atomic_late_disable. If the preceding element is a &drm_crtc it's + * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable + * hook. + * + * The @atomic_late_disable callback is optional. + */ + void (*atomic_late_disable)(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state); + /** * @atomic_duplicate_state: * @@ -873,6 +942,10 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge, struct drm_atomic_state *state); void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *state); +void drm_atomic_bridge_chain_late_disable(struct drm_bridge *bridge, + struct drm_atomic_state *state); +void drm_atomic_bridge_chain_early_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state); void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *state); void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
With the existing pre_enable and enable function hooks, the order of enable of display elements looks as follows, crtc_enable -> bridge[n]_pre_enable -> ... -> bridge[1]_pre_enable -> encoder_enable -> bridge[1]_enable -> ... -> bridge[n]_enable and vice versa for the disable. Some bridges need to be up and running before and after their source gets enabled and has run. In some case, that source is a display unit, controlled as part of &drm_crtc. For those bridges, add support for early_enable and late_disable function hooks. Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> --- drivers/gpu/drm/drm_atomic_helper.c | 67 +++++++++++++++++++++++ drivers/gpu/drm/drm_bridge.c | 84 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 73 +++++++++++++++++++++++++ 3 files changed, 224 insertions(+)