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 <ste...@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel