[PATCH] drm/i915: drrs_invalidate at flip schedule

[PATCH] drm/i915: drrs_invalidate at flip schedule

[Ramalingam C]

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);
 	intel_psr_single_frame_update(dev);
 }
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Ramalingam C]

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
>

-- 
Thanks,
--Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Ramalingam C]

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
>>

-- 
Thanks,
--Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: fb_obj invalidate is divided into two functions

[Ramalingam C]

fb_obj invalidate is divided into two functions. So that
we can invoke the frontbuffer invalidation from the flip
preparation also.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
---
 drivers/gpu/drm/i915/intel_frontbuffer.c |   42 +++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 57095f5..4cea589 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -119,6 +119,33 @@ static void intel_mark_fb_busy(struct drm_device *dev,
 }
 
 /**
+ * intel_frontbuffer_invalidate - Invalidate frontbuffer
+ * @dev: DRM device
+ * @ring: set for asynchronous rendering
+ * @origin: which operation caused the invalidation
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called every time rendering on the given planes has
+ * started and frontbuffer caching must be invalidated.
+ *
+ * Can be called without any locks held.
+ */
+void intel_frontbuffer_invalidate(struct drm_device *dev,
+				struct intel_engine_cs *ring,
+				enum fb_op_origin origin,
+				unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_mark_fb_busy(dev, frontbuffer_bits, ring);
+
+	intel_psr_invalidate(dev, frontbuffer_bits);
+	intel_edp_drrs_invalidate(dev, frontbuffer_bits);
+	intel_fbc_invalidate(dev_priv, frontbuffer_bits, origin);
+}
+
+
+/**
  * intel_fb_obj_invalidate - invalidate frontbuffer object
  * @obj: GEM object to invalidate
  * @ring: set for asynchronous rendering
@@ -136,26 +163,23 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned frontbuffer_bits;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	if (!obj->frontbuffer_bits)
 		return;
 
+	frontbuffer_bits = obj->frontbuffer_bits;
+
 	if (ring) {
 		mutex_lock(&dev_priv->fb_tracking.lock);
-		dev_priv->fb_tracking.busy_bits
-			|= obj->frontbuffer_bits;
-		dev_priv->fb_tracking.flip_bits
-			&= ~obj->frontbuffer_bits;
+		dev_priv->fb_tracking.busy_bits |= frontbuffer_bits;
+		dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
 		mutex_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
-
-	intel_psr_invalidate(dev, obj->frontbuffer_bits);
-	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
-	intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
+	intel_frontbuffer_invalidate(dev, ring, origin, frontbuffer_bits);
 }
 
 /**
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: frontbuffer invalidate at flip schedule

[Ramalingam C]

After scheduling a flip for obj, frontbuffer should be invalidated.
Hence intel_frontbuffer_invalidate is invoked at flip preparation.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
---
 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 4cea589..07147a7 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -268,6 +268,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_frontbuffer_invalidate(dev, NULL, ORIGIN_FLIP, frontbuffer_bits);
 	intel_psr_single_frame_update(dev);
 }
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: frontbuffer invalidate at flip schedule

[Chris Wilson]

On Thu, Jun 11, 2015 at 03:08:35PM +0530, Ramalingam C wrote:
> After scheduling a flip for obj, frontbuffer should be invalidated.
> Hence intel_frontbuffer_invalidate is invoked at flip preparation.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
> ---
>  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 4cea589..07147a7 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -268,6 +268,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_frontbuffer_invalidate(dev, NULL, ORIGIN_FLIP, frontbuffer_bits);
>  	intel_psr_single_frame_update(dev);

This then has the side effect of calling psr_invalidate (psr_exit)
followed by the single-shot psr update, which doesn't seem desirable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: frontbuffer invalidate at flip schedule

[Ramalingam C]

True.

Chris and Vivi,

I am trying to understand the PSR exit and entry along with SFU 
requirement here.
AFAIK in existing code also
         on rendering start fb_obj_invalidate is calling the 
psr_invalidate(psr_exit)
         on flip prepare single frame update is done
         on flip complete frontbuffer_invalidate is calling 
psr_flush(psr_reenable)
         on rendering complete fb_obj_flush is calling the 
psr_flush(psr_reenable)

On Every rendering start if we exit the PSR, then there is no advantage 
of setting SFU at flip prepare.
Please correct me if I am wrong.

On Thursday 11 June 2015 03:26 PM, Chris Wilson wrote:
> On Thu, Jun 11, 2015 at 03:08:35PM +0530, Ramalingam C wrote:
>> After scheduling a flip for obj, frontbuffer should be invalidated.
>> Hence intel_frontbuffer_invalidate is invoked at flip preparation.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=90418
>> ---
>>   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 4cea589..07147a7 100644
>> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> @@ -268,6 +268,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_frontbuffer_invalidate(dev, NULL, ORIGIN_FLIP, frontbuffer_bits);
>>   	intel_psr_single_frame_update(dev);
> This then has the side effect of calling psr_invalidate (psr_exit)
> followed by the single-shot psr update, which doesn't seem desirable.
> -Chris
>

-- 
Thanks,
--Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: frontbuffer invalidate at flip schedule

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6569
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                                  312/312              312/312
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Daniel Vetter]

On Thu, Jun 11, 2015 at 02:57:52PM +0530, Ramalingam C wrote:
> 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.

You need to fix up the broken DRRS code - the frontbuffer tracking code is
perfectly fine. See below diff with inline comments.

Cheers, Daniel

And my Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> in case the
patch works as-is.

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f73da99e66b8..b96a4abb7a98 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5487,6 +5487,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
+	/* invalidate means busy screen hence upclock */
 	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
 		intel_dp_set_drrs_state(dev_priv->dev,
 				dev_priv->drrs.dp->attached_connector->panel.
@@ -5532,8 +5533,16 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	pipe = to_intel_crtc(crtc)->pipe;
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
-			!dev_priv->drrs.busy_frontbuffer_bits)
+	/* flush means busy screen hence upclock */
+	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
+		intel_dp_set_drrs_state(dev_priv->dev,
+				dev_priv->drrs.dp->attached_connector->panel.
+				fixed_mode->vrefresh);
+	}
+
+	/* flush also means no more activity hence schedule downclock if all
+	 * other fbs are quiescent too */
+	if (!dev_priv->drrs.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->drrs.work,
 				msecs_to_jiffies(1000));
 	mutex_unlock(&dev_priv->drrs.mutex);
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: drrs_invalidate at flip schedule

[Ramalingam C]

Daniel,

Only now i am understanding your comment in previous mail that is "need 
to upclock in both flush and validate".
Yup this change is what needed. Tested to be working fine.

The Documentation for the intel_edp_drrs_flush() mislead me to expect
invalidate call for each framebuffer change. I have just corrected this 
documentation as part your change and sending it to you.

Thanks.

On Monday 15 June 2015 03:22 PM, Daniel Vetter wrote:
> On Thu, Jun 11, 2015 at 02:57:52PM +0530, Ramalingam C wrote:
>> 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.
> You need to fix up the broken DRRS code - the frontbuffer tracking code is
> perfectly fine. See below diff with inline comments.
>
> Cheers, Daniel
>
> And my Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> in case the
> patch works as-is.
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f73da99e66b8..b96a4abb7a98 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5487,6 +5487,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>   	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>   	pipe = to_intel_crtc(crtc)->pipe;
>   
> +	/* invalidate means busy screen hence upclock */
>   	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
>   		intel_dp_set_drrs_state(dev_priv->dev,
>   				dev_priv->drrs.dp->attached_connector->panel.
> @@ -5532,8 +5533,16 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>   	pipe = to_intel_crtc(crtc)->pipe;
>   	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
>   
> -	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
> -			!dev_priv->drrs.busy_frontbuffer_bits)
> +	/* flush means busy screen hence upclock */
> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> +		intel_dp_set_drrs_state(dev_priv->dev,
> +				dev_priv->drrs.dp->attached_connector->panel.
> +				fixed_mode->vrefresh);
> +	}
> +
> +	/* flush also means no more activity hence schedule downclock if all
> +	 * other fbs are quiescent too */
> +	if (!dev_priv->drrs.busy_frontbuffer_bits)
>   		schedule_delayed_work(&dev_priv->drrs.work,
>   				msecs_to_jiffies(1000));
>   	mutex_unlock(&dev_priv->drrs.mutex);

-- 
Thanks,
--Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Restarting the Idleness DRRS in drrs_flush

[Ramalingam C]

Corrected the documentation on the intel_edp_drrs_flush and
intel_edp_drrs_invalidate.

And accordingly edp_drrs_flush function is modified to restart the idleness
detection after upclocking.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fb3d7f1..d5d5980 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5455,13 +5455,12 @@ unlock:
 }
 
 /**
- * intel_edp_drrs_invalidate - Invalidate DRRS
+ * intel_edp_drrs_invalidate - Disable Idleness DRRS
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
  *
- * When there is a disturbance on screen (due to cursor movement/time
- * update etc), DRRS needs to be invalidated, i.e. need to switch to
- * high RR.
+ * This function gets called everytime rendering on the given planes start.
+ * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR).
  *
  * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
  */
@@ -5486,6 +5485,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
+	/* invalidate means busy screen hence upclock */
 	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
 		intel_dp_set_drrs_state(dev_priv->dev,
 				dev_priv->drrs.dp->attached_connector->panel.
@@ -5499,13 +5499,14 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 }
 
 /**
- * intel_edp_drrs_flush - Flush DRRS
+ * intel_edp_drrs_flush - Restart Idleness DRRS
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
  *
- * When there is no movement on screen, DRRS work can be scheduled.
- * This DRRS work is responsible for setting relevant registers after a
- * timeout of 1 second.
+ * This function gets called every time rendering on the given planes has
+ * completed or flip on a crtc is completed. So DRRS should be upclocked
+ * (LOW_RR -> HIGH_RR). And also Idleness detection should be started again,
+ * if no other planes are dirty.
  *
  * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
  */
@@ -5531,8 +5532,17 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	pipe = to_intel_crtc(crtc)->pipe;
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
-			!dev_priv->drrs.busy_frontbuffer_bits)
+	/* flush means busy screen hence upclock */
+	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+		intel_dp_set_drrs_state(dev_priv->dev,
+				dev_priv->drrs.dp->attached_connector->panel.
+				fixed_mode->vrefresh);
+
+	/*
+	 * flush also means no more activity hence schedule downclock, if all
+	 * other fbs are quiescent too
+	 */
+	if (!dev_priv->drrs.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->drrs.work,
 				msecs_to_jiffies(1000));
 	mutex_unlock(&dev_priv->drrs.mutex);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Restarting the Idleness DRRS in drrs_flush

[Daniel Vetter]

On Mon, Jun 15, 2015 at 08:50:05PM +0530, Ramalingam C wrote:
> Corrected the documentation on the intel_edp_drrs_flush and
> intel_edp_drrs_invalidate.
> 
> And accordingly edp_drrs_flush function is modified to restart the idleness
> detection after upclocking.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Thanks for testing and updating the comments. I've added a note to the
commit that the kerneldoc is from you and applied the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c |   30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb3d7f1..d5d5980 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5455,13 +5455,12 @@ unlock:
>  }
>  
>  /**
> - * intel_edp_drrs_invalidate - Invalidate DRRS
> + * intel_edp_drrs_invalidate - Disable Idleness DRRS
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
> - * When there is a disturbance on screen (due to cursor movement/time
> - * update etc), DRRS needs to be invalidated, i.e. need to switch to
> - * high RR.
> + * This function gets called everytime rendering on the given planes start.
> + * Hence DRRS needs to be Upclocked, i.e. (LOW_RR -> HIGH_RR).
>   *
>   * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
>   */
> @@ -5486,6 +5485,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>  	pipe = to_intel_crtc(crtc)->pipe;
>  
> +	/* invalidate means busy screen hence upclock */
>  	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
>  		intel_dp_set_drrs_state(dev_priv->dev,
>  				dev_priv->drrs.dp->attached_connector->panel.
> @@ -5499,13 +5499,14 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>  }
>  
>  /**
> - * intel_edp_drrs_flush - Flush DRRS
> + * intel_edp_drrs_flush - Restart Idleness DRRS
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
> - * When there is no movement on screen, DRRS work can be scheduled.
> - * This DRRS work is responsible for setting relevant registers after a
> - * timeout of 1 second.
> + * This function gets called every time rendering on the given planes has
> + * completed or flip on a crtc is completed. So DRRS should be upclocked
> + * (LOW_RR -> HIGH_RR). And also Idleness detection should be started again,
> + * if no other planes are dirty.
>   *
>   * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
>   */
> @@ -5531,8 +5532,17 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>  	pipe = to_intel_crtc(crtc)->pipe;
>  	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
>  
> -	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
> -			!dev_priv->drrs.busy_frontbuffer_bits)
> +	/* flush means busy screen hence upclock */
> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
> +		intel_dp_set_drrs_state(dev_priv->dev,
> +				dev_priv->drrs.dp->attached_connector->panel.
> +				fixed_mode->vrefresh);
> +
> +	/*
> +	 * flush also means no more activity hence schedule downclock, if all
> +	 * other fbs are quiescent too
> +	 */
> +	if (!dev_priv->drrs.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->drrs.work,
>  				msecs_to_jiffies(1000));
>  	mutex_unlock(&dev_priv->drrs.mutex);
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx