Re: [PATCH] drm: Deal with rotation in drm_plane_helper_check_update()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 18, 2016 at 06:14:32PM +0100, Daniel Vetter wrote:
> On Fri, Jan 15, 2016 at 08:51:06PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > drm_plane_helper_check_update() needs to account for the plane rotation
> > for correct clipping/scaling calculations. Do so.
> > 
> > There was an earlier attempt [1] to add this into
> > intel_check_primary_plane() but I requested that it'd be put into the
> > helper instead. An updated patch never materialized AFAICS, so I went
> > ahead and cooked one up myself.
> > 
> > [1] https://patchwork.freedesktop.org/patch/65177/
> > Cc: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/armada/armada_overlay.c     |  1 +
> >  drivers/gpu/drm/drm_plane_helper.c          | 28 ++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_display.c        |  2 ++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  1 +
> >  include/drm/drm_plane_helper.h              |  1 +
> >  5 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
> > index 148e8a42b2c6..1ee707ef6b8d 100644
> > --- a/drivers/gpu/drm/armada/armada_overlay.c
> > +++ b/drivers/gpu/drm/armada/armada_overlay.c
> > @@ -121,6 +121,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	int ret;
> >  
> >  	ret = drm_plane_helper_check_update(plane, crtc, fb, &src, &dest, &clip,
> > +					    BIT(DRM_ROTATE_0),
> >  					    0, INT_MAX, true, false, &visible);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> > index 369d2898ff9e..0c12fa1a696f 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -115,6 +115,7 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> >   * @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
> > @@ -134,16 +135,17 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
> >   * 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,
> > -				    int min_scale,
> > -				    int max_scale,
> > -				    bool can_position,
> > -				    bool can_update_disabled,
> > -				    bool *visible)
> > +				  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 hscale, vscale;
> >  
> > @@ -163,6 +165,8 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >  		return -EINVAL;
> >  	}
> >  
> > +	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);
> > @@ -174,6 +178,9 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >  	}
> >  
> >  	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
> > +
> > +	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
> > +
> >  	if (!*visible)
> >  		/*
> >  		 * Plane isn't visible; some drivers can handle this
> > @@ -265,6 +272,7 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> >  
> >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> >  					    &src, &dest, &clip,
> > +					    BIT(DRM_ROTATE_0),
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    false, false, &visible);
> 
> With this rotation is essentially unsupported on legacy setups. Shouldn't
> we instead wrap this up into an atomic helper which just takes state
> structs and fishes out everything it needs on its own? Then there's no
> need to change existing drivers all that much, and atomic ones would be
> simpler ...
> 
> Or do I miss something?

We'd need to move the various drm_rects to live under drm_plane_state.
Right now they live in intel_plane_state. No objection to doing that
from me, but requires someone to actually write the patch.

> -Daniel
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c4cd9c94faa6..a726c5e91220 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14029,6 +14029,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  
> >  	return drm_plane_helper_check_update(plane, crtc, fb, &state->src,
> >  					     &state->dst, &state->clip,
> > +					     state->base.rotation,
> >  					     min_scale, max_scale,
> >  					     can_position, true,
> >  					     &state->visible);
> > @@ -14192,6 +14193,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  
> >  	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
> >  					    &state->dst, &state->clip,
> > +					    state->base.rotation,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    true, true, &state->visible);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 46c2a8dfd8aa..532df0a8656a 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -583,6 +583,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
> >  
> >  	ret = drm_plane_helper_check_update(plane, crtc, state->fb,
> >  					    src, dest, &clip,
> > +					    state->rotation,
> >  					    min_scale,
> >  					    max_scale,
> >  					    true, true, &visible);
> > diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> > index 4421f3f4ca8d..0e0c3573cce0 100644
> > --- a/include/drm/drm_plane_helper.h
> > +++ b/include/drm/drm_plane_helper.h
> > @@ -46,6 +46,7 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >  				  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,
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel