[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6aca2e93-034f-4731-adc4-4eaa3d148193@ideasonboard.com>
Date: Wed, 26 Jun 2024 13:47:59 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <a-bhatia1@...com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Jyri Sarha <jyri.sarha@....fi>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: DRI Development List <dri-devel@...ts.freedesktop.org>,
Linux Kernel List <linux-kernel@...r.kernel.org>,
Dominik Haller <d.haller@...tec.de>, Sam Ravnborg <sam@...nborg.org>,
Thierry Reding <treding@...dia.com>,
Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
Praneeth Bajjuri <praneeth@...com>, Udit Kumar <u-kumar1@...com>,
Devarsh Thakkar <devarsht@...com>, Jayesh Choudhary <j-choudhary@...com>,
Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for
mode_valid()
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@...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@...asonboard.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
Powered by blists - more mailing lists