On Thursday, September 21, 2023 3:59 PM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > ---
> > > 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
malidp_crtc_atomic_check_gamma() calls it too.
> 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.
Up to you. No strong opinion.
Regards,
Liu Ying
>
> 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
> >
