| Message ID | 20180828153320.4725-1-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: Fix crtc color management when doing suspend/resume | expand |
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
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 >
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
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 > >
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
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
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);
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(-)