[PATCH 0/6] drm-intel-collector - update

[PATCH 0/6] drm-intel-collector - update

[Rodrigo Vivi]

This is another drm-intel-collector updated notice:
http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=drm-intel-collector

Here goes the update list in order for better reviewers assignment:

Patch     drm/i915: Bring UP Power Wells before disabling RC6. - Reviewer:
Patch     drm/i915: dma_buf_vunmap is presumed not to fail, don't let it - Reviewer:
Patch     drm/i915: Add support for stealing purgable stolen pages - Reviewer:
Patch     drm/i915: add flag to prevent dmesg spam on context banning - Reviewer:
Patch     drm/i915: Do not allow a pending forcewake put to unbalance across reset - Reviewer:
Patch     drm/i915: Don't save/restore RS when not used - Reviewer:

Overall Process:

drm-intel-collector - review request
 1. Daniel pushs drm-intel-testing (every 2 weeks)
 2. I rebase drm-intel-collector onto drm-intel-testing
 3. Add Reviewer: tag with voluntered reviewers. If you don't believe you should be assigned on a particular patch please don't get mad just tell you wont review or volunteer someone else.
 4. I resubmit remaining patches for review without picking any new (drm-intel-collector - review request)

drm-intel-collector - updated
 5. One week later I collect all simple (1-2) patches that wasn't yet reviewed and not queued by Daniel from one testing update until another.
 6. Request automated QA's PRTS automated i-g-t tests comparing drm-intel-testing x drm-intel-collector
 7. If tests are ok I send the update notification or the patches as a series to intel-gfx mailing list for better tracking and to be reviewed. (drm-intel-collector - updated)
 8. Let me know volunteers for review new patches and also let me know if I've picked any patch that I shouldn't.

There are some reasons that some patches can be left behind:
1. Your patch didn't applied cleanly and I couldn't easily solve the conflicts.
2. Kernel didn't compiled with your patch.
3. I simply missed it. If you believe this is the case please warn me.

Please help me to get these patches reviewed and queued by Daniel.

Also, please let me know if you have further ideas how to improve this process.

Thanks in advance,
Rodrigo.


Ben Widawsky (1):
  drm/i915: Don't save/restore RS when not used

Chris Wilson (3):
  drm/i915: dma_buf_vunmap is presumed not to fail, don't let it
  drm/i915: Add support for stealing purgable stolen pages
  drm/i915: Do not allow a pending forcewake put to unbalance across
    reset

Deepak S (1):
  drm/i915: Bring UP Power Wells before disabling RC6.

Mika Kuoppala (1):
  drm/i915: add flag to prevent dmesg spam on context banning

 drivers/gpu/drm/i915/i915_drv.h         |  18 ++++-
 drivers/gpu/drm/i915/i915_gem.c         |   5 +-
 drivers/gpu/drm/i915/i915_gem_context.c |  12 ++--
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  |   6 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c  | 119 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_pm.c         |   6 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   2 +-
 drivers/gpu/drm/i915/intel_uncore.c     |   5 +-
 8 files changed, 146 insertions(+), 27 deletions(-)

-- 
1.9.0

[PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Rodrigo Vivi]

From: Deepak S <deepak.s@intel.com>

We need do forcewake before Disabling RC6, This is what the BIOS
expects while going into suspend.

v2: updated commit message. (Daniel)

Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 04af065..ad2ff99 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* we're doing forcewake before Disabling RC6,
+	 * This what the BIOS expects when going into suspend */
+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+
 	gen6_disable_rps_interrupts(dev);
 }
 
-- 
1.9.0

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Ben Widawsky]

On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> We need do forcewake before Disabling RC6, This is what the BIOS
> expects while going into suspend.
> 
> v2: updated commit message. (Daniel)
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 04af065..ad2ff99 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	/* we're doing forcewake before Disabling RC6,
> +	 * This what the BIOS expects when going into suspend */
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
>  
> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +
>  	gen6_disable_rps_interrupts(dev);
>  }
>  

Isn't the forcewake done as part of I915_WRITE sufficient?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Ville Syrjälä]

On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
> > From: Deepak S <deepak.s@intel.com>
> > 
> > We need do forcewake before Disabling RC6, This is what the BIOS
> > expects while going into suspend.
> > 
> > v2: updated commit message. (Daniel)
> > 
> > Signed-off-by: Deepak S <deepak.s@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 04af065..ad2ff99 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	/* we're doing forcewake before Disabling RC6,
> > +	 * This what the BIOS expects when going into suspend */
> > +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +
> >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >  
> > +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +
> >  	gen6_disable_rps_interrupts(dev);
> >  }
> >  
> 
> Isn't the forcewake done as part of I915_WRITE sufficient?

Writes don't do forcewake, nor is the register even part of the
VLV forcewake ranges.

I guess the rationale for this patche is still a bit vague. But if it's
really needed, I wonder whether we should do this same dance for !VLV
too? Do we have any "GPU stuck in wrong power state after suspend" type of
bugs still around?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[S, Deepak]

On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
> On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
>> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
>>> From: Deepak S <deepak.s@intel.com>
>>>
>>> We need do forcewake before Disabling RC6, This is what the BIOS
>>> expects while going into suspend.
>>>
>>> v2: updated commit message. (Daniel)
>>>
>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 04af065..ad2ff99 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
>>>   {
>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>
>>> +	/* we're doing forcewake before Disabling RC6,
>>> +	 * This what the BIOS expects when going into suspend */
>>> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>>> +
>>>   	I915_WRITE(GEN6_RC_CONTROL, 0);
>>>
>>> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>>> +
>>>   	gen6_disable_rps_interrupts(dev);
>>>   }
>>>
>>
>> Isn't the forcewake done as part of I915_WRITE sufficient?
>
> Writes don't do forcewake, nor is the register even part of the
> VLV forcewake ranges.
>
> I guess the rationale for this patche is still a bit vague. But if it's
> really needed, I wonder whether we should do this same dance for !VLV
> too? Do we have any "GPU stuck in wrong power state after suspend" type of
> bugs still around?

One of suggestion form the HW team was to Bring the wells up before we 
disable RC6 at run-time. We did see some issue when we enabled D0ix.

I think the is a good practice to make sure we bring-up the wells before 
we disable RC6. At least this avoids the cases where wells are not up 
before we can access the Next register after disable.

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Ben Widawsky]

On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote:
> 
> 
> On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
> >On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
> >>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
> >>>From: Deepak S <deepak.s@intel.com>
> >>>
> >>>We need do forcewake before Disabling RC6, This is what the BIOS
> >>>expects while going into suspend.
> >>>
> >>>v2: updated commit message. (Daniel)
> >>>
> >>>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >>>---
> >>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>index 04af065..ad2ff99 100644
> >>>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
> >>>  {
> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>
> >>>+	/* we're doing forcewake before Disabling RC6,
> >>>+	 * This what the BIOS expects when going into suspend */
> >>>+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >>>+
> >>>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >>>
> >>>+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >>>+
> >>>  	gen6_disable_rps_interrupts(dev);
> >>>  }
> >>>
> >>
> >>Isn't the forcewake done as part of I915_WRITE sufficient?
> >
> >Writes don't do forcewake, nor is the register even part of the
> >VLV forcewake ranges.
> >
> >I guess the rationale for this patche is still a bit vague. But if it's
> >really needed, I wonder whether we should do this same dance for !VLV
> >too? Do we have any "GPU stuck in wrong power state after suspend" type of
> >bugs still around?
> 
> One of suggestion form the HW team was to Bring the wells up before we
> disable RC6 at run-time. We did see some issue when we enabled D0ix.
> 
> I think the is a good practice to make sure we bring-up the wells before we
> disable RC6. At least this avoids the cases where wells are not up before we
> can access the Next register after disable.

Ville was totally right. I do think a POSTING_READ is still sufficient.
Don't care much either way.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[S, Deepak]

On 4/9/2014 9:43 AM, Ben Widawsky wrote:
> On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote:
>>
>>
>> On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
>>> On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
>>>> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
>>>>> From: Deepak S <deepak.s@intel.com>
>>>>>
>>>>> We need do forcewake before Disabling RC6, This is what the BIOS
>>>>> expects while going into suspend.
>>>>>
>>>>> v2: updated commit message. (Daniel)
>>>>>
>>>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>>>>>   1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>> index 04af065..ad2ff99 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
>>>>>   {
>>>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>
>>>>> +	/* we're doing forcewake before Disabling RC6,
>>>>> +	 * This what the BIOS expects when going into suspend */
>>>>> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>>>>> +
>>>>>   	I915_WRITE(GEN6_RC_CONTROL, 0);
>>>>>
>>>>> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>>>>> +
>>>>>   	gen6_disable_rps_interrupts(dev);
>>>>>   }
>>>>>
>>>>
>>>> Isn't the forcewake done as part of I915_WRITE sufficient?
>>>
>>> Writes don't do forcewake, nor is the register even part of the
>>> VLV forcewake ranges.
>>>
>>> I guess the rationale for this patche is still a bit vague. But if it's
>>> really needed, I wonder whether we should do this same dance for !VLV
>>> too? Do we have any "GPU stuck in wrong power state after suspend" type of
>>> bugs still around?
>>
>> One of suggestion form the HW team was to Bring the wells up before we
>> disable RC6 at run-time. We did see some issue when we enabled D0ix.
>>
>> I think the is a good practice to make sure we bring-up the wells before we
>> disable RC6. At least this avoids the cases where wells are not up before we
>> can access the Next register after disable.
>
> Ville was totally right. I do think a POSTING_READ is still sufficient.
> Don't care much either way.
>

If feel this patch is not adding any value. I OK dropping this patch.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Daniel Vetter]

On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote:
> 
> 
> On 4/9/2014 9:43 AM, Ben Widawsky wrote:
> >On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote:
> >>
> >>
> >>On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
> >>>On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
> >>>>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
> >>>>>From: Deepak S <deepak.s@intel.com>
> >>>>>
> >>>>>We need do forcewake before Disabling RC6, This is what the BIOS
> >>>>>expects while going into suspend.
> >>>>>
> >>>>>v2: updated commit message. (Daniel)
> >>>>>
> >>>>>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>>>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >>>>>---
> >>>>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>index 04af065..ad2ff99 100644
> >>>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
> >>>>>  {
> >>>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>
> >>>>>+	/* we're doing forcewake before Disabling RC6,
> >>>>>+	 * This what the BIOS expects when going into suspend */
> >>>>>+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >>>>>+
> >>>>>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >>>>>
> >>>>>+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >>>>>+
> >>>>>  	gen6_disable_rps_interrupts(dev);
> >>>>>  }
> >>>>>
> >>>>
> >>>>Isn't the forcewake done as part of I915_WRITE sufficient?
> >>>
> >>>Writes don't do forcewake, nor is the register even part of the
> >>>VLV forcewake ranges.
> >>>
> >>>I guess the rationale for this patche is still a bit vague. But if it's
> >>>really needed, I wonder whether we should do this same dance for !VLV
> >>>too? Do we have any "GPU stuck in wrong power state after suspend" type of
> >>>bugs still around?
> >>
> >>One of suggestion form the HW team was to Bring the wells up before we
> >>disable RC6 at run-time. We did see some issue when we enabled D0ix.
> >>
> >>I think the is a good practice to make sure we bring-up the wells before we
> >>disable RC6. At least this avoids the cases where wells are not up before we
> >>can access the Next register after disable.
> >
> >Ville was totally right. I do think a POSTING_READ is still sufficient.
> >Don't care much either way.
> >
> 
> If feel this patch is not adding any value. I OK dropping this patch.

I think it makes a lot of sense - on the enable side we also grab the
wells (in case the bios has enabled rc6 already) to make sure we change
the rc6/rps state while everything is around.

Can you please update your patch to also roll this out for gen6/8 rps
disable functions?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Deepak S]

On Wednesday 09 April 2014 06:32 PM, Daniel Vetter wrote:
> On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote:
>>
>> On 4/9/2014 9:43 AM, Ben Widawsky wrote:
>>> On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote:
>>>>
>>>> On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
>>>>> On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
>>>>>> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
>>>>>>> From: Deepak S <deepak.s@intel.com>
>>>>>>>
>>>>>>> We need do forcewake before Disabling RC6, This is what the BIOS
>>>>>>> expects while going into suspend.
>>>>>>>
>>>>>>> v2: updated commit message. (Daniel)
>>>>>>>
>>>>>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>>>>>>>   1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> index 04af065..ad2ff99 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
>>>>>>>   {
>>>>>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>>>
>>>>>>> +	/* we're doing forcewake before Disabling RC6,
>>>>>>> +	 * This what the BIOS expects when going into suspend */
>>>>>>> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>>>>>>> +
>>>>>>>   	I915_WRITE(GEN6_RC_CONTROL, 0);
>>>>>>>
>>>>>>> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>>>>>>> +
>>>>>>>   	gen6_disable_rps_interrupts(dev);
>>>>>>>   }
>>>>>>>
>>>>>> Isn't the forcewake done as part of I915_WRITE sufficient?
>>>>> Writes don't do forcewake, nor is the register even part of the
>>>>> VLV forcewake ranges.
>>>>>
>>>>> I guess the rationale for this patche is still a bit vague. But if it's
>>>>> really needed, I wonder whether we should do this same dance for !VLV
>>>>> too? Do we have any "GPU stuck in wrong power state after suspend" type of
>>>>> bugs still around?
>>>> One of suggestion form the HW team was to Bring the wells up before we
>>>> disable RC6 at run-time. We did see some issue when we enabled D0ix.
>>>>
>>>> I think the is a good practice to make sure we bring-up the wells before we
>>>> disable RC6. At least this avoids the cases where wells are not up before we
>>>> can access the Next register after disable.
>>> Ville was totally right. I do think a POSTING_READ is still sufficient.
>>> Don't care much either way.
>>>
>> If feel this patch is not adding any value. I OK dropping this patch.
> I think it makes a lot of sense - on the enable side we also grab the
> wells (in case the bios has enabled rc6 already) to make sure we change
> the rc6/rps state while everything is around.
>
> Can you please update your patch to also roll this out for gen6/8 rps
> disable functions?
>
> Thanks, Daniel

Sure Daniel, I will add this to gen6/8 and submit the patch

Re: [PATCH 1/6] drm/i915: Bring UP Power Wells before disabling RC6.

[Ben Widawsky]

On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote:
> 
> 
> On 4/9/2014 9:43 AM, Ben Widawsky wrote:
> >On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote:
> >>
> >>
> >>On 4/8/2014 6:13 PM, Ville Syrjälä wrote:
> >>>On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote:
> >>>>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote:
> >>>>>From: Deepak S <deepak.s@intel.com>
> >>>>>
> >>>>>We need do forcewake before Disabling RC6, This is what the BIOS
> >>>>>expects while going into suspend.
> >>>>>
> >>>>>v2: updated commit message. (Daniel)
> >>>>>
> >>>>>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>>>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >>>>>---
> >>>>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>index 04af065..ad2ff99 100644
> >>>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
> >>>>>  {
> >>>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>>
> >>>>>+	/* we're doing forcewake before Disabling RC6,
> >>>>>+	 * This what the BIOS expects when going into suspend */
> >>>>>+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >>>>>+
> >>>>>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >>>>>
> >>>>>+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >>>>>+
> >>>>>  	gen6_disable_rps_interrupts(dev);
> >>>>>  }
> >>>>>
> >>>>
> >>>>Isn't the forcewake done as part of I915_WRITE sufficient?
> >>>
> >>>Writes don't do forcewake, nor is the register even part of the
> >>>VLV forcewake ranges.
> >>>
> >>>I guess the rationale for this patche is still a bit vague. But if it's
> >>>really needed, I wonder whether we should do this same dance for !VLV
> >>>too? Do we have any "GPU stuck in wrong power state after suspend" type of
> >>>bugs still around?
> >>
> >>One of suggestion form the HW team was to Bring the wells up before we
> >>disable RC6 at run-time. We did see some issue when we enabled D0ix.
> >>
> >>I think the is a good practice to make sure we bring-up the wells before we
> >>disable RC6. At least this avoids the cases where wells are not up before we
> >>can access the Next register after disable.
> >
> >Ville was totally right. I do think a POSTING_READ is still sufficient.
> >Don't care much either way.
> >
> 
> If feel this patch is not adding any value. I OK dropping this patch.

I didn't mean it's not adding value. In general I like to address what
the spec is trying to tell us to do, instead of following the letter of
the spec. Here is another example of a very much missing, "why" as Ville
was saying. If there is a bug it currently fixes, then I have no
question about how to proceed.


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: dma_buf_vunmap is presumed not to fail, don't let it

[Rodrigo Vivi]

From: Chris Wilson <chris@chris-wilson.co.uk>

Since dma_buf_vunmap() procedes blithely on ignorant of whether the
driver failed to actually unmap the backing storage for the dma-buf, we
need to make a best-effort to do so. This involves not allowing
ourselves to be susceptible to signals causing us to leak the storage.

This should have been detectable with the current i-g-t as a misplaced
signal should have left the pages pinned upon freeing the object where
we have a warning in place.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 9bb533e..321102a 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -161,12 +161,8 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
 	struct drm_device *dev = obj->base.dev;
-	int ret;
-
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return;
 
+	mutex_lock(&dev->struct_mutex);
 	if (--obj->vmapping_count == 0) {
 		vunmap(obj->dma_buf_vmapping);
 		obj->dma_buf_vmapping = NULL;
-- 
1.9.0

Re: [PATCH 2/6] drm/i915: dma_buf_vunmap is presumed not to fail, don't let it

[Daniel Vetter]

On Mon, Apr 07, 2014 at 05:01:47PM -0300, Rodrigo Vivi wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Since dma_buf_vunmap() procedes blithely on ignorant of whether the
> driver failed to actually unmap the backing storage for the dma-buf, we
> need to make a best-effort to do so. This involves not allowing
> ourselves to be susceptible to signals causing us to leak the storage.
> 
> This should have been detectable with the current i-g-t as a misplaced
> signal should have left the pages pinned upon freeing the object where
> we have a warning in place.

Apparently QA is asleep, or have I missed the bugzilla?
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Rodrigo Vivi]

From: Chris Wilson <chris@chris-wilson.co.uk>

If we run out of stolen memory when trying to allocate an object, see if
we can reap enough purgeable objects to free up enough contiguous free
space for the allocation. This is in principle very much like evicting
objects to free up enough contiguous space in the vma when binding
a new object - and you will be forgiven for thinking that the code looks
very similar.

At the moment, we do not allow userspace to allocate objects in stolen,
so there is neither the memory pressure to trigger stolen eviction nor
any purgeable objects inside the stolen arena. However, this will change
in the near future, and so better management and defragmentation of
stolen memory will become a real issue.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Gupta, Sourab" <sourab.gupta@intel.com>
Cc: "Goel, Akash" <akash.goel@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 119 ++++++++++++++++++++++++++++++---
 1 file changed, 108 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 62ef55b..1fc1986 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -329,18 +329,25 @@ cleanup:
 	return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
+{
+	if (obj->stolen == NULL)
+		return false;
+
+	if (obj->madv != I915_MADV_DONTNEED)
+		return false;
+
+	list_add(&obj->obj_exec_link, unwind);
+	return drm_mm_scan_add_block(obj->stolen);
+}
+
+static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	struct drm_i915_gem_object *obj;
+	struct list_head unwind, evict;
 	int ret;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
-
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
 		return NULL;
 
@@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
 				 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
+	if (ret == 0)
+		return stolen;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (drm_mm_scan_remove_block(obj->stolen)) {
+			list_add(&obj->obj_exec_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
 	}
 
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (ret == 0) {
+			struct i915_vma *vma, *vma_next;
+
+			list_for_each_entry_safe(vma, vma_next,
+						 &obj->vma_list,
+						 vma_link)
+				if (i915_vma_unbind(vma))
+					break;
+
+			/* Stolen pins its pages to prevent the
+			 * normal shrinker from processing stolen
+			 * objects.
+			 */
+			i915_gem_object_unpin_pages(obj);
+
+			ret = i915_gem_object_put_pages(obj);
+			if (ret == 0) {
+				obj->madv = __I915_MADV_PURGED;
+
+				kfree(obj->stolen);
+				obj->stolen = NULL;
+			} else
+				i915_gem_object_pin_pages(obj);
+		}
+
+		drm_gem_object_unreference(&obj->base);
+	}
+
+	if (ret == 0)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+					 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret == 0)
+		return stolen;
+
+	kfree(stolen);
+	return NULL;
+}
+
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return NULL;
+
+	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (stolen == NULL)
+		return NULL;
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
-- 
1.9.0

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Gupta, Sourab]

On Mon, 2014-04-07 at 20:01 +0000, Rodrigo Vivi wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> If we run out of stolen memory when trying to allocate an object, see if
> we can reap enough purgeable objects to free up enough contiguous free
> space for the allocation. This is in principle very much like evicting
> objects to free up enough contiguous space in the vma when binding
> a new object - and you will be forgiven for thinking that the code looks
> very similar.
> 
> At the moment, we do not allow userspace to allocate objects in stolen,
> so there is neither the memory pressure to trigger stolen eviction nor
> any purgeable objects inside the stolen arena. However, this will change
> in the near future, and so better management and defragmentation of
> stolen memory will become a real issue.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Gupta, Sourab" <sourab.gupta@intel.com>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 119 ++++++++++++++++++++++++++++++---
>  1 file changed, 108 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 62ef55b..1fc1986 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -329,18 +329,25 @@ cleanup:
>  	return NULL;
>  }
>  
> -struct drm_i915_gem_object *
> -i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> +{
> +	if (obj->stolen == NULL)
> +		return false;
> +
> +	if (obj->madv != I915_MADV_DONTNEED)
> +		return false;
> +
> +	list_add(&obj->obj_exec_link, unwind);
> +	return drm_mm_scan_add_block(obj->stolen);
> +}
> +
> +static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_gem_object *obj;
>  	struct drm_mm_node *stolen;
> +	struct drm_i915_gem_object *obj;
> +	struct list_head unwind, evict;
>  	int ret;
>  
> -	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> -
> -	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
>  	if (size == 0)
>  		return NULL;
>  
> @@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
>  
>  	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
>  				 4096, DRM_MM_SEARCH_DEFAULT);
> -	if (ret) {
> -		kfree(stolen);
> -		return NULL;
> +	if (ret == 0)
> +		return stolen;
> +
> +	/* No more stolen memory available, or too fragmented.
> +	 * Try evicting purgeable objects and search again.
> +	 */
> +
> +	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
> +	INIT_LIST_HEAD(&unwind);
> +
> +	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;
> +
> +	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;
> +
> +found:
> +	INIT_LIST_HEAD(&evict);
> +	while (!list_empty(&unwind)) {
> +		obj = list_first_entry(&unwind,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_link);
> +
> +		if (drm_mm_scan_remove_block(obj->stolen)) {
> +			list_add(&obj->obj_exec_link, &evict);
> +			drm_gem_object_reference(&obj->base);
> +		}
>  	}
>  
> +	ret = 0;
> +	while (!list_empty(&evict)) {
> +		obj = list_first_entry(&evict,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_link);
> +
> +		if (ret == 0) {
> +			struct i915_vma *vma, *vma_next;
> +
> +			list_for_each_entry_safe(vma, vma_next,
> +						 &obj->vma_list,
> +						 vma_link)
> +				if (i915_vma_unbind(vma))
> +					break;
> +
> +			/* Stolen pins its pages to prevent the
> +			 * normal shrinker from processing stolen
> +			 * objects.
> +			 */
> +			i915_gem_object_unpin_pages(obj);
> +
> +			ret = i915_gem_object_put_pages(obj);
> +			if (ret == 0) {
> +				obj->madv = __I915_MADV_PURGED;
> +
> +				kfree(obj->stolen);
> +				obj->stolen = NULL;
> +			} else
> +				i915_gem_object_pin_pages(obj);
> +		}
> +
> +		drm_gem_object_unreference(&obj->base);
> +	}
> +
> +	if (ret == 0)
> +		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
> +					 4096, DRM_MM_SEARCH_DEFAULT);
> +	if (ret == 0)
> +		return stolen;
> +
> +	kfree(stolen);
> +	return NULL;
> +}
> +
> +struct drm_i915_gem_object *
> +i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_object *obj;
> +	struct drm_mm_node *stolen;
> +
> +	lockdep_assert_held(&dev->struct_mutex);
> +
> +	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> +		return NULL;
> +
> +	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
> +
> +	stolen = stolen_alloc(dev_priv, size);
> +	if (stolen == NULL)
> +		return NULL;
> +
>  	obj = _i915_gem_object_create_stolen(dev, stolen);
>  	if (obj)
>  		return obj;

Hi Rodrigo,
In this patch, while freeing the purgeable stolen object, the memory
node also has to be freed, so as to make space for new object. We need
to call drm_mm_remove_node while freeing obj.

The below modification patch was floated earlier for this purpose:
http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html

Regards,
Sourab

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Chris Wilson]

On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> Hi Rodrigo,
> In this patch, while freeing the purgeable stolen object, the memory
> node also has to be freed, so as to make space for new object. We need
> to call drm_mm_remove_node while freeing obj.
> 
> The below modification patch was floated earlier for this purpose:
> http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html

Right, I have a v2 locally with the fix you identified.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Gupta, Sourab]

On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > Hi Rodrigo,
> > In this patch, while freeing the purgeable stolen object, the memory
> > node also has to be freed, so as to make space for new object. We need
> > to call drm_mm_remove_node while freeing obj.
> > 
> > The below modification patch was floated earlier for this purpose:
> > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> 
> Right, I have a v2 locally with the fix you identified.
> -Chris
> 
Ok, Thanks Chris.

Regards,
Sourab

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Daniel Vetter]

On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote:
> On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > > Hi Rodrigo,
> > > In this patch, while freeing the purgeable stolen object, the memory
> > > node also has to be freed, so as to make space for new object. We need
> > > to call drm_mm_remove_node while freeing obj.
> > > 
> > > The below modification patch was floated earlier for this purpose:
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> > 
> > Right, I have a v2 locally with the fix you identified.
> > -Chris
> > 
> Ok, Thanks Chris.

I'd really prefer if someone would pick up all the
stolen/create2_ioctl/whatever patches, pack them up into a polished
series, add the testcases and submit this all for review and merging.

Otherwise this will linger forever and we'll get nowhere. Chris seems
swamped with other stuff, so Sourab could you please take a look at this?

Please check with your manager that you have sufficient bandwidth to pull
this through.

Rodrigo, I think you can drop this patch from -collector, it only really
makes sense in the context of all the other stolen work.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Gupta, Sourab]

On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote:
> On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote:
> > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > > > Hi Rodrigo,
> > > > In this patch, while freeing the purgeable stolen object, the memory
> > > > node also has to be freed, so as to make space for new object. We need
> > > > to call drm_mm_remove_node while freeing obj.
> > > > 
> > > > The below modification patch was floated earlier for this purpose:
> > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> > > 
> > > Right, I have a v2 locally with the fix you identified.
> > > -Chris
> > > 
> > Ok, Thanks Chris.
> 
> I'd really prefer if someone would pick up all the
> stolen/create2_ioctl/whatever patches, pack them up into a polished
> series, add the testcases and submit this all for review and merging.
> 
> Otherwise this will linger forever and we'll get nowhere. Chris seems
> swamped with other stuff, so Sourab could you please take a look at this?
> 
> Please check with your manager that you have sufficient bandwidth to pull
> this through.
> 

I'll be on vacation from next week, so I'll be able to gauge this better
after coming back.
Nevertheless, I have some questions regarding the expectation of
userspace code changes required for these patches (i.e. libdrm changes
and igt testcases)

1) For libdrm , I am assuming, a counterpart of
drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
take in the parameters needed. 
Should the caching of objects from libdrm need to take care of both the
placement domains seperately (as in different sets of bo buckets)?
Should libdrm be transparent to all the combinations of different
parameters being passed by user or should the prohibited combinations be
disallowed from libdrm side?

2) For the igt, since we have a lot of parameters exposed to user, the
number of subtests required may be huge and still they may not test out
everything. 
So, Is the expectation here to have exhaustive test cases for all the
parameters (tiling/cache/domain/madvise/offset etc.) going in as input
to the create2 ioctl?
For eg. let us say we are going to check the render copy operation of
src and dest bo's. Do we need to provide all possible combinations of
different (create2 ioctl) input parameters to these src and dest bo's
and then run the render copy test for all these combinations.
Any guiding pointers from your side as to how we may go about the igt
testcases?

Thanks,
sourab

> Rodrigo, I think you can drop this patch from -collector, it only really
> makes sense in the context of all the other stolen work.
> 
> Thanks, Daniel

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Daniel Vetter]

On Thu, Apr 10, 2014 at 10:12:39AM +0000, Gupta, Sourab wrote:
> On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote:
> > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote:
> > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > > > > Hi Rodrigo,
> > > > > In this patch, while freeing the purgeable stolen object, the memory
> > > > > node also has to be freed, so as to make space for new object. We need
> > > > > to call drm_mm_remove_node while freeing obj.
> > > > > 
> > > > > The below modification patch was floated earlier for this purpose:
> > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> > > > 
> > > > Right, I have a v2 locally with the fix you identified.
> > > > -Chris
> > > > 
> > > Ok, Thanks Chris.
> > 
> > I'd really prefer if someone would pick up all the
> > stolen/create2_ioctl/whatever patches, pack them up into a polished
> > series, add the testcases and submit this all for review and merging.
> > 
> > Otherwise this will linger forever and we'll get nowhere. Chris seems
> > swamped with other stuff, so Sourab could you please take a look at this?
> > 
> > Please check with your manager that you have sufficient bandwidth to pull
> > this through.
> > 
> 
> I'll be on vacation from next week, so I'll be able to gauge this better
> after coming back.
> Nevertheless, I have some questions regarding the expectation of
> userspace code changes required for these patches (i.e. libdrm changes
> and igt testcases)
> 
> 1) For libdrm , I am assuming, a counterpart of
> drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
> take in the parameters needed. 
> Should the caching of objects from libdrm need to take care of both the
> placement domains seperately (as in different sets of bo buckets)?
> Should libdrm be transparent to all the combinations of different
> parameters being passed by user or should the prohibited combinations be
> disallowed from libdrm side?

I'm not sure whether we need a cache implemented in libdrm. Since stolen
objects are fairly special it's probably easier to just have a simple
linear cache tailor-made in the respective UMD. So just exposing
create2_ioctl should be good enough.

> 2) For the igt, since we have a lot of parameters exposed to user, the
> number of subtests required may be huge and still they may not test out
> everything. 
> So, Is the expectation here to have exhaustive test cases for all the
> parameters (tiling/cache/domain/madvise/offset etc.) going in as input
> to the create2 ioctl?
> For eg. let us say we are going to check the render copy operation of
> src and dest bo's. Do we need to provide all possible combinations of
> different (create2 ioctl) input parameters to these src and dest bo's
> and then run the render copy test for all these combinations.
> Any guiding pointers from your side as to how we may go about the igt
> testcases?

At a high-level there's two parts for igt tests:
- First is functional tests, where we try to make sure that the feature
  actually works. I.e. allocate some stolen memory and then do something
  with it, making sure that data access for the gpu and similar things
  work. For this we just want some reasonable base coverage so that when
  we hit a bug somewhere it's easy to extend the testcase to cover that
  bug with a specific subtest.

- Then there's interface testing. kernel/userspace is a trust barrier, so
  we need to make sure that evil userspace can't make the kernel crash
  with some crazy invalid combination of flags or operations (like create
  a stolen object and then try to mmap it). Since this is security
  relevant and also since we can't ever change established userspace ABI I
  want full coverage of all cases. But this is just about detecting abuse
  correctly, no functional tests here.

For details see my two blog posts on the topic:

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

[Gupta, Sourab]

On Fri, 2014-04-11 at 08:55 +0000, Daniel Vetter wrote:
> On Thu, Apr 10, 2014 at 10:12:39AM +0000, Gupta, Sourab wrote:
> > On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote:
> > > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote:
> > > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote:
> > > > > > Hi Rodrigo,
> > > > > > In this patch, while freeing the purgeable stolen object, the memory
> > > > > > node also has to be freed, so as to make space for new object. We need
> > > > > > to call drm_mm_remove_node while freeing obj.
> > > > > > 
> > > > > > The below modification patch was floated earlier for this purpose:
> > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
> > > > > 
> > > > > Right, I have a v2 locally with the fix you identified.
> > > > > -Chris
> > > > > 
> > > > Ok, Thanks Chris.
> > > 
> > > I'd really prefer if someone would pick up all the
> > > stolen/create2_ioctl/whatever patches, pack them up into a polished
> > > series, add the testcases and submit this all for review and merging.
> > > 
> > > Otherwise this will linger forever and we'll get nowhere. Chris seems
> > > swamped with other stuff, so Sourab could you please take a look at this?
> > > 
> > > Please check with your manager that you have sufficient bandwidth to pull
> > > this through.
> > > 
> > 
> > I'll be on vacation from next week, so I'll be able to gauge this better
> > after coming back.
> > Nevertheless, I have some questions regarding the expectation of
> > userspace code changes required for these patches (i.e. libdrm changes
> > and igt testcases)
> > 
> > 1) For libdrm , I am assuming, a counterpart of
> > drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
> > take in the parameters needed. 
> > Should the caching of objects from libdrm need to take care of both the
> > placement domains seperately (as in different sets of bo buckets)?
> > Should libdrm be transparent to all the combinations of different
> > parameters being passed by user or should the prohibited combinations be
> > disallowed from libdrm side?
> 
> I'm not sure whether we need a cache implemented in libdrm. Since stolen
> objects are fairly special it's probably easier to just have a simple
> linear cache tailor-made in the respective UMD. So just exposing
> create2_ioctl should be good enough.
> 
> > 2) For the igt, since we have a lot of parameters exposed to user, the
> > number of subtests required may be huge and still they may not test out
> > everything. 
> > So, Is the expectation here to have exhaustive test cases for all the
> > parameters (tiling/cache/domain/madvise/offset etc.) going in as input
> > to the create2 ioctl?
> > For eg. let us say we are going to check the render copy operation of
> > src and dest bo's. Do we need to provide all possible combinations of
> > different (create2 ioctl) input parameters to these src and dest bo's
> > and then run the render copy test for all these combinations.
> > Any guiding pointers from your side as to how we may go about the igt
> > testcases?
> 
> At a high-level there's two parts for igt tests:
> - First is functional tests, where we try to make sure that the feature
>   actually works. I.e. allocate some stolen memory and then do something
>   with it, making sure that data access for the gpu and similar things
>   work. For this we just want some reasonable base coverage so that when
>   we hit a bug somewhere it's easy to extend the testcase to cover that
>   bug with a specific subtest.
> 
> - Then there's interface testing. kernel/userspace is a trust barrier, so
>   we need to make sure that evil userspace can't make the kernel crash
>   with some crazy invalid combination of flags or operations (like create
>   a stolen object and then try to mmap it). Since this is security
>   relevant and also since we can't ever change established userspace ABI I
>   want full coverage of all cases. But this is just about detecting abuse
>   correctly, no functional tests here.
> 
> For details see my two blog posts on the topic:
> 
> http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
> 
> http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html
> 
> Cheers, Daniel

Thanks Daniel,
We'll take care of the above points for libdrm changes and igt.

Regards,
Sourab

[PATCH 4/6] drm/i915: add flag to prevent dmesg spam on context banning

[Rodrigo Vivi]

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Piglit runner and QA are both looking at the dmesg for
DRM_ERRORs with test cases.

Add flag to stop_rings debugfs interface to prevent DRM_ERROR
output when default context banning is being tested.

References: https://bugs.freedesktop.org/show_bug.cgi?id=75876
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h         | 18 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem.c         |  5 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 55addaa..d5cd981 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1099,8 +1099,12 @@ struct i915_gpu_error {
 	 */
 	wait_queue_head_t reset_queue;
 
-	/* For gpu hang simulation. */
-	unsigned int stop_rings;
+	/* For gpu hang simulation.
+	 * MSB for controlling DRM_ERROR with ctx bans
+	 */
+	u32 stop_rings;
+
+#define I915_SQUELCH_CTX_BAN_ERROR (1 << 31)
 
 	/* For missed irq/seqno simulation. */
 	unsigned int test_irq_rings;
@@ -2146,6 +2150,16 @@ static inline u32 i915_reset_count(struct i915_gpu_error *error)
 	return ((atomic_read(&error->reset_counter) & ~I915_WEDGED) + 1) / 2;
 }
 
+static inline u32 i915_stopped_rings(struct drm_i915_private *dev_priv)
+{
+	return dev_priv->gpu_error.stop_rings & ~I915_SQUELCH_CTX_BAN_ERROR;
+}
+
+static inline bool i915_squelch_ban_error(struct drm_i915_private *dev_priv)
+{
+	return dev_priv->gpu_error.stop_rings & I915_SQUELCH_CTX_BAN_ERROR;
+}
+
 void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c70121d..7598e86 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2277,8 +2277,9 @@ static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
 		if (!i915_gem_context_is_default(ctx)) {
 			DRM_DEBUG("context hanging too fast, banning!\n");
 			return true;
-		} else if (dev_priv->gpu_error.stop_rings == 0) {
-			DRM_ERROR("gpu hanging too fast, banning!\n");
+		} else if (!i915_stopped_rings(dev_priv)) {
+			if (!i915_squelch_ban_error(dev_priv))
+				DRM_ERROR("gpu hanging too fast, banning!\n");
 			return true;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3d76ce1..275afa5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -46,7 +46,7 @@ void __intel_ring_advance(struct intel_ring_buffer *ring)
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 
 	ring->tail &= ring->size - 1;
-	if (dev_priv->gpu_error.stop_rings & intel_ring_flag(ring))
+	if (i915_stopped_rings(dev_priv) & intel_ring_flag(ring))
 		return;
 	ring->write_tail(ring, ring->tail);
 }
-- 
1.9.0

Re: [PATCH 4/6] drm/i915: add flag to prevent dmesg spam on context banning

[Mika Kuoppala]

Rodrigo Vivi <rodrigo.vivi@gmail.com> writes:

> From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>
> Piglit runner and QA are both looking at the dmesg for
> DRM_ERRORs with test cases.
>
> Add flag to stop_rings debugfs interface to prevent DRM_ERROR
> output when default context banning is being tested.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=75876
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

This can be dropped. Superseded by:

1396023498-8635-1-git-send-email-mika.kuoppala@intel.com

-Mika

[PATCH 5/6] drm/i915: Do not allow a pending forcewake put to unbalance across reset

[Rodrigo Vivi]

From: Chris Wilson <chris@chris-wilson.co.uk>

Across a device reset, we try to restore and user forcewake reference
counts. This is complicated by our deferred forcewake put adding an
extra reference, that may or may not be flushed when we call
del_timer_sync. So we have to take that pending reference into account
when restoring the forcewake.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a72bab..c8969e3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -317,8 +317,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
+	int pending;
 
-	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+	pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
@@ -336,6 +337,8 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	if (restore) { /* If reset with a user forcewake, try to restore */
 		unsigned fw = 0;
 
+		dev_priv->uncore.forcewake_count -= pending;
+
 		if (IS_VALLEYVIEW(dev)) {
 			if (dev_priv->uncore.fw_rendercount)
 				fw |= FORCEWAKE_RENDER;
-- 
1.9.0

[PATCH 6/6] drm/i915: Don't save/restore RS when not used

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Cc: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 8827892..74369b7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -589,6 +589,7 @@ mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,
 	       u32 hw_flags)
 {
+	u32 flags = hw_flags | MI_MM_SPACE_GTT;
 	int ret;
 
 	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
@@ -602,6 +603,10 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
+	/* These flags are for resource streamer on HSW+ */
+	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
+		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
@@ -614,11 +619,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
-			MI_MM_SPACE_GTT |
-			MI_SAVE_EXT_STATE_EN |
-			MI_RESTORE_EXT_STATE_EN |
-			hw_flags);
+	intel_ring_emit(ring,
+			i915_gem_obj_ggtt_offset(new_context->obj) | flags);
 	/*
 	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
 	 * WaMiSetContext_Hang:snb,ivb,vlv
-- 
1.9.0