| Message ID | 63342ee4cb0d6fe3f2548700bd3e3a777134a098.1450202735.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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; +} + 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; }