[PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915

[Alex Goins]

> > >> IMHO the proposed patch does not sound like a reasonable long-term
> > >> solution. Ideally the driver will expose a flag, based on which Xorg
> > >> will enable/disable reverse prime. That said, as a short-term fix this
> > >> is fine, barring the issues mentioned below.
> > >
> > > The decision as to whether or not the slave can use the passed pixmap as
> > > its own scanout (or whether it requires a copy) is not part of the
> > > master's policy.
> > Hi Chris, it's this precisely what I've said/meant :-)
> > 
> > To put in other words:
> > IMHO the master/slave device should expose all their capabilities.
> > Based on these, Xorg should {en,dis}able reverse prime/etc.
> 
> Yes. But I would prefer it if the user made the choice, it may require
> to jump through 20 different hoops, but if it is the only way to achieve
> the user's configuration, that is what is need to be done.
> 
> This patch seems to be second guessing what the slave might be doing
> instead, as you said, exposing a method by which the master/slave can
> negotiate on what is being passed between them.
> -Chris

I'm not a huge fan of the term "reverse PRIME," since to me, reverse PRIME is
just PRIME. PRIME can be used for display sink support with any two
devices/drivers that expose the capabilities. I don't see why it should be
considered "reverse PRIME" just because the sink happens to be a dGPU (and
assumed that the source is an iGPU). What if sink and source are both dGPUs, or
both iGPUs? Is that still "reverse PRIME?" To me, it's all just PRIME, and the
extra copy is just an implementation detail in the slave.

I may be a little confused about what we're referring to by "master" and "slave"
and "driver" in the above convo, so excuse my wordiness as I try to clear things
up.

I think what Emil means is that the modesetting driver, when operating as the
Xorg slave driver, should be able to tell, via a flag, whether or not its DRM
driver needs a vidmem scanout surface. I agree, though IIRC there was some
pushback on dri-devel when I asked about adding a similarly fine grained flag
(for fenced pageflip support). Ideally we could query for everything we need and
act accordingly instead of using blacklists/whitelists, but currently we can't.
That goes for UDL as well; I'd like to be able to query if a driver's vblank
events can be trusted, but I cannot.

In the case of modesetting, I can see some value in exposing "reverse PRIME" to
the user, since it can be used with any DRM driver that may or may not be well
behaved. However, I don't think that should be part of the ABI, but a
modesetting-specific xconf option or output property, since it's controlling an
implementation detail internal to modesetting. It wouldn't make so much sense
when using a non-generic driver that presumably can have some better
expectations and/or vendor-specific IOCTLs for querying the capabilities of the
DRM driver(s) it supports.

This patch isn't second guessing what the slave is doing, assuming you mean
"slave" as in the output sink Xorg driver. This check is done BY the slave, in
an attempt to better determine whether or not it should do the extra copy to
vidmem. Previously, the only criterion was that it was accelerated by Glamor.
Ideally the criteria would be "accelerated by Glamor (to determine if we support
the extra copy) and DRM driver exposing the vidmem scanout flag (to determine if
we need or want the extra copy)." Lacking the latter, my stopgap solution is to
special-case the most commonly used iGPU DRM driver, likewise for UDL with its
misbehaved vblank events.

If these flags became available, I'd certainly be in favor of using them over
the blacklist to avoid drivers such as Hans' gm12u320 falling through the
cracks.

I'd be okay with dropping the blacklist patches if necessary to get the rest
merged, or otherwise merging the rest of the patches until there is a more
agreeable way to handle special-casing DRM drivers.

The UDL blacklist is just a matter of convenience for the user, since you could
try to run PRIME synchronization with UDL, but the results wouldn't be
desirable. PRIME synchronization is already configurable by the user, so a savvy
user could force UDL to use unsynchronized PRIME without this patch. It's
totally optional, but will probably save some bug reports.

The i915 reverse PRIME blacklist is necessary to use PRIME synchronization with
the modesetting driver as both the master and the slave. You need to enable
Glamor to use modesetting as a master, and enabling Glamor for the master
enables it for the slave as well. Thus, if Glamor is the only criterion for
reverse PRIME, reverse PRIME must be enabled in this configuration, even if the
slave is an iGPU. PRIME synchronization currently doesn't support reverse PRIME,
so it would be impossible to use a modesetting->modesetting configuration with
PRIME synchronization. This doesn't affect our driver, since we can just disable
Glamor. Leaving it out wouldn't prohibit us from releasing a driver supporting
the new functionality ASAP.

Thanks,
Alex