Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
> > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 10/04/15 10:07, Sonika Jindal wrote:
> > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > >---
> > > >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > >index ceb2e61..f0bbc22 100644
> > > >--- a/drivers/gpu/drm/i915/intel_display.c
> > > >+++ b/drivers/gpu/drm/i915/intel_display.c
> > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> > > >  	struct drm_rect *dest = &state->dst;
> > > >  	struct drm_rect *src = &state->src;
> > > >  	const struct drm_rect *clip = &state->clip;
> > > >+	bool can_position = false;
> > > >  	int ret;
> > > >
> > > >  	crtc = crtc ? crtc : plane->crtc;
> > > >  	intel_crtc = to_intel_crtc(crtc);
> > > >
> > > >+	if (INTEL_INFO(dev)->gen >= 9)
> > > >+		can_position = true;
> > > >+
> > > >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > > >  					    src, dest, clip,
> > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > >-					    false, true, &state->visible);
> > > >+					    can_position, true,
> > > >+					    &state->visible);
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > >
> > > 
> > > I have discovered today that, while this allows SetCrtc and SetPlane
> > > ioctls to work with frame buffers which do not cover the plane, page
> > > flips are not that lucky and fail roughly with:
> > > 
> > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> > > viewport 1920x1080+0+0.
> > 
> > Maybe I'm misunderstanding your explanation, but a framebuffer is always
> > required to fill/cover the plane scanning out of it.  What this patch is
> > supposed to be allowing is for the primary plane to not cover the entire
> > CRTC (since that's something that only became possible for Intel
> > hardware on the gen9+ platforms).  I.e., the primary plane is now
> > allowed to positioned and resized to cover a subset of the CRTC area,
> > just like "sprite" planes have always been able to.
> > 
> > If you've got a 1080x1080 framebuffer, then it's legal to have a
> > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
> > However it is not legal to size the primary plane as 1920x1080 and use
> > this same 1080x1080 framebuffer with any of our interfaces (setplane,
> > setcrtc, pageflip, or atomic).
> > 
> > Are you using ioctls/libdrm directly or are you using igt_kms helpers?
> > IIRC, the IGT helpers will try to be extra helpful and automatically
> > size the plane to match the framebuffer (unless you override that
> > behavior), so that might be what's causing the confusion here.
> 
> The problem is clear as day in drm_mode_page_flip_ioctl():
> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> if (ret)
> 	goto out;
> 
> The fix should be easy; just extract the current src coordinates from
> the plane state and check those against the new fb size. And then hope
> that the plane state is really up to date.

Yep, that's the conclusion we came to once Tvrtko explained what he was
seeing on IRC.  I'm not sure whether non-atomic drivers have enough
state setup by the default helpers to work properly.  Worst case we'll
just assume that a non-atomic driver won't support primary plane
windowing (since none have in the past) and fall back to looking at the
mode for legacy non-atomic drivers.

> 
> And I'm sure rotated cases will go boom in some other ways. Probably
> we should just switch over to using the full plane update for mmio
> flips to fix it.

Yeah; the core looks at a drm_plane->invert_dimensions field that's only
set by omap.  That should probably be updated to look at the state's
rotation on atomic-capable drivers.


Matt

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> > > Excercise page flips with 90 degree rotation". May not be that great
> > > but shows the failure.
> > > 
> > > I am not that hot on meddling with this code, nor do I feel
> > > competent to even try on my own at least. :/ Maybe just because the
> > > atomic and plane related rewrites have been going on for so long,
> > > and have multiple people involved, it all sounds pretty scary and
> > > fragile.
> > > 
> > > But I think some sort of plan on how to fix this could be in order?
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx