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

From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak Rawat <drawat@vmware.com>
Cc: thellstrom@vmware.com, dri-devel@lists.freedesktop.org,
	linux-graphics-maintainer@vmware.com,
	lukasz.spintzyk@displaylink.com
Subject: Re: [PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob
Date: Thu, 6 Sep 2018 09:51:07 +0200	[thread overview]
Message-ID: <20180906075107.GN4929@phenom.ffwll.local> (raw)
In-Reply-To: <20180905233901.2321-3-drawat@vmware.com>

On Wed, Sep 05, 2018 at 04:38:49PM -0700, Deepak Rawat wrote:
> With fb_damage_clips blob property in drm_plane_state, this patch adds
> helper iterator to traverse the damage clips that lie inside plane src.
> Iterator will return full plane src as damage in case need full plane
> update or damage is not specified.
> 
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>  drivers/gpu/drm/drm_damage_helper.c | 93 +++++++++++++++++++++++++++++
>  include/drm/drm_damage_helper.h     | 20 +++++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 3e7de5afe7f6..7d70f6001889 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -30,6 +30,7 @@
>   **************************************************************************/
>  
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
>  #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.

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

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

> +
> +	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
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel