| Message ID | 1373920155-3414-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote: > PSR tracking engine in HSW doesn't detect automagically some directly copy area > operations through scanout so we will have to kick it manually and > reschedule it to come back to normal operation as soon as possible. > > v2: Before PSR Hook. Don't force it when busy yet. > v3/v4: Solved small conflict. > v5: setup once function was already added on previous commit. > v6: Use POSTING_READ to make sure the mutex works as a write barrier as > suggested by Chris Wilson > > CC: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 3 files changed, 52 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 3bca337..dc10345 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1840,6 +1840,7 @@ > #define EDP_PSR_PERF_CNT_MASK 0xffffff > > #define EDP_PSR_DEBUG_CTL 0x64860 > +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) > #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) > #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) > #define EDP_PSR_DEBUG_MASK_HPD (1<<25) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 3c9473c..47e1676 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) > return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > } > > +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) > +{ > + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), > + struct intel_dp, > + edp_psr_delayed_normal_work); > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&intel_dp->psr_exit_mutex); > + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & > + ~EDP_PSR_DEBUG_FORCE_EXIT); > + POSTING_READ(EDP_PSR_DEBUG_CTL); > + mutex_unlock(&intel_dp->psr_exit_mutex); > +} > + > +void intel_edp_psr_force_exit(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_encoder *encoder; > + struct intel_dp *intel_dp = NULL; > + > + if (!intel_edp_is_psr_enabled(dev)) > + return; > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > + if (encoder->type == INTEL_OUTPUT_EDP) > + intel_dp = enc_to_intel_dp(&encoder->base); > + > + if (!intel_dp) > + return; > + > + if (WARN_ON(!intel_dp->psr_setup_done)) > + return; If you have to dig out your data like this it usually means that it's at the wrong spot. Imo we should track a psr_possible bit in the pipe_config and a psr_enabled (and the locking for the runtime mutex) in the crtc itself. Also, the psr_setup_done thing is another hint that we should move this into the crtc. Second I prefer if functions with tricky tie-in with existing code are used in the same patch they're created - if it's split over two patches review is a bit a pain. But if I read the follow-up patch correctly we call this function from the busy_ioctl unconditionally, which will obviously result in tons of falls postives - e.g. libdrm uses this ioctl to manage it's buffer cache. I think I'll punt on this patch and merge just the parts from the next one for normal backbuffer rendering with pageflips model. -Daniel > + > + mutex_lock(&intel_dp->psr_exit_mutex); > + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | > + EDP_PSR_DEBUG_FORCE_EXIT); > + POSTING_READ(EDP_PSR_DEBUG_CTL); > + mutex_unlock(&intel_dp->psr_exit_mutex); > + > + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, > + msecs_to_jiffies(100)); > +} > + > static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > struct edp_vsc_psr *vsc_psr) > { > @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) > I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP | > EDP_PSR_DEBUG_MASK_HPD); > > + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, > + intel_edp_psr_delayed_normal_work); > + mutex_init(&intel_dp->psr_exit_mutex); > + > intel_dp->psr_setup_done = true; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0f52362..e47f3f3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -499,6 +499,8 @@ struct intel_dp { > int backlight_off_delay; > struct delayed_work panel_vdd_work; > bool want_panel_vdd; > + struct delayed_work edp_psr_delayed_normal_work; > + struct mutex psr_exit_mutex; > bool psr_setup_done; > struct intel_connector *attached_connector; > }; > @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); > extern void intel_edp_psr_disable(struct intel_dp *intel_dp); > extern void intel_edp_psr_update(struct drm_device *dev); > extern bool intel_edp_is_psr_enabled(struct drm_device *dev); > +extern void intel_edp_psr_force_exit(struct drm_device *dev); > > #endif /* __INTEL_DRV_H__ */ > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 18, 2013 at 5:33 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote: >> PSR tracking engine in HSW doesn't detect automagically some directly copy area >> operations through scanout so we will have to kick it manually and >> reschedule it to come back to normal operation as soon as possible. >> >> v2: Before PSR Hook. Don't force it when busy yet. >> v3/v4: Solved small conflict. >> v5: setup once function was already added on previous commit. >> v6: Use POSTING_READ to make sure the mutex works as a write barrier as >> suggested by Chris Wilson >> >> CC: Chris Wilson <chris@chris-wilson.co.uk> >> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> 3 files changed, 52 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 3bca337..dc10345 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1840,6 +1840,7 @@ >> #define EDP_PSR_PERF_CNT_MASK 0xffffff >> >> #define EDP_PSR_DEBUG_CTL 0x64860 >> +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) >> #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) >> #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) >> #define EDP_PSR_DEBUG_MASK_HPD (1<<25) >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 3c9473c..47e1676 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) >> return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; >> } >> >> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) >> +{ >> + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), >> + struct intel_dp, >> + edp_psr_delayed_normal_work); >> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + mutex_lock(&intel_dp->psr_exit_mutex); >> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & >> + ~EDP_PSR_DEBUG_FORCE_EXIT); >> + POSTING_READ(EDP_PSR_DEBUG_CTL); >> + mutex_unlock(&intel_dp->psr_exit_mutex); >> +} >> + >> +void intel_edp_psr_force_exit(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_encoder *encoder; >> + struct intel_dp *intel_dp = NULL; >> + >> + if (!intel_edp_is_psr_enabled(dev)) >> + return; >> + >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> + if (encoder->type == INTEL_OUTPUT_EDP) >> + intel_dp = enc_to_intel_dp(&encoder->base); >> + >> + if (!intel_dp) >> + return; >> + >> + if (WARN_ON(!intel_dp->psr_setup_done)) >> + return; > > If you have to dig out your data like this it usually means that it's at > the wrong spot. Imo we should track a psr_possible bit in the pipe_config > and a psr_enabled (and the locking for the runtime mutex) in the crtc > itself. Also, the psr_setup_done thing is another hint that we should move > this into the crtc. Ok, I'm going to play a bit with this and see what I can come up with. > > Second I prefer if functions with tricky tie-in with existing code are > used in the same patch they're created - if it's split over two patches > review is a bit a pain. Agreed. I just splited out because I accepted review suggestions to let all hooks for the last patch to avoid breaking bisects or something like that. > But if I read the follow-up patch correctly we > call this function from the busy_ioctl unconditionally, which will > obviously result in tons of falls postives - e.g. libdrm uses this ioctl > to manage it's buffer cache. Yes, you are right. Although I noticed that psr was still working fine, I agree that this is too many unecessary calls :( But couldn't find a better way to fix xdm/kde issue. Other ideas was letting psr disabled for so long when it could be there saving power. But as I said, low priority on this right now... Maybe when following your suggestions and the list you made we get it fixed properly ;) > I think I'll punt on this patch and merge just the parts from the next one > for normal backbuffer rendering with pageflips model. > -Daniel Thanks again, Rodrigo > >> + >> + mutex_lock(&intel_dp->psr_exit_mutex); >> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | >> + EDP_PSR_DEBUG_FORCE_EXIT); >> + POSTING_READ(EDP_PSR_DEBUG_CTL); >> + mutex_unlock(&intel_dp->psr_exit_mutex); >> + >> + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, >> + msecs_to_jiffies(100)); >> +} >> + >> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> struct edp_vsc_psr *vsc_psr) >> { >> @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) >> I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP | >> EDP_PSR_DEBUG_MASK_HPD); >> >> + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, >> + intel_edp_psr_delayed_normal_work); >> + mutex_init(&intel_dp->psr_exit_mutex); >> + >> intel_dp->psr_setup_done = true; >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 0f52362..e47f3f3 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -499,6 +499,8 @@ struct intel_dp { >> int backlight_off_delay; >> struct delayed_work panel_vdd_work; >> bool want_panel_vdd; >> + struct delayed_work edp_psr_delayed_normal_work; >> + struct mutex psr_exit_mutex; >> bool psr_setup_done; >> struct intel_connector *attached_connector; >> }; >> @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); >> extern void intel_edp_psr_disable(struct intel_dp *intel_dp); >> extern void intel_edp_psr_update(struct drm_device *dev); >> extern bool intel_edp_is_psr_enabled(struct drm_device *dev); >> +extern void intel_edp_psr_force_exit(struct drm_device *dev); >> >> #endif /* __INTEL_DRV_H__ */ >> -- >> 1.8.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3bca337..dc10345 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1840,6 +1840,7 @@ #define EDP_PSR_PERF_CNT_MASK 0xffffff #define EDP_PSR_DEBUG_CTL 0x64860 +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30) #define EDP_PSR_DEBUG_MASK_LPSP (1<<27) #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26) #define EDP_PSR_DEBUG_MASK_HPD (1<<25) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3c9473c..47e1676 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev) return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; } +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work) +{ + struct intel_dp *intel_dp = container_of(to_delayed_work(__work), + struct intel_dp, + edp_psr_delayed_normal_work); + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&intel_dp->psr_exit_mutex); + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) & + ~EDP_PSR_DEBUG_FORCE_EXIT); + POSTING_READ(EDP_PSR_DEBUG_CTL); + mutex_unlock(&intel_dp->psr_exit_mutex); +} + +void intel_edp_psr_force_exit(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *encoder; + struct intel_dp *intel_dp = NULL; + + if (!intel_edp_is_psr_enabled(dev)) + return; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) + if (encoder->type == INTEL_OUTPUT_EDP) + intel_dp = enc_to_intel_dp(&encoder->base); + + if (!intel_dp) + return; + + if (WARN_ON(!intel_dp->psr_setup_done)) + return; + + mutex_lock(&intel_dp->psr_exit_mutex); + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) | + EDP_PSR_DEBUG_FORCE_EXIT); + POSTING_READ(EDP_PSR_DEBUG_CTL); + mutex_unlock(&intel_dp->psr_exit_mutex); + + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work, + msecs_to_jiffies(100)); +} + static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, struct edp_vsc_psr *vsc_psr) { @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp) I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP | EDP_PSR_DEBUG_MASK_HPD); + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work, + intel_edp_psr_delayed_normal_work); + mutex_init(&intel_dp->psr_exit_mutex); + intel_dp->psr_setup_done = true; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0f52362..e47f3f3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -499,6 +499,8 @@ struct intel_dp { int backlight_off_delay; struct delayed_work panel_vdd_work; bool want_panel_vdd; + struct delayed_work edp_psr_delayed_normal_work; + struct mutex psr_exit_mutex; bool psr_setup_done; struct intel_connector *attached_connector; }; @@ -839,5 +841,6 @@ extern void intel_edp_psr_enable(struct intel_dp *intel_dp); extern void intel_edp_psr_disable(struct intel_dp *intel_dp); extern void intel_edp_psr_update(struct drm_device *dev); extern bool intel_edp_is_psr_enabled(struct drm_device *dev); +extern void intel_edp_psr_force_exit(struct drm_device *dev); #endif /* __INTEL_DRV_H__ */
PSR tracking engine in HSW doesn't detect automagically some directly copy area operations through scanout so we will have to kick it manually and reschedule it to come back to normal operation as soon as possible. v2: Before PSR Hook. Don't force it when busy yet. v3/v4: Solved small conflict. v5: setup once function was already added on previous commit. v6: Use POSTING_READ to make sure the mutex works as a write barrier as suggested by Chris Wilson CC: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 3 +++ 3 files changed, 52 insertions(+)