On Thu, Sep 06, 2018 at 09:44:25PM +0000, Deepak Singh Rawat wrote:
>
> > > #include <drm/drm_damage_helper.h>
> > >
> > > /**
> > > @@ -75,6 +76,11 @@
> > > * While kernel does not error for overlapped damage clips, it is
> > discouraged.
> > > */
> > >
> > > +static int convert_fixed_to_32(int fixed)
> > > +{
> > > + return ((fixed >> 15) & 1) + (fixed >> 16);
> > > +}
> > > +
> > > /**
> > > * drm_plane_enable_fb_damage_clips - enables plane fb damage clips
> > property
> > > * @plane: plane on which to enable damage clips property
> > > @@ -90,3 +96,90 @@ void drm_plane_enable_fb_damage_clips(struct
> > drm_plane *plane)
> > > 0);
> > > }
> > > EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
> > > +
> > > +/**
> > > + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
> > > + * @iter: The iterator to initialize.
> > > + * @old_state: old plane state for validation.
> > > + * @new_state: plane state from which to iterate the damage clips.
> > > + *
> > > + * Initialize an iterator that clip framebuffer damage in plane
> > fb_damage_clips
> > > + * blob to plane src clip. The iterator returns full plane src in case needing
> > > + * full update e.g. during full modeset.
> > > + *
> > > + * With this helper iterator, drivers which enabled fb_damage_clips
> > property can
> > > + * iterate over the damage clips that falls inside plane src during plane
> > > + * update.
> > > + *
> > > + * Returns: 0 on success and negative error code on error. If an error code
> > is
> > > + * returned then it means the plane state shouldn't update with attached
> > fb.
> > > + */
> > > +int
> > > +drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> > > + const struct drm_plane_state *old_state,
> > > + const struct drm_plane_state *new_state)
> > > +{
> > > + if (!new_state || !new_state->crtc || !new_state->fb)
> > > + return -EINVAL;
> > > +
> > > + if (!new_state->visible)
> > > + return -EINVAL;
> >
> > Can't we handle this by iterating 0 damage rects instead? Would make the
> > code a bit cleaner I think.
>
> Agreed.
>
> >
> > > +
> > > + memset(iter, 0, sizeof(*iter));
> > > + iter->clips = drm_plane_get_damage_clips(new_state);
> > > + iter->num_clips = drm_plane_get_damage_clips_count(new_state);
> > > +
> > > + if (!iter->clips)
> > > + iter->full_update = true;
> > > +
> > > + if (!drm_rect_equals(&new_state->src, &old_state->src))
> > > + iter->full_update = true;
> > > +
> > > + iter->plane_src.x1 = convert_fixed_to_32(new_state->src.x1);
> > > + iter->plane_src.y1 = convert_fixed_to_32(new_state->src.y1);
> > > + iter->plane_src.x2 = convert_fixed_to_32(new_state->src.x2);
> > > + iter->plane_src.y2 = convert_fixed_to_32(new_state->src.y2);
> >
> > I think you want to clip with the clipped rectangles here, not with the
> > ones userspace provides.
>
> new_state->src.x1 is the clipped one, clipped in the function
> drm_atomic_helper_check_plane_state. Or am I missing something ?
You're right, I misremembered. This looks correct. Would be good to
explain this in the kerneldoc, and that you must call
drm_atomic_helper_check_plane_state() first before calling this.
Cheers, Daniel
> > Also I think you're rounding is wrong here - I think you need to round
> > down for x/y1, and round up for x/y2 to make sure you catch all the
> > pixels?
> >
> > Unit tests for this, in the form of a drm selftest (like we have for
> > drm_mm.c already, but for kms helpers) would be perfect. Much easier to
> > review a testcase than do bitmath in my head :-)
>
> Sure I will work on a unit test case for this scenario and work on round up
> of plane_src.
>
> >
> > > +
> > > + if (iter->full_update) {
> > > + iter->clips = 0;
> > > + iter->curr_clip = 0;
> > > + iter->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
> > > +
> > > +/**
> > > + * drm_atomic_helper_damage_iter_next - advance the damage iterator
> > > + * @iter: The iterator to advance.
> > > + * @rect: Return a rectangle in fb coordinate clipped to plane src.
> > > + *
> > > + * Returns: true if the output is valid, false if we've reached the end of
> > the
> > > + * rectangle list.
> > > + */
> > > +bool
> > > +drm_atomic_helper_damage_iter_next(struct
> > drm_atomic_helper_damage_iter *iter,
> > > + struct drm_rect *rect)
> > > +{
> > > + bool ret = false;
> > > +
> > > + if (iter->full_update) {
> > > + *rect = iter->plane_src;
> > > + iter->full_update = false;
> > > + return true;
> > > + }
> > > +
> > > + while (iter->curr_clip < iter->num_clips) {
> > > + *rect = iter->clips[iter->curr_clip];
> > > + iter->curr_clip++;
> > > +
> > > + if (drm_rect_intersect(rect, &iter->plane_src)) {
> > > + ret = true;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > > diff --git a/include/drm/drm_damage_helper.h
> > b/include/drm/drm_damage_helper.h
> > > index 217694e9168c..f1282b459a4f 100644
> > > --- a/include/drm/drm_damage_helper.h
> > > +++ b/include/drm/drm_damage_helper.h
> > > @@ -32,6 +32,19 @@
> > > #ifndef DRM_DAMAGE_HELPER_H_
> > > #define DRM_DAMAGE_HELPER_H_
> > >
> > > +/**
> > > + * struct drm_atomic_helper_damage_iter - damage clip iterator
> > > + *
> > > + * This iterator tracks state needed to walk the list of damage clips.
> > > + */
> > > +struct drm_atomic_helper_damage_iter {
> > > + const struct drm_rect *clips;
> > > + struct drm_rect plane_src;
> > > + uint32_t num_clips;
> > > + uint32_t curr_clip;
> > > + bool full_update;
> > > +};
> > > +
> > > /**
> > > * drm_plane_get_damage_clips_count - returns damage clips count
> > > * @state: Plane state
> > > @@ -59,5 +72,12 @@ drm_plane_get_damage_clips(const struct
> > drm_plane_state *state)
> > > }
> > >
> > > 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,
> > > + const struct drm_plane_state *old_state,
> > > + const struct drm_plane_state *new_state);
> >
> > I think a for_each_damage macro would be sweet on top of these here. But
> > easy to add later on.
> > -Daniel
> >
> > > +bool
> > > +drm_atomic_helper_damage_iter_next(struct
> > drm_atomic_helper_damage_iter *iter,
> > > + struct drm_rect *rect);
> > >
> > > #endif
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff
> > wll.ch&data=02%7C01%7Cdrawat%40vmware.com%7C117739774a7341f
> > d8c0208d613cd83d5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C
> > 636718170756943488&sdata=tNK2zTqJrq8U5U4W96wp49FZ8LAB9fl2WV
> > UixbOSWsU%3D&reserved=0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
