| Message ID | 1431635902-2185-1-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 '*'
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 >
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 >>
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); }
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(+)