drm/i915: drrs_invalidate at flip schedule
diff mbox

Message ID 1431635902-2185-1-git-send-email-ramalingam.c@intel.com
State New
Headers show

Commit Message

Ramalingam C May 14, 2015, 8:38 p.m. UTC
After scheduling a flip for obj, we are supposed to invalidate the
drrs.

Action:
    Adding a call to intel_edp_drrs_invalidate at
    intel_frontbuffer_flip_prepare.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_frontbuffer.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Chris Wilson May 15, 2015, 11:58 a.m. UTC | #1
On Fri, May 15, 2015 at 02:08:22AM +0530, Ramalingam C wrote:
> After scheduling a flip for obj, we are supposed to invalidate the
> drrs.
> 
> Action:
>     Adding a call to intel_edp_drrs_invalidate at
>     intel_frontbuffer_flip_prepare.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Just Cc: Chris Wilson <chris@chris-wilson.co.uk>
References: https://bugs.freedesktop.org/show_bug.cgi?id=90418

Ok, looks correct. This invalidate will be paired with a flush after the
flip completes to reschedule the downclock of the refresh rates.

I think a comment would be useful to explain the relationship here, or
better would be a new intel_edp_drrs_flip_prepare() stub so that the
internal details of drrs are kept out of intel_frontbuffer.c and the
comment can refer to the adjacent functions for reference.
-Chris
Ramalingam C May 15, 2015, 1:24 p.m. UTC | #2
On Friday 15 May 2015 05:28 PM, Chris Wilson wrote:
> On Fri, May 15, 2015 at 02:08:22AM +0530, Ramalingam C wrote:
>> After scheduling a flip for obj, we are supposed to invalidate the
>> drrs.
>>
>> Action:
>>      Adding a call to intel_edp_drrs_invalidate at
>>      intel_frontbuffer_flip_prepare.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Just Cc: Chris Wilson <chris@chris-wilson.co.uk>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
>
> Ok, looks correct. This invalidate will be paired with a flush after the
> flip completes to reschedule the downclock of the refresh rates.
>
> I think a comment would be useful to explain the relationship here, or
> better would be a new intel_edp_drrs_flip_prepare() stub so that the
> internal details of drrs are kept out of intel_frontbuffer.c and the
> comment can refer to the adjacent functions for reference.
But in flip preparation we would want to invalidate the PSR (software 
implementation) also.
In that case we could create a function called 
intel_frontbuffer_flip_invalidate() instead of edp_drrs_flip_prepare.
This will be invoking the invalidation for the PSR and DRRS. And this 
function could be called from
intel_frontbuffer_flip_prepare().

Incase If FBC invalidate also needed at flip preparation, then we could 
create a common function called
intel_frontbuffer_invalidate parallel to intel_frontbuffer_flush which 
will be used by
intel_fb_obj_invalidate and intel_frontbuffer_flip_prepare.

Please share your view. whether FBC invalidate is required on flip 
preparation?
> -Chris
>
Chris Wilson May 15, 2015, 1:56 p.m. UTC | #3
On Fri, May 15, 2015 at 06:54:54PM +0530, Ramalingam C wrote:
> 
> On Friday 15 May 2015 05:28 PM, Chris Wilson wrote:
> >On Fri, May 15, 2015 at 02:08:22AM +0530, Ramalingam C wrote:
> >>After scheduling a flip for obj, we are supposed to invalidate the
> >>drrs.
> >>
> >>Action:
> >>     Adding a call to intel_edp_drrs_invalidate at
> >>     intel_frontbuffer_flip_prepare.
> >>
> >>Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> >>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Just Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
> >
> >Ok, looks correct. This invalidate will be paired with a flush after the
> >flip completes to reschedule the downclock of the refresh rates.
> >
> >I think a comment would be useful to explain the relationship here, or
> >better would be a new intel_edp_drrs_flip_prepare() stub so that the
> >internal details of drrs are kept out of intel_frontbuffer.c and the
> >comment can refer to the adjacent functions for reference.
> But in flip preparation we would want to invalidate the PSR
> (software implementation) also.
> In that case we could create a function called
> intel_frontbuffer_flip_invalidate() instead of
> edp_drrs_flip_prepare.
> This will be invoking the invalidation for the PSR and DRRS. And
> this function could be called from
> intel_frontbuffer_flip_prepare().
> 
> Incase If FBC invalidate also needed at flip preparation, then we
> could create a common function called
> intel_frontbuffer_invalidate parallel to intel_frontbuffer_flush
> which will be used by
> intel_fb_obj_invalidate and intel_frontbuffer_flip_prepare.
> 
> Please share your view. whether FBC invalidate is required on flip
> preparation?

It is (intel_disable_fbc is being directly by the flip code).

I would stick to calling it intel_frontbuffer_flip_prepare/flip_complete
to distinguish it from invalidate/flush - they are not equivalent in all
cases. And I would push that naming convention down to the backends.
Given that we have 3 (almost 4) users of this, we may want to move this
over to a notifier system and have the backends register themselves
rather than continually adapting intel_frontbuffer.c to the requirements
of different backends. Task for a rainy day.
-Chris
Shuang He May 18, 2015, 2:49 a.m. UTC | #4
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6412
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                 -1              314/314              313/314
IVB                                  338/338              338/338
BYT                                  286/286              286/286
BDW                                  320/320              320/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(13)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
Daniel Vetter May 18, 2015, 8:20 a.m. UTC | #5
On Fri, May 15, 2015 at 02:08:22AM +0530, Ramalingam C wrote:
> After scheduling a flip for obj, we are supposed to invalidate the
> drrs.
> 
> Action:
>     Adding a call to intel_edp_drrs_invalidate at
>     intel_frontbuffer_flip_prepare.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_frontbuffer.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 57095f5..44387ed 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -244,6 +244,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>  	mutex_unlock(&dev_priv->fb_tracking.lock);
>  
> +	intel_edp_drrs_invalidate(dev, frontbuffer_bits);

Nope. The problem here is that drrs wants business, but the frontbuffer
tracking only gives you coherency signals (flush/invalidate). But for
business both flush and invalidate indicate that there's something going
on on the screen. Which means you _must_ uplock the display in both
drrs_flush and drrs_invalidate.

By applaying the upclocking only to the flip codepath you're only papering
over this bug in one specific case, but not for all the other paths where
frontbuffer rendering is possible.
-Daniel

>  	intel_psr_single_frame_update(dev);
>  }
>  
> -- 
> 1.7.9.5
>
Ramalingam C June 11, 2015, 9:27 a.m. UTC | #6
Sorry for late response. I was away for longer.

Daniel,

As we have the intel_frontbuffer_flush, I have created the 
intel_frontbuffer_invalidate.
This can be called from flip preparation notification to handle the 
frontbuffer invalidation.
I will share the patches now.

On Monday 18 May 2015 01:50 PM, Daniel Vetter wrote:
> On Fri, May 15, 2015 at 02:08:22AM +0530, Ramalingam C wrote:
>> After scheduling a flip for obj, we are supposed to invalidate the
>> drrs.
>>
>> Action:
>>      Adding a call to intel_edp_drrs_invalidate at
>>      intel_frontbuffer_flip_prepare.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/intel_frontbuffer.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> index 57095f5..44387ed 100644
>> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> @@ -244,6 +244,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>>   	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>>   	mutex_unlock(&dev_priv->fb_tracking.lock);
>>   
>> +	intel_edp_drrs_invalidate(dev, frontbuffer_bits);
> Nope. The problem here is that drrs wants business, but the frontbuffer
> tracking only gives you coherency signals (flush/invalidate). But for
> business both flush and invalidate indicate that there's something going
> on on the screen. Which means you _must_ uplock the display in both
> drrs_flush and drrs_invalidate.
>
> By applaying the upclocking only to the flip codepath you're only papering
> over this bug in one specific case, but not for all the other paths where
> frontbuffer rendering is possible.
> -Daniel
>
>>   	intel_psr_single_frame_update(dev);
>>   }
>>   
>> -- 
>> 1.7.9.5
>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 57095f5..44387ed 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -244,6 +244,7 @@  void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
+	intel_edp_drrs_invalidate(dev, frontbuffer_bits);
 	intel_psr_single_frame_update(dev);
 }