| Message ID | 20180823151117.14834-1-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix crtc color management when doing suspend/resume | expand |
Hi Alex, On Thu, Aug 23, 2018 at 04:11:17PM +0100, Alexandru Gheorghe wrote: >When doing suspend/resume drivers usually use the >drm_atomic_helper_suspend/drm_atomic_helper_resume pair for saving the >state and then re-comitting it. > >The problems is that drm_crtc_state has a bool field called >color_mgmt_changed, which mali-dp and other drivers uses it to detect >if coefficients need to be reprogrammed, but that never happens >because the save state has color_mgmt_changed set to 0. > >Fix that by setting color_mgmt_changed to true if the crtc duplicated >state differs from the reset state. > >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> LGTM. Reviewed-by: Brian Starkey <brian.starkey@arm.com> >--- > drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >index 6dd5036545ec..e88aa62bc822 100644 >--- a/drivers/gpu/drm/drm_atomic_helper.c >+++ b/drivers/gpu/drm/drm_atomic_helper.c >@@ -3196,8 +3196,13 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > for_each_new_plane_in_state(state, plane, new_plane_state, i) > state->planes[i].old_state = plane->state; > >- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) >+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > state->crtcs[i].old_state = crtc->state; >+ new_crtc_state->color_mgmt_changed = >+ new_crtc_state->degamma_lut != crtc->state->degamma_lut || >+ new_crtc_state->ctm != crtc->state->ctm || >+ new_crtc_state->gamma_lut != crtc->state->gamma_lut; >+ } > > for_each_new_connector_in_state(state, connector, new_conn_state, i) > state->connectors[i].old_state = connector->state; >-- >2.18.0 >
Op 23-08-18 om 17:11 schreef Alexandru Gheorghe: > When doing suspend/resume drivers usually use the > drm_atomic_helper_suspend/drm_atomic_helper_resume pair for saving the > state and then re-comitting it. > > The problems is that drm_crtc_state has a bool field called > color_mgmt_changed, which mali-dp and other drivers uses it to detect > if coefficients need to be reprogrammed, but that never happens > because the save state has color_mgmt_changed set to 0. > > Fix that by setting color_mgmt_changed to true if the crtc duplicated > state differs from the reset state. > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 6dd5036545ec..e88aa62bc822 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3196,8 +3196,13 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > for_each_new_plane_in_state(state, plane, new_plane_state, i) > state->planes[i].old_state = plane->state; > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > state->crtcs[i].old_state = crtc->state; > + new_crtc_state->color_mgmt_changed = > + new_crtc_state->degamma_lut != crtc->state->degamma_lut || > + new_crtc_state->ctm != crtc->state->ctm || > + new_crtc_state->gamma_lut != crtc->state->gamma_lut; Don't look at $obj->state please, use for_each_oldnew. But it looks like we should set color_mgmt_changed in drm_atomic_helper_check_planes(), (or check_modeset if we want to know color_mgmt_changed from the planes atomic_check() too. The existing places that set color_mgmt_changed should be removed, because that check is now done in a single place. :) > for_each_new_connector_in_state(state, connector, new_conn_state, i) > state->connectors[i].old_state = connector->state;
Hi, On Fri, Aug 24, 2018 at 09:51:03AM +0200, Maarten Lankhorst wrote: > Op 23-08-18 om 17:11 schreef Alexandru Gheorghe: > > When doing suspend/resume drivers usually use the > > drm_atomic_helper_suspend/drm_atomic_helper_resume pair for saving the > > state and then re-comitting it. > > > > The problems is that drm_crtc_state has a bool field called > > color_mgmt_changed, which mali-dp and other drivers uses it to detect > > if coefficients need to be reprogrammed, but that never happens > > because the save state has color_mgmt_changed set to 0. > > > > Fix that by setting color_mgmt_changed to true if the crtc duplicated > > state differs from the reset state. > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 6dd5036545ec..e88aa62bc822 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3196,8 +3196,13 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > for_each_new_plane_in_state(state, plane, new_plane_state, i) > > state->planes[i].old_state = plane->state; > > > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > > state->crtcs[i].old_state = crtc->state; > > + new_crtc_state->color_mgmt_changed = > > + new_crtc_state->degamma_lut != crtc->state->degamma_lut || > > + new_crtc_state->ctm != crtc->state->ctm || > > + new_crtc_state->gamma_lut != crtc->state->gamma_lut; > Don't look at $obj->state please, use for_each_oldnew. > > But it looks like we should set color_mgmt_changed in drm_atomic_helper_check_planes(), > (or check_modeset if we want to know color_mgmt_changed from the planes atomic_check() too. > The existing places that set color_mgmt_changed should be removed, because that check is now done in a single place. :) drm_atomic_helper_check_modeset was the first candidate for me as well, I think that makes more sense than drm_atomic_helper_check_planes, don't you think ? > > for_each_new_connector_in_state(state, connector, new_conn_state, i) > > state->connectors[i].old_state = connector->state; >
Op 24-08-18 om 10:18 schreef Alexandru-Cosmin Gheorghe: > Hi, > > On Fri, Aug 24, 2018 at 09:51:03AM +0200, Maarten Lankhorst wrote: >> Op 23-08-18 om 17:11 schreef Alexandru Gheorghe: >>> When doing suspend/resume drivers usually use the >>> drm_atomic_helper_suspend/drm_atomic_helper_resume pair for saving the >>> state and then re-comitting it. >>> >>> The problems is that drm_crtc_state has a bool field called >>> color_mgmt_changed, which mali-dp and other drivers uses it to detect >>> if coefficients need to be reprogrammed, but that never happens >>> because the save state has color_mgmt_changed set to 0. >>> >>> Fix that by setting color_mgmt_changed to true if the crtc duplicated >>> state differs from the reset state. >>> >>> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index 6dd5036545ec..e88aa62bc822 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -3196,8 +3196,13 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, >>> for_each_new_plane_in_state(state, plane, new_plane_state, i) >>> state->planes[i].old_state = plane->state; >>> >>> - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) >>> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { >>> state->crtcs[i].old_state = crtc->state; >>> + new_crtc_state->color_mgmt_changed = >>> + new_crtc_state->degamma_lut != crtc->state->degamma_lut || >>> + new_crtc_state->ctm != crtc->state->ctm || >>> + new_crtc_state->gamma_lut != crtc->state->gamma_lut; >> Don't look at $obj->state please, use for_each_oldnew. >> >> But it looks like we should set color_mgmt_changed in drm_atomic_helper_check_planes(), >> (or check_modeset if we want to know color_mgmt_changed from the planes atomic_check() too. >> The existing places that set color_mgmt_changed should be removed, because that check is now done in a single place. :) > drm_atomic_helper_check_modeset was the first candidate for me as > well, I think that makes more sense than > drm_atomic_helper_check_planes, don't you think ? > Either works for me. ~Maarten
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6dd5036545ec..e88aa62bc822 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3196,8 +3196,13 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state; - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { state->crtcs[i].old_state = crtc->state; + new_crtc_state->color_mgmt_changed = + new_crtc_state->degamma_lut != crtc->state->degamma_lut || + new_crtc_state->ctm != crtc->state->ctm || + new_crtc_state->gamma_lut != crtc->state->gamma_lut; + } for_each_new_connector_in_state(state, connector, new_conn_state, i) state->connectors[i].old_state = connector->state;
When doing suspend/resume drivers usually use the drm_atomic_helper_suspend/drm_atomic_helper_resume pair for saving the state and then re-comitting it. The problems is that drm_crtc_state has a bool field called color_mgmt_changed, which mali-dp and other drivers uses it to detect if coefficients need to be reprogrammed, but that never happens because the save state has color_mgmt_changed set to 0. Fix that by setting color_mgmt_changed to true if the crtc duplicated state differs from the reset state. Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> --- drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)