Re: [PATCH v3 02/18] drm: Add a new helper to validate damage during atomic_check

[Sinclair Yeh <syeh@vmware.com>]

On Wed, Oct 10, 2018 at 05:16:41PM -0700, Deepak Rawat wrote:
> This helper function makes sure that damage from plane state is
> discarded for full modeset cycle. For some reason, which makes damage
> irrelevant, driver might want to do a full plane update for e.g. full
> modeset. Such cases must be checked here.
> 
> Cc: ville.syrjala@linux.intel.com
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  3 +++
>  drivers/gpu/drm/drm_damage_helper.c | 38 +++++++++++++++++++++++++++++
>  include/drm/drm_damage_helper.h     |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 35395577ca86..41dabb817c57 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -32,6 +32,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"
> @@ -828,6 +829,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  
>  		drm_atomic_helper_plane_changed(state, old_plane_state, new_plane_state, plane);
>  
> +		drm_atomic_helper_check_plane_damage(state, new_plane_state);
> +
>  		if (!funcs || !funcs->atomic_check)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 8dc906a489a9..c130514bbb21 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -29,6 +29,7 @@
>   *
>   **************************************************************************/
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_damage_helper.h>
>  
>  /**
> @@ -81,3 +82,40 @@ void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
>  				   0);
>  }
>  EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
> +
> +/**
> + * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check.
> + * @state: The driver state object.
> + * @plane_state: Plane state for which to verify damage.
> + *
> + * This helper function makes sure that damage from plane state is discarded
> + * for full modeset cycle. For some reason, which makes damage irrelevant,
          remove "cycle" ^     ^Reads weird.. maybe...

	 "If there are more reasons a driver would want to do a full plane
	  update rather than processing individual damage regions, then
	  those cases should be taken care of here."

> + * driver might want to do a full plane update for e.g. full modeset. Such
> + * cases must be checked here. Note that NULL &drm_plane_state.fb_damage_clips
                                            ^ drm_plane_state.fb_damage_clips == NULL

> + * in plane state means that full update should happen. It also ensue helper
                                                                   ^ ensure(?) that the
> + * iterator to return &drm_plane_state.src as damage.
               ^s/to/will


> + *
> + * Currently this helper discard damage during full modeset only. This is
                            ^ discards

The rest of the paragraph reads kind of strange, and pretty much repeats
what has already been said, so maybe just remove it?

> + * because, presently only vmwgfx exposes damage interface, which need full
> + * plane update during full modeset only. As more driver add damage support,
> + * should any state change need full plane update, must be added here.
> + */
> +void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
> +					  struct drm_plane_state *plane_state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (plane_state->crtc) {
> +		crtc_state = drm_atomic_get_new_crtc_state(state,
> +							   plane_state->crtc);
> +
> +		if (WARN_ON(!crtc_state))
> +			return;
> +
> +		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +			drm_property_blob_put(plane_state->fb_damage_clips);
> +			plane_state->fb_damage_clips = NULL;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index 4947c614fff9..59584cbf3d40 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -35,5 +35,7 @@
>  #include <drm/drm_atomic_helper.h>
>  
>  void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
> +void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
> +					  struct drm_plane_state *plane_state);
>  
>  #endif
> -- 
> 2.17.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel