[RFC] Exposing plane type mask and handling 'all' planes

[Matt Roper]

On Fri, Jun 28, 2019 at 07:54:06PM +0100, Emil Velikov wrote:
> On 2019/06/28, Matt Roper wrote:
> > On Fri, Jun 28, 2019 at 05:14:51PM +0100, Emil Velikov wrote:
> > > Hi Matt,
> > > 
> > > Thanks for the enlightening input :-)
> > > 
> > > On 2019/06/25, Matt Roper wrote:
> > > 
> > > > PLANE_CURSOR is basically just an indication that that specific plane is
> > > > the one that's also hooked up to the legacy cursor ioctls; like Ville
> > > > says, it shouldn't directly indicate that the plane is less
> > > > feature-capable than other planes.  You can either detect the true
> > > > capabilities of the cursor plane by checking for the presence/absence of
> > > > other plane properties and/or experimenting with atomic TEST_ONLY
> > > > commits to see what's really possible.
> > > > 
> > > Interesting, my understanding was the plane type was a hint about the
> > > capabilities. Although yes, userspace must check via TEST_ONLY to ensure
> > > the properties chosen will work.
> > > 
> > > 
> > > > The ideal solution for Intel gen9 hardware would have been to just never
> > > > have the driver advertise or program the dedicated hardware cursor at
> > > > all, but to instead expose the top-most universal plane to userspace,
> > > > describe it as PLANE_CURSOR, and route the legacy cursor ioctl's to that
> > > > plane instead.  That would allow legacy cursor behavior to work as
> > > > usual, but would also allow atomic userspace to use the plane in a more
> > > > full-featured manner.  I wrote patches to do exactly this a couple years
> > > > ago, but sadly we discovered that the universal planes on gen9 have a
> > > > slight alpha blending defect that the dedicated hardware cursor does not
> > > > exhibit.  Thus replacing the hardware cursor with the topmost universal
> > > > plane led to a slight regression for existing users and we had to scrap
> > > > the whole idea.  :-(
> > > > 
> > > > For reference, the relevant patch from a few years ago is here:
> > > >         https://patchwork.kernel.org/patch/9398571/
> > > > 
> > > In that thread you mention:
> > > 
> > > "... I believe the color correction settings are different for the
> > > universal plane vs the cursor plane (which causes IGT CRC mismatches at
> > > the moment and may be visually noticeable if you have good eyes); that
> > > shouldn't be hard to track down and fix."
> > > 
> > > Yet above you mention that universal planes have alpha blending defect.
> > > Did you confirm that with HW/simulation teams or is that based on the
> > > documentation? I would love to read a bit more on the topic.
> > > 
> > > In particular, but not limited to, if this defect is applicable only for
> > > plane3 or literally all universal planes.
> > 
> > We only figured out exactly what was going on a while after I wrote that
> > message in the thread, but we did ultimately confirm the problem with
> > the hardware architects.  Sadly, all of the gen9 universal planes suffer
> > from the slight blending issue and only the dedicated hardware cursor is
> > immune because it has some special bypass logic.
> > 
> > Specifically, you'd usually expect blending between two planes' pixels
> > to give you a final color value of
> > 
> >         bottom * (1.0-alpha) + top * alpha
> > 
> > However due to the way the alpha values are interpreted in the hardware,
> > there's a problematic case when the top plane's pixel alpha is 0.  You
> > wind up getting
> > 
> >         bottom * (255/256) + top * 0  = .996 * bottom
> > 
> > meaning pixels with a fully transparent plane above them are very
> > slightly fainter than pixels that didn't go through blending.  You'll
> > pretty much only perceive the difference when you have transparent
> > pixels at the edge of a plane (and even then only if you have a good
> > monitor and good eyes).  Of course "transparent pixels at the edge of
> > plane" is pretty common when you're using the plane as a mouse pointer.
> > You wind up with a faint ghost rectangle around your mouse pointer. :-(
> > 
> So we have a couple of problems here:
>  - general faided bottom image - not much we can do here, also an issue
> even w/o using plane3 as cursor.
> 
>  - ghosting - w/a: make sure the edge of a plane, is never within the
> viewport (scanout region)
> 
> Since the fading is a generic problem, we might as well make use of
> plane3 regardless.
> 
> What I'm proposing is that we pick up dimensions X and Y as cut-off.
> Everything larger will use plane3, smaller - cursor.
> 
> To determine X/Y I see two approaches:
>  - based off the max cursor size exposed by DRM drivers - say Zx larger
>  - consider the current resolution +/- delta
> 
> We could also extrapolate that with the fact that most drivers expose
> and hence user-space handle cursor identical width and height.
> 
> Personally, I'm inclined to pick 512x512 as a threshold and re-spin your
> patches. Let me know what you think.

It's definitely possible and something some of us have discussed in the
past.  However it's also a little more complicated code-wise than it
appears at first glance since you'll have to be pretty careful with the
handling of the watermark code.  There's a lot of special case branches
in watermark and DDB handling that would need to be updated to act
differently according to which hardware plane is actually backing your
uapi cursor plane on any given frame.  I.e., some of the skl_* functions
in intel_pm.c are the ones that probably need extra attention.  Changing
the code to never use the hardware cursor was easy (and removed a lot of
special cases), but changing the code to sometimes use the hardware
cursor and sometimes not for the same uapi plane is more complicated.
It's certainly not an insurmountable problem, but would just require
some extra effort and careful code review to make sure no conditions
were missed.


Matt

> 
> Thanks,
> Emil

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795