[7/7] drm/i915: GEM operations need to be done under the big lock
diff mbox

Message ID 20160111150458.GI652@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson Jan. 11, 2016, 3:04 p.m. UTC
On Mon, Jan 11, 2016 at 03:00:20PM +0000, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 02:47:17PM +0000, Tvrtko Ursulin wrote:
> > 
> > On 11/01/16 14:38, Chris Wilson wrote:
> > >On Mon, Jan 11, 2016 at 02:08:41PM +0000, Tvrtko Ursulin wrote:
> > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >>
> > >>VMA creation and GEM list management need the big lock.
> > >>
> > >>v2:
> > >>
> > >>Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> > >>
> > >>Not to mention drm_gem_object_unreference was there in existing
> > >>code with no mutex held.
> > >>
> > >>v3:
> > >>
> > >>Some callers of i915_gem_object_create_stolen_for_preallocated
> > >>already hold the lock so move the mutex into the other caller
> > >>as well.
> > >
> > >intel_pm.c valleyview_setup_pctx?
> > 
> > Already holds it traced by the WARN_ON at its top.
> 
> Which is a nice little mutex inversion of its own. :|
> i.e. rpm vs struct_mutex bug

-Chris

Comments

Tvrtko Ursulin Jan. 11, 2016, 3:16 p.m. UTC | #1
On 11/01/16 15:04, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 03:00:20PM +0000, Chris Wilson wrote:
>> On Mon, Jan 11, 2016 at 02:47:17PM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 11/01/16 14:38, Chris Wilson wrote:
>>>> On Mon, Jan 11, 2016 at 02:08:41PM +0000, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> VMA creation and GEM list management need the big lock.
>>>>>
>>>>> v2:
>>>>>
>>>>> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
>>>>>
>>>>> Not to mention drm_gem_object_unreference was there in existing
>>>>> code with no mutex held.
>>>>>
>>>>> v3:
>>>>>
>>>>> Some callers of i915_gem_object_create_stolen_for_preallocated
>>>>> already hold the lock so move the mutex into the other caller
>>>>> as well.
>>>>
>>>> intel_pm.c valleyview_setup_pctx?
>>>
>>> Already holds it traced by the WARN_ON at its top.
>>
>> Which is a nice little mutex inversion of its own. :|
>> i.e. rpm vs struct_mutex bug
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1fb43fcfeea..c8f684f8799c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16018,9 +16018,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>          struct drm_crtc *c;
>          struct drm_i915_gem_object *obj;
>
> -       mutex_lock(&dev->struct_mutex);
>          intel_init_gt_powersave(dev);
> -       mutex_unlock(&dev->struct_mutex);
>
>          intel_modeset_init_hw(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a082b4577599..90e5bf7a2402 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5224,8 +5224,6 @@ static void cherryview_setup_pctx(struct drm_device *dev)
>          u32 pcbr;
>          int pctx_size = 32*1024;
>
> -       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
>          pcbr = I915_READ(VLV_PCBR);
>          if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
>                  DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
> @@ -5247,8 +5245,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>          u32 pcbr;
>          int pctx_size = 24*1024;
>
> -       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> +       mutex_lock(&dev->struct_mutex);
>          pcbr = I915_READ(VLV_PCBR);
>          if (pcbr) {
>                  /* BIOS set it up already, grab the pre-alloc'd space */
> @@ -5275,7 +5272,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>          pctx = i915_gem_object_create_stolen(dev, pctx_size);
>          if (!pctx) {
>                  DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
> -               return;
> +               goto out;
>          }
>
>          pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
> @@ -5284,6 +5281,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>   out:
>          DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
>          dev_priv->vlv_pctx = pctx;
> +       mutex_unlock(&dev->struct_mutex);
>   }
>
>   static void valleyview_cleanup_pctx(struct drm_device *dev)

Don't know, I leave this one to whoever grabbed the lock around 
intel_init_gt_powersave in the first place. Maybe there was a special 
reason.. after git blame od intel_display.c eventually completed, adding 
Imre and Ville to cc.

Regards,

Tvrtko
Ville Syrjälä Jan. 11, 2016, 3:36 p.m. UTC | #2
On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 11/01/16 15:04, Chris Wilson wrote:
> > On Mon, Jan 11, 2016 at 03:00:20PM +0000, Chris Wilson wrote:
> >> On Mon, Jan 11, 2016 at 02:47:17PM +0000, Tvrtko Ursulin wrote:
> >>>
> >>> On 11/01/16 14:38, Chris Wilson wrote:
> >>>> On Mon, Jan 11, 2016 at 02:08:41PM +0000, Tvrtko Ursulin wrote:
> >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>
> >>>>> VMA creation and GEM list management need the big lock.
> >>>>>
> >>>>> v2:
> >>>>>
> >>>>> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> >>>>>
> >>>>> Not to mention drm_gem_object_unreference was there in existing
> >>>>> code with no mutex held.
> >>>>>
> >>>>> v3:
> >>>>>
> >>>>> Some callers of i915_gem_object_create_stolen_for_preallocated
> >>>>> already hold the lock so move the mutex into the other caller
> >>>>> as well.
> >>>>
> >>>> intel_pm.c valleyview_setup_pctx?
> >>>
> >>> Already holds it traced by the WARN_ON at its top.
> >>
> >> Which is a nice little mutex inversion of its own. :|
> >> i.e. rpm vs struct_mutex bug
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b1fb43fcfeea..c8f684f8799c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -16018,9 +16018,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
> >          struct drm_crtc *c;
> >          struct drm_i915_gem_object *obj;
> >
> > -       mutex_lock(&dev->struct_mutex);
> >          intel_init_gt_powersave(dev);
> > -       mutex_unlock(&dev->struct_mutex);
> >
> >          intel_modeset_init_hw(dev);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a082b4577599..90e5bf7a2402 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5224,8 +5224,6 @@ static void cherryview_setup_pctx(struct drm_device *dev)
> >          u32 pcbr;
> >          int pctx_size = 32*1024;
> >
> > -       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > -
> >          pcbr = I915_READ(VLV_PCBR);
> >          if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
> >                  DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
> > @@ -5247,8 +5245,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
> >          u32 pcbr;
> >          int pctx_size = 24*1024;
> >
> > -       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > -
> > +       mutex_lock(&dev->struct_mutex);
> >          pcbr = I915_READ(VLV_PCBR);
> >          if (pcbr) {
> >                  /* BIOS set it up already, grab the pre-alloc'd space */
> > @@ -5275,7 +5272,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
> >          pctx = i915_gem_object_create_stolen(dev, pctx_size);
> >          if (!pctx) {
> >                  DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
> > -               return;
> > +               goto out;
> >          }
> >
> >          pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
> > @@ -5284,6 +5281,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
> >   out:
> >          DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
> >          dev_priv->vlv_pctx = pctx;
> > +       mutex_unlock(&dev->struct_mutex);
> >   }
> >
> >   static void valleyview_cleanup_pctx(struct drm_device *dev)
> 
> Don't know, I leave this one to whoever grabbed the lock around 
> intel_init_gt_powersave in the first place. Maybe there was a special 
> reason.. after git blame od intel_display.c eventually completed, adding 
> Imre and Ville to cc.

Hmm. I don't recall the details anymore, but looking at the code pushing
the locking down to valleyview_setup_pctx() looks entirely reasonable to
me. And yeah, doesn't look like it's really protecting anything in the
chv function, so can just be dropped there.

The cleanup path could use the same treatemnt I think.
Chris Wilson Jan. 11, 2016, 4:56 p.m. UTC | #3
On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
> > Don't know, I leave this one to whoever grabbed the lock around 
> > intel_init_gt_powersave in the first place. Maybe there was a special 
> > reason.. after git blame od intel_display.c eventually completed, adding 
> > Imre and Ville to cc.
> 
> Hmm. I don't recall the details anymore, but looking at the code pushing
> the locking down to valleyview_setup_pctx() looks entirely reasonable to
> me.

iirc, this locking only exists to keep the WARN() at bay. But it is
pedagogical, I guess.
-Chris
Tvrtko Ursulin Jan. 13, 2016, 12:46 p.m. UTC | #4
On 11/01/16 16:56, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote:
>> On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
>>> Don't know, I leave this one to whoever grabbed the lock around
>>> intel_init_gt_powersave in the first place. Maybe there was a special
>>> reason.. after git blame od intel_display.c eventually completed, adding
>>> Imre and Ville to cc.
>>
>> Hmm. I don't recall the details anymore, but looking at the code pushing
>> the locking down to valleyview_setup_pctx() looks entirely reasonable to
>> me.
>
> iirc, this locking only exists to keep the WARN() at bay. But it is
> pedagogical, I guess.

Don't really know this area, but what about the 
intel_gen6_powersave_work->valleyview_enable_rps->valleyview_check_pctx 
which dereferences the dev_priv->vlv_pctx, which is set/cleared in 
valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be 
outside both struct_mutex and the rps lock?

Regards,

Tvrtko
Imre Deak Jan. 13, 2016, 1:36 p.m. UTC | #5
On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
> On 11/01/16 16:56, Chris Wilson wrote:
> > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
> > > > Don't know, I leave this one to whoever grabbed the lock around
> > > > intel_init_gt_powersave in the first place. Maybe there was a
> > > > special
> > > > reason.. after git blame od intel_display.c eventually
> > > > completed, adding
> > > > Imre and Ville to cc.
> > > 
> > > Hmm. I don't recall the details anymore, but looking at the code
> > > pushing
> > > the locking down to valleyview_setup_pctx() looks entirely
> > > reasonable to
> > > me.
> > 
> > iirc, this locking only exists to keep the WARN() at bay. But it is
> > pedagogical, I guess.
> 
> Don't really know this area, but what about the 
> intel_gen6_powersave_work->valleyview_enable_rps-
> >valleyview_check_pctx 
> which dereferences the dev_priv->vlv_pctx, which is set/cleared in 
> valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be 
> outside both struct_mutex and the rps lock?

dev_priv->vlv_pctx is not protected on the premise that the driver
init/cleanup functions can't race and gen6_powersave_work() is
scheduled only after intel_init_gt_powersave() and flushed
before intel_cleanup_gt_powersave().

rps_lock protects the RPS HW accesses themselves and struct_mutex was
taken for the GEM allocation. Taking it at high level around 
intel_init_gt_powersave() was kind of a copy&paste in the commit you
found, there is more on that in 79f5b2c75992.

--Imre
Tvrtko Ursulin Jan. 13, 2016, 2:11 p.m. UTC | #6
On 13/01/16 13:36, Imre Deak wrote:
> On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
>> On 11/01/16 16:56, Chris Wilson wrote:
>>> On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote:
>>>> On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
>>>>> Don't know, I leave this one to whoever grabbed the lock around
>>>>> intel_init_gt_powersave in the first place. Maybe there was a
>>>>> special
>>>>> reason.. after git blame od intel_display.c eventually
>>>>> completed, adding
>>>>> Imre and Ville to cc.
>>>>
>>>> Hmm. I don't recall the details anymore, but looking at the code
>>>> pushing
>>>> the locking down to valleyview_setup_pctx() looks entirely
>>>> reasonable to
>>>> me.
>>>
>>> iirc, this locking only exists to keep the WARN() at bay. But it is
>>> pedagogical, I guess.
>>
>> Don't really know this area, but what about the
>> intel_gen6_powersave_work->valleyview_enable_rps-
>>> valleyview_check_pctx
>> which dereferences the dev_priv->vlv_pctx, which is set/cleared in
>> valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be
>> outside both struct_mutex and the rps lock?
>
> dev_priv->vlv_pctx is not protected on the premise that the driver
> init/cleanup functions can't race and gen6_powersave_work() is
> scheduled only after intel_init_gt_powersave() and flushed
> before intel_cleanup_gt_powersave().
>
> rps_lock protects the RPS HW accesses themselves and struct_mutex was
> taken for the GEM allocation. Taking it at high level around
> intel_init_gt_powersave() was kind of a copy&paste in the commit you
> found, there is more on that in 79f5b2c75992.

Thanks for digging this out.

It is more involved than what Chris pasted then, and while I hoped to be 
able to quickly shove that into a patch, I cannot allow the time for 
more the extensive analysis.

So my patch fixes the issue of struct_mutex not being held in 
intel_alloc_initial_plane_obj while calling 
i915_gem_object_create_stolen_for_preallocated and I'll leave 
struct_mutex/rps.lock locking inversion in RPM to someone else.

Regards,

Tvrtko
Chris Wilson Jan. 13, 2016, 2:32 p.m. UTC | #7
On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
> 
> On 13/01/16 13:36, Imre Deak wrote:
> >On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
> >>On 11/01/16 16:56, Chris Wilson wrote:
> >>>On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote:
> >>>>On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote:
> >>>>>Don't know, I leave this one to whoever grabbed the lock around
> >>>>>intel_init_gt_powersave in the first place. Maybe there was a
> >>>>>special
> >>>>>reason.. after git blame od intel_display.c eventually
> >>>>>completed, adding
> >>>>>Imre and Ville to cc.
> >>>>
> >>>>Hmm. I don't recall the details anymore, but looking at the code
> >>>>pushing
> >>>>the locking down to valleyview_setup_pctx() looks entirely
> >>>>reasonable to
> >>>>me.
> >>>
> >>>iirc, this locking only exists to keep the WARN() at bay. But it is
> >>>pedagogical, I guess.
> >>
> >>Don't really know this area, but what about the
> >>intel_gen6_powersave_work->valleyview_enable_rps-
> >>>valleyview_check_pctx
> >>which dereferences the dev_priv->vlv_pctx, which is set/cleared in
> >>valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be
> >>outside both struct_mutex and the rps lock?
> >
> >dev_priv->vlv_pctx is not protected on the premise that the driver
> >init/cleanup functions can't race and gen6_powersave_work() is
> >scheduled only after intel_init_gt_powersave() and flushed
> >before intel_cleanup_gt_powersave().
> >
> >rps_lock protects the RPS HW accesses themselves and struct_mutex was
> >taken for the GEM allocation. Taking it at high level around
> >intel_init_gt_powersave() was kind of a copy&paste in the commit you
> >found, there is more on that in 79f5b2c75992.
> 
> Thanks for digging this out.
> 
> It is more involved than what Chris pasted then, and while I hoped
> to be able to quickly shove that into a patch, I cannot allow the
> time for more the extensive analysis.

Imre just confirmed that struct_mutex is only for the GEM
allocations in the vlv_setup_ctx, right Imre?
-Chris
Imre Deak Jan. 13, 2016, 2:41 p.m. UTC | #8
On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote:
> On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
> > 
> > On 13/01/16 13:36, Imre Deak wrote:
> > > On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
> > > > On 11/01/16 16:56, Chris Wilson wrote:
> > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä
> > > > > wrote:
> > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin
> > > > > > wrote:
> > > > > > > Don't know, I leave this one to whoever grabbed the lock
> > > > > > > around
> > > > > > > intel_init_gt_powersave in the first place. Maybe there
> > > > > > > was a
> > > > > > > special
> > > > > > > reason.. after git blame od intel_display.c eventually
> > > > > > > completed, adding
> > > > > > > Imre and Ville to cc.
> > > > > > 
> > > > > > Hmm. I don't recall the details anymore, but looking at the
> > > > > > code
> > > > > > pushing
> > > > > > the locking down to valleyview_setup_pctx() looks entirely
> > > > > > reasonable to
> > > > > > me.
> > > > > 
> > > > > iirc, this locking only exists to keep the WARN() at bay. But
> > > > > it is
> > > > > pedagogical, I guess.
> > > > 
> > > > Don't really know this area, but what about the
> > > > intel_gen6_powersave_work->valleyview_enable_rps-
> > > > > valleyview_check_pctx
> > > > which dereferences the dev_priv->vlv_pctx, which is set/cleared
> > > > in
> > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would now
> > > > be
> > > > outside both struct_mutex and the rps lock?
> > > 
> > > dev_priv->vlv_pctx is not protected on the premise that the
> > > driver
> > > init/cleanup functions can't race and gen6_powersave_work() is
> > > scheduled only after intel_init_gt_powersave() and flushed
> > > before intel_cleanup_gt_powersave().
> > > 
> > > rps_lock protects the RPS HW accesses themselves and struct_mutex
> > > was
> > > taken for the GEM allocation. Taking it at high level around
> > > intel_init_gt_powersave() was kind of a copy&paste in the commit
> > > you
> > > found, there is more on that in 79f5b2c75992.
> > 
> > Thanks for digging this out.
> > 
> > It is more involved than what Chris pasted then, and while I hoped
> > to be able to quickly shove that into a patch, I cannot allow the
> > time for more the extensive analysis.
> 
> Imre just confirmed that struct_mutex is only for the GEM
> allocations in the vlv_setup_ctx, right Imre?

Yes, that's my understanding.

--Imre
Tvrtko Ursulin Jan. 13, 2016, 2:53 p.m. UTC | #9
On 13/01/16 14:41, Imre Deak wrote:
> On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote:
>> On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 13/01/16 13:36, Imre Deak wrote:
>>>> On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
>>>>> On 11/01/16 16:56, Chris Wilson wrote:
>>>>>> On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä
>>>>>> wrote:
>>>>>>> On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin
>>>>>>> wrote:
>>>>>>>> Don't know, I leave this one to whoever grabbed the lock
>>>>>>>> around
>>>>>>>> intel_init_gt_powersave in the first place. Maybe there
>>>>>>>> was a
>>>>>>>> special
>>>>>>>> reason.. after git blame od intel_display.c eventually
>>>>>>>> completed, adding
>>>>>>>> Imre and Ville to cc.
>>>>>>>
>>>>>>> Hmm. I don't recall the details anymore, but looking at the
>>>>>>> code
>>>>>>> pushing
>>>>>>> the locking down to valleyview_setup_pctx() looks entirely
>>>>>>> reasonable to
>>>>>>> me.
>>>>>>
>>>>>> iirc, this locking only exists to keep the WARN() at bay. But
>>>>>> it is
>>>>>> pedagogical, I guess.
>>>>>
>>>>> Don't really know this area, but what about the
>>>>> intel_gen6_powersave_work->valleyview_enable_rps-
>>>>>> valleyview_check_pctx
>>>>> which dereferences the dev_priv->vlv_pctx, which is set/cleared
>>>>> in
>>>>> valleyview_setup_pctx/valleyview_cleanup_pctx, which would now
>>>>> be
>>>>> outside both struct_mutex and the rps lock?
>>>>
>>>> dev_priv->vlv_pctx is not protected on the premise that the
>>>> driver
>>>> init/cleanup functions can't race and gen6_powersave_work() is
>>>> scheduled only after intel_init_gt_powersave() and flushed
>>>> before intel_cleanup_gt_powersave().
>>>>
>>>> rps_lock protects the RPS HW accesses themselves and struct_mutex
>>>> was
>>>> taken for the GEM allocation. Taking it at high level around
>>>> intel_init_gt_powersave() was kind of a copy&paste in the commit
>>>> you
>>>> found, there is more on that in 79f5b2c75992.
>>>
>>> Thanks for digging this out.
>>>
>>> It is more involved than what Chris pasted then, and while I hoped
>>> to be able to quickly shove that into a patch, I cannot allow the
>>> time for more the extensive analysis.
>>
>> Imre just confirmed that struct_mutex is only for the GEM
>> allocations in the vlv_setup_ctx, right Imre?
>
> Yes, that's my understanding.

I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking from 
lower down to the top level so it sounded like doing the reverse would 
not be straightforward. Perhaps I overestimated it.

Regards,

Tvrtko
Imre Deak Jan. 13, 2016, 3:25 p.m. UTC | #10
On ke, 2016-01-13 at 14:53 +0000, Tvrtko Ursulin wrote:
> On 13/01/16 14:41, Imre Deak wrote:
> > On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote:
> > > On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
> > > > 
> > > > On 13/01/16 13:36, Imre Deak wrote:
> > > > > On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
> > > > > > On 11/01/16 16:56, Chris Wilson wrote:
> > > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä
> > > > > > > wrote:
> > > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko
> > > > > > > > Ursulin
> > > > > > > > wrote:
> > > > > > > > > Don't know, I leave this one to whoever grabbed the
> > > > > > > > > lock
> > > > > > > > > around
> > > > > > > > > intel_init_gt_powersave in the first place. Maybe
> > > > > > > > > there
> > > > > > > > > was a
> > > > > > > > > special
> > > > > > > > > reason.. after git blame od intel_display.c
> > > > > > > > > eventually
> > > > > > > > > completed, adding
> > > > > > > > > Imre and Ville to cc.
> > > > > > > > 
> > > > > > > > Hmm. I don't recall the details anymore, but looking at
> > > > > > > > the
> > > > > > > > code
> > > > > > > > pushing
> > > > > > > > the locking down to valleyview_setup_pctx() looks
> > > > > > > > entirely
> > > > > > > > reasonable to
> > > > > > > > me.
> > > > > > > 
> > > > > > > iirc, this locking only exists to keep the WARN() at bay.
> > > > > > > But
> > > > > > > it is
> > > > > > > pedagogical, I guess.
> > > > > > 
> > > > > > Don't really know this area, but what about the
> > > > > > intel_gen6_powersave_work->valleyview_enable_rps-
> > > > > > > valleyview_check_pctx
> > > > > > which dereferences the dev_priv->vlv_pctx, which is
> > > > > > set/cleared
> > > > > > in
> > > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would
> > > > > > now
> > > > > > be
> > > > > > outside both struct_mutex and the rps lock?
> > > > > 
> > > > > dev_priv->vlv_pctx is not protected on the premise that the
> > > > > driver
> > > > > init/cleanup functions can't race and gen6_powersave_work()
> > > > > is
> > > > > scheduled only after intel_init_gt_powersave() and flushed
> > > > > before intel_cleanup_gt_powersave().
> > > > > 
> > > > > rps_lock protects the RPS HW accesses themselves and
> > > > > struct_mutex
> > > > > was
> > > > > taken for the GEM allocation. Taking it at high level around
> > > > > intel_init_gt_powersave() was kind of a copy&paste in the
> > > > > commit
> > > > > you
> > > > > found, there is more on that in 79f5b2c75992.
> > > > 
> > > > Thanks for digging this out.
> > > > 
> > > > It is more involved than what Chris pasted then, and while I
> > > > hoped
> > > > to be able to quickly shove that into a patch, I cannot allow
> > > > the
> > > > time for more the extensive analysis.
> > > 
> > > Imre just confirmed that struct_mutex is only for the GEM
> > > allocations in the vlv_setup_ctx, right Imre?
> > 
> > Yes, that's my understanding.
> 
> I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking
> from 
> lower down to the top level so it sounded like doing the reverse
> would 
> not be straightforward. Perhaps I overestimated it.

Ok, some more background info :)

Initially the HW access was also protected by struct_mutex but
4fc688ce7 added rps_lock for just the the HW access. So as I understand
we don't need struct_mutex for anything else besides the GEM
allocation. So feel free to add my ACK on the change moving the lock to
vlv_setup_ctx().

Btw, we could also remove struct_mutex from intel_enable_gt_powersave()
where it's taken for old platforms and rely on mchdev_lock or take
rps_lock instead, but someone needs to double-check this.

--Imre
Daniel Vetter Jan. 13, 2016, 3:55 p.m. UTC | #11
On Wed, Jan 13, 2016 at 05:25:09PM +0200, Imre Deak wrote:
> On ke, 2016-01-13 at 14:53 +0000, Tvrtko Ursulin wrote:
> > On 13/01/16 14:41, Imre Deak wrote:
> > > On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote:
> > > > On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 13/01/16 13:36, Imre Deak wrote:
> > > > > > On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
> > > > > > > On 11/01/16 16:56, Chris Wilson wrote:
> > > > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä
> > > > > > > > wrote:
> > > > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko
> > > > > > > > > Ursulin
> > > > > > > > > wrote:
> > > > > > > > > > Don't know, I leave this one to whoever grabbed the
> > > > > > > > > > lock
> > > > > > > > > > around
> > > > > > > > > > intel_init_gt_powersave in the first place. Maybe
> > > > > > > > > > there
> > > > > > > > > > was a
> > > > > > > > > > special
> > > > > > > > > > reason.. after git blame od intel_display.c
> > > > > > > > > > eventually
> > > > > > > > > > completed, adding
> > > > > > > > > > Imre and Ville to cc.
> > > > > > > > > 
> > > > > > > > > Hmm. I don't recall the details anymore, but looking at
> > > > > > > > > the
> > > > > > > > > code
> > > > > > > > > pushing
> > > > > > > > > the locking down to valleyview_setup_pctx() looks
> > > > > > > > > entirely
> > > > > > > > > reasonable to
> > > > > > > > > me.
> > > > > > > > 
> > > > > > > > iirc, this locking only exists to keep the WARN() at bay.
> > > > > > > > But
> > > > > > > > it is
> > > > > > > > pedagogical, I guess.
> > > > > > > 
> > > > > > > Don't really know this area, but what about the
> > > > > > > intel_gen6_powersave_work->valleyview_enable_rps-
> > > > > > > > valleyview_check_pctx
> > > > > > > which dereferences the dev_priv->vlv_pctx, which is
> > > > > > > set/cleared
> > > > > > > in
> > > > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would
> > > > > > > now
> > > > > > > be
> > > > > > > outside both struct_mutex and the rps lock?
> > > > > > 
> > > > > > dev_priv->vlv_pctx is not protected on the premise that the
> > > > > > driver
> > > > > > init/cleanup functions can't race and gen6_powersave_work()
> > > > > > is
> > > > > > scheduled only after intel_init_gt_powersave() and flushed
> > > > > > before intel_cleanup_gt_powersave().
> > > > > > 
> > > > > > rps_lock protects the RPS HW accesses themselves and
> > > > > > struct_mutex
> > > > > > was
> > > > > > taken for the GEM allocation. Taking it at high level around
> > > > > > intel_init_gt_powersave() was kind of a copy&paste in the
> > > > > > commit
> > > > > > you
> > > > > > found, there is more on that in 79f5b2c75992.
> > > > > 
> > > > > Thanks for digging this out.
> > > > > 
> > > > > It is more involved than what Chris pasted then, and while I
> > > > > hoped
> > > > > to be able to quickly shove that into a patch, I cannot allow
> > > > > the
> > > > > time for more the extensive analysis.
> > > > 
> > > > Imre just confirmed that struct_mutex is only for the GEM
> > > > allocations in the vlv_setup_ctx, right Imre?
> > > 
> > > Yes, that's my understanding.
> > 
> > I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking
> > from 
> > lower down to the top level so it sounded like doing the reverse
> > would 
> > not be straightforward. Perhaps I overestimated it.
> 
> Ok, some more background info :)
> 
> Initially the HW access was also protected by struct_mutex but
> 4fc688ce7 added rps_lock for just the the HW access. So as I understand
> we don't need struct_mutex for anything else besides the GEM
> allocation. So feel free to add my ACK on the change moving the lock to
> vlv_setup_ctx().
> 
> Btw, we could also remove struct_mutex from intel_enable_gt_powersave()
> where it's taken for old platforms and rely on mchdev_lock or take
> rps_lock instead, but someone needs to double-check this.

For more context we needed dev->struct_mutex for ilk rc6 support, which
needed a powerctx allocated from gem (not just stolen). That support died
in fire in

commit a561165493e5fec2f74bd3ae0577ed659e44ab7f
Author: John Harrison <John.C.Harrison@Intel.com>
Date:   Thu Mar 5 14:03:03 2015 +0000

    drm/i915: Remove ironlake rc6 support

Maybe reference that too, on top of the commit Imre dug out?

Thanks, Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1fb43fcfeea..c8f684f8799c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16018,9 +16018,7 @@  void intel_modeset_gem_init(struct drm_device *dev)
        struct drm_crtc *c;
        struct drm_i915_gem_object *obj;
 
-       mutex_lock(&dev->struct_mutex);
        intel_init_gt_powersave(dev);
-       mutex_unlock(&dev->struct_mutex);
 
        intel_modeset_init_hw(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a082b4577599..90e5bf7a2402 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5224,8 +5224,6 @@  static void cherryview_setup_pctx(struct drm_device *dev)
        u32 pcbr;
        int pctx_size = 32*1024;
 
-       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
        pcbr = I915_READ(VLV_PCBR);
        if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
                DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
@@ -5247,8 +5245,7 @@  static void valleyview_setup_pctx(struct drm_device *dev)
        u32 pcbr;
        int pctx_size = 24*1024;
 
-       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
+       mutex_lock(&dev->struct_mutex);
        pcbr = I915_READ(VLV_PCBR);
        if (pcbr) {
                /* BIOS set it up already, grab the pre-alloc'd space */
@@ -5275,7 +5272,7 @@  static void valleyview_setup_pctx(struct drm_device *dev)
        pctx = i915_gem_object_create_stolen(dev, pctx_size);
        if (!pctx) {
                DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
-               return;
+               goto out;
        }
 
        pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
@@ -5284,6 +5281,7 @@  static void valleyview_setup_pctx(struct drm_device *dev)
 out:
        DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
        dev_priv->vlv_pctx = pctx;
+       mutex_unlock(&dev->struct_mutex);
 }
 
 static void valleyview_cleanup_pctx(struct drm_device *dev)