>
> 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=
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
