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@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; } 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);
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.
Do all current drivers actually call drm_atomic_helper_check_modeset() for every commit?
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@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;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;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;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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 28, 2018 at 06:46:07PM +0300, Ville Syrjälä wrote:
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.
Do all current drivers actually call drm_atomic_helper_check_modeset() for every commit?
Yes, all drivers that use color_mgmt_changed either call directly drm_atomic_helper_check_modeset or they use drm_atomic_helper_check which calls drm_atomic_helper_check_modeset.
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@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;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;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;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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel
On Tue, Aug 28, 2018 at 04:58:42PM +0100, Alexandru-Cosmin Gheorghe wrote:
On Tue, Aug 28, 2018 at 06:46:07PM +0300, Ville Syrjälä wrote:
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.
Do all current drivers actually call drm_atomic_helper_check_modeset() for every commit?
Yes, all drivers that use color_mgmt_changed either call directly drm_atomic_helper_check_modeset or they use drm_atomic_helper_check which calls drm_atomic_helper_check_modeset.
Awesome.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
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@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;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;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;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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel
-- Cheers, Alex G
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@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;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;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.
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
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@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;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;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.
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
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@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;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;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
dri-devel@lists.freedesktop.org
-
Alexandru Gheorghe -
Alexandru-Cosmin Gheorghe
-
Brian Starkey -
Ville Syrjälä