[PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Am Mittwoch, den 19.10.2016, 13:31 +0300 schrieb Ville Syrjälä:
> On Wed, Oct 19, 2016 at 12:21:17PM +0200, Philipp Zabel wrote:
> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> > bounds. This checks for full plane coverage and scaling already, so
> > we can drop some custom checks. Use the clipped coordinates everywhere.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
> >  1 file changed, 36 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 6a97e396..f0ce899 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
> >  {
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> >  	BUG_ON(!cma_obj);
> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> >  	unsigned long eba = drm_plane_state_to_eba(state);
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
> >  	BUG_ON(!cma_obj);
> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> >  	unsigned long eba = drm_plane_state_to_eba(state);
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
> >  	BUG_ON(!cma_obj);
> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_framebuffer *old_fb = old_state->fb;
> >  	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> > +	bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> > +	struct drm_rect clip;
> >  	int hsub, vsub;
> > +	int ret;
> >  
> >  	/* Ok to disable */
> >  	if (!fb)
> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	if (WARN_ON(!crtc_state))
> >  		return -EINVAL;
> >  
> > +	clip.x1 = 0;
> > +	clip.y1 = 0;
> > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +	ret = drm_plane_helper_check_state(state, &clip,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   can_position, true);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* CRTC should be enabled */
> >  	if (!crtc_state->enable)
> >  		return -EINVAL;
> >  
> > -	/* no scaling */
> > -	if (state->src_w >> 16 != state->crtc_w ||
> > -	    state->src_h >> 16 != state->crtc_h)
> > -		return -EINVAL;
> > -
> >  	switch (plane->type) {
> >  	case DRM_PLANE_TYPE_PRIMARY:
> > -		/* full plane doesn't support partial off screen */
> > -		if (state->crtc_x || state->crtc_y ||
> > -		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> > -		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> > -			return -EINVAL;
> > -
> >  		/* full plane minimum width is 13 pixels */
> > -		if (state->crtc_w < 13)
> > +		if (drm_rect_width(&state->dst) < 13)
> >  			return -EINVAL;
> >  		break;
> >  	case DRM_PLANE_TYPE_OVERLAY:
> > -		if (state->crtc_x < 0 || state->crtc_y < 0)
> > -			return -EINVAL;
> > -
> > -		if (state->crtc_x + state->crtc_w >
> > -		    crtc_state->adjusted_mode.hdisplay)
> > -			return -EINVAL;
> > -		if (state->crtc_y + state->crtc_h >
> > -		    crtc_state->adjusted_mode.vdisplay)
> > -			return -EINVAL;
> >  		break;
> >  	default:
> > -		dev_warn(dev, "Unsupported plane type\n");
> > +		dev_warn(dev, "Unsupported plane type %d\n", plane->type);
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (state->crtc_h < 2)
> > +	if (drm_rect_height(&state->dst) < 2)
> >  		return -EINVAL;
> >  
> >  	/*
> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	 * callback.  The planes will be reenabled in plane's ->atomic_update
> >  	 * callback.
> >  	 */
> > -	if (old_fb && (state->src_w != old_state->src_w ||
> > -			      state->src_h != old_state->src_h ||
> > -			      fb->pixel_format != old_fb->pixel_format))
> > +	if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
> 
> I presume you don't want to force a modeset for moving a plane around.
> Using drm_rect_equals() here would end up doing that.
> 
> Otherwise it all looks quite decent to me.

Thanks, I'll use drm_rect_width / drm_rect_height as I should have.

regards
Philipp