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

[Jyri Sarha <jsarha@ti.com>]

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.

> 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 :).

Cheers,
Jyri


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel