[PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob

[Deepak Singh Rawat]

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

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