Re: [PATCH v2 2/2] drm: rcar-du: Clip planes to screen boundaries

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ville,

On Thursday, 16 November 2017 19:04:26 EET Ville Syrjälä wrote:
> On Mon, Nov 13, 2017 at 10:47:00AM +0200, Laurent Pinchart wrote:
> > Unlike the KMS API, the hardware doesn't support planes exceeding the
> > screen boundaries or planes being located fully off-screen. We need to
> > clip plane coordinates to support the use case.
> > 
> > Fortunately the DRM core offers the drm_plane_helper_check_state()
> > helper that valides the scaling factor and clips the plane coordinates.
> > Use it to implement the plane atomic check and use the clipped source
> > and destination rectangles from the plane state instead of the unclipped
> > source and CRTC coordinates to configure the device.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  3 ++-
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 37 +++++++++++++++++++++-------
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 10 ++++++---
> >  3 files changed, 39 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index b492063a6e1f..5685d5af6998
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -319,7 +319,8 @@ static void rcar_du_crtc_update_planes(struct
> > rcar_du_crtc *rcrtc)> 
> >  		struct rcar_du_plane *plane = &rcrtc->group->planes[i];
> >  		unsigned int j;
> > 
> > -		if (plane->plane.state->crtc != &rcrtc->crtc)
> > +		if (plane->plane.state->crtc != &rcrtc->crtc ||
> > +		    !plane->plane.state->visible)
> > 
> >  			continue;
> >  		
> >  		/* Insert the plane in the sorted planes array. */
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index
> > 4f076c364f25..9cf02b44902d 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > @@ -570,16 +570,39 @@ int __rcar_du_plane_atomic_check(struct drm_plane
> > *plane,
> >  				 const struct rcar_du_format_info **format)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_rect clip;
> > +	int ret;
> > 
> > -	if (!state->fb || !state->crtc) {
> > +	if (!state->crtc) {
> > +		/*
> > +		 * The visible field is not reset by the DRM core but only
> > +		 * updated by drm_plane_helper_check_state(), set it manually.
> > +		 */
> > +		state->visible = false;
> >  		*format = NULL;
> >  		return 0;
> > -	}
> > +	};
> 
> spurious ;

Oops, my bad, I'll fix that.

> > -	if (state->src_w >> 16 != state->crtc_w ||
> > -	    state->src_h >> 16 != state->crtc_h) {
> > -		dev_dbg(dev->dev, "%s: scaling not supported\n", __func__);
> > -		return -EINVAL;
> > +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	clip.x1 = 0;
> > +	clip.y1 = 0;
> > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> 
> crtc_state->mode would be more correct. I messed that up too in my
> recent vmwgfx fix [1]. But this should probably work just as well
> if you don't have a crtc scaler in your pipeline.

Indeed, my CRTC can't scale, so this works, but I'll fix it nonetheless.

> Also you may want to leave the clip empty when !crtc_state->enable.
> That way you'll be guaranteed to get visible==false. The helper is
> currently a bit broken wrt. the crtc->enable vs. crtc_state->enable.
> I've fixed that in [1] as well but those patches haven't been pushed
> yet.

[1] has landed in drm-misc, I'll rebase this series on top of that, and will 
send a pull request when drm-misc gets merged in Dave's tree.

> After getting that stuff in, I'm going to attempt moving this
> clipping stuff entirely into the helper to avoid these kinds of
> mistakes in the future.

Good idea, thanks.

> [1] https://patchwork.freedesktop.org/series/33001/
> 
> > +
> > +	ret = drm_plane_helper_check_state(state, &clip,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   true, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!state->visible) {
> > +		*format = NULL;
> > +		return 0;
> >  	}
> >  	
> >  	*format = rcar_du_format_info(state->fb->format->format);

-- 
Regards,

Laurent Pinchart