Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Tue, Sep 08, 2020 at 03:33:04PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 08, 2020 at 02:29:02PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner wrote:
> > > On 2020-09-08 10:48, Daniel Vetter wrote:
> > >> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> > >>> On 08/09/2020 10:55, Stefan Agner wrote:
> > On 2020-09-07 20:18, Daniel Vetter wrote:
> > > On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> > >> Hi Stefan,
> > >>
> > >> Thank you for the patch.
> > >>
> > >> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> > >>> The lcdif IP does not support a framebuffer pitch (stride) other
> > >>> than
> > >>> the CRTC width. Check for equality and reject the state otherwise.
> > >>>
> > >>> This prevents a distorted picture when using 640x800 and running the
> > >>> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> > >>
> > >> s/tires/tries/
> > >>
> > >>> leads at that particular resolution to width != stride. Currently
> > >>> Mesa has no fallback behavior, but rejecting this configuration
> > >>> allows
> > >>> userspace to handle the issue correctly.
> > >>
> > >> I'm increasingly impressed by how featureful this IP core is :-)
> > >>
> > >>> Signed-off-by: Stefan Agner
> > >>> ---
> > >>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> > >>> 1 file changed, 18 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>> index b721b8b262ce..79aa14027f91 100644
> > >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
> > >>> drm_plane *plane,
> > >>> {
> > >>> struct mxsfb_drm_private *mxsfb =
> > >>> to_mxsfb_drm_private(plane->dev);
> > >>> struct drm_crtc_state *crtc_state;
> > >>> + unsigned int pitch;
> > >>> + int ret;
> > >>>
> > >>> crtc_state =
> > >>> drm_atomic_get_new_crtc_state(plane_state->state,
> > >>>>crtc);
> > >>>
> > >>> - return drm_atomic_helper_check_plane_state(plane_state,
> > >>> crtc_state,
> > >>> -
> > >>> DRM_PLANE_HELPER_NO_SCALING,
> > >>> -
> > >>> DRM_PLANE_HELPER_NO_SCALING,
> > >>> - false, true);
> > >>> + ret = drm_atomic_helper_check_plane_state(plane_state,
> > >>> crtc_state,
> > >>> +
> > >>> DRM_PLANE_HELPER_NO_SCALING,
> > >>> +
> > >>> DRM_PLANE_HELPER_NO_SCALING,
> > >>> + false, true);
> > >>> + if (ret || !plane_state->visible)
> > >>
> > >> Would it be more explict to check for !plane_state->fb ? Otherwise
> > >> I'll
> > >> have to verify that !fb always implies !visible :-)
> > >>
> > >>> + return ret;
> > >>> +
> > >>> + pitch = crtc_state->mode.hdisplay *
> > >>> + plane_state->fb->format->cpp[0];
> > >>
> > >> This holds on a single line.
> > >>
> > >>> + if (plane_state->fb->pitches[0] != pitch) {
> > >>> + dev_err(plane->dev->dev,
> > >>> + "Invalid pitch: fb and crtc widths must be
> > >>> the same");
> > >>
> > >> I'd turn this into a dev_dbg(), printing error messages to the kernel
> > >> log in response to user-triggered conditions is a bit too verbose and
> > >> could flood the log.
> > >>
> > >> Wouldn't it be best to catch this issue when creating the
> > >> framebuffer ?
> > >
> > > Yeah this should be verified at addfb time. We try to validate as
> > > early as
> > > possible.
> > > -Daniel
> > >
> >
> > Sounds sensible. From what I can tell fb_create is the proper callback
> > to implement this at addfb time. Will give this a try.
> >
> > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> > should be moved to addfb there too?
> > >>>
> > >>> But you don't know the crtc width when creating the framebuffer.
> > >>
> > >> Hm right this is a different check. What we could check in fb_create for
> > >> both is that the logical fb size matches exactly the pitch. That's not
> > >> sufficient criteria, but it will at least catch some of them already.
> > >>
> > >> But yeah we'd need both here.
> > >
> > > After validating width of framebuffer
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On 2020-09-08 14:33, Laurent Pinchart wrote:
> On Tue, Sep 08, 2020 at 02:29:02PM +0200, Daniel Vetter wrote:
>> On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner wrote:
>> > On 2020-09-08 10:48, Daniel Vetter wrote:
>> >> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
>> >>> On 08/09/2020 10:55, Stefan Agner wrote:
>> On 2020-09-07 20:18, Daniel Vetter wrote:
>> > On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
>> >> Hi Stefan,
>> >>
>> >> Thank you for the patch.
>> >>
>> >> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
>> >>> The lcdif IP does not support a framebuffer pitch (stride) other than
>> >>> the CRTC width. Check for equality and reject the state otherwise.
>> >>>
>> >>> This prevents a distorted picture when using 640x800 and running the
>> >>> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>> >>
>> >> s/tires/tries/
>> >>
>> >>> leads at that particular resolution to width != stride. Currently
>> >>> Mesa has no fallback behavior, but rejecting this configuration
>> >>> allows
>> >>> userspace to handle the issue correctly.
>> >>
>> >> I'm increasingly impressed by how featureful this IP core is :-)
>> >>
>> >>> Signed-off-by: Stefan Agner
>> >>> ---
>> >>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
>> >>> 1 file changed, 18 insertions(+), 4 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> >>> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> >>> index b721b8b262ce..79aa14027f91 100644
>> >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> >>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
>> >>> drm_plane *plane,
>> >>> {
>> >>> struct mxsfb_drm_private *mxsfb =
>> >>> to_mxsfb_drm_private(plane->dev);
>> >>> struct drm_crtc_state *crtc_state;
>> >>> + unsigned int pitch;
>> >>> + int ret;
>> >>>
>> >>> crtc_state =
>> >>> drm_atomic_get_new_crtc_state(plane_state->state,
>> >>>>crtc);
>> >>>
>> >>> - return drm_atomic_helper_check_plane_state(plane_state,
>> >>> crtc_state,
>> >>> -
>> >>> DRM_PLANE_HELPER_NO_SCALING,
>> >>> -
>> >>> DRM_PLANE_HELPER_NO_SCALING,
>> >>> - false, true);
>> >>> + ret = drm_atomic_helper_check_plane_state(plane_state,
>> >>> crtc_state,
>> >>> +
>> >>> DRM_PLANE_HELPER_NO_SCALING,
>> >>> +
>> >>> DRM_PLANE_HELPER_NO_SCALING,
>> >>> + false, true);
>> >>> + if (ret || !plane_state->visible)
>> >>
>> >> Would it be more explict to check for !plane_state->fb ? Otherwise
>> >> I'll
>> >> have to verify that !fb always implies !visible :-)
>> >>
>> >>> + return ret;
>> >>> +
>> >>> + pitch = crtc_state->mode.hdisplay *
>> >>> + plane_state->fb->format->cpp[0];
>> >>
>> >> This holds on a single line.
>> >>
>> >>> + if (plane_state->fb->pitches[0] != pitch) {
>> >>> + dev_err(plane->dev->dev,
>> >>> + "Invalid pitch: fb and crtc widths must be
>> >>> the same");
>> >>
>> >> I'd turn this into a dev_dbg(), printing error messages to the kernel
>> >> log in response to user-triggered conditions is a bit too verbose and
>> >> could flood the log.
>> >>
>> >> Wouldn't it be best to catch this issue when creating the framebuffer
>> >> ?
>> >
>> > Yeah this should be verified at addfb time. We try to validate as
>> > early as
>> > possible.
>> > -Daniel
>> >
>>
>> Sounds sensible. From what I can tell fb_create is the proper callback
>> to implement this at addfb time. Will give this a try.
>>
>> FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
>> should be moved to addfb there too?
>> >>>
>> >>> But you don't know the crtc width when creating the framebuffer.
>> >>
>> >> Hm right this is a different check. What we could check in fb_create for
>> >> both is that the logical fb size matches exactly the pitch. That's not
>> >> sufficient criteria, but it will at least catch some of them already.
>> >>
>> >> But yeah we'd need both here.
>> >
>> > After validating width of framebuffer against pitch, the only thing we
>> > need to check here is that the width matches. From what I can tell,
>> > least for mxsfb, this should be
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Tue, Sep 08, 2020 at 02:29:02PM +0200, Daniel Vetter wrote:
> On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner wrote:
> > On 2020-09-08 10:48, Daniel Vetter wrote:
> >> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> >>> On 08/09/2020 10:55, Stefan Agner wrote:
> On 2020-09-07 20:18, Daniel Vetter wrote:
> > On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> >> Hi Stefan,
> >>
> >> Thank you for the patch.
> >>
> >> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> >>> The lcdif IP does not support a framebuffer pitch (stride) other than
> >>> the CRTC width. Check for equality and reject the state otherwise.
> >>>
> >>> This prevents a distorted picture when using 640x800 and running the
> >>> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> >>
> >> s/tires/tries/
> >>
> >>> leads at that particular resolution to width != stride. Currently
> >>> Mesa has no fallback behavior, but rejecting this configuration allows
> >>> userspace to handle the issue correctly.
> >>
> >> I'm increasingly impressed by how featureful this IP core is :-)
> >>
> >>> Signed-off-by: Stefan Agner
> >>> ---
> >>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> >>> 1 file changed, 18 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >>> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >>> index b721b8b262ce..79aa14027f91 100644
> >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
> >>> drm_plane *plane,
> >>> {
> >>> struct mxsfb_drm_private *mxsfb =
> >>> to_mxsfb_drm_private(plane->dev);
> >>> struct drm_crtc_state *crtc_state;
> >>> + unsigned int pitch;
> >>> + int ret;
> >>>
> >>> crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> >>>>crtc);
> >>>
> >>> - return drm_atomic_helper_check_plane_state(plane_state,
> >>> crtc_state,
> >>> -
> >>> DRM_PLANE_HELPER_NO_SCALING,
> >>> -
> >>> DRM_PLANE_HELPER_NO_SCALING,
> >>> - false, true);
> >>> + ret = drm_atomic_helper_check_plane_state(plane_state,
> >>> crtc_state,
> >>> +
> >>> DRM_PLANE_HELPER_NO_SCALING,
> >>> +
> >>> DRM_PLANE_HELPER_NO_SCALING,
> >>> + false, true);
> >>> + if (ret || !plane_state->visible)
> >>
> >> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> >> have to verify that !fb always implies !visible :-)
> >>
> >>> + return ret;
> >>> +
> >>> + pitch = crtc_state->mode.hdisplay *
> >>> + plane_state->fb->format->cpp[0];
> >>
> >> This holds on a single line.
> >>
> >>> + if (plane_state->fb->pitches[0] != pitch) {
> >>> + dev_err(plane->dev->dev,
> >>> + "Invalid pitch: fb and crtc widths must be
> >>> the same");
> >>
> >> I'd turn this into a dev_dbg(), printing error messages to the kernel
> >> log in response to user-triggered conditions is a bit too verbose and
> >> could flood the log.
> >>
> >> Wouldn't it be best to catch this issue when creating the framebuffer ?
> >
> > Yeah this should be verified at addfb time. We try to validate as early
> > as
> > possible.
> > -Daniel
> >
>
> Sounds sensible. From what I can tell fb_create is the proper callback
> to implement this at addfb time. Will give this a try.
>
> FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> should be moved to addfb there too?
> >>>
> >>> But you don't know the crtc width when creating the framebuffer.
> >>
> >> Hm right this is a different check. What we could check in fb_create for
> >> both is that the logical fb size matches exactly the pitch. That's not
> >> sufficient criteria, but it will at least catch some of them already.
> >>
> >> But yeah we'd need both here.
> >
> > After validating width of framebuffer against pitch, the only thing we
> > need to check here is that the width matches. From what I can tell,
> > least for mxsfb, this should be covered by
> > drm_atomic_helper_check_plane_state's can_position parameter set to
> > false.
>
> This only checks against the src rectangle of the crtc state, there's
> nothing forcing that the size of
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner wrote:
>
> On 2020-09-08 10:48, Daniel Vetter wrote:
> > On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> >> Hi,
> >>
> >> On 08/09/2020 10:55, Stefan Agner wrote:
> >> > On 2020-09-07 20:18, Daniel Vetter wrote:
> >> >> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> >> >>> Hi Stefan,
> >> >>>
> >> >>> Thank you for the patch.
> >> >>>
> >> >>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> >> The lcdif IP does not support a framebuffer pitch (stride) other than
> >> the CRTC width. Check for equality and reject the state otherwise.
> >>
> >> This prevents a distorted picture when using 640x800 and running the
> >> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> >> >>>
> >> >>> s/tires/tries/
> >> >>>
> >> leads at that particular resolution to width != stride. Currently
> >> Mesa has no fallback behavior, but rejecting this configuration allows
> >> userspace to handle the issue correctly.
> >> >>>
> >> >>> I'm increasingly impressed by how featureful this IP core is :-)
> >> >>>
> >> Signed-off-by: Stefan Agner
> >> ---
> >> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> >> 1 file changed, 18 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >> index b721b8b262ce..79aa14027f91 100644
> >> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> >> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
> >> drm_plane *plane,
> >> {
> >> struct mxsfb_drm_private *mxsfb =
> >> to_mxsfb_drm_private(plane->dev);
> >> struct drm_crtc_state *crtc_state;
> >> + unsigned int pitch;
> >> + int ret;
> >>
> >> crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> >> >crtc);
> >>
> >> - return drm_atomic_helper_check_plane_state(plane_state,
> >> crtc_state,
> >> -
> >> DRM_PLANE_HELPER_NO_SCALING,
> >> -
> >> DRM_PLANE_HELPER_NO_SCALING,
> >> - false, true);
> >> + ret = drm_atomic_helper_check_plane_state(plane_state,
> >> crtc_state,
> >> +
> >> DRM_PLANE_HELPER_NO_SCALING,
> >> +
> >> DRM_PLANE_HELPER_NO_SCALING,
> >> + false, true);
> >> + if (ret || !plane_state->visible)
> >> >>>
> >> >>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> >> >>> have to verify that !fb always implies !visible :-)
> >> >>>
> >> + return ret;
> >> +
> >> + pitch = crtc_state->mode.hdisplay *
> >> + plane_state->fb->format->cpp[0];
> >> >>>
> >> >>> This holds on a single line.
> >> >>>
> >> + if (plane_state->fb->pitches[0] != pitch) {
> >> + dev_err(plane->dev->dev,
> >> + "Invalid pitch: fb and crtc widths must be
> >> the same");
> >> >>>
> >> >>> I'd turn this into a dev_dbg(), printing error messages to the kernel
> >> >>> log in response to user-triggered conditions is a bit too verbose and
> >> >>> could flood the log.
> >> >>>
> >> >>> Wouldn't it be best to catch this issue when creating the framebuffer ?
> >> >>
> >> >> Yeah this should be verified at addfb time. We try to validate as early
> >> >> as
> >> >> possible.
> >> >> -Daniel
> >> >>
> >> >
> >> > Sounds sensible. From what I can tell fb_create is the proper callback
> >> > to implement this at addfb time. Will give this a try.
> >> >
> >> > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> >> > should be moved to addfb there too?
> >>
> >> But you don't know the crtc width when creating the framebuffer.
> >
> > Hm right this is a different check. What we could check in fb_create for
> > both is that the logical fb size matches exactly the pitch. That's not
> > sufficient criteria, but it will at least catch some of them already.
> >
> > But yeah we'd need both here.
>
> After validating width of framebuffer against pitch, the only thing we
> need to check here is that the width matches. From what I can tell,
> least for mxsfb, this should be covered by
> drm_atomic_helper_check_plane_state's can_position parameter set to
> false.
This only checks against the src rectangle of the crtc state, there's
nothing forcing that the size of the fb matches the src rectangle
exactly. I guess we could maybe add that as
Re: [PATCH] drm: mxsfb: check framebuffer pitch
Hi,
On 08/09/2020 10:55, Stefan Agner wrote:
> On 2020-09-07 20:18, Daniel Vetter wrote:
>> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
>>> Hi Stefan,
>>>
>>> Thank you for the patch.
>>>
>>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
The lcdif IP does not support a framebuffer pitch (stride) other than
the CRTC width. Check for equality and reject the state otherwise.
This prevents a distorted picture when using 640x800 and running the
Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>>>
>>> s/tires/tries/
>>>
leads at that particular resolution to width != stride. Currently
Mesa has no fallback behavior, but rejecting this configuration allows
userspace to handle the issue correctly.
>>>
>>> I'm increasingly impressed by how featureful this IP core is :-)
>>>
Signed-off-by: Stefan Agner
---
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index b721b8b262ce..79aa14027f91 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
*plane,
{
struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
struct drm_crtc_state *crtc_state;
+ unsigned int pitch;
+ int ret;
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
>crtc);
- return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
- DRM_PLANE_HELPER_NO_SCALING,
- DRM_PLANE_HELPER_NO_SCALING,
- false, true);
+ ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+DRM_PLANE_HELPER_NO_SCALING,
+DRM_PLANE_HELPER_NO_SCALING,
+false, true);
+ if (ret || !plane_state->visible)
>>>
>>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
>>> have to verify that !fb always implies !visible :-)
>>>
+ return ret;
+
+ pitch = crtc_state->mode.hdisplay *
+ plane_state->fb->format->cpp[0];
>>>
>>> This holds on a single line.
>>>
+ if (plane_state->fb->pitches[0] != pitch) {
+ dev_err(plane->dev->dev,
+ "Invalid pitch: fb and crtc widths must be the same");
>>>
>>> I'd turn this into a dev_dbg(), printing error messages to the kernel
>>> log in response to user-triggered conditions is a bit too verbose and
>>> could flood the log.
>>>
>>> Wouldn't it be best to catch this issue when creating the framebuffer ?
>>
>> Yeah this should be verified at addfb time. We try to validate as early as
>> possible.
>> -Daniel
>>
>
> Sounds sensible. From what I can tell fb_create is the proper callback
> to implement this at addfb time. Will give this a try.
>
> FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> should be moved to addfb there too?
But you don't know the crtc width when creating the framebuffer.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Tue, Sep 08, 2020 at 10:48:55AM +0200, Daniel Vetter wrote:
> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> > On 08/09/2020 10:55, Stefan Agner wrote:
> > > On 2020-09-07 20:18, Daniel Vetter wrote:
> > >> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> > >>> Hi Stefan,
> > >>>
> > >>> Thank you for the patch.
> > >>>
> > >>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> > The lcdif IP does not support a framebuffer pitch (stride) other than
> > the CRTC width. Check for equality and reject the state otherwise.
> >
> > This prevents a distorted picture when using 640x800 and running the
> > Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> > >>>
> > >>> s/tires/tries/
> > >>>
> > leads at that particular resolution to width != stride. Currently
> > Mesa has no fallback behavior, but rejecting this configuration allows
> > userspace to handle the issue correctly.
> > >>>
> > >>> I'm increasingly impressed by how featureful this IP core is :-)
> > >>>
> > Signed-off-by: Stefan Agner
> > ---
> > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > index b721b8b262ce..79aa14027f91 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
> > drm_plane *plane,
> > {
> > struct mxsfb_drm_private *mxsfb =
> > to_mxsfb_drm_private(plane->dev);
> > struct drm_crtc_state *crtc_state;
> > + unsigned int pitch;
> > + int ret;
> >
> > crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> > >crtc);
> >
> > - return drm_atomic_helper_check_plane_state(plane_state,
> > crtc_state,
> > -
> > DRM_PLANE_HELPER_NO_SCALING,
> > -
> > DRM_PLANE_HELPER_NO_SCALING,
> > - false, true);
> > + ret = drm_atomic_helper_check_plane_state(plane_state,
> > crtc_state,
> > +
> > DRM_PLANE_HELPER_NO_SCALING,
> > +
> > DRM_PLANE_HELPER_NO_SCALING,
> > +false, true);
> > + if (ret || !plane_state->visible)
> > >>>
> > >>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> > >>> have to verify that !fb always implies !visible :-)
> > >>>
> > + return ret;
> > +
> > + pitch = crtc_state->mode.hdisplay *
> > + plane_state->fb->format->cpp[0];
> > >>>
> > >>> This holds on a single line.
> > >>>
> > + if (plane_state->fb->pitches[0] != pitch) {
> > + dev_err(plane->dev->dev,
> > + "Invalid pitch: fb and crtc widths must be the
> > same");
> > >>>
> > >>> I'd turn this into a dev_dbg(), printing error messages to the kernel
> > >>> log in response to user-triggered conditions is a bit too verbose and
> > >>> could flood the log.
> > >>>
> > >>> Wouldn't it be best to catch this issue when creating the framebuffer ?
> > >>
> > >> Yeah this should be verified at addfb time. We try to validate as early
> > >> as
> > >> possible.
> > >
> > > Sounds sensible. From what I can tell fb_create is the proper callback
> > > to implement this at addfb time. Will give this a try.
> > >
> > > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> > > should be moved to addfb there too?
> >
> > But you don't know the crtc width when creating the framebuffer.
>
> Hm right this is a different check. What we could check in fb_create for
> both is that the logical fb size matches exactly the pitch. That's not
> sufficient criteria, but it will at least catch some of them already.
>
> But yeah we'd need both here.
Correct. At addfb time we can check the pitch, and at atomic check time
we should check that the plane spans the whole CRTC.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On 2020-09-08 10:48, Daniel Vetter wrote:
> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 08/09/2020 10:55, Stefan Agner wrote:
>> > On 2020-09-07 20:18, Daniel Vetter wrote:
>> >> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
>> >>> Hi Stefan,
>> >>>
>> >>> Thank you for the patch.
>> >>>
>> >>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
>> The lcdif IP does not support a framebuffer pitch (stride) other than
>> the CRTC width. Check for equality and reject the state otherwise.
>>
>> This prevents a distorted picture when using 640x800 and running the
>> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>> >>>
>> >>> s/tires/tries/
>> >>>
>> leads at that particular resolution to width != stride. Currently
>> Mesa has no fallback behavior, but rejecting this configuration allows
>> userspace to handle the issue correctly.
>> >>>
>> >>> I'm increasingly impressed by how featureful this IP core is :-)
>> >>>
>> Signed-off-by: Stefan Agner
>> ---
>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
>> 1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> index b721b8b262ce..79aa14027f91 100644
>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
>> drm_plane *plane,
>> {
>> struct mxsfb_drm_private *mxsfb =
>> to_mxsfb_drm_private(plane->dev);
>> struct drm_crtc_state *crtc_state;
>> + unsigned int pitch;
>> + int ret;
>>
>> crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
>> >crtc);
>>
>> - return drm_atomic_helper_check_plane_state(plane_state,
>> crtc_state,
>> -
>> DRM_PLANE_HELPER_NO_SCALING,
>> -
>> DRM_PLANE_HELPER_NO_SCALING,
>> - false, true);
>> + ret = drm_atomic_helper_check_plane_state(plane_state,
>> crtc_state,
>> +
>> DRM_PLANE_HELPER_NO_SCALING,
>> +
>> DRM_PLANE_HELPER_NO_SCALING,
>> + false, true);
>> + if (ret || !plane_state->visible)
>> >>>
>> >>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
>> >>> have to verify that !fb always implies !visible :-)
>> >>>
>> + return ret;
>> +
>> + pitch = crtc_state->mode.hdisplay *
>> + plane_state->fb->format->cpp[0];
>> >>>
>> >>> This holds on a single line.
>> >>>
>> + if (plane_state->fb->pitches[0] != pitch) {
>> + dev_err(plane->dev->dev,
>> + "Invalid pitch: fb and crtc widths must be the
>> same");
>> >>>
>> >>> I'd turn this into a dev_dbg(), printing error messages to the kernel
>> >>> log in response to user-triggered conditions is a bit too verbose and
>> >>> could flood the log.
>> >>>
>> >>> Wouldn't it be best to catch this issue when creating the framebuffer ?
>> >>
>> >> Yeah this should be verified at addfb time. We try to validate as early as
>> >> possible.
>> >> -Daniel
>> >>
>> >
>> > Sounds sensible. From what I can tell fb_create is the proper callback
>> > to implement this at addfb time. Will give this a try.
>> >
>> > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
>> > should be moved to addfb there too?
>>
>> But you don't know the crtc width when creating the framebuffer.
>
> Hm right this is a different check. What we could check in fb_create for
> both is that the logical fb size matches exactly the pitch. That's not
> sufficient criteria, but it will at least catch some of them already.
>
> But yeah we'd need both here.
After validating width of framebuffer against pitch, the only thing we
need to check here is that the width matches. From what I can tell,
least for mxsfb, this should be covered by
drm_atomic_helper_check_plane_state's can_position parameter set to
false.
So I think in my case I can get away by only checking the framebuffer.
--
Stefan
> -Daniel
>
>>
>> Tomi
>>
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 08/09/2020 10:55, Stefan Agner wrote:
> > On 2020-09-07 20:18, Daniel Vetter wrote:
> >> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> >>> Hi Stefan,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> The lcdif IP does not support a framebuffer pitch (stride) other than
> the CRTC width. Check for equality and reject the state otherwise.
>
> This prevents a distorted picture when using 640x800 and running the
> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> >>>
> >>> s/tires/tries/
> >>>
> leads at that particular resolution to width != stride. Currently
> Mesa has no fallback behavior, but rejecting this configuration allows
> userspace to handle the issue correctly.
> >>>
> >>> I'm increasingly impressed by how featureful this IP core is :-)
> >>>
> Signed-off-by: Stefan Agner
> ---
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index b721b8b262ce..79aa14027f91 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct
> drm_plane *plane,
> {
> struct mxsfb_drm_private *mxsfb =
> to_mxsfb_drm_private(plane->dev);
> struct drm_crtc_state *crtc_state;
> +unsigned int pitch;
> +int ret;
>
> crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> >crtc);
>
> -return drm_atomic_helper_check_plane_state(plane_state,
> crtc_state,
> -
> DRM_PLANE_HELPER_NO_SCALING,
> -
> DRM_PLANE_HELPER_NO_SCALING,
> - false, true);
> +ret = drm_atomic_helper_check_plane_state(plane_state,
> crtc_state,
> +
> DRM_PLANE_HELPER_NO_SCALING,
> +
> DRM_PLANE_HELPER_NO_SCALING,
> + false, true);
> +if (ret || !plane_state->visible)
> >>>
> >>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> >>> have to verify that !fb always implies !visible :-)
> >>>
> +return ret;
> +
> +pitch = crtc_state->mode.hdisplay *
> +plane_state->fb->format->cpp[0];
> >>>
> >>> This holds on a single line.
> >>>
> +if (plane_state->fb->pitches[0] != pitch) {
> +dev_err(plane->dev->dev,
> +"Invalid pitch: fb and crtc widths must be the
> same");
> >>>
> >>> I'd turn this into a dev_dbg(), printing error messages to the kernel
> >>> log in response to user-triggered conditions is a bit too verbose and
> >>> could flood the log.
> >>>
> >>> Wouldn't it be best to catch this issue when creating the framebuffer ?
> >>
> >> Yeah this should be verified at addfb time. We try to validate as early as
> >> possible.
> >> -Daniel
> >>
> >
> > Sounds sensible. From what I can tell fb_create is the proper callback
> > to implement this at addfb time. Will give this a try.
> >
> > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> > should be moved to addfb there too?
>
> But you don't know the crtc width when creating the framebuffer.
Hm right this is a different check. What we could check in fb_create for
both is that the logical fb size matches exactly the pitch. That's not
sufficient criteria, but it will at least catch some of them already.
But yeah we'd need both here.
-Daniel
>
> Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
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
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On 2020-09-07 20:18, Daniel Vetter wrote:
> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
>> Hi Stefan,
>>
>> Thank you for the patch.
>>
>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
>> > The lcdif IP does not support a framebuffer pitch (stride) other than
>> > the CRTC width. Check for equality and reject the state otherwise.
>> >
>> > This prevents a distorted picture when using 640x800 and running the
>> > Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>>
>> s/tires/tries/
>>
>> > leads at that particular resolution to width != stride. Currently
>> > Mesa has no fallback behavior, but rejecting this configuration allows
>> > userspace to handle the issue correctly.
>>
>> I'm increasingly impressed by how featureful this IP core is :-)
>>
>> > Signed-off-by: Stefan Agner
>> > ---
>> > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
>> > 1 file changed, 18 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> > index b721b8b262ce..79aa14027f91 100644
>> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> > @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
>> > *plane,
>> > {
>> >struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
>> >struct drm_crtc_state *crtc_state;
>> > + unsigned int pitch;
>> > + int ret;
>> >
>> >crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
>> > >crtc);
>> >
>> > - return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> > - DRM_PLANE_HELPER_NO_SCALING,
>> > - DRM_PLANE_HELPER_NO_SCALING,
>> > - false, true);
>> > + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> > +DRM_PLANE_HELPER_NO_SCALING,
>> > +DRM_PLANE_HELPER_NO_SCALING,
>> > +false, true);
>> > + if (ret || !plane_state->visible)
>>
>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
>> have to verify that !fb always implies !visible :-)
>>
>> > + return ret;
>> > +
>> > + pitch = crtc_state->mode.hdisplay *
>> > + plane_state->fb->format->cpp[0];
>>
>> This holds on a single line.
>>
>> > + if (plane_state->fb->pitches[0] != pitch) {
>> > + dev_err(plane->dev->dev,
>> > + "Invalid pitch: fb and crtc widths must be the same");
>>
>> I'd turn this into a dev_dbg(), printing error messages to the kernel
>> log in response to user-triggered conditions is a bit too verbose and
>> could flood the log.
>>
>> Wouldn't it be best to catch this issue when creating the framebuffer ?
>
> Yeah this should be verified at addfb time. We try to validate as early as
> possible.
> -Daniel
>
Sounds sensible. From what I can tell fb_create is the proper callback
to implement this at addfb time. Will give this a try.
FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
should be moved to addfb there too?
[added Jyri/Tomi]
--
Stefan
>>
>> > + return -EINVAL;
>> > + }
>> > +
>> > + return 0;
>> > }
>> >
>> > static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: mxsfb: check framebuffer pitch
Am 2020-09-07 16:03, schrieb Stefan Agner: The lcdif IP does not support a framebuffer pitch (stride) other than the CRTC width. This may be true for some SOCs, but not for all of them. I used to have this working flawlessly on the imx8mq like so: http://projects.dpa.li/git/?p=linux.git;a=commitdiff;h=5a7baa8ba7f1f30139cfcd0f9e13b3773f3605ff That in turn was based on some patches which weren't fully upstreamed yet at the time, and was inspired by another patch I had seen here: https://lore.kernel.org/linux-arm-kernel/20190722174853.GA31795@bogus/t/#m7ac218480eb1d827ff65f82f2e03a5a84c94a5e0 I had almost forgotten about this already, since I currently don't need it anymore, and I was waiting for the patches it was based on to get taken up. But if anyone else needs this, feel free to do whatever you want with it. I could also take another look at this and submit some patches if this is preferred, but I'm quiet busy right now, so that could take a while. In the meantime and for other SOCs, I think checking for an unsupported pitch is still needed regardless. I'm not against this patch, this is just a note to make sure it's known that and how this can be done on the imx8mq. Regards, Daniel Abrecht ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: mxsfb: check framebuffer pitch
On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> > The lcdif IP does not support a framebuffer pitch (stride) other than
> > the CRTC width. Check for equality and reject the state otherwise.
> >
> > This prevents a distorted picture when using 640x800 and running the
> > Mesa graphics stack. Mesa tires to use a cache aligned stride, which
>
> s/tires/tries/
>
> > leads at that particular resolution to width != stride. Currently
> > Mesa has no fallback behavior, but rejecting this configuration allows
> > userspace to handle the issue correctly.
>
> I'm increasingly impressed by how featureful this IP core is :-)
>
> > Signed-off-by: Stefan Agner
> > ---
> > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > index b721b8b262ce..79aa14027f91 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
> > *plane,
> > {
> > struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
> > struct drm_crtc_state *crtc_state;
> > + unsigned int pitch;
> > + int ret;
> >
> > crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> >>crtc);
> >
> > - return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > - DRM_PLANE_HELPER_NO_SCALING,
> > - DRM_PLANE_HELPER_NO_SCALING,
> > - false, true);
> > + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + false, true);
> > + if (ret || !plane_state->visible)
>
> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> have to verify that !fb always implies !visible :-)
>
> > + return ret;
> > +
> > + pitch = crtc_state->mode.hdisplay *
> > + plane_state->fb->format->cpp[0];
>
> This holds on a single line.
>
> > + if (plane_state->fb->pitches[0] != pitch) {
> > + dev_err(plane->dev->dev,
> > + "Invalid pitch: fb and crtc widths must be the same");
>
> I'd turn this into a dev_dbg(), printing error messages to the kernel
> log in response to user-triggered conditions is a bit too verbose and
> could flood the log.
>
> Wouldn't it be best to catch this issue when creating the framebuffer ?
Yeah this should be verified at addfb time. We try to validate as early as
possible.
-Daniel
>
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > }
> >
> > static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
>
> --
> Regards,
>
> Laurent Pinchart
--
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
Re: [PATCH] drm: mxsfb: check framebuffer pitch
Hi Stefan,
Thank you for the patch.
On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> The lcdif IP does not support a framebuffer pitch (stride) other than
> the CRTC width. Check for equality and reject the state otherwise.
>
> This prevents a distorted picture when using 640x800 and running the
> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
s/tires/tries/
> leads at that particular resolution to width != stride. Currently
> Mesa has no fallback behavior, but rejecting this configuration allows
> userspace to handle the issue correctly.
I'm increasingly impressed by how featureful this IP core is :-)
> Signed-off-by: Stefan Agner
> ---
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index b721b8b262ce..79aa14027f91 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
> *plane,
> {
> struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
> struct drm_crtc_state *crtc_state;
> + unsigned int pitch;
> + int ret;
>
> crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> >crtc);
>
> - return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> -DRM_PLANE_HELPER_NO_SCALING,
> -DRM_PLANE_HELPER_NO_SCALING,
> -false, true);
> + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> + DRM_PLANE_HELPER_NO_SCALING,
> + DRM_PLANE_HELPER_NO_SCALING,
> + false, true);
> + if (ret || !plane_state->visible)
Would it be more explict to check for !plane_state->fb ? Otherwise I'll
have to verify that !fb always implies !visible :-)
> + return ret;
> +
> + pitch = crtc_state->mode.hdisplay *
> + plane_state->fb->format->cpp[0];
This holds on a single line.
> + if (plane_state->fb->pitches[0] != pitch) {
> + dev_err(plane->dev->dev,
> + "Invalid pitch: fb and crtc widths must be the same");
I'd turn this into a dev_dbg(), printing error messages to the kernel
log in response to user-triggered conditions is a bit too verbose and
could flood the log.
Wouldn't it be best to catch this issue when creating the framebuffer ?
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: mxsfb: check framebuffer pitch
The lcdif IP does not support a framebuffer pitch (stride) other than
the CRTC width. Check for equality and reject the state otherwise.
This prevents a distorted picture when using 640x800 and running the
Mesa graphics stack. Mesa tires to use a cache aligned stride, which
leads at that particular resolution to width != stride. Currently
Mesa has no fallback behavior, but rejecting this configuration allows
userspace to handle the issue correctly.
Signed-off-by: Stefan Agner
---
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index b721b8b262ce..79aa14027f91 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane
*plane,
{
struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
struct drm_crtc_state *crtc_state;
+ unsigned int pitch;
+ int ret;
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
>crtc);
- return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
- DRM_PLANE_HELPER_NO_SCALING,
- DRM_PLANE_HELPER_NO_SCALING,
- false, true);
+ ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, true);
+ if (ret || !plane_state->visible)
+ return ret;
+
+ pitch = crtc_state->mode.hdisplay *
+ plane_state->fb->format->cpp[0];
+ if (plane_state->fb->pitches[0] != pitch) {
+ dev_err(plane->dev->dev,
+ "Invalid pitch: fb and crtc widths must be the same");
+ return -EINVAL;
+ }
+
+ return 0;
}
static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
