| Message ID | 1522885748-67122-4-git-send-email-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vmware.com> > --- > 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 >
> > 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@vmware.com> > > --- > > 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=
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@vmware.com> > > > --- > > > 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
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. + * + * 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
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@vmware.com> --- drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 49 insertions(+)