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@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
