| Message ID | 1469549224-1860-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -Chris
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.
On Tue, Jul 26, 2016 at 07:42:16PM +0300, Ville Syrjälä wrote: > 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. While you expand docs, pls link the atomic to the legacy version with a cross reference in each. Also I wonder whether it would make sense to put this into the atomic namespace and into drm_atomic_helper.c and call it something like drm_atomic_helper_check_plane_state. For greater visibility and chances of it being used. Finally I think it'd be good to put a reference to this into the plane_helper_funcs->atomic_check kerneldoc. -Daniel
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; + 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 +170,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); - drm_rect_debug_print("dst: ", dest, false); + drm_rect_debug_print("src: ", &state->src, true); + drm_rect_debug_print("dst: ", &state->dst, false); return -ERANGE; } - *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale); + state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); - if (!*visible) + if (!state->visible) /* * Plane isn't visible; some drivers can handle this * so we just return success here. Drivers that can't @@ -191,15 +193,87 @@ int drm_plane_helper_check_update(struct drm_plane *plane, */ return 0; - if (!can_position && !drm_rect_equals(dest, clip)) { + if (!can_position && !drm_rect_equals(dst, clip)) { DRM_DEBUG_KMS("Plane must cover entire CRTC\n"); - drm_rect_debug_print("dst: ", dest, false); + drm_rect_debug_print("dst: ", dst, false); drm_rect_debug_print("clip: ", clip, false); return -EINVAL; } return 0; } +EXPORT_SYMBOL(drm_plane_helper_check_state); + +/** + * 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 + * @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 + * 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? + * @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 + * still wish to call this function to avoid duplication of error checking + * code. + * + * 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 *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, + .src_w = drm_rect_width(src), + .src_h = drm_rect_height(src), + .crtc_x = dst->x1, + .crtc_y = dst->x2, + .crtc_w = drm_rect_width(dst), + .crtc_h = drm_rect_height(dst), + .rotation = rotation, + .visible = *visible, + }; + int ret; + + ret = drm_plane_helper_check_state(&state, clip, + min_scale, max_scale, + can_position, + can_update_disabled); + if (ret) + return ret; + + *src = state.src; + *dst = state.dst; + *visible = state.visible; + + return 0; +} EXPORT_SYMBOL(drm_plane_helper_check_update); /** diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index 0e0c3573cce0..fbc8ecb3e5e8 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -40,6 +40,11 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs); +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); int drm_plane_helper_check_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb,