Message ID | 20250114055626.18816-4-aradhya.bhatia@linux.dev (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
Hi, On 14/01/2025 07:56, Aradhya Bhatia wrote: > From: Aradhya Bhatia <a-bhatia1@ti.com> > > The driver code doesn't have a Phy de-initialization path as yet, and so > it does not clear the phy_initialized flag while suspending. This is a > problem because after resume the driver looks at this flag to determine > if a Phy re-initialization is required or not. It is in fact required > because the hardware is resuming from a suspend, but the driver does not > carry out any re-initialization causing the D-Phy to not work at all. > > Call the counterparts of phy_init() and phy_power_on(), that are > phy_exit() and phy_power_off(), from _bridge_disable(), and clear the > flags so that the Phy can be initialized again when required. > > Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> > --- > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > index 056583e81155..039c5eb7fb66 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) > if (dsi->platform_ops && dsi->platform_ops->disable) > dsi->platform_ops->disable(dsi); > > + dsi->phy_initialized = false; > + dsi->link_initialized = false; > + phy_power_off(dsi->dphy); > + phy_exit(dsi->dphy); > + The phy related lines are counterparts to what's done in cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(), But is the phy_initialized even needed? phy_initialized() is called from cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the call in cdns_dsi_bridge_enable() be always skipped, as cdns_dsi_bridge_pre_enable() already set phy_initialized? Same question for cdns_dsi_init_link(), although that's also called from cdns_dsi_transfer(), so we probably need dsi->link_initialized. Tomi > pm_runtime_put(dsi->base.dev); > } > > @@ -1130,7 +1135,6 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev) > clk_disable_unprepare(dsi->dsi_sys_clk); > clk_disable_unprepare(dsi->dsi_p_clk); > reset_control_assert(dsi->dsi_p_rst); > - dsi->link_initialized = false; > return 0; > } >
Hi Tomi, On 1/14/25 18:00, Tomi Valkeinen wrote: > Hi, > > On 14/01/2025 07:56, Aradhya Bhatia wrote: >> From: Aradhya Bhatia <a-bhatia1@ti.com> >> >> The driver code doesn't have a Phy de-initialization path as yet, and so >> it does not clear the phy_initialized flag while suspending. This is a >> problem because after resume the driver looks at this flag to determine >> if a Phy re-initialization is required or not. It is in fact required >> because the hardware is resuming from a suspend, but the driver does not >> carry out any re-initialization causing the D-Phy to not work at all. >> >> Call the counterparts of phy_init() and phy_power_on(), that are >> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the >> flags so that the Phy can be initialized again when required. >> >> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> >> --- >> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> index 056583e81155..039c5eb7fb66 100644 >> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct >> drm_bridge *bridge) >> if (dsi->platform_ops && dsi->platform_ops->disable) >> dsi->platform_ops->disable(dsi); >> + dsi->phy_initialized = false; >> + dsi->link_initialized = false; >> + phy_power_off(dsi->dphy); >> + phy_exit(dsi->dphy); >> + > > The phy related lines are counterparts to what's done in > cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(), > > But is the phy_initialized even needed? phy_initialized() is called from > cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the > call in cdns_dsi_bridge_enable() be always skipped, as > cdns_dsi_bridge_pre_enable() already set phy_initialized? Yes, that is how the behavior has been. The initialization calls inside the _bridge_enable() end-up getting skipped. My first thought after reading your comments was to remove the init calls entirely from the _bridge_pre_enable(), and drop the phy_initialized flag too, and let _bridge_enable() only handle the init. The _bridge_enable() will anyway get renamed to _bridge_pre_enable(), while the existing _bridge_pre_enable() will get dropped, by the last patch of this series. But since this patch is intended as a fix, it will get applied to previous versions while that last patch of the series won't... and then we may end up having init calls only from _bridge_enable() for the older versions. Also, given all the fixes in the series, there is a possibility that an older-version of the driver might become functional (except for the color shift issue). My question then is, would it be a cause for concern if all the init calls are handled from the _bridge_enable() only? (one of the potential concerns detailed below) > > Same question for cdns_dsi_init_link(), although that's also called from > cdns_dsi_transfer(), so we probably need dsi->link_initialized. > Don't you think we'd need the phy to also be initialized for the DCS command to work? Usually, since DSI is among the initial bridges to get pre_enabled, the Link and Phy are both initialized by the time cdns_dsi_transfer() is called. So, even if cdns_dsi_transfer() doesn't call for cdns_dsi_hs_init(), it is able to work fine. If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting it to _bridge_enable() (like I suggested above) would be problematic without fixing it here. Regards Aradhya > >> pm_runtime_put(dsi->base.dev); >> } >> @@ -1130,7 +1135,6 @@ static int __maybe_unused >> cdns_dsi_suspend(struct device *dev) >> clk_disable_unprepare(dsi->dsi_sys_clk); >> clk_disable_unprepare(dsi->dsi_p_clk); >> reset_control_assert(dsi->dsi_p_rst); >> - dsi->link_initialized = false; >> return 0; >> } >> >
Hi, On 14/01/2025 16:44, Aradhya Bhatia wrote: > Hi Tomi, > > On 1/14/25 18:00, Tomi Valkeinen wrote: >> Hi, >> >> On 14/01/2025 07:56, Aradhya Bhatia wrote: >>> From: Aradhya Bhatia <a-bhatia1@ti.com> >>> >>> The driver code doesn't have a Phy de-initialization path as yet, and so >>> it does not clear the phy_initialized flag while suspending. This is a >>> problem because after resume the driver looks at this flag to determine >>> if a Phy re-initialization is required or not. It is in fact required >>> because the hardware is resuming from a suspend, but the driver does not >>> carry out any re-initialization causing the D-Phy to not work at all. >>> >>> Call the counterparts of phy_init() and phy_power_on(), that are >>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the >>> flags so that the Phy can be initialized again when required. >>> >>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> >>> --- >>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>> index 056583e81155..039c5eb7fb66 100644 >>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct >>> drm_bridge *bridge) >>> if (dsi->platform_ops && dsi->platform_ops->disable) >>> dsi->platform_ops->disable(dsi); >>> + dsi->phy_initialized = false; >>> + dsi->link_initialized = false; >>> + phy_power_off(dsi->dphy); >>> + phy_exit(dsi->dphy); >>> + >> >> The phy related lines are counterparts to what's done in >> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(), >> >> But is the phy_initialized even needed? phy_initialized() is called from >> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the >> call in cdns_dsi_bridge_enable() be always skipped, as >> cdns_dsi_bridge_pre_enable() already set phy_initialized? > > Yes, that is how the behavior has been. The initialization calls inside > the _bridge_enable() end-up getting skipped. > > My first thought after reading your comments was to remove the init > calls entirely from the _bridge_pre_enable(), and drop the > phy_initialized flag too, and let _bridge_enable() only handle the init. Isn't that the wrong way around? If currently bridge_pre_enable enables the phy, your suggestion above would change that. I would think keeping the init calls in bridge_pre_enable, and drop from bridge_enable. > The _bridge_enable() will anyway get renamed to _bridge_pre_enable(), > while the existing _bridge_pre_enable() will get dropped, by the last > patch of this series. Ok, but you can't do a fix that'll only be right after some future patch does more changes =). > But since this patch is intended as a fix, it will get applied to > previous versions while that last patch of the series won't... and then Speaking of which, I think you should cc: stable for the ones that should be applied to earlier kernels. And it would be good to have all such patches first in the series, to decrease any dependencies. > we may end up having init calls only from _bridge_enable() for the older > versions. > Also, given all the fixes in the series, there is a possibility that an > older-version of the driver might become functional (except for the > color shift issue). > > My question then is, would it be a cause for concern if all the init > calls are handled from the _bridge_enable() only? I'm not sure I follow here. Don't we want the init calls to happen in the pre_enable phase, both before and after the sequence change (patch 12)? But generally speaking, yes, it's good to keep fixes simple, and do cleanups later on top. Keeping that in mind, maybe this current patch is fine as it is. Although... if the init is done in pre_enable, shouldn't the deinit be done in post_disable? > (one of the potential concerns detailed below) > >> >> Same question for cdns_dsi_init_link(), although that's also called from >> cdns_dsi_transfer(), so we probably need dsi->link_initialized. >> > > Don't you think we'd need the phy to also be initialized for the DCS > command to work? I'm sure we do. But the driver doesn't do that currently, does it? Which I did find a bit odd, but I'm not familiar with the HW. However, my comment was related to calling cdns_dsi_init_link() in both cdns_dsi_bridge_enable and cdns_dsi_bridge_pre_enable functions. In this case the call in the cdns_dsi_bridge_enable function is a no-op, similar to calling cdns_dsi_hs_init(). But, if changed, that's also a cleanup, so maybe better keep away from fix patches. > Usually, since DSI is among the initial bridges to get pre_enabled, the > Link and Phy are both initialized by the time cdns_dsi_transfer() is > called. So, even if cdns_dsi_transfer() doesn't call for > cdns_dsi_hs_init(), it is able to work fine. > > If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting > it to _bridge_enable() (like I suggested above) would be problematic > without fixing it here. I don't know what how the HW works, but we definitely need PHY to send DCS commands. But we don't necessarily need HS mode, LP works fine (usually). It's just not clear to me what exactly cdns_dsi_hs_init() and cdns_dsi_init_link() do. What is "link"? Looks like cdns_dsi_init_link is doing some PHY stuff, which is kind of strange thing to do, as phy_init() and phy_power_on() are only done later. In any case, yes, the cdns_dsi_transfer() has to make sure we have LP/HS working. So indeed it might mean calling both functions. This is, however, perhaps a different topic, best left out of this series. Tomi
Hi, On 1/14/25 20:50, Tomi Valkeinen wrote: > Hi, > > On 14/01/2025 16:44, Aradhya Bhatia wrote: >> Hi Tomi, >> >> On 1/14/25 18:00, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 14/01/2025 07:56, Aradhya Bhatia wrote: >>>> From: Aradhya Bhatia <a-bhatia1@ti.com> >>>> >>>> The driver code doesn't have a Phy de-initialization path as yet, >>>> and so >>>> it does not clear the phy_initialized flag while suspending. This is a >>>> problem because after resume the driver looks at this flag to determine >>>> if a Phy re-initialization is required or not. It is in fact required >>>> because the hardware is resuming from a suspend, but the driver does >>>> not >>>> carry out any re-initialization causing the D-Phy to not work at all. >>>> >>>> Call the counterparts of phy_init() and phy_power_on(), that are >>>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the >>>> flags so that the Phy can be initialized again when required. >>>> >>>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") >>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> >>>> --- >>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>>> index 056583e81155..039c5eb7fb66 100644 >>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >>>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct >>>> drm_bridge *bridge) >>>> if (dsi->platform_ops && dsi->platform_ops->disable) >>>> dsi->platform_ops->disable(dsi); >>>> + dsi->phy_initialized = false; >>>> + dsi->link_initialized = false; >>>> + phy_power_off(dsi->dphy); >>>> + phy_exit(dsi->dphy); >>>> + >>> >>> The phy related lines are counterparts to what's done in >>> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(), >>> >>> But is the phy_initialized even needed? phy_initialized() is called from >>> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the >>> call in cdns_dsi_bridge_enable() be always skipped, as >>> cdns_dsi_bridge_pre_enable() already set phy_initialized? >> >> Yes, that is how the behavior has been. The initialization calls inside >> the _bridge_enable() end-up getting skipped. >> >> My first thought after reading your comments was to remove the init >> calls entirely from the _bridge_pre_enable(), and drop the >> phy_initialized flag too, and let _bridge_enable() only handle the init. > > Isn't that the wrong way around? If currently bridge_pre_enable enables > the phy, your suggestion above would change that. I would think keeping > the init calls in bridge_pre_enable, and drop from bridge_enable. > >> The _bridge_enable() will anyway get renamed to _bridge_pre_enable(), >> while the existing _bridge_pre_enable() will get dropped, by the last >> patch of this series. > > Ok, but you can't do a fix that'll only be right after some future patch > does more changes =). Yeah, that would be wrong. =) > >> But since this patch is intended as a fix, it will get applied to >> previous versions while that last patch of the series won't... and then > > Speaking of which, I think you should cc: stable for the ones that > should be applied to earlier kernels. And it would be good to have all > such patches first in the series, to decrease any dependencies. Will do! > >> we may end up having init calls only from _bridge_enable() for the older >> versions. >> Also, given all the fixes in the series, there is a possibility that an >> older-version of the driver might become functional (except for the >> color shift issue). >> >> My question then is, would it be a cause for concern if all the init >> calls are handled from the _bridge_enable() only? > > I'm not sure I follow here. Don't we want the init calls to happen in > the pre_enable phase, both before and after the sequence change (patch 12)? > It is, now. For that brief period, I was considering to keep them only in _bridge_enable(). > But generally speaking, yes, it's good to keep fixes simple, and do > cleanups later on top. Keeping that in mind, maybe this current patch is > fine as it is. Although... if the init is done in pre_enable, shouldn't > the deinit be done in post_disable? Yes, I will move the deinit to _bridge_post_disable(). So, if we keep the fix limited to deinit in _bridge_post_disable(), then the cleanup would involve dropping the init calls from _bridge_enable(). And then the patch-12 would do 3 things - 1. Drop older _bridge_pre_enable() 2. Rename old _bridge_enable() to _bridge_pre_enable() 3. Since the _old_ _bridge_enable() has the calls dropped in the cleanup patch, add those calls again in the _new_ _bridge_pre_enable() (which are really the same function bodies). Do you think we can instead skip the cleanup patch, as well as #3 from patch-12? Fun fact: We already have patch-4 which fixes the order of init calls in _bridge_enable()! =) > >> (one of the potential concerns detailed below) >> >>> >>> Same question for cdns_dsi_init_link(), although that's also called from >>> cdns_dsi_transfer(), so we probably need dsi->link_initialized. >>> >> >> Don't you think we'd need the phy to also be initialized for the DCS >> command to work? > > I'm sure we do. But the driver doesn't do that currently, does it? Which > I did find a bit odd, but I'm not familiar with the HW. > > However, my comment was related to calling cdns_dsi_init_link() in both > cdns_dsi_bridge_enable and cdns_dsi_bridge_pre_enable functions. In this > case the call in the cdns_dsi_bridge_enable function is a no-op, similar > to calling cdns_dsi_hs_init(). > > But, if changed, that's also a cleanup, so maybe better keep away from > fix patches. > >> Usually, since DSI is among the initial bridges to get pre_enabled, the >> Link and Phy are both initialized by the time cdns_dsi_transfer() is >> called. So, even if cdns_dsi_transfer() doesn't call for >> cdns_dsi_hs_init(), it is able to work fine. >> >> If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting >> it to _bridge_enable() (like I suggested above) would be problematic >> without fixing it here. > > I don't know what how the HW works, but we definitely need PHY to send > DCS commands. But we don't necessarily need HS mode, LP works fine > (usually). It's just not clear to me what exactly cdns_dsi_hs_init() and > cdns_dsi_init_link() do. What is "link"? Looks like cdns_dsi_init_link > is doing some PHY stuff, which is kind of strange thing to do, as > phy_init() and phy_power_on() are only done later. That is where my confusion is too. A quick look into the TRM didn't help me with distinctions either. > > In any case, yes, the cdns_dsi_transfer() has to make sure we have LP/HS > working. So indeed it might mean calling both functions. This is, > however, perhaps a different topic, best left out of this series. > Alright. Since it is decided to keep the init calls in _bridge_pre_enable(), cdns_dsi_transfer() is not going to be affected any more than it already is, and we won't be breaking anything new. I guess there can be some trial and error done to find whether cdns_dsi_transfer() is really dependent on cdns_dsi_hs_init() - but yes, let's keep that out of this series' scope. Regards Aradhya
Hi, On 14/01/2025 18:32, Aradhya Bhatia wrote: >> But generally speaking, yes, it's good to keep fixes simple, and do >> cleanups later on top. Keeping that in mind, maybe this current patch is >> fine as it is. Although... if the init is done in pre_enable, shouldn't >> the deinit be done in post_disable? > > Yes, I will move the deinit to _bridge_post_disable(). > > > So, if we keep the fix limited to deinit in _bridge_post_disable(), then > the cleanup would involve dropping the init calls from _bridge_enable(). > And then the patch-12 would do 3 things - > > 1. Drop older _bridge_pre_enable() > 2. Rename old _bridge_enable() to _bridge_pre_enable() > 3. Since the _old_ _bridge_enable() has the calls dropped in the > cleanup patch, add those calls again in the _new_ > _bridge_pre_enable() (which are really the same function > bodies). I would think patch-12 differently: it doesn't do what you list above, but rather combines the current pre_enable() and enable() into a new pre_enable(). > Do you think we can instead skip the cleanup patch, as well as #3 from > patch-12? Yes, I think the cleanup patch can just be dropped. It's not really relevant. > Fun fact: We already have patch-4 which fixes the order of init calls in > _bridge_enable()! =) Right. And I guess that fix doesn't fix anything in practice, as those init calls are no-ops in the bridge_enable()... It's a bit difficult to make meaningful fixes when things are so badly messed up =). So, maybe try to arrange the series so that the obvious "makes-sense" fixes for stable are in the beginning. So... patches 1, 3, 5? And then work towards the patch 12. And I'll try to not nit-pick too much, so that we can actually get this series merged, and then later do the cleanups on top =). Tomi
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 056583e81155..039c5eb7fb66 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) if (dsi->platform_ops && dsi->platform_ops->disable) dsi->platform_ops->disable(dsi); + dsi->phy_initialized = false; + dsi->link_initialized = false; + phy_power_off(dsi->dphy); + phy_exit(dsi->dphy); + pm_runtime_put(dsi->base.dev); } @@ -1130,7 +1135,6 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev) clk_disable_unprepare(dsi->dsi_sys_clk); clk_disable_unprepare(dsi->dsi_p_clk); reset_control_assert(dsi->dsi_p_rst); - dsi->link_initialized = false; return 0; }