diff mbox

drm: avoid spurious EBUSY due to nonblocking atomic modesets

Message ID 20180705102121.5091-1-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 5, 2018, 10:21 a.m. UTC
When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
pull in arbitrary other resources, including CRTCs (e.g. when
reconfiguring global resources).

But in nonblocking mode userspace has then no idea this happened,
which can lead to spurious EBUSY calls, both:
- when that other CRTC is currently busy doing a page_flip the
  ALLOW_MODESET commit can fail with an EBUSY
- on the other CRTC a normal atomic flip can fail with EBUSY because
  of the additional commit inserted by the kernel without userspace's
  knowledge

For blocking commits this isn't a problem, because everyone else will
just block until all the CRTC are reconfigured. Only thing userspace
can notice is the dropped frames without any reason for why frames got
dropped.

Consensus is that we need new uapi to handle this properly, but no one
has any idea what exactly the new uapi should look like. As a stop-gap
plug this problem by demoting nonblocking commits which might cause
issues by including CRTCs not in the original request to blocking
commits.

v2: Add comments and a WARN_ON to enforce this only when allowed - we
don't want to silently convert page flips into blocking plane updates
just because the driver is buggy.

v3: Fix inverted WARN_ON (Pekka).

References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

Comments

Daniel Stone Jan. 31, 2020, 7:34 a.m. UTC | #1
On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> pull in arbitrary other resources, including CRTCs (e.g. when
> reconfiguring global resources).
>
> But in nonblocking mode userspace has then no idea this happened,
> which can lead to spurious EBUSY calls, both:
> - when that other CRTC is currently busy doing a page_flip the
>   ALLOW_MODESET commit can fail with an EBUSY
> - on the other CRTC a normal atomic flip can fail with EBUSY because
>   of the additional commit inserted by the kernel without userspace's
>   knowledge
>
> For blocking commits this isn't a problem, because everyone else will
> just block until all the CRTC are reconfigured. Only thing userspace
> can notice is the dropped frames without any reason for why frames got
> dropped.
>
> Consensus is that we need new uapi to handle this properly, but no one
> has any idea what exactly the new uapi should look like. As a stop-gap
> plug this problem by demoting nonblocking commits which might cause
> issues by including CRTCs not in the original request to blocking
> commits.

Thanks for writing this up Daniel, and for reminding me about it some
time later as well ...

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
Marius Vlad Sept. 22, 2020, 1:36 p.m. UTC | #2
On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > pull in arbitrary other resources, including CRTCs (e.g. when
> > reconfiguring global resources).
> >
> > But in nonblocking mode userspace has then no idea this happened,
> > which can lead to spurious EBUSY calls, both:
> > - when that other CRTC is currently busy doing a page_flip the
> >   ALLOW_MODESET commit can fail with an EBUSY
> > - on the other CRTC a normal atomic flip can fail with EBUSY because
> >   of the additional commit inserted by the kernel without userspace's
> >   knowledge
> >
> > For blocking commits this isn't a problem, because everyone else will
> > just block until all the CRTC are reconfigured. Only thing userspace
> > can notice is the dropped frames without any reason for why frames got
> > dropped.
> >
> > Consensus is that we need new uapi to handle this properly, but no one
> > has any idea what exactly the new uapi should look like. As a stop-gap
> > plug this problem by demoting nonblocking commits which might cause
> > issues by including CRTCs not in the original request to blocking
> > commits.
Gentle ping. I've tried out Linus's master tree and, and like Pekka,
I've noticed this isn't integrated/added.

Noticed this is fixing (also) DPMS when multiple outputs are in use.
Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
the warning quite often.

> 
> Thanks for writing this up Daniel, and for reminding me about it some
> time later as well ...
> 
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> 
> Cheers,
> Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Sept. 22, 2020, 2:04 p.m. UTC | #3
On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
>
> On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> > On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > reconfiguring global resources).
> > >
> > > But in nonblocking mode userspace has then no idea this happened,
> > > which can lead to spurious EBUSY calls, both:
> > > - when that other CRTC is currently busy doing a page_flip the
> > >   ALLOW_MODESET commit can fail with an EBUSY
> > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > >   of the additional commit inserted by the kernel without userspace's
> > >   knowledge
> > >
> > > For blocking commits this isn't a problem, because everyone else will
> > > just block until all the CRTC are reconfigured. Only thing userspace
> > > can notice is the dropped frames without any reason for why frames got
> > > dropped.
> > >
> > > Consensus is that we need new uapi to handle this properly, but no one
> > > has any idea what exactly the new uapi should look like. As a stop-gap
> > > plug this problem by demoting nonblocking commits which might cause
> > > issues by including CRTCs not in the original request to blocking
> > > commits.
> Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> I've noticed this isn't integrated/added.

Defacto the uapi we have now is that userspace needs to ignore "spurious" EBUSY.

> Noticed this is fixing (also) DPMS when multiple outputs are in use.
> Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
> the warning quite often.

This would be a driver bug I think. That really shouldn't happen for
normal page flips.
-Daniel

> >
> > Thanks for writing this up Daniel, and for reminding me about it some
> > time later as well ...
> >
> > Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> > Cheers,
> > Daniel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Stone Sept. 22, 2020, 2:14 p.m. UTC | #4
On Tue, 22 Sep 2020 at 15:04, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> > Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> > I've noticed this isn't integrated/added.
>
> Defacto the uapi we have now is that userspace needs to ignore "spurious" EBUSY.

This really, really, really, bites.

I think we need a guarantee that this never happens if ALLOW_MODESET
is always used in blocking mode, plus in future a cap we can use to
detect that we won't be getting spurious EBUSY events.

I really don't want to ever paper over this, because it's one of the
clearest indications that userspace has its timing/signalling wrong.

Cheers,
Daniel
Daniel Vetter Sept. 22, 2020, 4:01 p.m. UTC | #5
On Tue, Sep 22, 2020 at 4:14 PM Daniel Stone <daniel@fooishbar.org> wrote:
>
> On Tue, 22 Sep 2020 at 15:04, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> > > Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> > > I've noticed this isn't integrated/added.
> >
> > Defacto the uapi we have now is that userspace needs to ignore "spurious" EBUSY.
>
> This really, really, really, bites.
>
> I think we need a guarantee that this never happens if ALLOW_MODESET
> is always used in blocking mode, plus in future a cap we can use to
> detect that we won't be getting spurious EBUSY events.
>
> I really don't want to ever paper over this, because it's one of the
> clearest indications that userspace has its timing/signalling wrong.

Ok so the hang-up last time around iirc was that I broke igt by making
a few things more synchronous. Let's hope I'm not also breaking stuff
with the WARN_ON ...

New plan:
- make this patch here only document existing behaviour and enforce it
with the WARN_ON
- new uapi would be behind a flag or something, with userspace and
everything hanging off it.

Thoughts?

Cheers, Daniel
Daniel Stone Sept. 22, 2020, 7:02 p.m. UTC | #6
Hi,

On Tue, 22 Sep 2020 at 17:02, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Sep 22, 2020 at 4:14 PM Daniel Stone <daniel@fooishbar.org> wrote:
> > I think we need a guarantee that this never happens if ALLOW_MODESET
> > is always used in blocking mode, plus in future a cap we can use to
> > detect that we won't be getting spurious EBUSY events.
> >
> > I really don't want to ever paper over this, because it's one of the
> > clearest indications that userspace has its timing/signalling wrong.
>
> Ok so the hang-up last time around iirc was that I broke igt by making
> a few things more synchronous. Let's hope I'm not also breaking stuff
> with the WARN_ON ...
>
> New plan:
> - make this patch here only document existing behaviour and enforce it
> with the WARN_ON
> - new uapi would be behind a flag or something, with userspace and
> everything hanging off it.
>
> Thoughts?

What do you mean by 'new uapi'? The proposal that the kernel
communicates back which object IDs have been added to the state behind
your back? That it's been made automatically blocking? Something else?

Cheers,
Daniel (the other one)
Daniel Vetter Sept. 23, 2020, 10:58 a.m. UTC | #7
On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
>
> On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> > On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > reconfiguring global resources).
> > >
> > > But in nonblocking mode userspace has then no idea this happened,
> > > which can lead to spurious EBUSY calls, both:
> > > - when that other CRTC is currently busy doing a page_flip the
> > >   ALLOW_MODESET commit can fail with an EBUSY
> > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > >   of the additional commit inserted by the kernel without userspace's
> > >   knowledge
> > >
> > > For blocking commits this isn't a problem, because everyone else will
> > > just block until all the CRTC are reconfigured. Only thing userspace
> > > can notice is the dropped frames without any reason for why frames got
> > > dropped.
> > >
> > > Consensus is that we need new uapi to handle this properly, but no one
> > > has any idea what exactly the new uapi should look like. As a stop-gap
> > > plug this problem by demoting nonblocking commits which might cause
> > > issues by including CRTCs not in the original request to blocking
> > > commits.
> Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> I've noticed this isn't integrated/added.
>
> Noticed this is fixing (also) DPMS when multiple outputs are in use.
> Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
> the warning quite often.

On which driver/chip does this happen?
-Daniel

>
> >
> > Thanks for writing this up Daniel, and for reminding me about it some
> > time later as well ...
> >
> > Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> > Cheers,
> > Daniel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Marius Vlad Sept. 23, 2020, 11:14 a.m. UTC | #8
On Wed, Sep 23, 2020 at 12:58:30PM +0200, Daniel Vetter wrote:
> On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> >
> > On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> > > On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > > reconfiguring global resources).
> > > >
> > > > But in nonblocking mode userspace has then no idea this happened,
> > > > which can lead to spurious EBUSY calls, both:
> > > > - when that other CRTC is currently busy doing a page_flip the
> > > >   ALLOW_MODESET commit can fail with an EBUSY
> > > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > > >   of the additional commit inserted by the kernel without userspace's
> > > >   knowledge
> > > >
> > > > For blocking commits this isn't a problem, because everyone else will
> > > > just block until all the CRTC are reconfigured. Only thing userspace
> > > > can notice is the dropped frames without any reason for why frames got
> > > > dropped.
> > > >
> > > > Consensus is that we need new uapi to handle this properly, but no one
> > > > has any idea what exactly the new uapi should look like. As a stop-gap
> > > > plug this problem by demoting nonblocking commits which might cause
> > > > issues by including CRTCs not in the original request to blocking
> > > > commits.
> > Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> > I've noticed this isn't integrated/added.
> >
> > Noticed this is fixing (also) DPMS when multiple outputs are in use.
> > Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
> > the warning quite often.
> 
> On which driver/chip does this happen?
I've tried it out on i915.
> -Daniel
> 
> >
> > >
> > > Thanks for writing this up Daniel, and for reminding me about it some
> > > time later as well ...
> > >
> > > Reviewed-by: Daniel Stone <daniels@collabora.com>
> > >
> > > Cheers,
> > > Daniel
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Sept. 23, 2020, 11:16 a.m. UTC | #9
On Wed, Sep 23, 2020 at 1:14 PM Marius Vlad <marius.vlad@collabora.com> wrote:
>
> On Wed, Sep 23, 2020 at 12:58:30PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> > >
> > > On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> > > > On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > > > reconfiguring global resources).
> > > > >
> > > > > But in nonblocking mode userspace has then no idea this happened,
> > > > > which can lead to spurious EBUSY calls, both:
> > > > > - when that other CRTC is currently busy doing a page_flip the
> > > > >   ALLOW_MODESET commit can fail with an EBUSY
> > > > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > > > >   of the additional commit inserted by the kernel without userspace's
> > > > >   knowledge
> > > > >
> > > > > For blocking commits this isn't a problem, because everyone else will
> > > > > just block until all the CRTC are reconfigured. Only thing userspace
> > > > > can notice is the dropped frames without any reason for why frames got
> > > > > dropped.
> > > > >
> > > > > Consensus is that we need new uapi to handle this properly, but no one
> > > > > has any idea what exactly the new uapi should look like. As a stop-gap
> > > > > plug this problem by demoting nonblocking commits which might cause
> > > > > issues by including CRTCs not in the original request to blocking
> > > > > commits.
> > > Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> > > I've noticed this isn't integrated/added.
> > >
> > > Noticed this is fixing (also) DPMS when multiple outputs are in use.
> > > Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
> > > the warning quite often.
> >
> > On which driver/chip does this happen?
> I've tried it out on i915.

lspci -nn please.

Also adding Ville, who has an idea where this can all go wrong. The
one he pointed out thus far is gen12+ only though.
-Daniel

> > -Daniel
> >
> > >
> > > >
> > > > Thanks for writing this up Daniel, and for reminding me about it some
> > > > time later as well ...
> > > >
> > > > Reviewed-by: Daniel Stone <daniels@collabora.com>
> > > >
> > > > Cheers,
> > > > Daniel
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
Marius Vlad Sept. 23, 2020, 11:31 a.m. UTC | #10
On Wed, Sep 23, 2020 at 01:16:42PM +0200, Daniel Vetter wrote:
> On Wed, Sep 23, 2020 at 1:14 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 12:58:30PM +0200, Daniel Vetter wrote:
> > > On Tue, Sep 22, 2020 at 3:36 PM Marius Vlad <marius.vlad@collabora.com> wrote:
> > > >
> > > > On Fri, Jan 31, 2020 at 07:34:00AM +0000, Daniel Stone wrote:
> > > > > On Thu, 5 Jul 2018 at 11:21, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > > > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > > > > reconfiguring global resources).
> > > > > >
> > > > > > But in nonblocking mode userspace has then no idea this happened,
> > > > > > which can lead to spurious EBUSY calls, both:
> > > > > > - when that other CRTC is currently busy doing a page_flip the
> > > > > >   ALLOW_MODESET commit can fail with an EBUSY
> > > > > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > > > > >   of the additional commit inserted by the kernel without userspace's
> > > > > >   knowledge
> > > > > >
> > > > > > For blocking commits this isn't a problem, because everyone else will
> > > > > > just block until all the CRTC are reconfigured. Only thing userspace
> > > > > > can notice is the dropped frames without any reason for why frames got
> > > > > > dropped.
> > > > > >
> > > > > > Consensus is that we need new uapi to handle this properly, but no one
> > > > > > has any idea what exactly the new uapi should look like. As a stop-gap
> > > > > > plug this problem by demoting nonblocking commits which might cause
> > > > > > issues by including CRTCs not in the original request to blocking
> > > > > > commits.
> > > > Gentle ping. I've tried out Linus's master tree and, and like Pekka,
> > > > I've noticed this isn't integrated/added.
> > > >
> > > > Noticed this is fixing (also) DPMS when multiple outputs are in use.
> > > > Wondering if we can just use a _ONCE() variant instead of WARN_ON(). I'm seeing
> > > > the warning quite often.
> > >
> > > On which driver/chip does this happen?
> > I've tried it out on i915.
> 
> lspci -nn please.
Sure,

$ lspci -nn
00:00.0 Host bridge [0600]: Intel Corporation Xeon E3-1200 v6/7th Gen Core Processor Host Bridge/DRAM Registers [8086:5914] (rev 08)
00:02.0 VGA compatible controller [0300]: Intel Corporation UHD Graphics 620 [8086:5917] (rev 07)
00:04.0 Signal processing controller [1180]: Intel Corporation Xeon E3-1200 v5/E3-1500 v5/6th Gen Core Processor Thermal Subsystem [8086:1903] (rev 08)
00:14.0 USB controller [0c03]: Intel Corporation Sunrise Point-LP USB 3.0 xHCI Controller [8086:9d2f] (rev 21)
00:14.2 Signal processing controller [1180]: Intel Corporation Sunrise Point-LP Thermal subsystem [8086:9d31] (rev 21)
00:15.0 Signal processing controller [1180]: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 [8086:9d60] (rev 21)
00:15.1 Signal processing controller [1180]: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #1 [8086:9d61] (rev 21)
00:16.0 Communication controller [0780]: Intel Corporation Sunrise Point-LP CSME HECI #1 [8086:9d3a] (rev 21)
00:1c.0 PCI bridge [0604]: Intel Corporation Sunrise Point-LP PCI Express Root Port #1 [8086:9d10] (rev f1)
00:1c.2 PCI bridge [0604]: Intel Corporation Sunrise Point-LP PCI Express Root Port #3 [8086:9d12] (rev f1)
00:1c.4 PCI bridge [0604]: Intel Corporation Sunrise Point-LP PCI Express Root Port #5 [8086:9d14] (rev f1)
00:1d.0 PCI bridge [0604]: Intel Corporation Sunrise Point-LP PCI Express Root Port #9 [8086:9d18] (rev f1)
00:1f.0 ISA bridge [0601]: Intel Corporation Sunrise Point LPC Controller/eSPI Controller [8086:9d4e] (rev 21)
00:1f.2 Memory controller [0580]: Intel Corporation Sunrise Point-LP PMC [8086:9d21] (rev 21)
00:1f.3 Audio device [0403]: Intel Corporation Sunrise Point-LP HD Audio [8086:9d71] (rev 21)
00:1f.4 SMBus [0c05]: Intel Corporation Sunrise Point-LP SMBus [8086:9d23] (rev 21)
01:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. RTS525A PCI Express Card Reader [10ec:525a] (rev 01)
02:00.0 Network controller [0280]: Qualcomm Atheros QCA6174 802.11ac Wireless Network Adapter [168c:003e] (rev 32)
6e:00.0 Non-Volatile memory controller [0108]: Toshiba Corporation Device [1179:0116]

(it's a xps laptop)

> 
> Also adding Ville, who has an idea where this can all go wrong. The
> one he pointed out thus far is gen12+ only though.
> -Daniel
> 
> > > -Daniel
> > >
> > > >
> > > > >
> > > > > Thanks for writing this up Daniel, and for reminding me about it some
> > > > > time later as well ...
> > > > >
> > > > > Reviewed-by: Daniel Stone <daniels@collabora.com>
> > > > >
> > > > > Cheers,
> > > > > Daniel
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d5cefb1cb2a2..3c46a4ef7898 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2018,15 +2018,43 @@  EXPORT_SYMBOL(drm_atomic_commit);
 int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
 {
 	struct drm_mode_config *config = &state->dev->mode_config;
-	int ret;
+	unsigned requested_crtc = 0;
+	unsigned affected_crtc = 0;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	bool nonblocking = true;
+	int ret, i;
+
+	/*
+	 * For commits that allow modesets drivers can add other CRTCs to the
+	 * atomic commit, e.g. when they need to reallocate global resources.
+	 *
+	 * But when userspace also requests a nonblocking commit then userspace
+	 * cannot know that the commit affects other CRTCs, which can result in
+	 * spurious EBUSY failures. Until we have better uapi plug this by
+	 * demoting such commits to blocking mode.
+	 */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+		requested_crtc |= drm_crtc_mask(crtc);
 
 	ret = drm_atomic_check_only(state);
 	if (ret)
 		return ret;
 
-	DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+		affected_crtc |= drm_crtc_mask(crtc);
+
+	if (affected_crtc != requested_crtc) {
+		/* adding other CRTC is only allowed for modeset commits */
+		WARN_ON(!state->allow_modeset);
+
+		DRM_DEBUG_ATOMIC("demoting %p to blocking mode to avoid EBUSY\n", state);
+		nonblocking = false;
+	} else {
+		DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+	}
 
-	return config->funcs->atomic_commit(state->dev, state, true);
+	return config->funcs->atomic_commit(state->dev, state, nonblocking);
 }
 EXPORT_SYMBOL(drm_atomic_nonblocking_commit);