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

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:55:29PM +0000, Deepak Singh Rawat wrote:
> > 
> > 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.

Yeah, if some of the future helpers need to e.g. allocate memory, then we
need to do any necessary prep steps from ->atomic_check.

But this isn't just prep, it clears stuff, so giving it a name that
indicates better what it does seems like a good idea to me. Just make it
clear that this should be called from ->atomic_check callbacks.

> > 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.

Documentation is the worst documentation. Function names, or just outright
implemented behaviour is much better (e.g. runtime checks). For full
damage if there's no clip rect I think the iterator should implement that
for us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch