[PATCH v2] drm: Fix crtc color management when doing suspend/resume

[Brian Starkey]

On Tue, Aug 28, 2018 at 05:08:51PM +0100, Alexandru-Cosmin Gheorghe wrote:
>On Tue, Aug 28, 2018 at 04:48:45PM +0100, Brian Starkey wrote:
>> Hi Alex,
>>
>> On Tue, Aug 28, 2018 at 04:33:20PM +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 problem 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 saved state has color_mgmt_changed set to 0.
>> >
>> >Fix that by setting color_mgmt_changed to true in
>> >drm_atomic_helper_check_modeset when at least one of gamma_lut,
>> >degamma_lut, ctm is different between the new and the old crtc state.
>> >
>> >Also, this makes unnecessary the setting of color_mgmt_changed in
>> >places where gamma_lut/degamma_lut/ctm are set in the new crtc_state.
>> >
>> >Changes since v2:
>> > - Instead of setting color_mgmt_changed in commit_duplicated_set
>> >   just set it in check_modeset and clean up other place where it was
>> >   set, suggested by Maarten Lankhorst.
>> >
>> >Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe at arm.com>
>> >---
>> >drivers/gpu/drm/drm_atomic.c        | 12 +++---------
>> >drivers/gpu/drm/drm_atomic_helper.c |  8 +++++++-
>> >drivers/gpu/drm/drm_fb_helper.c     |  1 -
>> >3 files changed, 10 insertions(+), 11 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >index d0478abc01bd..9bcada3c299e 100644
>> >--- a/drivers/gpu/drm/drm_atomic.c
>> >+++ b/drivers/gpu/drm/drm_atomic.c
>> >@@ -554,29 +554,23 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>> >		drm_property_blob_put(mode);
>> >		return ret;
>> >	} else if (property == config->degamma_lut_property) {
>> >-		ret = drm_atomic_replace_property_blob_from_id(dev,
>> >+		return drm_atomic_replace_property_blob_from_id(dev,
>> >					&state->degamma_lut,
>> >					val,
>> >					-1, sizeof(struct drm_color_lut),
>> >					&replaced);
>> >-		state->color_mgmt_changed |= replaced;
>> >-		return ret;
>> >	} else if (property == config->ctm_property) {
>> >-		ret = drm_atomic_replace_property_blob_from_id(dev,
>> >+		return drm_atomic_replace_property_blob_from_id(dev,
>> >					&state->ctm,
>> >					val,
>> >					sizeof(struct drm_color_ctm), -1,
>> >					&replaced);
>> >-		state->color_mgmt_changed |= replaced;
>> >-		return ret;
>> >	} else if (property == config->gamma_lut_property) {
>> >-		ret = drm_atomic_replace_property_blob_from_id(dev,
>> >+		return drm_atomic_replace_property_blob_from_id(dev,
>> >					&state->gamma_lut,
>> >					val,
>> >					-1, sizeof(struct drm_color_lut),
>> >					&replaced);
>> >-		state->color_mgmt_changed |= replaced;
>> >-		return ret;
>>
>> Looks like 'replaced' is now unused, so you can remove it.
>
>It's needed as an argument for
>drm_atomic_replace_property_blob_from_id, and the prototype of the
>function makes sense to me.

Silly me, I thought drm_atomic_replace_property_blob_from_id would
check it against NULL and skip, but I see it doesn't, and that'll
teach me to not trust my memory :-)

Sorry for the noise,
-Brian

>
>>
>> Cheers,
>> -Brian
>>
>> >	} else if (property == config->prop_out_fence_ptr) {
>> >		s32 __user *fence_ptr = u64_to_user_ptr(val);
>> >
>> >diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> >index 2c23a48482da..fe22e1ad468a 100644
>> >--- a/drivers/gpu/drm/drm_atomic_helper.c
>> >+++ b/drivers/gpu/drm/drm_atomic_helper.c
>> >@@ -611,6 +611,13 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>> >
>> >			return -EINVAL;
>> >		}
>> >+		if (new_crtc_state->degamma_lut != old_crtc_state->degamma_lut ||
>> >+		    new_crtc_state->ctm != old_crtc_state->ctm ||
>> >+		    new_crtc_state->gamma_lut != old_crtc_state->gamma_lut) {
>> >+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] color management changed\n",
>> >+					 crtc->base.id, crtc->name);
>> >+			new_crtc_state->color_mgmt_changed = true;
>> >+		}
>> >	}
>> >
>> >	ret = handle_conflicting_encoders(state, false);
>> >@@ -3947,7 +3954,6 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>> >	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
>> >	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
>> >	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
>> >-	crtc_state->color_mgmt_changed |= replaced;
>> >
>> >	ret = drm_atomic_commit(state);
>> >
>> >diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> >index 4b0dd20bccb8..8541e95a5410 100644
>> >--- a/drivers/gpu/drm/drm_fb_helper.c
>> >+++ b/drivers/gpu/drm/drm_fb_helper.c
>> >@@ -1442,7 +1442,6 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>> >		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
>> >		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
>> >						      gamma_lut);
>> >-		crtc_state->color_mgmt_changed |= replaced;
>> >	}
>> >
>> >	ret = drm_atomic_commit(state);
>> >--
>> >2.18.0
>> >
>
>-- 
>Cheers,
>Alex G