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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 10, 2016 at 03:18:01PM +0200, Jyri Sarha wrote:
> On 12/16/15 10:37, Daniel Vetter wrote:
> >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.
> >
> 
> That would not work because we do not know the mode of the crtc when the
> framebuffer is going to be used.

My apologies, I didn't read your code careful. Let me blame jetlag on this
;-) Makes sense on 2nd look.

> >Also with atomic you'd only need to place this in one function, even if
> >you have different per-plane limitations ... hint, hint;-)
> 
> I am working on atomic modeset for tilcdc, but I am still a newbie on DRM
> front so it takes some time :).

No worries, there's tons of good atomic drivers as examples now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel