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.valkei...@ideasonboard.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.bha...@linux.dev>
--
Regards
Aradhya
