| Message ID | 1372726331-31617-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/7/1 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > PSR must be enabled after transcoder and port are running. > And it is only available for HSW. > > v2: move enable/disable to intel_ddi > v3: The spec suggests PSR should be disabled even before backlight (by pzanoni) > v4: also disabling and enabling whenever panel is disabled/enabled. > v5: make it last patch to avoid breaking whenever bisecting. So calling for > update and force exit came to this patch along with enable/disable calls. > v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo. > > CC: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ > drivers/gpu/drm/i915/intel_display.c | 1 + > 3 files changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c68b90f..f2e135b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3786,6 +3786,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > + intel_edp_psr_force_exit(dev); Shouldn't we enter/leave PSR in exactly the same cases as we enter/leave FBC? This function doesn't seem to touch FBC. I also see, for example, intel_unpin_work_fn calling intel_update_fbc, but at that point we don't update PSR. Also intel_enable_primary and intel_disable_primary also call intel_update_fbc. If we conclude that PSR and FBC need to be disabled/enabled at exactly the same places, can we do some rework to unify this? > + > /* Count all active objects as busy, even if they are currently not used > * by the gpu. Users of this interface expect objects to eventually > * become non-busy without any further actions, therefore emit any > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 324211a..4211925 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > intel_dp_stop_link_train(intel_dp); > > ironlake_edp_backlight_on(intel_dp); > + intel_edp_psr_enable(intel_dp); > } > > if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { > @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > if (type == INTEL_OUTPUT_EDP) { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + intel_edp_psr_disable(intel_dp); > ironlake_edp_backlight_off(intel_dp); > } > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 6fafa43..5bbfed0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2236,6 +2236,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > } > > intel_update_fbc(dev); > + intel_edp_psr_update(dev); > mutex_unlock(&dev->struct_mutex); > > intel_crtc_update_sarea_pos(crtc, x, y); > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 08, 2013 at 10:13:30AM -0300, Paulo Zanoni wrote: > 2013/7/1 Rodrigo Vivi <rodrigo.vivi@gmail.com>: > > PSR must be enabled after transcoder and port are running. > > And it is only available for HSW. > > > > v2: move enable/disable to intel_ddi > > v3: The spec suggests PSR should be disabled even before backlight (by pzanoni) > > v4: also disabling and enabling whenever panel is disabled/enabled. > > v5: make it last patch to avoid breaking whenever bisecting. So calling for > > update and force exit came to this patch along with enable/disable calls. > > v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo. > > > > CC: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ > > drivers/gpu/drm/i915/intel_display.c | 1 + > > 3 files changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index c68b90f..f2e135b 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3786,6 +3786,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > > goto unlock; > > } > > > > + intel_edp_psr_force_exit(dev); > > Shouldn't we enter/leave PSR in exactly the same cases as we > enter/leave FBC? This function doesn't seem to touch FBC. I also see, > for example, intel_unpin_work_fn calling intel_update_fbc, but at that > point we don't update PSR. Also intel_enable_primary and > intel_disable_primary also call intel_update_fbc. If we conclude that > PSR and FBC need to be disabled/enabled at exactly the same places, > can we do some rework to unify this? I think there's some room for fbc and psr handling code to differ, since fbc only works on the primary plane, whereas psr works on the entire display (so also needs to update when cursor/sprites change or gamma table changes or any other disaplay attribute). But I agree with you that we should share calls to psr/fbc placed in common gem/modeset code as much as possible. -Daniel > > > > + > > /* Count all active objects as busy, even if they are currently not used > > * by the gpu. Users of this interface expect objects to eventually > > * become non-busy without any further actions, therefore emit any > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 324211a..4211925 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > > intel_dp_stop_link_train(intel_dp); > > > > ironlake_edp_backlight_on(intel_dp); > > + intel_edp_psr_enable(intel_dp); > > } > > > > if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { > > @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > if (type == INTEL_OUTPUT_EDP) { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > > + intel_edp_psr_disable(intel_dp); > > ironlake_edp_backlight_off(intel_dp); > > } > > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 6fafa43..5bbfed0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2236,6 +2236,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > } > > > > intel_update_fbc(dev); > > + intel_edp_psr_update(dev); > > mutex_unlock(&dev->struct_mutex); > > > > intel_crtc_update_sarea_pos(crtc, x, y); > > -- > > 1.8.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
I tried to enable psr using fbc calls and all I got was a frozen machine... I prefer to stay with the way I'm sure it works On Mon, Jul 8, 2013 at 4:46 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jul 08, 2013 at 10:13:30AM -0300, Paulo Zanoni wrote: >> 2013/7/1 Rodrigo Vivi <rodrigo.vivi@gmail.com>: >> > PSR must be enabled after transcoder and port are running. >> > And it is only available for HSW. >> > >> > v2: move enable/disable to intel_ddi >> > v3: The spec suggests PSR should be disabled even before backlight (by pzanoni) >> > v4: also disabling and enabling whenever panel is disabled/enabled. >> > v5: make it last patch to avoid breaking whenever bisecting. So calling for >> > update and force exit came to this patch along with enable/disable calls. >> > v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo. >> > >> > CC: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> > --- >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ >> > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ >> > drivers/gpu/drm/i915/intel_display.c | 1 + >> > 3 files changed, 5 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> > index c68b90f..f2e135b 100644 >> > --- a/drivers/gpu/drm/i915/i915_gem.c >> > +++ b/drivers/gpu/drm/i915/i915_gem.c >> > @@ -3786,6 +3786,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >> > goto unlock; >> > } >> > >> > + intel_edp_psr_force_exit(dev); >> >> Shouldn't we enter/leave PSR in exactly the same cases as we >> enter/leave FBC? This function doesn't seem to touch FBC. I also see, >> for example, intel_unpin_work_fn calling intel_update_fbc, but at that >> point we don't update PSR. Also intel_enable_primary and >> intel_disable_primary also call intel_update_fbc. If we conclude that >> PSR and FBC need to be disabled/enabled at exactly the same places, >> can we do some rework to unify this? > > I think there's some room for fbc and psr handling code to differ, since > fbc only works on the primary plane, whereas psr works on the entire > display (so also needs to update when cursor/sprites change or gamma table > changes or any other disaplay attribute). But I agree with you that we > should share calls to psr/fbc placed in common gem/modeset code as much as > possible. > -Daniel > >> >> >> > + >> > /* Count all active objects as busy, even if they are currently not used >> > * by the gpu. Users of this interface expect objects to eventually >> > * become non-busy without any further actions, therefore emit any >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> > index 324211a..4211925 100644 >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> > @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) >> > intel_dp_stop_link_train(intel_dp); >> > >> > ironlake_edp_backlight_on(intel_dp); >> > + intel_edp_psr_enable(intel_dp); >> > } >> > >> > if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { >> > @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) >> > if (type == INTEL_OUTPUT_EDP) { >> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> > >> > + intel_edp_psr_disable(intel_dp); >> > ironlake_edp_backlight_off(intel_dp); >> > } >> > } >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index 6fafa43..5bbfed0 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -2236,6 +2236,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, >> > } >> > >> > intel_update_fbc(dev); >> > + intel_edp_psr_update(dev); >> > mutex_unlock(&dev->struct_mutex); >> > >> > intel_crtc_update_sarea_pos(crtc, x, y); >> > -- >> > 1.8.1.4 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni >> _______________________________________________ >> 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
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c68b90f..f2e135b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3786,6 +3786,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, goto unlock; } + intel_edp_psr_force_exit(dev); + /* Count all active objects as busy, even if they are currently not used * by the gpu. Users of this interface expect objects to eventually * become non-busy without any further actions, therefore emit any diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 324211a..4211925 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) intel_dp_stop_link_train(intel_dp); ironlake_edp_backlight_on(intel_dp); + intel_edp_psr_enable(intel_dp); } if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) { @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) if (type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + intel_edp_psr_disable(intel_dp); ironlake_edp_backlight_off(intel_dp); } } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6fafa43..5bbfed0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2236,6 +2236,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, } intel_update_fbc(dev); + intel_edp_psr_update(dev); mutex_unlock(&dev->struct_mutex); intel_crtc_update_sarea_pos(crtc, x, y);
PSR must be enabled after transcoder and port are running. And it is only available for HSW. v2: move enable/disable to intel_ddi v3: The spec suggests PSR should be disabled even before backlight (by pzanoni) v4: also disabling and enabling whenever panel is disabled/enabled. v5: make it last patch to avoid breaking whenever bisecting. So calling for update and force exit came to this patch along with enable/disable calls. v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo. CC: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ drivers/gpu/drm/i915/intel_ddi.c | 2 ++ drivers/gpu/drm/i915/intel_display.c | 1 + 3 files changed, 5 insertions(+)