Re: [PATCH 24/29] drm/tidss: dispc: Improve mode checking logs

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 02.09.25 um 10:32 schrieb Maxime Ripard:
> The dispc_vp_mode_valid() function checks whether a mode can be handled
> by the display controller.
>
> There's a whole bunch of criteria, and it's not clear when a rejection
> happens why it did. Let's add a bunch of logs on error to make it
> clearer.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 47 +++++++++++++++++++++++++++++--------
>   1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 32248b5f71b7566dc33d7a7db0efb26d3a9ed1c3..ef948e3041e10bc65cf2c4794a4e4cffa7e3fb3a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1349,47 +1349,63 @@ static void dispc_vp_set_default_color(struct dispc_device *dispc,
>   
>   enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   					 u32 hw_videoport,
>   					 const struct drm_display_mode *mode)
>   {
> +	struct tidss_device *tidss = dispc->tidss;
> +	struct drm_device *dev = &tidss->ddev;
>   	u32 hsw, hfp, hbp, vsw, vfp, vbp;
>   	enum dispc_vp_bus_type bus_type;
>   	int max_pclk;
>   
>   	bus_type = dispc->feat->vp_bus_type[hw_videoport];
>   
>   	max_pclk = dispc->feat->max_pclk_khz[bus_type];
>   
> -	if (WARN_ON(max_pclk == 0))
> +	if (WARN_ON(max_pclk == 0)) {

Better remove this WARN_ON(). User-space could trigger it and spam the 
kernel logs. It's also a driver bug, I think.

> +		drm_dbg(dev, "Invalid maximum pixel clock");
>   		return MODE_BAD;
> +	}
>   
> -	if (mode->clock < dispc->feat->min_pclk_khz)
> +	if (mode->clock < dispc->feat->min_pclk_khz) {
> +		drm_dbg(dev, "Mode pixel clock below hardware minimum pixel clock");
>   		return MODE_CLOCK_LOW;
> +	}
>   
> -	if (mode->clock > max_pclk)
> +	if (mode->clock > max_pclk) {
> +		drm_dbg(dev, "Mode pixel clock above hardware maximum pixel clock");
>   		return MODE_CLOCK_HIGH;
> +	}
>   
> -	if (mode->hdisplay > 4096)
> +	if (mode->hdisplay > 4096) {
> +		drm_dbg(dev, "Number of active horizontal pixels above hardware limits.");
>   		return MODE_BAD;
> +	}
>   
> -	if (mode->vdisplay > 4096)
> +	if (mode->vdisplay > 4096) {
> +		drm_dbg(dev, "Number of active vertical lines above hardware limits.");
>   		return MODE_BAD;
> +	}
>   
>   	/* TODO: add interlace support */
> -	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +		drm_dbg(dev, "Interlace modes not suppported.");
>   		return MODE_NO_INTERLACE;
> +	}
>   
>   	/*
>   	 * Enforce the output width is divisible by 2. Actually this
>   	 * is only needed in following cases:
>   	 * - YUV output selected (BT656, BT1120)
>   	 * - Dithering enabled
>   	 * - TDM with TDMCycleFormat == 3
>   	 * But for simplicity we enforce that always.
>   	 */
> -	if ((mode->hdisplay % 2) != 0)
> +	if ((mode->hdisplay % 2) != 0) {
> +		drm_dbg(dev, "Number of active horizontal pixels must be even.");
>   		return MODE_BAD_HVALUE;
> +	}
>   
>   	hfp = mode->hsync_start - mode->hdisplay;
>   	hsw = mode->hsync_end - mode->hsync_start;
>   	hbp = mode->htotal - mode->hsync_end;
>   
> @@ -1397,29 +1413,40 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   	vsw = mode->vsync_end - mode->vsync_start;
>   	vbp = mode->vtotal - mode->vsync_end;
>   
>   	if (hsw < 1 || hsw > 256 ||
>   	    hfp < 1 || hfp > 4096 ||
> -	    hbp < 1 || hbp > 4096)
> +	    hbp < 1 || hbp > 4096) {
> +		drm_dbg(dev,
> +			"Horizontal blanking or sync outside of hardware limits (fp: %u, sw: %u, bp: %u).",
> +			hfp, hsw, hbp);
>   		return MODE_BAD_HVALUE;
> +	}
>   
>   	if (vsw < 1 || vsw > 256 ||
> -	    vfp > 4095 || vbp > 4095)
> +	    vfp > 4095 || vbp > 4095) {
> +		drm_dbg(dev,
> +			"Vertical blanking or sync outside of hardware limits (fp: %u, sw: %u, bp: %u).",
> +			vfp, vsw, vbp);
>   		return MODE_BAD_VVALUE;
> +	}
>   
>   	if (dispc->memory_bandwidth_limit) {
>   		const unsigned int bpp = 4;
>   		u64 bandwidth;
>   
>   		bandwidth = 1000 * mode->clock;
>   		bandwidth = bandwidth * mode->hdisplay * mode->vdisplay * bpp;
>   		bandwidth = div_u64(bandwidth, mode->htotal * mode->vtotal);
>   
> -		if (dispc->memory_bandwidth_limit < bandwidth)
> +		if (dispc->memory_bandwidth_limit < bandwidth) {
> +			drm_dbg(dev, "Required memory bandwidth outside of hardware limits.");
>   			return MODE_BAD;
> +		}
>   	}
>   
> +	drm_dbg(dev, "Mode is valid.");
>   	return MODE_OK;
>   }
>   
>   int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport)
>   {
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)