Re: [PATCH v3] drm/msm: dpu: Allow planes to extend past active display

[Sean Paul <sean@poorly.run>]

On Tue, Sep 04, 2018 at 05:14:01PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-29 10:49, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > The atomic_check is a bit too aggressive with respect to planes which
> > leave the active area. This caused a bunch of log spew when the cursor
> > got to the edge of the screen and stopped it from going all the way.
> > 
> > This patch removes the conservative bounds checks from atomic and clips
> > the dst rect such that we properly display planes which go off the
> > screen.
> > 
> > Changes in v2:
> > - Apply the clip to src as well (taking into account scaling)
> > Changes in v3:
> > - Use drm_atomic_helper_check_plane_state() to clip src/dst
> > 
> > Cc: Sravanthi Kollukuduru <skolluku@codeaurora.org>
> > Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> 
> Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

Thanks for your review, I've pushed to dpu-staging/for-next.

Sean

> 
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c   |  3 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c  | 34 +++++++++++++---------
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c |  1 -
> >  drivers/gpu/drm/msm/msm_drv.h              |  2 ++
> >  4 files changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 07c2d15b45f2..f0a5e776ba32 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1551,8 +1551,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> >  		cnt++;
> > 
> >  		dst = drm_plane_state_dest(pstate);
> > -		if (!drm_rect_intersect(&clip, &dst) ||
> > -		    !drm_rect_equals(&clip, &dst)) {
> > +		if (!drm_rect_intersect(&clip, &dst)) {
> >  			DPU_ERROR("invalid vertical/horizontal destination\n");
> >  			DPU_ERROR("display: " DRM_RECT_FMT " plane: "
> >  				  DRM_RECT_FMT "\n", DRM_RECT_ARG(&crtc_rect),
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index efdf9b200dd9..46de0de466ff 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -1254,7 +1254,7 @@ static int dpu_plane_sspp_atomic_update(struct
> > drm_plane *plane,
> >  	const struct dpu_format *fmt;
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb;
> > -	struct drm_rect src, dst;
> > +	int ret, min_scale;
> > 
> >  	if (!plane) {
> >  		DPU_ERROR("invalid plane\n");
> > @@ -1293,21 +1293,29 @@ static int dpu_plane_sspp_atomic_update(struct
> > drm_plane *plane,
> >  	pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
> >  	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
> > 
> > -	src.x1 = state->src_x >> 16;
> > -	src.y1 = state->src_y >> 16;
> > -	src.x2 = src.x1 + (state->src_w >> 16);
> > -	src.y2 = src.y1 + (state->src_h >> 16);
> > +	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
> > +	ret = drm_atomic_helper_check_plane_state(state, crtc->state,
> > min_scale,
> > +					  pdpu->pipe_sblk->maxupscale << 16,
> > +					  true, false);
> > +	if (ret) {
> > +		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
> > +		return ret;
> > +	}
> > 
> > -	dst = drm_plane_state_dest(state);
> > +	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
> > DRM_RECT_FMT
> > +			", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
> > +			crtc->base.id, DRM_RECT_ARG(&state->dst),
> > +			(char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
> > 
> > -	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FMT "->crtc%u " DRM_RECT_FMT
> > -			", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_ARG(&src),
> > -			crtc->base.id, DRM_RECT_ARG(&dst),
> > -			(char *)&fmt->base.pixel_format,
> > -			DPU_FORMAT_IS_UBWC(fmt));
> > +	pdpu->pipe_cfg.src_rect = state->src;
> > +
> > +	/* state->src is 16.16, src_rect is not */
> > +	pdpu->pipe_cfg.src_rect.x1 >>= 16;
> > +	pdpu->pipe_cfg.src_rect.x2 >>= 16;
> > +	pdpu->pipe_cfg.src_rect.y1 >>= 16;
> > +	pdpu->pipe_cfg.src_rect.y2 >>= 16;
> > 
> > -	pdpu->pipe_cfg.src_rect = src;
> > -	pdpu->pipe_cfg.dst_rect = dst;
> > +	pdpu->pipe_cfg.dst_rect = state->dst;
> > 
> >  	_dpu_plane_setup_scaler(pdpu, pstate, fmt, false);
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > index 7d306c5acd09..273cbbe27c2e 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > @@ -259,7 +259,6 @@ static void mdp5_plane_cleanup_fb(struct drm_plane
> > *plane,
> >  	msm_framebuffer_cleanup(fb, kms->aspace);
> >  }
> > 
> > -#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
> >  static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state
> > *crtc_state,
> >  					      struct drm_plane_state *state)
> >  {
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h
> > b/drivers/gpu/drm/msm/msm_drv.h
> > index 8e510d5c758a..9d11f321f5a9 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -62,6 +62,8 @@ struct msm_gem_vma;
> >  #define MAX_BRIDGES    8
> >  #define MAX_CONNECTORS 8
> > 
> > +#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
> > +
> >  struct msm_file_private {
> >  	rwlock_t queuelock;
> >  	struct list_head submitqueues;
> 
> -- 
> Jeykumar S

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel