Re: [PATCH 3/9] drm/plane-helper: Add drm_plane_helper_check_state()

On Tue, Jul 26, 2016 at 05:33:29PM +0100, Chris Wilson wrote:

On Tue, Jul 26, 2016 at 07:06:58PM +0300, ville.syrjala@linux.intel.com wrote:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Add a version of drm_plane_helper_check_update() which takes a plane state instead of having the caller pass in everything.

And to reduce code duplication, let's reimplement drm_plane_helper_check_update() in terms of the new function, by having a tempororary plane state on the stack.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/drm_plane_helper.c | 136 ++++++++++++++++++++++++++++--------- include/drm/drm_plane_helper.h | 5 ++ 2 files changed, 110 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 16c4a7bd7465..1124eb827671 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -108,14 +108,9 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, }

/**

• • drm_plane_helper_check_update() - Check plane update for validity
• • @plane: plane object to update
• • @crtc: owning CRTC of owning plane
• • @fb: framebuffer to flip onto plane
• • @src: source coordinates in 16.16 fixed point
• • @dest: integer destination coordinates

• • drm_plane_helper_check_state() - Check plane state for validity
• • @state: plane state to check
  • @clip: integer clipping coordinates

• • @rotation: plane rotation
  • @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

@@ -123,8 +118,6 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,

•            only be false for primary planes.
  

• @can_update_disabled: can the plane be updated while the crtc

•                   is disabled?
  

• • @visible: output parameter indicating whether plane is still visible after

• •       clipping
    

  • Checks that a desired plane update is valid. Drivers that provide
  • their own plane handling rather than helper-provided implementations may

@@ -134,29 +127,38 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,

• RETURNS:
• Zero if update appears valid, error code on failure

*/ -int drm_plane_helper_check_update(struct drm_plane *plane,

• 		  struct drm_crtc *crtc,
  

• 		  struct drm_framebuffer *fb,
  

• 		  struct drm_rect *src,
  

• 		  struct drm_rect *dest,
  

• 		  const struct drm_rect *clip,
  

• 		  unsigned int rotation,
  

• 		  int min_scale,
  

• 		  int max_scale,
  

• 		  bool can_position,
  

• 		  bool can_update_disabled,
  

• 		  bool *visible)
  

+int drm_plane_helper_check_state(struct drm_plane_state *state,

• 		 const struct drm_rect *clip,
  

• 		 int min_scale,
  

• 		 int max_scale,
  

• 		 bool can_position,
  

• 		 bool can_update_disabled)
  

{

• struct drm_crtc *crtc = state->crtc;

• struct drm_framebuffer *fb = state->fb;

• struct drm_rect *src = &state->src;

• struct drm_rect *dst = &state->dst;

• unsigned int rotation = state->rotation; int hscale, vscale;

• src->x1 = state->src_x;

• src->y1 = state->src_y;

• src->x2 = state->src_x + state->src_w;

• src->y2 = state->src_y + state->src_h;

• dst->x1 = state->crtc_x;

• dst->y1 = state->crtc_y;

• dst->x2 = state->crtc_x + state->crtc_w;

• dst->y2 = state->crtc_y + state->crtc_h;

It's a little surprising to me that the check writes fields into the drm_plane_state. Care to at least mention the side-effects in the kernel doc?

That's a lot of what the atomic "check" functions do. A bit of a bad naming convention that. Not sure who came up with it. Should probably rename them all to "compute" or something.

But I can at leat expand the docs here a bit in the meantime.