Re: [PATCH] drm: add a drm_atomic_helper_plane_check_update

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jul 13, 2015 at 08:21:32AM +0800, John Hunter wrote:
> From: Zhao Junwang <zhjwpku@gmail.com>

A bit of text to motivate this would be good - i.e. why is this useful,
how did it blow up in the bochs conversion?

> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |   55 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |    7 +++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 536ae4d..3d94ff8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1336,6 +1336,61 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_swap_state);
>  
>  /**
> + * drm_atomic_helper_plane_check_update
> + * @plane: plane object to update
> + * @state: drm plane state
> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> + * @can_position: is it legal to position the plane such that it
> + *                doesn't cover the entire crtc?  This will generally
> + *                only be false for primary planes.
> + * @can_update_disabled: can the plane be updated while the crtc
> + *                       is disabled?
> + *
> + * Provide a default plane check update handler

It's not a default handler since the signatures don't match. Also I think
we should mention the legacy plane helper function here too. What about

	This provides a helper to be used in a driver's plane atomic_check
	callback. It is the atomic equivalent of
	drm_plane_helper_check_update() and has the exact same semantics,
	except that it looks at the atomit CRTC state in the atomic update
	instead of legacy state directly embedded in struct &drm_crtc.

Also adding a comment to drm_plane_helper_check_update's kerneldoc would
be good, like this:

	Note: When converting over to atomic drivers you need to switch
	over to using drm_atomic_helper_plane_check_update() since only
	that correctly checks atomic state - this function here only looks
	at legacy state and hence will check against stale values in
	atomic updates.

> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_helper_plane_check_update(struct drm_plane *plane,
> +					 struct drm_plane_state *state,
> +					 int min_scale,
> +					 int max_scale,
> +					 bool can_position,
> +					 bool can_update_disabled,
> +					 bool *visible)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_framebuffer *fb = state->fb;
> +
> +	if (!fb)
> +		return 0;
> +
> +	struct drm_rect src = {
> +		.x1 = state->src_x,
> +		.y1 = state->src_y,
> +		.x2 = state->src_x + state->src_w,
> +		.y2 = state->src_y + state->src_h,
> +	};
> +	struct drm_rect dest = {
> +		.x1 = state->crtc_x,
> +		.y1 = state->crtc_y,
> +		.x2 = state->crtc_x + state->crtc_w,
> +		.y2 = state->crtc_y + state->crtc_h,
> +	};
> +	const struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,

crtc->mode is legacy state - we need to look at crtc_state here instead.

> +	};
> +
> +	return drm_plane_helper_check_update(plane, crtc, fb,
> +			&src, &dest, &clip,
> +			min_scale, max_scale,
> +			can_position, can_update_disabled, visible);

This looks at crtc->enabled (which is also legacy state) for
can_update_disabled. If we want to reuse this function we need to check
can_updated_disabled here and it unconditionally to true when calling the
plane helpers.

Cheers, Daniel
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_plane_check_update);
> +
> +/**
>   * drm_atomic_helper_update_plane - Helper for primary plane update using atomic
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index cc1fee8..5305a01 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -64,6 +64,13 @@ void drm_atomic_helper_swap_state(struct drm_device *dev,
>  				  struct drm_atomic_state *state);
>  
>  /* implementations for legacy interfaces */
> +int drm_atomic_helper_plane_check_update(struct drm_plane *plane,
> +					 struct drm_plane_state *state,
> +					 int min_scale,
> +					 int max_scale,
> +					 bool can_position,
> +					 bool can_update_disabled,
> +					 bool *visible);
>  int drm_atomic_helper_update_plane(struct drm_plane *plane,
>  				   struct drm_crtc *crtc,
>  				   struct drm_framebuffer *fb,
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel