Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()
On Mon, Aug 08, 2016 at 03:29:24PM +0800, Daniel Kurtz wrote: > Hi Ville, > > Two fixes inline... > > On Wed, Jul 27, 2016 at 1:34 AM, wrote: > > > > From: Ville Syrjälä > > > > 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. > > > > v2: Add a note that the functions modifies the state (Chris) > > > > Cc: Chris Wilson > > Signed-off-by: Ville Syrjälä > > [snip...] > > > +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 *dst, > > + const struct drm_rect *clip, > > + unsigned int rotation, > > + int min_scale, > > + int max_scale, > > + bool can_position, > > + bool can_update_disabled, > > + bool *visible) > > +{ > > + struct drm_plane_state state = { > > + .plane = plane, > > + .crtc = crtc, > > + .fb = fb, > > + .src_x = src->x1, > > + .src_y = src->x2, > > This should be: > src->y1 > > > + .src_w = drm_rect_width(src), > > + .src_h = drm_rect_height(src), > > + .crtc_x = dst->x1, > > + .crtc_y = dst->x2, > > And this should be: > dst->y1 Whoops. Copypaste fail, or something. Thanks for catching those. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()
Hi Ville, Two fixes inline... On Wed, Jul 27, 2016 at 1:34 AM, wrote: > > From: Ville Syrjälä > > 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. > > v2: Add a note that the functions modifies the state (Chris) > > Cc: Chris Wilson > Signed-off-by: Ville Syrjälä [snip...] > +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 *dst, > + const struct drm_rect *clip, > + unsigned int rotation, > + int min_scale, > + int max_scale, > + bool can_position, > + bool can_update_disabled, > + bool *visible) > +{ > + struct drm_plane_state state = { > + .plane = plane, > + .crtc = crtc, > + .fb = fb, > + .src_x = src->x1, > + .src_y = src->x2, This should be: src->y1 > + .src_w = drm_rect_width(src), > + .src_h = drm_rect_height(src), > + .crtc_x = dst->x1, > + .crtc_y = dst->x2, And this should be: dst->y1 Thanks, -Dan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()
On Tue, Jul 26, 2016 at 1:34 PM, wrote: > From: Ville Syrjälä > > 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. > > v2: Add a note that the functions modifies the state (Chris) > > Cc: Chris Wilson > Signed-off-by: Ville Syrjälä Reviewed-by: Sean Paul > --- > drivers/gpu/drm/drm_plane_helper.c | 139 > - > include/drm/drm_plane_helper.h | 5 ++ > 2 files changed, 112 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c > b/drivers/gpu/drm/drm_plane_helper.c > index 16c4a7bd7465..ca02997b1c36 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,10 +118,9 @@ 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 > + * Checks that a desired plane update is valid, and updates various > + * bits of derived state (clipped coordinates etc.). Drivers that provide > * their own plane handling rather than helper-provided implementations may > * still wish to call this function to avoid duplication of error checking > * code. > @@ -134,29 +128,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; > + > if (!fb) { > - *visible = false; > + state->visible = false; > return 0; > } > > /* crtc should only be NULL when disabling (i.e., !fb) */ > if (WARN_ON(!crtc)) { > - *visible = false; > + state->visible = false; > return 0; > } > > @@ -168,20 +171,20 @@ int drm_plane_helper_check_update(struct drm_plane > *plane, > drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); > > /* Check scaling */ > - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); > - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); > + hscale =
[Intel-gfx] [PATCH v2 3/9] drm/plane-helper: Add drm_plane_helper_check_state()
From: Ville Syrjälä 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. v2: Add a note that the functions modifies the state (Chris) Cc: Chris Wilson Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_plane_helper.c | 139 - include/drm/drm_plane_helper.h | 5 ++ 2 files changed, 112 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 16c4a7bd7465..ca02997b1c36 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,10 +118,9 @@ 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 + * Checks that a desired plane update is valid, and updates various + * bits of derived state (clipped coordinates etc.). Drivers that provide * their own plane handling rather than helper-provided implementations may * still wish to call this function to avoid duplication of error checking * code. @@ -134,29 +128,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; + if (!fb) { - *visible = false; + state->visible = false; return 0; } /* crtc should only be NULL when disabling (i.e., !fb) */ if (WARN_ON(!crtc)) { - *visible = false; + state->visible = false; return 0; } @@ -168,20 +171,20 @@ int drm_plane_helper_check_update(struct drm_plane *plane, drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); /* Check scaling */ - hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale); - vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale); + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); if (hscale < 0 || vscale < 0) { DRM_DEBUG_KMS("Invalid scaling of plane\n"); - drm_rect_debug_print("src: ", src, true); -