[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0072bb93-5456-40c4-96bc-a7afb3523238@linux.dev>
Date: Sun, 20 Apr 2025 23:40:22 +0530
From: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jyri Sarha <jyri.sarha@....fi>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.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>,
Jayesh Choudhary <j-choudhary@...com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-phy@...ts.infradead.org, Francesco Dolcini <francesco@...cini.it>,
Devarsh Thakkar <devarsht@...com>
Subject: Re: [PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean
up cdns_dsi_mode2cfg()
Hi,
On 14/04/25 16:41, Tomi Valkeinen wrote:
> The driver does all the calculations and programming with video timings
> (hftp, hbp, etc.) instead of the modeline values (hsync_start, ...).
> Thus it makes sense to use struct videomode instead of struct
> drm_display_mode internally.
>
> Switch to videomode and do some cleanups in cdns_dsi_mode2cfg() along
> the way.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> ---
> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 ++++++++++++++------------
> 1 file changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index fb0623d3f854..a55f851711f0 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -9,6 +9,7 @@
> #include <drm/drm_drv.h>
> #include <drm/drm_probe_helper.h>
> #include <video/mipi_display.h>
> +#include <video/videomode.h>
>
> #include <linux/clk.h>
> #include <linux/interrupt.h>
> @@ -467,36 +468,35 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
> }
>
> static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
> - const struct drm_display_mode *mode,
> + const struct videomode *vm,
> struct cdns_dsi_cfg *dsi_cfg)
> {
> struct cdns_dsi_output *output = &dsi->output;
> - unsigned int tmp;
> - bool sync_pulse = false;
> + u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
> + bool sync_pulse;
> int bpp;
>
> + dpi_hsa = vm->hsync_len;
> + dpi_hbp = vm->hback_porch;
> + dpi_hfp = vm->hfront_porch;
> + dpi_hact = vm->hactive;
> +
> memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>
> - if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> - sync_pulse = true;
> + sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>
> bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>
> - tmp = mode->htotal -
> - (sync_pulse ? mode->hsync_end : mode->hsync_start);
> + dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
> + bpp, DSI_HBP_FRAME_OVERHEAD);
>
> - dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
> + if (sync_pulse)
> + dsi_cfg->hsa =
> + dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
>
> - if (sync_pulse) {
> - tmp = mode->hsync_end - mode->hsync_start;
> + dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
>
> - dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
> - DSI_HSA_FRAME_OVERHEAD);
> - }
> -
> - dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
> - dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
> - bpp, DSI_HFP_FRAME_OVERHEAD);
> + dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
>
> dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
> if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
I think at this stage, the dsi_cfg->htotal will always come out to be
((dpi_htotal * bitspp) / 8),
no matter whether the sync_pulse or the event_mode is set or not.
Whatever the overheads are there, they get cancelled out. So, it doesn't
need to be individually tracked.
> @@ -509,7 +509,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
> }
>
> static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
> - const struct drm_display_mode *mode,
> + const struct videomode *vm,
> struct cdns_dsi_cfg *dsi_cfg)
> {
> struct cdns_dsi_output *output = &dsi->output;
> @@ -517,11 +517,11 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
> unsigned int nlanes = output->dev->lanes;
> int ret;
>
> - ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
> + ret = cdns_dsi_mode2cfg(dsi, vm, dsi_cfg);
> if (ret)
> return ret;
>
> - ret = phy_mipi_dphy_get_default_config(mode->clock * 1000,
> + ret = phy_mipi_dphy_get_default_config(vm->pixelclock,
> mipi_dsi_pixel_format_to_bpp(output->dev->format),
> nlanes, phy_cfg);
> if (ret)
> @@ -909,12 +909,15 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
> struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
> struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
> + struct videomode vm;
>
> /* cdns-dsi requires negative syncs */
> adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>
> - return cdns_dsi_check_conf(dsi, adjusted_mode, dsi_cfg);
> + drm_display_mode_to_videomode(adjusted_mode, &vm);
> +
> + return cdns_dsi_check_conf(dsi, &vm, dsi_cfg);
> }
>
> static struct drm_bridge_state *
>
With the above taken care of,
Reviewed-by: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
--
Regards
Aradhya
Powered by blists - more mailing lists