* [PATCH] drm: mxsfb: check framebuffer pitch @ 2020-09-07 16:03 Stefan Agner 2020-09-07 16:17 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Stefan Agner @ 2020-09-07 16:03 UTC (permalink / raw) To: marex, stefan Cc: kernel, airlied, festevam, s.hauer, linux-kernel, dri-devel, laurent.pinchart, daniel, shawnguo, linux-arm-kernel, linux-imx 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 <stefan@agner.ch> --- 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, &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-07 16:03 [PATCH] drm: mxsfb: check framebuffer pitch Stefan Agner @ 2020-09-07 16:17 ` Laurent Pinchart 2020-09-07 18:18 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Laurent Pinchart @ 2020-09-07 16:17 UTC (permalink / raw) To: Stefan Agner Cc: marex, daniel, airlied, festevam, s.hauer, linux-kernel, dri-devel, linux-imx, kernel, shawnguo, linux-arm-kernel 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 <stefan@agner.ch> > --- > 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, > &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-07 16:17 ` Laurent Pinchart @ 2020-09-07 18:18 ` Daniel Vetter 2020-09-08 7:55 ` Stefan Agner 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2020-09-07 18:18 UTC (permalink / raw) To: Laurent Pinchart Cc: marex, kernel, dri-devel, airlied, festevam, s.hauer, linux-kernel, Stefan Agner, linux-imx, daniel, shawnguo, linux-arm-kernel 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 <stefan@agner.ch> > > --- > > 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, > > &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-07 18:18 ` Daniel Vetter @ 2020-09-08 7:55 ` Stefan Agner 2020-09-08 8:18 ` Tomi Valkeinen 0 siblings, 1 reply; 12+ messages in thread From: Stefan Agner @ 2020-09-08 7:55 UTC (permalink / raw) To: Laurent Pinchart, jsarha, tomi.valkeinen Cc: marex, daniel, airlied, festevam, s.hauer, linux-kernel, dri-devel, linux-imx, kernel, shawnguo, linux-arm-kernel 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 <stefan@agner.ch> >> > --- >> > 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, >> > &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 7:55 ` Stefan Agner @ 2020-09-08 8:18 ` Tomi Valkeinen 2020-09-08 8:48 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Tomi Valkeinen @ 2020-09-08 8:18 UTC (permalink / raw) To: Stefan Agner, Laurent Pinchart, jsarha Cc: marex, daniel, airlied, festevam, s.hauer, linux-kernel, dri-devel, linux-imx, kernel, shawnguo, linux-arm-kernel 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 <stefan@agner.ch> >>>> --- >>>> 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, >>>> &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 8:18 ` Tomi Valkeinen @ 2020-09-08 8:48 ` Daniel Vetter 2020-09-08 12:07 ` Laurent Pinchart 2020-09-08 12:07 ` Stefan Agner 0 siblings, 2 replies; 12+ messages in thread From: Daniel Vetter @ 2020-09-08 8:48 UTC (permalink / raw) To: Tomi Valkeinen Cc: marex, Stefan Agner, kernel, dri-devel, airlied, festevam, s.hauer, linux-kernel, jsarha, Laurent Pinchart, daniel, shawnguo, linux-arm-kernel, linux-imx 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 <stefan@agner.ch> > >>>> --- > >>>> 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, > >>>> &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 8:48 ` Daniel Vetter @ 2020-09-08 12:07 ` Laurent Pinchart 2020-09-08 12:07 ` Stefan Agner 1 sibling, 0 replies; 12+ messages in thread From: Laurent Pinchart @ 2020-09-08 12:07 UTC (permalink / raw) To: Daniel Vetter Cc: marex, dri-devel, airlied, festevam, s.hauer, linux-kernel, jsarha, Tomi Valkeinen, Stefan Agner, kernel, shawnguo, linux-arm-kernel, linux-imx 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 <stefan@agner.ch> > > >>>> --- > > >>>> 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, > > >>>> &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 8:48 ` Daniel Vetter 2020-09-08 12:07 ` Laurent Pinchart @ 2020-09-08 12:07 ` Stefan Agner 2020-09-08 12:29 ` Daniel Vetter 1 sibling, 1 reply; 12+ messages in thread From: Stefan Agner @ 2020-09-08 12:07 UTC (permalink / raw) To: Tomi Valkeinen Cc: marex, kernel, dri-devel, airlied, festevam, s.hauer, linux-kernel, jsarha, Laurent Pinchart, daniel, shawnguo, linux-arm-kernel, linux-imx 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 <stefan@agner.ch> >> >>>> --- >> >>>> 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, >> >>>> &mxsfb->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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 12:07 ` Stefan Agner @ 2020-09-08 12:29 ` Daniel Vetter 2020-09-08 12:33 ` Laurent Pinchart 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2020-09-08 12:29 UTC (permalink / raw) To: Stefan Agner Cc: Marek Vasut, dri-devel, Dave Airlie, Fabio Estevam, Sascha Hauer, Linux Kernel Mailing List, Jyri Sarha, Tomi Valkeinen, Laurent Pinchart, Sascha Hauer, Shawn Guo, Linux ARM, dl-linux-imx On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner <stefan@agner.ch> 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 <stefan@agner.ch> > >> >>>> --- > >> >>>> 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, > >> >>>> &mxsfb->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 another parameter for hw like yours or tilcdc. Naming is a bit tricky, maybe require_matching_fb or src_must_match_fb or something like that. > So I think in my case I can get away by only checking the framebuffer. You still need both I think. -Daniel > > -- > Stefan > > > > -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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 12:29 ` Daniel Vetter @ 2020-09-08 12:33 ` Laurent Pinchart 2020-09-08 12:49 ` Ville Syrjälä 2020-09-08 12:49 ` Stefan Agner 0 siblings, 2 replies; 12+ messages in thread From: Laurent Pinchart @ 2020-09-08 12:33 UTC (permalink / raw) To: Daniel Vetter Cc: Marek Vasut, dri-devel, Dave Airlie, Fabio Estevam, Sascha Hauer, Linux Kernel Mailing List, Jyri Sarha, Tomi Valkeinen, Stefan Agner, Sascha Hauer, Shawn Guo, Linux ARM, dl-linux-imx On Tue, Sep 08, 2020 at 02:29:02PM +0200, Daniel Vetter wrote: > On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner <stefan@agner.ch> 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 <stefan@agner.ch> > >>>>>>> --- > >>>>>>> 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, > >>>>>>> &mxsfb->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 another parameter for hw > like yours or tilcdc. Naming is a bit tricky, maybe > require_matching_fb or src_must_match_fb or something like that. Can we turn those parameters into flags ? false, true, false is hard to read. > > So I think in my case I can get away by only checking the framebuffer. > > You still need both I think. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 12:33 ` Laurent Pinchart @ 2020-09-08 12:49 ` Ville Syrjälä 2020-09-08 12:49 ` Stefan Agner 1 sibling, 0 replies; 12+ messages in thread From: Ville Syrjälä @ 2020-09-08 12:49 UTC (permalink / raw) To: Laurent Pinchart Cc: Marek Vasut, Sascha Hauer, Dave Airlie, Sascha Hauer, Linux Kernel Mailing List, dri-devel, Tomi Valkeinen, Jyri Sarha, Daniel Vetter, Shawn Guo, Linux ARM, dl-linux-imx 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 <stefan@agner.ch> 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 <stefan@agner.ch> > > >>>>>>> --- > > >>>>>>> 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, > > >>>>>>> &mxsfb->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 another parameter for hw > > like yours or tilcdc. Naming is a bit tricky, maybe > > require_matching_fb or src_must_match_fb or something like that. > > Can we turn those parameters into flags ? false, true, false is hard to > read. Even the flags approach doesn't really scale past some point. Is there a particularly convincing reason for stuffing yet another check into this function as opposed to just introducing a separate function? I prefer clear single purpose functions over swiss army knives. > > > > So I think in my case I can get away by only checking the framebuffer. > > > > You still need both I think. > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm: mxsfb: check framebuffer pitch 2020-09-08 12:33 ` Laurent Pinchart 2020-09-08 12:49 ` Ville Syrjälä @ 2020-09-08 12:49 ` Stefan Agner 1 sibling, 0 replies; 12+ messages in thread From: Stefan Agner @ 2020-09-08 12:49 UTC (permalink / raw) To: Laurent Pinchart Cc: Marek Vasut, Sascha Hauer, dri-devel, Dave Airlie, Fabio Estevam, Sascha Hauer, Linux Kernel Mailing List, Jyri Sarha, Tomi Valkeinen, dl-linux-imx, Daniel Vetter, Shawn Guo, Linux ARM 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 <stefan@agner.ch> 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 <stefan@agner.ch> >> >>>>>>> --- >> >>>>>>> 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, >> >>>>>>> &mxsfb->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 another parameter for hw >> like yours or tilcdc. Naming is a bit tricky, maybe >> require_matching_fb or src_must_match_fb or something like that. > > Can we turn those parameters into flags ? false, true, false is hard to > read. > Since it must match, in this case, it would be false, true, true, obviously ;-) I guess this would mean to convert the two existing boolean parameters to flags first, and then introduce a new flag handling fb size vs. CRTC src. Hm, this gets all a bit more involved. It is actually not the issue at hand (in my case the fb width does match the CRTC). Not sure if that case is actually a problem in real world? I can give this a shot still, if preferred. But I would do it independently of the framebuffer pitch validation. -- Stefan >> > So I think in my case I can get away by only checking the framebuffer. >> >> You still need both I think. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-08 12:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-07 16:03 [PATCH] drm: mxsfb: check framebuffer pitch Stefan Agner 2020-09-07 16:17 ` Laurent Pinchart 2020-09-07 18:18 ` Daniel Vetter 2020-09-08 7:55 ` Stefan Agner 2020-09-08 8:18 ` Tomi Valkeinen 2020-09-08 8:48 ` Daniel Vetter 2020-09-08 12:07 ` Laurent Pinchart 2020-09-08 12:07 ` Stefan Agner 2020-09-08 12:29 ` Daniel Vetter 2020-09-08 12:33 ` Laurent Pinchart 2020-09-08 12:49 ` Ville Syrjälä 2020-09-08 12:49 ` Stefan Agner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).