[Intel-gfx] [PATCH 1/2] drm/i915/skl: Allow universal planes to position

Wed Oct 7 07:19:54 PDT 2015

On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
> > 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 at intel.com>
> > > > > >Reviewed-by: Matt Roper <matthew.d.roper at 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.
> 
> We can just look at the src coordinates. Those always match the fb
> orientation.

Can we just not bother with legacy pageflips on rotated planes? setplane
works and once you rotate it kinda gets nasty anyway.

The problem I see is that with legacy pageflip we also need to hack up
something that doesn't look at plane->state for legacy and all that for a
grand total of about 2 drivers, both getting converted to atomic.
-Daniel

> 
> > 
> > 
> > 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
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch