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

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:55:29PM +, 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 
> > > ---
> > >  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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Daniel Vetter]

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

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.

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.
-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
http://blog.ffwll.ch