Re: [PATCH 03/12] drm/tilcdc: verify fb pitch

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 15, 2015 at 09:03:14PM +0200, Jyri Sarha wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> LCDC hardware does not support fb pitch that is different (i.e. larger)
> than the screen size. The driver currently does no checks for this, and
> the results of too big pitch are are flickering and lower fps.
> 
> This issue easily happens when using libdrm's modetest tool with non-32
> bpp modes. As modetest always allocated 4 bytes per pixel, it implies a
> bigger pitch for 16 or 24 bpp modes.
> 
> This patch adds a check to reject pitches the hardware cannot support.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 7b687ae..105f286 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -151,6 +151,22 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  	kfree(tilcdc_crtc);
>  }
>  
> +static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int depth, bpp;
> +
> +	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> +
> +	if (fb->pitches[0] != crtc->mode.hdisplay * bpp / 8) {
> +		dev_err(dev->dev,
> +			"Invalid pitch: fb and crtc widths must be the same");
> +		return -EINVAL;
> +	}
> +
> +	return 0;

This should be done in framebuffer_create instead if tilcdc has this
requirement everywhere. No point in allowing userspace to create an fb you
can't use. Only if you have planes with different limits should this be
checked after fb creation.

Also with atomic you'd only need to place this in one function, even if
you have different per-plane limitations ... hint, hint ;-)

Cheers, Daniel


> +}
> +
>  static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
>  		struct drm_framebuffer *fb,
>  		struct drm_pending_vblank_event *event,
> @@ -158,6 +174,11 @@ static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
>  {
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
> +	int r;
> +
> +	r = tilcdc_verify_fb(crtc, fb);
> +	if (r)
> +		return r;
>  
>  	if (tilcdc_crtc->event) {
>  		dev_err(dev->dev, "already pending page flip!\n");
> @@ -272,6 +293,10 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
>  	if (WARN_ON(!info))
>  		return -EINVAL;
>  
> +	ret = tilcdc_verify_fb(crtc, crtc->primary->fb);
> +	if (ret)
> +		return ret;
> +
>  	pm_runtime_get_sync(dev->dev);
>  
>  	/* Configure the Burst Size and fifo threshold of DMA: */
> @@ -431,6 +456,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
>  static int tilcdc_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>  		struct drm_framebuffer *old_fb)
>  {
> +	int r;
> +
> +	r = tilcdc_verify_fb(crtc, crtc->primary->fb);
> +	if (r)
> +		return r;
> +
>  	update_scanout(crtc);
>  	return 0;
>  }
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel