Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer
On Tue, Nov 03, 2020 at 06:28:24PM +0200, Ville Syrjälä wrote: > On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote: > > Hi Ville, > > > > On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote: > > > On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > > > > Many drivers reference the crtc->pointer in order to get the current > > > > CRTC > > > > state in their atomic_begin or atomic_flush hooks, which would be the > > > > new > > > > CRTC state in the global atomic state since _swap_state happened when > > > > those > > > > hooks are run. > > > > > > > > Use the drm_atomic_get_new_crtc_state helper to get that state to make > > > > it > > > > more obvious. > > > > > > > > This was made using the coccinelle script below: > > > > > > > > @ crtc_atomic_func @ > > > > identifier helpers; > > > > identifier func; > > > > @@ > > > > > > > > ( > > > > static struct drm_crtc_helper_funcs helpers = { > > > > ..., > > > > .atomic_begin = func, > > > > ..., > > > > }; > > > > | > > > > static struct drm_crtc_helper_funcs helpers = { > > > > ..., > > > > .atomic_flush = func, > > > > ..., > > > > }; > > > > ) > > > > > > > > @@ > > > > identifier crtc_atomic_func.func; > > > > identifier crtc, state; > > > > symbol crtc_state; > > > > expression e; > > > > @@ > > > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > > ... > > > > - struct tegra_dc_state *crtc_state = e; > > > > + struct tegra_dc_state *dc_state = e; > > > > <+... > > > > - crtc_state > > > > + dc_state > > > > ...+> > > > > } > > > > > > > > @@ > > > > identifier crtc_atomic_func.func; > > > > identifier crtc, state; > > > > symbol crtc_state; > > > > expression e; > > > > @@ > > > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > > ... > > > > - struct mtk_crtc_state *crtc_state = e; > > > > + struct mtk_crtc_state *mtk_crtc_state = e; > > > > <+... > > > > - crtc_state > > > > + mtk_crtc_state > > > > ...+> > > > > } > > > > > > These reanames seem a bit out of scpe for this patch. But I guess you > > > needed then to get the rest of the cocci to work on some drivers? > > > > Yeah, those two drivers already had a variable named crtc_state, calling > > container_of on crtc->state > > > > It was cleaner to me to have an intermediate variable storing the result > > of drm_atomic_get_new_crtc_state, but then the most obvious name was > > taken so I had to rename those two variables before doing so. > > > > > The basic idea looks good: > > > Reviewed-by: Ville Syrjälä > > > > > > But I guess up to the individual driver folks to bikeshed the variable > > > naming and whatnot. > > > > > > One thing I spotted is that a few drivers now gained two aliasing crtc > > > state pointers in the function: one with the drm type, the other with > > > a driver specific type. That's something we've outlawed in i915 since > > > it was making life rather confusing. In i915 we now prefer to use only > > > the i915 specific types in most places. > > > > I didn't spot any of those cases, do you have an example of where it > > happened? > > eg. ast: > + struct drm_crtc_state *crtc_state = > drm_atomic_get_new_crtc_state(state, > + > crtc); > struct drm_crtc_state *old_crtc_state = > drm_atomic_get_old_crtc_state(state, > > crtc); > struct ast_private *ast = to_ast_private(crtc->dev); > > - struct ast_crtc_state *ast_crtc_state = > to_ast_crtc_state(crtc->state); > + struct ast_crtc_state *ast_crtc_state = > to_ast_crtc_state(crtc_state); > > So here both 'crtc_state' and 'ast_crtc_state' are basically the same > thing, which can get a bit confusing especially within larger functions > with lots of variables. Ah, that kind of aliasing, ok. So it's purely an issue with ergonomics/convenience? It seems to be a widespread pattern (I know we're using it in vc4 and sun4i, and from those reworks I've seen a number of drivers doing so). I'm not entirely sure how we should fix it through coccinelle too :/ Thanks! Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer
On Thu, Nov 05, 2020 at 05:35:28PM +0100, Maxime Ripard wrote: > On Tue, Nov 03, 2020 at 06:28:24PM +0200, Ville Syrjälä wrote: > > On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote: > > > Hi Ville, > > > > > > On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote: > > > > On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > > > > > Many drivers reference the crtc->pointer in order to get the current > > > > > CRTC > > > > > state in their atomic_begin or atomic_flush hooks, which would be the > > > > > new > > > > > CRTC state in the global atomic state since _swap_state happened when > > > > > those > > > > > hooks are run. > > > > > > > > > > Use the drm_atomic_get_new_crtc_state helper to get that state to > > > > > make it > > > > > more obvious. > > > > > > > > > > This was made using the coccinelle script below: > > > > > > > > > > @ crtc_atomic_func @ > > > > > identifier helpers; > > > > > identifier func; > > > > > @@ > > > > > > > > > > ( > > > > > static struct drm_crtc_helper_funcs helpers = { > > > > > ..., > > > > > .atomic_begin = func, > > > > > ..., > > > > > }; > > > > > | > > > > > static struct drm_crtc_helper_funcs helpers = { > > > > > ..., > > > > > .atomic_flush = func, > > > > > ..., > > > > > }; > > > > > ) > > > > > > > > > > @@ > > > > > identifier crtc_atomic_func.func; > > > > > identifier crtc, state; > > > > > symbol crtc_state; > > > > > expression e; > > > > > @@ > > > > > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > > > ... > > > > > - struct tegra_dc_state *crtc_state = e; > > > > > + struct tegra_dc_state *dc_state = e; > > > > > <+... > > > > > - crtc_state > > > > > + dc_state > > > > > ...+> > > > > > } > > > > > > > > > > @@ > > > > > identifier crtc_atomic_func.func; > > > > > identifier crtc, state; > > > > > symbol crtc_state; > > > > > expression e; > > > > > @@ > > > > > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > > > ... > > > > > - struct mtk_crtc_state *crtc_state = e; > > > > > + struct mtk_crtc_state *mtk_crtc_state = e; > > > > > <+... > > > > > - crtc_state > > > > > + mtk_crtc_state > > > > > ...+> > > > > > } > > > > > > > > These reanames seem a bit out of scpe for this patch. But I guess you > > > > needed then to get the rest of the cocci to work on some drivers? > > > > > > Yeah, those two drivers already had a variable named crtc_state, calling > > > container_of on crtc->state > > > > > > It was cleaner to me to have an intermediate variable storing the result > > > of drm_atomic_get_new_crtc_state, but then the most obvious name was > > > taken so I had to rename those two variables before doing so. > > > > > > > The basic idea looks good: > > > > Reviewed-by: Ville Syrjälä > > > > > > > > But I guess up to the individual driver folks to bikeshed the variable > > > > naming and whatnot. > > > > > > > > One thing I spotted is that a few drivers now gained two aliasing crtc > > > > state pointers in the function: one with the drm type, the other with > > > > a driver specific type. That's something we've outlawed in i915 since > > > > it was making life rather confusing. In i915 we now prefer to use only > > > > the i915 specific types in most places. > > > > > > I didn't spot any of those cases, do you have an example of where it > > > happened? > > > > eg. ast: > > + struct drm_crtc_state *crtc_state = > > drm_atomic_get_new_crtc_state(state, > > + > > crtc); > > struct drm_crtc_state *old_crtc_state = > > drm_atomic_get_old_crtc_state(state, > > > > crtc); > > struct ast_private *ast = to_ast_private(crtc->dev); > > > > - struct ast_crtc_state *ast_crtc_state = > > to_ast_crtc_state(crtc->state); > > + struct ast_crtc_state *ast_crtc_state = > > to_ast_crtc_state(crtc_state); > > > > So here both 'crtc_state' and 'ast_crtc_state' are basically the same > > thing, which can get a bit confusing especially within larger functions > > with lots of variables. > > Ah, that kind of aliasing, ok. So it's purely an issue with > ergonomics/convenience? Yeah, I got fed up with it in i915 because I had a hard time reasoning how the state was being mutated when it was being accesed through multiple pointers in the same function. And didn't help that the variable names were all over the place. > > It seems to be a widespread pattern (I know we're using it in vc4 and > sun4i, and from those reworks I've seen a number of drivers doing so). > I'm not entirely sure how we
Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer
Hi Ville, On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote: > On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > > Many drivers reference the crtc->pointer in order to get the current CRTC > > state in their atomic_begin or atomic_flush hooks, which would be the new > > CRTC state in the global atomic state since _swap_state happened when those > > hooks are run. > > > > Use the drm_atomic_get_new_crtc_state helper to get that state to make it > > more obvious. > > > > This was made using the coccinelle script below: > > > > @ crtc_atomic_func @ > > identifier helpers; > > identifier func; > > @@ > > > > ( > > static struct drm_crtc_helper_funcs helpers = { > > ..., > > .atomic_begin = func, > > ..., > > }; > > | > > static struct drm_crtc_helper_funcs helpers = { > > ..., > > .atomic_flush = func, > > ..., > > }; > > ) > > > > @@ > > identifier crtc_atomic_func.func; > > identifier crtc, state; > > symbol crtc_state; > > expression e; > > @@ > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > ... > > - struct tegra_dc_state *crtc_state = e; > > + struct tegra_dc_state *dc_state = e; > > <+... > > - crtc_state > > + dc_state > > ...+> > > } > > > > @@ > > identifier crtc_atomic_func.func; > > identifier crtc, state; > > symbol crtc_state; > > expression e; > > @@ > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > ... > > - struct mtk_crtc_state *crtc_state = e; > > + struct mtk_crtc_state *mtk_crtc_state = e; > > <+... > > - crtc_state > > + mtk_crtc_state > > ...+> > > } > > These reanames seem a bit out of scpe for this patch. But I guess you > needed then to get the rest of the cocci to work on some drivers? Yeah, those two drivers already had a variable named crtc_state, calling container_of on crtc->state It was cleaner to me to have an intermediate variable storing the result of drm_atomic_get_new_crtc_state, but then the most obvious name was taken so I had to rename those two variables before doing so. > The basic idea looks good: > Reviewed-by: Ville Syrjälä > > But I guess up to the individual driver folks to bikeshed the variable > naming and whatnot. > > One thing I spotted is that a few drivers now gained two aliasing crtc > state pointers in the function: one with the drm type, the other with > a driver specific type. That's something we've outlawed in i915 since > it was making life rather confusing. In i915 we now prefer to use only > the i915 specific types in most places. I didn't spot any of those cases, do you have an example of where it happened? Thanks! Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer
On Tue, Nov 03, 2020 at 05:15:51PM +0100, Maxime Ripard wrote: > Hi Ville, > > On Mon, Nov 02, 2020 at 06:04:06PM +0200, Ville Syrjälä wrote: > > On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > > > Many drivers reference the crtc->pointer in order to get the current CRTC > > > state in their atomic_begin or atomic_flush hooks, which would be the new > > > CRTC state in the global atomic state since _swap_state happened when > > > those > > > hooks are run. > > > > > > Use the drm_atomic_get_new_crtc_state helper to get that state to make it > > > more obvious. > > > > > > This was made using the coccinelle script below: > > > > > > @ crtc_atomic_func @ > > > identifier helpers; > > > identifier func; > > > @@ > > > > > > ( > > > static struct drm_crtc_helper_funcs helpers = { > > > ..., > > > .atomic_begin = func, > > > ..., > > > }; > > > | > > > static struct drm_crtc_helper_funcs helpers = { > > > ..., > > > .atomic_flush = func, > > > ..., > > > }; > > > ) > > > > > > @@ > > > identifier crtc_atomic_func.func; > > > identifier crtc, state; > > > symbol crtc_state; > > > expression e; > > > @@ > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > ... > > > - struct tegra_dc_state *crtc_state = e; > > > + struct tegra_dc_state *dc_state = e; > > > <+... > > > - crtc_state > > > + dc_state > > > ...+> > > > } > > > > > > @@ > > > identifier crtc_atomic_func.func; > > > identifier crtc, state; > > > symbol crtc_state; > > > expression e; > > > @@ > > > > > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > > > ... > > > - struct mtk_crtc_state *crtc_state = e; > > > + struct mtk_crtc_state *mtk_crtc_state = e; > > > <+... > > > - crtc_state > > > + mtk_crtc_state > > > ...+> > > > } > > > > These reanames seem a bit out of scpe for this patch. But I guess you > > needed then to get the rest of the cocci to work on some drivers? > > Yeah, those two drivers already had a variable named crtc_state, calling > container_of on crtc->state > > It was cleaner to me to have an intermediate variable storing the result > of drm_atomic_get_new_crtc_state, but then the most obvious name was > taken so I had to rename those two variables before doing so. > > > The basic idea looks good: > > Reviewed-by: Ville Syrjälä > > > > But I guess up to the individual driver folks to bikeshed the variable > > naming and whatnot. > > > > One thing I spotted is that a few drivers now gained two aliasing crtc > > state pointers in the function: one with the drm type, the other with > > a driver specific type. That's something we've outlawed in i915 since > > it was making life rather confusing. In i915 we now prefer to use only > > the i915 specific types in most places. > > I didn't spot any of those cases, do you have an example of where it > happened? eg. ast: + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, + crtc); struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); struct ast_private *ast = to_ast_private(crtc->dev); - struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state); + struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state); So here both 'crtc_state' and 'ast_crtc_state' are basically the same thing, which can get a bit confusing especially within larger functions with lots of variables. In i915 we would just have struct intel_crtc_state *crtc_state = whatever; and any access to the base class would be done via crtc_state->base. -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm: Use state helper instead of CRTC state pointer
On Mon, Nov 02, 2020 at 02:38:33PM +0100, Maxime Ripard wrote: > Many drivers reference the crtc->pointer in order to get the current CRTC > state in their atomic_begin or atomic_flush hooks, which would be the new > CRTC state in the global atomic state since _swap_state happened when those > hooks are run. > > Use the drm_atomic_get_new_crtc_state helper to get that state to make it > more obvious. > > This was made using the coccinelle script below: > > @ crtc_atomic_func @ > identifier helpers; > identifier func; > @@ > > ( > static struct drm_crtc_helper_funcs helpers = { > ..., > .atomic_begin = func, > ..., > }; > | > static struct drm_crtc_helper_funcs helpers = { > ..., > .atomic_flush = func, > ..., > }; > ) > > @@ > identifier crtc_atomic_func.func; > identifier crtc, state; > symbol crtc_state; > expression e; > @@ > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > ... > - struct tegra_dc_state *crtc_state = e; > + struct tegra_dc_state *dc_state = e; > <+... > - crtc_state > + dc_state > ...+> > } > > @@ > identifier crtc_atomic_func.func; > identifier crtc, state; > symbol crtc_state; > expression e; > @@ > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > ... > - struct mtk_crtc_state *crtc_state = e; > + struct mtk_crtc_state *mtk_crtc_state = e; > <+... > - crtc_state > + mtk_crtc_state > ...+> > } These reanames seem a bit out of scpe for this patch. But I guess you needed then to get the rest of the cocci to work on some drivers? The basic idea looks good: Reviewed-by: Ville Syrjälä But I guess up to the individual driver folks to bikeshed the variable naming and whatnot. One thing I spotted is that a few drivers now gained two aliasing crtc state pointers in the function: one with the drm type, the other with a driver specific type. That's something we've outlawed in i915 since it was making life rather confusing. In i915 we now prefer to use only the i915 specific types in most places. > > @ replaces_new_state @ > identifier crtc_atomic_func.func; > identifier crtc, state, crtc_state; > @@ > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > ... > - struct drm_crtc_state *crtc_state = crtc->state; > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > crtc); > ... > } > > @@ > identifier crtc_atomic_func.func; > identifier crtc, state, crtc_state; > @@ > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > crtc); > ... > - crtc->state > + crtc_state > ... > } > > @ adds_new_state @ > identifier crtc_atomic_func.func; > identifier crtc, state; > @@ > > func(struct drm_crtc *crtc, struct drm_atomic_state *state) { > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > crtc); > ... > - crtc->state > + crtc_state > ... > } > > @ include depends on adds_new_state || replaces_new_state @ > @@ > > #include > > @ no_include depends on !include && (adds_new_state || replaces_new_state) @ > @@ > > + #include > #include > > Suggested-by: Ville Syrjälä > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 4 +++- > drivers/gpu/drm/armada/armada_crtc.c | 8 ++-- > drivers/gpu/drm/ast/ast_mode.c | 4 +++- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c| 7 +-- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 15 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 -- > drivers/gpu/drm/tegra/dc.c | 8 +--- > drivers/gpu/drm/virtio/virtgpu_display.c | 4 +++- > 8 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > index df0b9eeb8933..4b485eb512e2 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c > @@ -387,10 +387,12 @@ static void > komeda_crtc_atomic_flush(struct drm_crtc *crtc, >struct drm_atomic_state *state) > { > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > + crtc); > struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state, > crtc); > /* commit with modeset will be handled in enable/disable */ > - if (drm_atomic_crtc_needs_modeset(crtc->state)) > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > return; > > komeda_crtc_do_flush(crtc, old); > diff --git a/drivers/gpu/drm/armada/armada_crtc.c > b/drivers/gpu/drm/armada/armada_crtc.c > index ca643f4e2064..3ebcf5a52c8b 100644 > ---
[PATCH 2/3] drm: Use state helper instead of CRTC state pointer
Many drivers reference the crtc->pointer in order to get the current CRTC state in their atomic_begin or atomic_flush hooks, which would be the new CRTC state in the global atomic state since _swap_state happened when those hooks are run. Use the drm_atomic_get_new_crtc_state helper to get that state to make it more obvious. This was made using the coccinelle script below: @ crtc_atomic_func @ identifier helpers; identifier func; @@ ( static struct drm_crtc_helper_funcs helpers = { ..., .atomic_begin = func, ..., }; | static struct drm_crtc_helper_funcs helpers = { ..., .atomic_flush = func, ..., }; ) @@ identifier crtc_atomic_func.func; identifier crtc, state; symbol crtc_state; expression e; @@ func(struct drm_crtc *crtc, struct drm_atomic_state *state) { ... - struct tegra_dc_state *crtc_state = e; + struct tegra_dc_state *dc_state = e; <+... - crtc_state + dc_state ...+> } @@ identifier crtc_atomic_func.func; identifier crtc, state; symbol crtc_state; expression e; @@ func(struct drm_crtc *crtc, struct drm_atomic_state *state) { ... - struct mtk_crtc_state *crtc_state = e; + struct mtk_crtc_state *mtk_crtc_state = e; <+... - crtc_state + mtk_crtc_state ...+> } @ replaces_new_state @ identifier crtc_atomic_func.func; identifier crtc, state, crtc_state; @@ func(struct drm_crtc *crtc, struct drm_atomic_state *state) { ... - struct drm_crtc_state *crtc_state = crtc->state; + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); ... } @@ identifier crtc_atomic_func.func; identifier crtc, state, crtc_state; @@ func(struct drm_crtc *crtc, struct drm_atomic_state *state) { struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); ... - crtc->state + crtc_state ... } @ adds_new_state @ identifier crtc_atomic_func.func; identifier crtc, state; @@ func(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); ... - crtc->state + crtc_state ... } @ include depends on adds_new_state || replaces_new_state @ @@ #include @ no_include depends on !include && (adds_new_state || replaces_new_state) @ @@ + #include #include Suggested-by: Ville Syrjälä Signed-off-by: Maxime Ripard --- drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 4 +++- drivers/gpu/drm/armada/armada_crtc.c | 8 ++-- drivers/gpu/drm/ast/ast_mode.c | 4 +++- drivers/gpu/drm/ingenic/ingenic-drm-drv.c| 7 +-- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 15 +-- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 -- drivers/gpu/drm/tegra/dc.c | 8 +--- drivers/gpu/drm/virtio/virtgpu_display.c | 4 +++- 8 files changed, 38 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index df0b9eeb8933..4b485eb512e2 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -387,10 +387,12 @@ static void komeda_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, + crtc); struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state, crtc); /* commit with modeset will be handled in enable/disable */ - if (drm_atomic_crtc_needs_modeset(crtc->state)) + if (drm_atomic_crtc_needs_modeset(crtc_state)) return; komeda_crtc_do_flush(crtc, old); diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index ca643f4e2064..3ebcf5a52c8b 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -431,11 +431,13 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc *crtc, static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, + crtc); struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc); DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name); - if (crtc->state->color_mgmt_changed) + if (crtc_state->color_mgmt_changed) armada_drm_update_gamma(crtc); dcrtc->regs_idx = 0; @@ -445,6 +447,8 @@ static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc, static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,