RE: [RFC 3/3] drm: Add helper to validate damage during modeset_check

[Deepak Singh Rawat]

> 
> On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> > This patch adds a helper which should be called by driver which enable
> > damage (by calling drm_plane_enable_damage_clips) from atomic_check
> > hook. This helper for now set the damage to NULL for the planes on crtc
> > which need full modeset.
> >
> > The driver also need to check for other crtc properties which can
> > affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> > properties which affect damage can be handled in damage iterator.
> >
> > Signed-off-by: Deepak Rawat <drawat@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 47
> +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 355b514..23f44ab 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> >  	return true;
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > +
> > +/**
> > + * drm_atomic_helper_check_damage - validate state object for damage
> changes
> > + * @dev: DRM device
> > + * @state: the driver state object
> > + *
> > + * Check the state object if damage is required or not. In case damage is
> not
> > + * required e.g. need modeset, the damage blob is set to NULL.
> 
> Why is that needed?
> 
> I can imagine that drivers unconditionally upload everything for a
> modeset, and hence don't need special damage tracking. But for that it's
> imo better to have a clear_damage() helper.

Don't we need a generic helper which all drivers can use to see if anything
in drm_atomic_state will result in full update? My intention for calling that
function from atomic_check hook was because state access can
return -EDEADLK.

I think for now having drm_damage_helper_clear_damage helper and 
calling it from atomic_check seems better option. In future once I have
clear idea, a generic function can be done.


> 
> But even for modesets (e.g. resolution changes) I'd expect that virtual
> drivers don't want to upload unecessary amounts of data. Manual upload
> hw drivers probably need to upload everything, because the panel will have
> forgotten all the old data once power cycled.

AFAIK current vmwgfx will do full upload for resolution change.

> 
> And if you think this is really the right thing, then we need to rename
> the function to tell what it does, e.g.
> 
> drm_damage_helper_clear_damage_on_modeset()
> 
> drm_damage_helper because I think we should stuff this all into
> drm_damage_helper.c, see previous patch.
> 
> But I think a
> 
> drm_damage_helper_clear_damage(crtc_state)
> 
> which you can use in your crtc->atomic_check function like
> 
> crtc_atomic_check(state)
> {
> 	if (drm_atomic_crtc_needs_modeset(state))
> 		drm_damage_helper_clear_damage(state);
> }
> 
> is more flexible and useful for drivers. There might be other cases where
> clearing damage is the right thing to do. Also, there's the question of
> whether no damage clips == full damage or not, so maybe we should call
> this helper full_damage() instead.

In my opinion if via proper documentation it is conveyed that no damage
means full-update the clear_damage makes sense.

> -Daniel
> 
> > + *
> > + * NOTE: This helper is not called by core. Those driver which enable
> damage
> > + * using drm_plane_enable_damage_clips should call this from their
> @atomic_check
> > + * hook.
> > + */
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > +				   struct drm_atomic_state *state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_plane *plane;
> > +	struct drm_crtc_state *crtc_state;
> > +	unsigned plane_mask;
> > +	int i;
> > +
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +		if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > +			continue;
> > +
> > +		plane_mask = crtc_state->plane_mask;
> > +		plane_mask |= crtc->state->plane_mask;
> > +
> > +		drm_for_each_plane_mask(plane, dev, plane_mask) {
> > +			struct drm_plane_state *plane_state =
> > +				drm_atomic_get_plane_state(state, plane);
> > +
> > +			if (IS_ERR(plane_state))
> > +				return PTR_ERR(plane_state);
> > +
> > +			if (plane_state->damage_clips) {
> > +				drm_property_blob_put(plane_state-
> >damage_clips);
> > +				plane_state->damage_clips = NULL;
> > +				plane_state->num_clips = 0;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_damage);
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h
> > index ebd4b66..b12335c 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -224,6 +224,8 @@ drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> >  bool
> >  drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> >  				   struct drm_rect *rect);
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > +				   struct drm_atomic_state *state);
> >
> >  /**
> >   * drm_atomic_crtc_for_each_plane - iterate over planes currently
> attached to CRTC
> > --
> > 2.7.4
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=70sQYwsOsrWAPt1SdaK8gDC1Fr3KTUpJLw
> 008Coi8rY&s=wCKqHwASJhJBVWlirJDaofj0YDju_QHCPE4uZw8W3Mg&e=