diff mbox series

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

Message ID 20230920103126.2759601-6-l.stach@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series imx-lcdif modeset changes | expand

Commit Message

Lucas Stach Sept. 20, 2023, 10:31 a.m. UTC
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@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(-)

Comments

Marek Vasut Sept. 20, 2023, 5:33 p.m. UTC | #1
On 9/20/23 12:31, Lucas Stach 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@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)
> +{
> +	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);
> +}
> +
>   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,

Reviewed-by: Marek Vasut <marex@denx.de>
Liu Ying Sept. 21, 2023, 7:34 a.m. UTC | #2
Hi,

On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@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@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)

> +{
> +	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?

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
Lucas Stach Sept. 21, 2023, 7:59 a.m. UTC | #3
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@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@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
>
Liu Ying Sept. 21, 2023, 8:30 a.m. UTC | #4
On Thursday, September 21, 2023 3:59 PM Lucas Stach <l.stach@pengutronix.de> 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@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@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

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
> >
diff mbox series

Patch

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)
+{
+	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);
+}
+
 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,