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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> > > > >Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > >---
> > > > >  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.

> 
> 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux