Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
Inline > > 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. > > Well the naming came from the fact that the way PRIME was designed > isn't the way that Windows does "Optimus". > > On Windows with optimus they have dynamic compositor switching (and on OSX). > > So the two use cases the Optimus model handles are: > > a) compositor runs on Integrated GPU, panel is connected to Intel GPU, no > other > outputs are connected. 3D apps can be offloaded to sleeping discrete GPU. > > b) external output is connected. compositor runs on discrete GPU, slaves Intel > GPU to run the panel, everything is rendered on the dGPU, it doesn't sleep. > > However to do that model you really need runtime smooth compositor switching. > > So when I added the hack to have the Intel GPU be the master, and the discrete > GPU outputs be slaves, I called it "reverse PRIME" because it really > is a backwards > use case for how the hw was designed. Thanks for the explanation, and the really quick reply. It does make more sense with that context. It's easy for me to overlook the offloading functionality since the current implementation is incompatible with our driver. > FYI I'm fine with the black/whitelist changes for now, we'd need kernel > queries > I suspect to do better. Well, definitely don't merge them until I send out the revised versions fixing the leak/lack of NULL check and blacklisting Hans' driver. The rest of the patches are probably good to go, unless there's any more feedback. I'll send out the revised versions of the blacklist changes in a day or two to make sure the feedback is resolved, or ASAP if you'd prefer. None of the other patches strictly depend on them. Thanks again, Alex ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
On 14 June 2016 at 10:39, Alex Goinswrote: >> > >> 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. Well the naming came from the fact that the way PRIME was designed isn't the way that Windows does "Optimus". On Windows with optimus they have dynamic compositor switching (and on OSX). So the two use cases the Optimus model handles are: a) compositor runs on Integrated GPU, panel is connected to Intel GPU, no other outputs are connected. 3D apps can be offloaded to sleeping discrete GPU. b) external output is connected. compositor runs on discrete GPU, slaves Intel GPU to run the panel, everything is rendered on the dGPU, it doesn't sleep. However to do that model you really need runtime smooth compositor switching. So when I added the hack to have the Intel GPU be the master, and the discrete GPU outputs be slaves, I called it "reverse PRIME" because it really is a backwards use case for how the hw was designed. FYI I'm fine with the black/whitelist changes for now, we'd need kernel queries I suspect to do better. Dave. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
> > >> 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
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
Thanks for the feedback, Emil! > 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. I'll address this lower in the thread. > > diff --git a/hw/xfree86/drivers/modesetting/driver.c > > b/hw/xfree86/drivers/modesetting/driver.c > > index f7abd1e..403cb96 100644 > > --- a/hw/xfree86/drivers/modesetting/driver.c > > +++ b/hw/xfree86/drivers/modesetting/driver.c > > @@ -1378,9 +1378,13 @@ ScreenInit(ScreenPtr pScreen, int argc, char **argv) > > xf86DrvMsg(pScrn->scrnIndex, X_ERROR, > > "Failed to initialize the Present extension.\n"); > > } > > -/* enable reverse prime if we are a GPU screen, and accelerated */ > > -if (pScreen->isGPU) > > -ms->drmmode.reverse_prime_offload_mode = TRUE; > > +/* enable reverse prime if we are a GPU screen, and accelerated, > > and not > > + * i915. i915 is happy scanning out from sysmem. */ > > +if (pScreen->isGPU) { > > +drmVersionPtr version = drmGetVersion(ms->drmmode.fd); > > +if (strncmp("i915", version->name, version->name_len)) > 'version' can be null. > > > +ms->drmmode.reverse_prime_offload_mode = TRUE; > > +} > We're leaking 'version'. > > Same two apply for the UDL patch. > > Regards, > Emil You're right, thanks for the catch. I have these fixed locally now, but I'll wait a little while to see if there's any more feedback before sending out another revision. Thanks, Alex ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
On Sun, Jun 12, 2016 at 06:23:25PM +0100, Emil Velikov wrote: > On 12 June 2016 at 17:09, Chris Wilsonwrote: > > On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote: > >> Hi all, > >> > >> On 11 June 2016 at 01:21, Alex Goins wrote: > >> > Reverse PRIME seems to be designed with discrete graphics as a sink in > >> > mind, designed to do an extra copy from sysmem to vidmem to prevent a > >> > discrete chip from needing to scan out from sysmem. > >> > > >> > The criteria it used to detect this case is if we are a GPU screen and > >> > Glamor accelerated. It's possible for i915 to fulfill these conditions, > >> > despite the fact that the additional copy doesn't make sense for i915. > >> > > >> > Normally, you could just set AccelMethod = none as an option for the > >> > device > >> > and call it a day. However, when running with modesetting as both the > >> > sink > >> > and the source, Glamor must be enabled. > >> > > >> > Ideally, you would be able to set AccelMethod individually for devices > >> > using the same driver, but there seems to be a bug in X option parsing > >> > that > >> > makes all devices on a driver inherit the options from the first detected > >> > device. Thus, glamor needs to be enabled for all or for none until that > >> > bug > >> > (if it's even a bug) is fixed. > >> > > >> > Nonetheless, it probably doesn't make sense to do the extra copy on i915 > >> > even if Glamor is enabled for the device, so this is more user friendly > >> > by > >> > not requiring users to disable acceleration for i915. > >> > > >> 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 -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
On 12 June 2016 at 17:09, Chris Wilsonwrote: > On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote: >> Hi all, >> >> On 11 June 2016 at 01:21, Alex Goins wrote: >> > Reverse PRIME seems to be designed with discrete graphics as a sink in >> > mind, designed to do an extra copy from sysmem to vidmem to prevent a >> > discrete chip from needing to scan out from sysmem. >> > >> > The criteria it used to detect this case is if we are a GPU screen and >> > Glamor accelerated. It's possible for i915 to fulfill these conditions, >> > despite the fact that the additional copy doesn't make sense for i915. >> > >> > Normally, you could just set AccelMethod = none as an option for the device >> > and call it a day. However, when running with modesetting as both the sink >> > and the source, Glamor must be enabled. >> > >> > Ideally, you would be able to set AccelMethod individually for devices >> > using the same driver, but there seems to be a bug in X option parsing that >> > makes all devices on a driver inherit the options from the first detected >> > device. Thus, glamor needs to be enabled for all or for none until that bug >> > (if it's even a bug) is fixed. >> > >> > Nonetheless, it probably doesn't make sense to do the extra copy on i915 >> > even if Glamor is enabled for the device, so this is more user friendly by >> > not requiring users to disable acceleration for i915. >> > >> 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. Or perhaps you meant something entirely different ? If so can you please elaborate. Thanks Emil ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
On Sun, Jun 12, 2016 at 02:35:43PM +0100, Emil Velikov wrote: > Hi all, > > On 11 June 2016 at 01:21, Alex Goinswrote: > > Reverse PRIME seems to be designed with discrete graphics as a sink in > > mind, designed to do an extra copy from sysmem to vidmem to prevent a > > discrete chip from needing to scan out from sysmem. > > > > The criteria it used to detect this case is if we are a GPU screen and > > Glamor accelerated. It's possible for i915 to fulfill these conditions, > > despite the fact that the additional copy doesn't make sense for i915. > > > > Normally, you could just set AccelMethod = none as an option for the device > > and call it a day. However, when running with modesetting as both the sink > > and the source, Glamor must be enabled. > > > > Ideally, you would be able to set AccelMethod individually for devices > > using the same driver, but there seems to be a bug in X option parsing that > > makes all devices on a driver inherit the options from the first detected > > device. Thus, glamor needs to be enabled for all or for none until that bug > > (if it's even a bug) is fixed. > > > > Nonetheless, it probably doesn't make sense to do the extra copy on i915 > > even if Glamor is enabled for the device, so this is more user friendly by > > not requiring users to disable acceleration for i915. > > > 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. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
Hi all, On 11 June 2016 at 01:21, Alex Goinswrote: > Reverse PRIME seems to be designed with discrete graphics as a sink in > mind, designed to do an extra copy from sysmem to vidmem to prevent a > discrete chip from needing to scan out from sysmem. > > The criteria it used to detect this case is if we are a GPU screen and > Glamor accelerated. It's possible for i915 to fulfill these conditions, > despite the fact that the additional copy doesn't make sense for i915. > > Normally, you could just set AccelMethod = none as an option for the device > and call it a day. However, when running with modesetting as both the sink > and the source, Glamor must be enabled. > > Ideally, you would be able to set AccelMethod individually for devices > using the same driver, but there seems to be a bug in X option parsing that > makes all devices on a driver inherit the options from the first detected > device. Thus, glamor needs to be enabled for all or for none until that bug > (if it's even a bug) is fixed. > > Nonetheless, it probably doesn't make sense to do the extra copy on i915 > even if Glamor is enabled for the device, so this is more user friendly by > not requiring users to disable acceleration for i915. > 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. > v1: N/A > v2: N/A > v3: N/A > v4: Initial commit > v5: Unchanged > v6: Rebase onto ToT > > Signed-off-by: Alex Goins > --- > hw/xfree86/drivers/modesetting/driver.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/hw/xfree86/drivers/modesetting/driver.c > b/hw/xfree86/drivers/modesetting/driver.c > index f7abd1e..403cb96 100644 > --- a/hw/xfree86/drivers/modesetting/driver.c > +++ b/hw/xfree86/drivers/modesetting/driver.c > @@ -1378,9 +1378,13 @@ ScreenInit(ScreenPtr pScreen, int argc, char **argv) > xf86DrvMsg(pScrn->scrnIndex, X_ERROR, > "Failed to initialize the Present extension.\n"); > } > -/* enable reverse prime if we are a GPU screen, and accelerated */ > -if (pScreen->isGPU) > -ms->drmmode.reverse_prime_offload_mode = TRUE; > +/* enable reverse prime if we are a GPU screen, and accelerated, and > not > + * i915. i915 is happy scanning out from sysmem. */ > +if (pScreen->isGPU) { > +drmVersionPtr version = drmGetVersion(ms->drmmode.fd); > +if (strncmp("i915", version->name, version->name_len)) 'version' can be null. > +ms->drmmode.reverse_prime_offload_mode = TRUE; > +} We're leaking 'version'. Same two apply for the UDL patch. Regards, Emil ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915
Reverse PRIME seems to be designed with discrete graphics as a sink in mind, designed to do an extra copy from sysmem to vidmem to prevent a discrete chip from needing to scan out from sysmem. The criteria it used to detect this case is if we are a GPU screen and Glamor accelerated. It's possible for i915 to fulfill these conditions, despite the fact that the additional copy doesn't make sense for i915. Normally, you could just set AccelMethod = none as an option for the device and call it a day. However, when running with modesetting as both the sink and the source, Glamor must be enabled. Ideally, you would be able to set AccelMethod individually for devices using the same driver, but there seems to be a bug in X option parsing that makes all devices on a driver inherit the options from the first detected device. Thus, glamor needs to be enabled for all or for none until that bug (if it's even a bug) is fixed. Nonetheless, it probably doesn't make sense to do the extra copy on i915 even if Glamor is enabled for the device, so this is more user friendly by not requiring users to disable acceleration for i915. v1: N/A v2: N/A v3: N/A v4: Initial commit v5: Unchanged v6: Rebase onto ToT Signed-off-by: Alex Goins--- hw/xfree86/drivers/modesetting/driver.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index f7abd1e..403cb96 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -1378,9 +1378,13 @@ ScreenInit(ScreenPtr pScreen, int argc, char **argv) xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "Failed to initialize the Present extension.\n"); } -/* enable reverse prime if we are a GPU screen, and accelerated */ -if (pScreen->isGPU) -ms->drmmode.reverse_prime_offload_mode = TRUE; +/* enable reverse prime if we are a GPU screen, and accelerated, and not + * i915. i915 is happy scanning out from sysmem. */ +if (pScreen->isGPU) { +drmVersionPtr version = drmGetVersion(ms->drmmode.fd); +if (strncmp("i915", version->name, version->name_len)) +ms->drmmode.reverse_prime_offload_mode = TRUE; +} } #endif -- 1.9.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel