[PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[Daniel Vetter]

The update is horribly racy since it doesn't protect at all against
concurrent closing of the master fd. And it can't really since that
requires us to grab a mutex.

Instead of jumping through hoops and offloading this to a worker
thread just block this bit of code for the modesetting driver.

Reported-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 197db0993a24..0a42e2f7bcf8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -83,6 +83,9 @@ void i915_update_dri1_breadcrumb(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_master_private *master_priv;
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
+
 	if (dev->primary->master) {
 		master_priv = dev->primary->master->driver_priv;
 		if (master_priv->sarea_priv)
-- 
1.8.4.3

Re: [Intel-gfx] [PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[Chris Wilson]

On Tue, Dec 10, 2013 at 01:24:01PM +0100, Daniel Vetter wrote:
> The update is horribly racy since it doesn't protect at all against
> concurrent closing of the master fd. And it can't really since that
> requires us to grab a mutex.
> 
> Instead of jumping through hoops and offloading this to a worker
> thread just block this bit of code for the modesetting driver.
> 
> Reported-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

We should also not be calling update_dri1_breadcrumb unless we have a
USER_INTERRUPT. The other updates to the breadcrumb hws index do not
appear to be serialised by anything other than polling.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[Daniel Vetter]

On Tue, Dec 10, 2013 at 12:44:12PM +0000, Chris Wilson wrote:
> On Tue, Dec 10, 2013 at 01:24:01PM +0100, Daniel Vetter wrote:
> > The update is horribly racy since it doesn't protect at all against
> > concurrent closing of the master fd. And it can't really since that
> > requires us to grab a mutex.
> > 
> > Instead of jumping through hoops and offloading this to a worker
> > thread just block this bit of code for the modesetting driver.
> > 
> > Reported-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> > Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> We should also not be calling update_dri1_breadcrumb unless we have a
> USER_INTERRUPT. The other updates to the breadcrumb hws index do not
> appear to be serialised by anything other than polling.

Well, this is what's been there before and I don't really want to touch
dri1/ums code at all. So I think we'll just keep this here until we'll all
put it on the big pyre ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[Daniel Vetter]

On Tue, Dec 10, 2013 at 02:48:23PM +0100, Daniel Vetter wrote:
> On Tue, Dec 10, 2013 at 12:44:12PM +0000, Chris Wilson wrote:
> > On Tue, Dec 10, 2013 at 01:24:01PM +0100, Daniel Vetter wrote:
> > > The update is horribly racy since it doesn't protect at all against
> > > concurrent closing of the master fd. And it can't really since that
> > > requires us to grab a mutex.
> > > 
> > > Instead of jumping through hoops and offloading this to a worker
> > > thread just block this bit of code for the modesetting driver.
> > > 
> > > Reported-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> > > Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > We should also not be calling update_dri1_breadcrumb unless we have a
> > USER_INTERRUPT. The other updates to the breadcrumb hws index do not
> > appear to be serialised by anything other than polling.
> 
> Well, this is what's been there before and I don't really want to touch
> dri1/ums code at all. So I think we'll just keep this here until we'll all
> put it on the big pyre ;-)

Chris and I discusssed the commit message a bit on irc and now the hack
also spurts a code comment. Merged to -fixes for now, but I'd still like
to get a tested by from Eugene before sending off the pull request.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: don't update the dri1 breadcrumb with modesetting

[Eugene Shatokhin]

On 12/11/2013 03:06 PM, Daniel Vetter wrote:
> On Tue, Dec 10, 2013 at 02:48:23PM +0100, Daniel Vetter wrote:
>> On Tue, Dec 10, 2013 at 12:44:12PM +0000, Chris Wilson wrote:
>>> On Tue, Dec 10, 2013 at 01:24:01PM +0100, Daniel Vetter wrote:
>>>> The update is horribly racy since it doesn't protect at all against
>>>> concurrent closing of the master fd. And it can't really since that
>>>> requires us to grab a mutex.
>>>>
>>>> Instead of jumping through hoops and offloading this to a worker
>>>> thread just block this bit of code for the modesetting driver.
>>>>
>>>> Reported-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
>>>> Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> We should also not be calling update_dri1_breadcrumb unless we have a
>>> USER_INTERRUPT. The other updates to the breadcrumb hws index do not
>>> appear to be serialised by anything other than polling.
>>
>> Well, this is what's been there before and I don't really want to touch
>> dri1/ums code at all. So I think we'll just keep this here until we'll all
>> put it on the big pyre ;-)
>
> Chris and I discusssed the commit message a bit on irc and now the hack
> also spurts a code comment. Merged to -fixes for now, but I'd still like
> to get a tested by from Eugene before sending off the pull request.
> -Daniel
>

Built 3.10.23 with the fix, tested today. The problem has not shown up 
so far.

Tested-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>

-- 
Eugene Shatokhin, ROSA Laboratory.
www.rosalab.com