[PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 07:34 +0000 schrieb Ying Liu:
> Hi,
> 
> On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach at pengutronix.de> wrote:
> > Force a modeset if the new FB has a different format than the
> > currently active one. While it might be possible to change between
> > compatible formats without a full modeset as the format control is
> > also supposed to be double buffered, the colorspace conversion is
> > not, so when the CSC changes we need a modeset.
> > 
> > For now just always force a modeset when the FB format changes to
> > properly reconfigure all parts of the device for the new format.
> > 
> > Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
> >  2 files changed, 37 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > index 18de2f17e249..b74f0cf1e240 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > @@ -30,9 +30,25 @@
> >  #include "lcdif_drv.h"
> >  #include "lcdif_regs.h"
> > 
> > +static int lcdif_atomic_check(struct drm_device *dev,
> > +				struct drm_atomic_state *state)
> 
> " checkpatch.pl --strict" complains:
> CHECK: Alignment should match open parenthesis
> #31: FILE: drivers/gpu/drm/mxsfb/lcdif_drv.c:34:
> +static int lcdif_atomic_check(struct drm_device *dev,
> +                               struct drm_atomic_state *state)
> 
Right, I'll fix that.

> > +{
> > +	int ret;
> > +
> > +	ret = drm_atomic_helper_check(dev, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Check modeset again in case crtc_state->mode_changed is
> > +	 * updated in plane's ->atomic_check callback.
> > +	 */
> > +	return drm_atomic_helper_check_modeset(dev, state);
> 
> This additional check looks fine, but it's done for every commit.
> Is it ok to move it to lcdif_plane_atomic_check() where mode_changed
> is set for those commits in question?

Potentially yes, as we only have a single plane in the LCDIF, but it
would be a deviation of how every other DRM driver is implementing this
check. If there are multiple planes than this call must happen after
all of them checked the state, so this is the most logical place to
have this check. Doing this check on every commit doesn't seem to hurt
other drivers, so I'm not really keen on doing things differently here.

Regards,
Lucas
 
> 
> Regards,
> Liu Ying
> 
> > +}
> > +
> >  static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
> >  	.fb_create		= drm_gem_fb_create,
> > -	.atomic_check		= drm_atomic_helper_check,
> > +	.atomic_check		= lcdif_atomic_check,
> >  	.atomic_commit		= drm_atomic_helper_commit,
> >  };
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 3ebf55d06027..8167604bd3f8 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -647,18 +647,32 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs
> > = {
> >  static int lcdif_plane_atomic_check(struct drm_plane *plane,
> >  				    struct drm_atomic_state *state)
> >  {
> > -	struct drm_plane_state *plane_state =
> > drm_atomic_get_new_plane_state(state,
> > -
> > plane);
> > +	struct drm_plane_state *new_state =
> > drm_atomic_get_new_plane_state(state,
> > +
> > plane);
> > +	struct drm_plane_state *old_state =
> > drm_atomic_get_old_plane_state(state,
> > +
> > plane);
> >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
> >  	struct drm_crtc_state *crtc_state;
> > +	int ret;
> > +
> > +	/* always okay to disable the plane */
> > +	if (!new_state->fb)
> > +		return 0;
> > 
> >  	crtc_state = drm_atomic_get_new_crtc_state(state,
> >  						   &lcdif->crtc);
> > 
> > -	return drm_atomic_helper_check_plane_state(plane_state,
> > crtc_state,
> > -						   DRM_PLANE_NO_SCALING,
> > -						   DRM_PLANE_NO_SCALING,
> > -						   false, true);
> > +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  false, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (old_state->fb && new_state->fb->format != old_state->fb->format)
> > +		crtc_state->mode_changed = true;
> > +
> > +	return 0;
> >  }
> > 
> >  static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
> > --
> > 2.39.2
>