| Message ID | 20180905233901.2321-4-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plane update with damage | expand |
On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote: > Plane damage is irrelevant when full modeset happens so clear the damage > blob property(If set by user-space). With damage blob cleared damage > helper iterator will return full plane src as damage clip. > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > include/drm/drm_damage_helper.h | 10 ++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index be83e2763c18..e06d2d5d582f 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -31,6 +31,7 @@ > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_writeback.h> > +#include <drm/drm_damage_helper.h> > #include <linux/dma-fence.h> > > #include "drm_crtc_helper_internal.h" > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, > return; > > crtc_state->planes_changed = true; > + > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > + drm_plane_clear_damage(plane_state); I'm not 100% sure this is the best place to put this. I'm also wondering whether we should clear damage when moving planes between crtc on top of this. But I guess vmwgfx doesn't allow that, we can figure this out when the first driver with moveable planes adds damage support. > } > } > > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > index f1282b459a4f..1f988f7fdd72 100644 > --- a/include/drm/drm_damage_helper.h > +++ b/include/drm/drm_damage_helper.h > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) > state->fb_damage_clips->data : NULL); > } > > +/** > + * drm_plane_clear_damage - clears damage blob in a plane state > + * @state: Plane state A bit more kerneldoc would be good. Maybe explain how that impacts the damage iterator - you get full damage after calling this, which is a bit confusing for a function called clear_damage. So definitely worth explaining. With the kerneldoc beefed up: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + */ > +static inline void drm_plane_clear_damage(struct drm_plane_state *state) > +{ > + drm_property_blob_put(state->fb_damage_clips); > + state->fb_damage_clips = NULL; > +} > + > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); > int > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > -- > 2.17.1 >
On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote: > Plane damage is irrelevant when full modeset happens so clear the damage > blob property(If set by user-space). With damage blob cleared damage > helper iterator will return full plane src as damage clip. > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > include/drm/drm_damage_helper.h | 10 ++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index be83e2763c18..e06d2d5d582f 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -31,6 +31,7 @@ > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_writeback.h> > +#include <drm/drm_damage_helper.h> > #include <linux/dma-fence.h> > > #include "drm_crtc_helper_internal.h" > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, > return; > > crtc_state->planes_changed = true; > + > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > + drm_plane_clear_damage(plane_state); So if we sometimes magically clear it, where do we reinitialize it with the user provided damage for the next update? > } > } > > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > index f1282b459a4f..1f988f7fdd72 100644 > --- a/include/drm/drm_damage_helper.h > +++ b/include/drm/drm_damage_helper.h > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) > state->fb_damage_clips->data : NULL); > } > > +/** > + * drm_plane_clear_damage - clears damage blob in a plane state > + * @state: Plane state > + */ > +static inline void drm_plane_clear_damage(struct drm_plane_state *state) > +{ > + drm_property_blob_put(state->fb_damage_clips); > + state->fb_damage_clips = NULL; Ah. So you're trying to clear out the user provided damage here. That doesn't really seem like sane sematics to me. Either the user provided damage should stick always instead of just sometimes, or it should always be cleared once the update is done (ie. could clear in duplicate_state() in that case). > +} > + > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); > int > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 06, 2018 at 03:02:32PM +0300, Ville Syrjälä wrote: > On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote: > > Plane damage is irrelevant when full modeset happens so clear the damage > > blob property(If set by user-space). With damage blob cleared damage > > helper iterator will return full plane src as damage clip. > > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > include/drm/drm_damage_helper.h | 10 ++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index be83e2763c18..e06d2d5d582f 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -31,6 +31,7 @@ > > #include <drm/drm_crtc_helper.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_writeback.h> > > +#include <drm/drm_damage_helper.h> > > #include <linux/dma-fence.h> > > > > #include "drm_crtc_helper_internal.h" > > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, > > return; > > > > crtc_state->planes_changed = true; > > + > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > > + drm_plane_clear_damage(plane_state); > > So if we sometimes magically clear it, where do we reinitialize it with > the user provided damage for the next update? > > > } > > } > > > > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > > index f1282b459a4f..1f988f7fdd72 100644 > > --- a/include/drm/drm_damage_helper.h > > +++ b/include/drm/drm_damage_helper.h > > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) > > state->fb_damage_clips->data : NULL); > > } > > > > +/** > > + * drm_plane_clear_damage - clears damage blob in a plane state > > + * @state: Plane state > > + */ > > +static inline void drm_plane_clear_damage(struct drm_plane_state *state) > > +{ > > + drm_property_blob_put(state->fb_damage_clips); > > + state->fb_damage_clips = NULL; > > Ah. So you're trying to clear out the user provided damage here. That > doesn't really seem like sane sematics to me. Either the user provided > damage should stick always instead of just sometimes, or it should > always be cleared once the update is done (ie. could clear in > duplicate_state() in that case). Read patch 2 more careful, we always reset damage when duplicating state. So it's one-shot (like fences). But you might also want to ignore damage (and do a full upload) in other cases, like a modeset, or when you move your plane between crtc. This function here is for that. And yes this should be explained in the kerneldoc. And _clear_damage is not a great name for what it's meant to do, that's just what it does. I think in the past I've suggested _full_damage() or something like that. -Daniel > > > +} > > + > > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); > > int > > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > > -- > > 2.17.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 06, 2018 at 04:12:39PM +0200, Daniel Vetter wrote: > On Thu, Sep 06, 2018 at 03:02:32PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote: > > > Plane damage is irrelevant when full modeset happens so clear the damage > > > blob property(If set by user-space). With damage blob cleared damage > > > helper iterator will return full plane src as damage clip. > > > > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > > include/drm/drm_damage_helper.h | 10 ++++++++++ > > > 2 files changed, 14 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index be83e2763c18..e06d2d5d582f 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -31,6 +31,7 @@ > > > #include <drm/drm_crtc_helper.h> > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_writeback.h> > > > +#include <drm/drm_damage_helper.h> > > > #include <linux/dma-fence.h> > > > > > > #include "drm_crtc_helper_internal.h" > > > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, > > > return; > > > > > > crtc_state->planes_changed = true; > > > + > > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > > > + drm_plane_clear_damage(plane_state); > > > > So if we sometimes magically clear it, where do we reinitialize it with > > the user provided damage for the next update? > > > > > } > > > } > > > > > > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > > > index f1282b459a4f..1f988f7fdd72 100644 > > > --- a/include/drm/drm_damage_helper.h > > > +++ b/include/drm/drm_damage_helper.h > > > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) > > > state->fb_damage_clips->data : NULL); > > > } > > > > > > +/** > > > + * drm_plane_clear_damage - clears damage blob in a plane state > > > + * @state: Plane state > > > + */ > > > +static inline void drm_plane_clear_damage(struct drm_plane_state *state) > > > +{ > > > + drm_property_blob_put(state->fb_damage_clips); > > > + state->fb_damage_clips = NULL; > > > > Ah. So you're trying to clear out the user provided damage here. That > > doesn't really seem like sane sematics to me. Either the user provided > > damage should stick always instead of just sometimes, or it should > > always be cleared once the update is done (ie. could clear in > > duplicate_state() in that case). > > Read patch 2 more careful, we always reset damage when duplicating state. > So it's one-shot (like fences). Ah, thanks for pointing it out. Was actually in patch 1 :) > > But you might also want to ignore damage (and do a full upload) in other > cases, like a modeset, or when you move your plane between crtc. This > function here is for that. > > And yes this should be explained in the kerneldoc. And _clear_damage is > not a great name for what it's meant to do, that's just what it does. I > think in the past I've suggested _full_damage() or something like that. > -Daniel > > > > > > +} > > > + > > > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); > > > int > > > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
> > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > include/drm/drm_damage_helper.h | 10 ++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > > index be83e2763c18..e06d2d5d582f 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -31,6 +31,7 @@ > > #include <drm/drm_crtc_helper.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_writeback.h> > > +#include <drm/drm_damage_helper.h> > > #include <linux/dma-fence.h> > > > > #include "drm_crtc_helper_internal.h" > > @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct > drm_atomic_state *state, > > return; > > > > crtc_state->planes_changed = true; > > + > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > > + drm_plane_clear_damage(plane_state); > > I'm not 100% sure this is the best place to put this. I'm also wondering > whether we should clear damage when moving planes between crtc on top > of > this. But I guess vmwgfx doesn't allow that, we can figure this out when > the first driver with moveable planes adds damage support. Yes I agree this is not the best place but it was the one with minimal code change. IMO should have a separate function for clearing damage. > > } > > } > > > > diff --git a/include/drm/drm_damage_helper.h > b/include/drm/drm_damage_helper.h > > index f1282b459a4f..1f988f7fdd72 100644 > > --- a/include/drm/drm_damage_helper.h > > +++ b/include/drm/drm_damage_helper.h > > @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct > drm_plane_state *state) > > state->fb_damage_clips->data : NULL); > > } > > > > +/** > > + * drm_plane_clear_damage - clears damage blob in a plane state > > + * @state: Plane state > > A bit more kerneldoc would be good. Maybe explain how that impacts the > damage iterator - you get full damage after calling this, which is a bit > confusing for a function called clear_damage. So definitely worth > explaining. > > With the kerneldoc beefed up: Agreed. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > + */ > > +static inline void drm_plane_clear_damage(struct drm_plane_state > *state) > > +{ > > + drm_property_blob_put(state->fb_damage_clips); > > + state->fb_damage_clips = NULL; > > +} > > + > > void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); > > int > > drm_atomic_helper_damage_iter_init(struct > drm_atomic_helper_damage_iter *iter, > > -- > > 2.17.1
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index be83e2763c18..e06d2d5d582f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_writeback.h> +#include <drm/drm_damage_helper.h> #include <linux/dma-fence.h> #include "drm_crtc_helper_internal.h" @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, return; crtc_state->planes_changed = true; + + if (drm_atomic_crtc_needs_modeset(crtc_state)) + drm_plane_clear_damage(plane_state); } } diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index f1282b459a4f..1f988f7fdd72 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) state->fb_damage_clips->data : NULL); } +/** + * drm_plane_clear_damage - clears damage blob in a plane state + * @state: Plane state + */ +static inline void drm_plane_clear_damage(struct drm_plane_state *state) +{ + drm_property_blob_put(state->fb_damage_clips); + state->fb_damage_clips = NULL; +} + void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); int drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
Plane damage is irrelevant when full modeset happens so clear the damage blob property(If set by user-space). With damage blob cleared damage helper iterator will return full plane src as damage clip. Signed-off-by: Deepak Rawat <drawat@vmware.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ include/drm/drm_damage_helper.h | 10 ++++++++++ 2 files changed, 14 insertions(+)