| Message ID | 20240622110929.3115714-6-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
On 22/06/2024 14:09, Aradhya Bhatia wrote: > Allow the D-Phy config checks to use mode->clock instead of > mode->crtc_clock during mode_valid checks, like everywhere else in the > driver. > > Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +- > 1 file changed, 1 insertion(+), 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 03a5af52ec0b..426f77092341 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > @@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi, > if (ret) > return ret; > > - phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000, > + phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock : mode->crtc_clock) * 1000, > mipi_dsi_pixel_format_to_bpp(output->dev->format), > nlanes, phy_cfg); > I think this is fine as a fix. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> However... The code looks a bit messy. Maybe the first one is something that could be addressed in this series. - Return value of phy_mipi_dphy_get_default_config() is not checked - Using the non-crtc and crtc versions of the timings this way looks bad, but that's not a problem of the driver. It would be better to have a struct that contains the timings, and struct drm_display_mode would contain two instances of that struct. The driver code could then just pick the correct instance, instead of making the choice for each and every field. This would be an interesting coccinelle project ;) - Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd. Everything should already have been checked. In fact, at the check phase the resulting config values could have been stored somewhere, so that they're ready for use by cdns_dsi_bridge_enable(). But this rises the question if the non-crtc and crtc timings can actually be different, and if they are... doesn't it break everything if at the check phase we use the non-crtc ones, but at enable phase we use crtc ones? Ah, I see, this is with non-atomic. Maybe after you switch to atomic callbacks, atomic_check could be used so that there's no need for the WARN_ON_ONCE() in enable callback. Tomi
On 26/06/24 16:17, Tomi Valkeinen wrote: > On 22/06/2024 14:09, Aradhya Bhatia wrote: >> Allow the D-Phy config checks to use mode->clock instead of >> mode->crtc_clock during mode_valid checks, like everywhere else in the >> driver. >> >> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +- >> 1 file changed, 1 insertion(+), 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 03a5af52ec0b..426f77092341 100644 >> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> @@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi, >> if (ret) >> return ret; >> - phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000, >> + phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock >> : mode->crtc_clock) * 1000, >> mipi_dsi_pixel_format_to_bpp(output->dev->format), >> nlanes, phy_cfg); >> > > I think this is fine as a fix. > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > However... The code looks a bit messy. Maybe the first one is something > that could be addressed in this series. > > - Return value of phy_mipi_dphy_get_default_config() is not checked Sure, I can fix that. > > - Using the non-crtc and crtc versions of the timings this way looks > bad, but that's not a problem of the driver. It would be better to have > a struct that contains the timings, and struct drm_display_mode would > contain two instances of that struct. The driver code could then just > pick the correct instance, instead of making the choice for each and > every field. This would be an interesting coccinelle project ;) > > - Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd. > Everything should already have been checked. In fact, at the check phase > the resulting config values could have been stored somewhere, so that > they're ready for use by cdns_dsi_bridge_enable(). But this rises the > question if the non-crtc and crtc timings can actually be different, and > if they are... doesn't it break everything if at the check phase we use > the non-crtc ones, but at enable phase we use crtc ones? It'd appear that it does. I don't fully understand why the driver uses non-crtc_* timing parameters during the check phase, only to use the crtc_* timing parameters during _enable(). Since with tidss, both the sets are same, I haven't had to think too much about this! =) What is the ideal way that this should get addressed though? If we have an agreeable resolution then maybe I can fix that as well. > > Ah, I see, this is with non-atomic. Maybe after you switch to atomic > callbacks, atomic_check could be used so that there's no need for the > WARN_ON_ONCE() in enable callback. > Yes, I think this would be better. We can use atomic_check() to verify the crtc_* timing parameters, while the already existing mode_valid() can continue checking the non-crtc_* ones. I will add this change when I am adding other atomic_* APIs later in the series. Regards Aradhya
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 03a5af52ec0b..426f77092341 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi, if (ret) return ret; - phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000, + phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock : mode->crtc_clock) * 1000, mipi_dsi_pixel_format_to_bpp(output->dev->format), nlanes, phy_cfg);
Allow the D-Phy config checks to use mode->clock instead of mode->crtc_clock during mode_valid checks, like everywhere else in the driver. Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> --- drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)