| Message ID | 1373579105-1732-12-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote: > 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> Ok, I've slurped this series in with the few bikesheds from Paulo applied and two patches not merged: - The userspace interface part, since I don't want to commit yet to an interface as long as things are unclear. - And the invalidation part since imo that parts isn't properly thought through yet. See my other mail to Chris' RFC for ideas. Now on the topic fbc, psr and frontbuffer rendering: Now that I've appeased our dear PDT it's time to stop building castles on sand imo. Items to pay back a bit of the technical dept: - Untangle the "is fbc/psr" allowed mess. Imo we should add more static (i.e. only changes at modesets) information to the pipe config and track dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this way update_fbc/psr would only need to check dynamic state and more important would never need to frob the basic setup (like reallocating the compressed buffer and similar things). - Implement precise frontbuffer rendering tracking and disabling of fbc/psr for legacy userspace and rip out the hw tracking. If we have to add tons of workarounds (like for fbc), have performance this or just can't use it in a bunch of cases (sprites for psr) we might as well just track everything in the kernel. - Testcases. With pipe CRC checksums we should be able to make sure that the frontbuffer invalidation actually happens. And if we do it all in the kernel the difference should be all-or-nothing, so much easier to test than with hw tracking where sometimes something seems to slip through. - low-level improvements. I've only really reviewed the fbc code in-depth and discussed a few things with Art, but there's definitely a few things we need to do. One of the important things imo is to enable fbc everywhere we can to have as wide test coverage as possible for this feature. I'll block future hw enabling in this area on the above list (i.e. vlv psr or patches for -internal). I'll reconsider my stance if e.g. vlv psr is used as a demonstration vehicle for the refactored psr tracking. But since fbc can be used on many more machines (even desktops and apparently even when other pipes are enabled) I think we should push that forward first and use fbc to create solid tests and interfaces for userspace&kernel to cooperate on frontbuffer rendering. Cheers, Daniel > --- > 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 46bf7e3..703bc69 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3758,6 +3758,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 10a3629..eb4e49b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2272,6 +2272,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.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 18, 2013 at 6:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote: >> 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> > > Ok, I've slurped this series in with the few bikesheds from Paulo applied > and two patches not merged: > - The userspace interface part, since I don't want to commit yet to an > interface as long as things are unclear. > - And the invalidation part since imo that parts isn't properly thought > through yet. See my other mail to Chris' RFC for ideas. Thank you very much. > > Now on the topic fbc, psr and frontbuffer rendering: > > Now that I've appeased our dear PDT it's time to stop building castles on > sand imo. What about psr at Baytrail? :/ > Items to pay back a bit of the technical dept: > > - Untangle the "is fbc/psr" allowed mess. Imo we should add more static > (i.e. only changes at modesets) information to the pipe config and track > dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this > way update_fbc/psr would only need to check dynamic state and more > important would never need to frob the basic setup (like reallocating > the compressed buffer and similar things). > > - Implement precise frontbuffer rendering tracking and disabling of > fbc/psr for legacy userspace and rip out the hw tracking. If we have to > add tons of workarounds (like for fbc), have performance this or just > can't use it in a bunch of cases (sprites for psr) we might as well just > track everything in the kernel. > > - Testcases. With pipe CRC checksums we should be able to make sure that > the frontbuffer invalidation actually happens. And if we do it all in > the kernel the difference should be all-or-nothing, so much easier to > test than with hw tracking where sometimes something seems to slip > through. > > - low-level improvements. I've only really reviewed the fbc code in-depth > and discussed a few things with Art, but there's definitely a few things > we need to do. One of the important things imo is to enable fbc > everywhere we can to have as wide test coverage as possible for this > feature. > > I'll block future hw enabling in this area on the above list (i.e. vlv psr > or patches for -internal). I'll reconsider my stance if e.g. vlv psr is > used as a demonstration vehicle for the refactored psr tracking. But since > fbc can be used on many more machines (even desktops and apparently even > when other pipes are enabled) I think we should push that forward first > and use fbc to create solid tests and interfaces for userspace&kernel to > cooperate on frontbuffer rendering. I fully agree with you. I'm going to start playing with fbc and psr at pipe_config, etc and check that discussion with art and try other changes in fbc. I'm just afraid the pressure for psr-vlv will be big as well. Anyway, I give up on those 2 patches you didn't accepted because I think this other things have more priority compared to xdm/kde bug. Since it is disabled by default it won't cause troubles for kde users and it is working well on gnome and unity that by any chance has a hsw with edp that supports psr and use i915.enable_psr=1 parameter :/ > > Cheers, Daniel > >> --- >> 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 46bf7e3..703bc69 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -3758,6 +3758,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 10a3629..eb4e49b 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2272,6 +2272,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.7.11.7 >> >> _______________________________________________ >> 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_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 46bf7e3..703bc69 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3758,6 +3758,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 10a3629..eb4e49b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2272,6 +2272,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(+)