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
