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