* [PATCH v2] drm: Fix crtc color management when doing suspend/resume
@ 2018-08-28 15:33 Alexandru Gheorghe
2018-08-28 15:46 ` Ville Syrjälä
2018-08-28 15:48 ` Brian Starkey
0 siblings, 2 replies; 7+ messages in thread
From: Alexandru Gheorghe @ 2018-08-28 15:33 UTC (permalink / raw)
To: seanpaul, maarten.lankhorst, liviu.dudau, brian.starkey, airlied,
dri-devel, gustavo, ayan.halder
Cc: nd, 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 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);
--
2.18.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 15:33 [PATCH v2] drm: Fix crtc color management when doing suspend/resume Alexandru Gheorghe
@ 2018-08-28 15:46 ` Ville Syrjälä
2018-08-28 15:58 ` Alexandru-Cosmin Gheorghe
2018-08-28 15:48 ` Brian Starkey
1 sibling, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2018-08-28 15:46 UTC (permalink / raw)
To: Alexandru Gheorghe
Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
> } 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);
> --
> 2.18.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 15:33 [PATCH v2] drm: Fix crtc color management when doing suspend/resume Alexandru Gheorghe
2018-08-28 15:46 ` Ville Syrjälä
@ 2018-08-28 15:48 ` Brian Starkey
2018-08-28 16:08 ` Alexandru-Cosmin Gheorghe
1 sibling, 1 reply; 7+ messages in thread
From: Brian Starkey @ 2018-08-28 15:48 UTC (permalink / raw)
To: Alexandru Gheorghe
Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
> } 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.
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 15:46 ` Ville Syrjälä
@ 2018-08-28 15:58 ` Alexandru-Cosmin Gheorghe
2018-08-28 16:37 ` Ville Syrjälä
0 siblings, 1 reply; 7+ messages in thread
From: Alexandru-Cosmin Gheorghe @ 2018-08-28 15:58 UTC (permalink / raw)
To: Ville Syrjälä
Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
> > } 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);
> > --
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 15:48 ` Brian Starkey
@ 2018-08-28 16:08 ` Alexandru-Cosmin Gheorghe
2018-08-28 16:13 ` Brian Starkey
0 siblings, 1 reply; 7+ messages in thread
From: Alexandru-Cosmin Gheorghe @ 2018-08-28 16:08 UTC (permalink / raw)
To: Brian Starkey; +Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
> > } 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.
>
> 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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 16:08 ` Alexandru-Cosmin Gheorghe
@ 2018-08-28 16:13 ` Brian Starkey
0 siblings, 0 replies; 7+ messages in thread
From: Brian Starkey @ 2018-08-28 16:13 UTC (permalink / raw)
To: Alexandru-Cosmin Gheorghe
Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
>> > } 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm: Fix crtc color management when doing suspend/resume
2018-08-28 15:58 ` Alexandru-Cosmin Gheorghe
@ 2018-08-28 16:37 ` Ville Syrjälä
0 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2018-08-28 16:37 UTC (permalink / raw)
To: Alexandru-Cosmin Gheorghe
Cc: nd, airlied, liviu.dudau, dri-devel, seanpaul, ayan.halder
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;
> > > } 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);
> > > --
> > > 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
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-28 16:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-28 15:33 [PATCH v2] drm: Fix crtc color management when doing suspend/resume Alexandru Gheorghe
2018-08-28 15:46 ` Ville Syrjälä
2018-08-28 15:58 ` Alexandru-Cosmin Gheorghe
2018-08-28 16:37 ` Ville Syrjälä
2018-08-28 15:48 ` Brian Starkey
2018-08-28 16:08 ` Alexandru-Cosmin Gheorghe
2018-08-28 16:13 ` Brian Starkey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.