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@xxxxxxxxxxxxxxx>
>
> >
> > >
> > > 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@xxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
