[Intel-gfx] [PATCH 5/6] drm/i915: Add 180 degree primary plane rotation support

[Daniel Vetter]

On Mon, Aug 11, 2014 at 05:37:20PM +0530, Jindal, Sonika wrote:
> 
> 
> On 8/11/2014 5:12 PM, Daniel Vetter wrote:
> >On Mon, Aug 11, 2014 at 04:37:00PM +0530, Jindal, Sonika wrote:
> >>
> >>Hi Daniel,
> >>Are you taking this patch back in drm-intel?
> >
> >We should still call the primary_plane->update hook directly instead of
> >open-coding it.
> >-Daniel
> >
> But we are already doing dev_priv->display.update_primary_plane. Can you
> please elaborate? Last time we had a discussion on this same point, and we
> thought for some platforms more work might be needed but for current
> platforms this looks ok. Following was the discussion:

That's a different cook used internally in the driver (mostly for
historical reasons nowadays). I'm talking about the
drm_plane->fops->update_plane hook, which is implemented for primary
planes by intel_primary_plane_setplane.

The idea is to have the exact same code flow as for sprite planes, since
once we have atomic modesets that will be what we need. In the sprite
set_prop function you pretty directly call the update_plane hook, and I
think we should do the same here. Actually we might as well directly reuse
the intel_plane_restore function, it seems to exactly do what we want.

> "> >Also Chris mentioned that on some platforms this won't work and it's
> > >more future-proof to just do a full modeset until we have the proper
> > >infrastructure.
> > >
> > Can you please elaborate on this? What needs to be done?
> 
> Apparently nothing, it turned out that on the platforms currently supported
> everything is fine with your patch. Damien promised to follow up with the
> details on internal mailing lists.

Damien follow up internally. Since we're now allowed to talk about skl
I'll paste this here:

"On SKL, for 90/270, we'll also need to update the watermarks and remap
the fb.

"We already duplicate some of the logic: the FBC update, wait for
pending flips, ...

"So maybe we should try to find a way to take the same path for all
updates. I'm not too sure how practical this is though."

Damien also said that he doesn't want to still the series on this, but
since we now have universal plane support for the primary plane I think it
makes a lot of sense.
-Daniel


> -Daniel
> "
> 
> >>
> >>-Sonika
> >>
> >>On 8/7/2014 5:41 PM, Daniel Vetter wrote:
> >>>On Thu, Aug 07, 2014 at 01:45:31PM +0200, Daniel Vetter wrote:
> >>>>On Tue, Jul 15, 2014 at 11:11:37AM +0200, Daniel Vetter wrote:
> >>>>>On Tue, Jul 15, 2014 at 02:10:28PM +0530, sonika.jindal at intel.com wrote:
> >>>>>>+		/* FBC does not work on some platforms for rotated planes */
> >>>>>>+			if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
> >>>>>>+				if (dev_priv->fbc.plane == intel_crtc->plane &&
> >>>>>>+				intel_plane->rotation != BIT(DRM_ROTATE_0))
> >>>>>>+					intel_disable_fbc(dev);
> >>>>>>+			/* If rotation was set earlier and new rotation is 0,
> >>>>>>+			we might have disabled fbc earlier. So update it now */
> >>>>>>+				else if (intel_plane->rotation == BIT(DRM_ROTATE_0)
> >>>>>>+					&& old_val != BIT(DRM_ROTATE_0)) {
> >>>>>>+					mutex_lock(&dev->struct_mutex);
> >>>>>>+					intel_update_fbc(dev);
> >>>>>>+					mutex_unlock(&dev->struct_mutex);
> >>>>>>+				}
> >>>>>>+			}
> >>>>>
> >>>>>Indentation is screwed up here. Also if we convert some of the checks into
> >>>>>early bails we could de-indent this by one level.
> >>>>>
> >>>>>Also Chris mentioned that on some platforms this won't work and it's more
> >>>>>future-proof to just do a full modeset until we have the proper
> >>>>>infrastructure.
> >>>>
> >>>>Apparently this review here was never addressed, as Chris just pointed out
> >>>>on irc. I've dropped the patch again.
> >>>>
> >>>>I think we need:
> >>>>- The same sequence as with the sprite set_property function, i.e. we need
> >>>>   to call the update_plane function (not the raw low-level one, the
> >>>>   high-level with all the checks).
> >>>>- The fbc check is wrong and will miss updates when the crtc is off. We
> >>>>   need to move this into the general list of checks in intel_update_fbc.
> >>>>- Since this seems to be buggy I want added testcases to combine fbc
> >>>>   correctness with screen rotation. Probably best to reuse the existing
> >>>>   fbc testcase and add a bunch or rotated tests.
> >>>
> >>>Ok, the check in update_fbc is there, I've been blind. Sorry about all the
> >>>confusion. So just amounts of calling the higher level function and we can
> >>>forgo the fbc testing.
> >>>-Daniel
> >>>
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch