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

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-08-23 07:21, Sean Paul wrote:
> On Wed, Aug 22, 2018 at 02:56:22PM -0700, Jeykumar Sankaran wrote:
>> On 2018-08-22 13:41, 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.
>> >
>> > Cc: Sravanthi Kollukuduru <skolluku@codeaurora.org>
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  3 +--
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++++++---
>> >  2 files changed, 8 insertions(+), 5 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..720005a8ea25 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;
>> > +	struct drm_rect clip = { 0 }, src, dst;
>> >
>> >  	if (!plane) {
>> >  		DPU_ERROR("invalid plane\n");
>> > @@ -1300,14 +1300,18 @@ static int dpu_plane_sspp_atomic_update(struct
>> > drm_plane *plane,
>> >
>> >  	dst = drm_plane_state_dest(state);
>> >
>> > +	clip.x2 = crtc->state->adjusted_mode.hdisplay;
>> > +	clip.y2 = crtc->state->adjusted_mode.vdisplay;
>> > +	drm_rect_intersect(&clip, &dst);
>> > +
>> >  	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),
>> > +			crtc->base.id, DRM_RECT_ARG(&clip),
>> >  			(char *)&fmt->base.pixel_format,
>> >  			DPU_FORMAT_IS_UBWC(fmt));
>> >
>> >  	pdpu->pipe_cfg.src_rect = src;
>> > -	pdpu->pipe_cfg.dst_rect = dst;
>> > +	pdpu->pipe_cfg.dst_rect = clip;
>> 
>> If clip width/height is not equal to dst(which will be same as src for
>> cursor)
>> width/height it will require scaling which is not supported by DMA 
>> pipe
>> (used for cursor planes).
> 
> It doesn't require scaling, it just requires that the scanout stops 
> short.
> 
> Perhaps some ascii-art will explain better. This is what u/s is trying 
> to
> do:
> 
> +----------------------+
> |     active display   |
> |                      |
> |                  +---------+
> |                  |  plane  |
> |                  |         |
> |                  +---------+
> |                      |
> +----------------------+
> 
> This is what the patch does:
> 
> +----------------------+
> |     active display   |
> |                      |
> |                  +---+
> |                  |  p|
> |                  |   |
> |                  +---+
> |                      |
> +----------------------+
> 
> So we're not scaling anything, just stopping short. On my device, the
> first
> scenario results in immediate blue screen when the cursor exits the 
> active
> area
> (underrun?). With the patch, it works just fine since everything is
> confined to
> the screen.
> 
> Sean

In order to stop the scanout at the boundary and partially display the 
cursor image,
you should be clipping both the src and dst rects.

e.g if the source dimension is 100 x 100 and the original dst rect is 
100 x 100 (no scaling).
If you are clipping the dst to 50 x 100 and keeping the original src 
image,
you are basically scaling down the image from 100 x 100 to 50 x 100.

Thanks and Regards,
Jeykumar S.

> 
> 
>> Ideally, we should suppress the error log and reset the cursor x/y to
> w/h
>> offset from the boundaries.
>> 
>> Jeykumar S.
>> 
>> >
>> >  	_dpu_plane_setup_scaler(pdpu, pstate, fmt, false);
>> 
>> --
>> Jeykumar S

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno