| Message ID | 1405099819-11119-11-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 11, 2014 at 10:30:19AM -0700, Rodrigo Vivi wrote: > Panel Self Refresh is an eDP power saving feature specified by VESA's eDP v1.3, > that allows some panel componets to shutdown while you still see static images on > the screen. Besides being supported on the platform it must be supported by the > eDP panel itself. > > Now that we have the propper frontbuffer tracking support and correct locks on place > we can enabled this feature by default. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Yay! Entire series pulled in, thanks a lot for the testing and review. Besides byt psr the big chunk is now polishing the testcase. Iirc I've written down a detailed description of the task in internal jira and assigned it to you, right? I want to have the tests before we enable byt psr to make sure we really catch all the regressions that might crop up. Thanks, Daniel > --- > drivers/gpu/drm/i915/i915_params.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 8145729..bbdee21 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = { > .enable_fbc = -1, > .enable_hangcheck = true, > .enable_ppgtt = -1, > - .enable_psr = 0, > + .enable_psr = 1, > .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), > .disable_power_well = 1, > .enable_ips = 1, > @@ -118,7 +118,7 @@ MODULE_PARM_DESC(enable_ppgtt, > "(-1=auto [default], 0=disabled, 1=aliasing, 2=full)"); > > module_param_named(enable_psr, i915.enable_psr, int, 0600); > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); > +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)"); > > module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); > MODULE_PARM_DESC(preliminary_hw_support, > -- > 1.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Jul 12, 2014 at 12:05:13PM +0200, Daniel Vetter wrote: > On Fri, Jul 11, 2014 at 10:30:19AM -0700, Rodrigo Vivi wrote: > > Panel Self Refresh is an eDP power saving feature specified by VESA's eDP v1.3, > > that allows some panel componets to shutdown while you still see static images on > > the screen. Besides being supported on the platform it must be supported by the > > eDP panel itself. > > > > Now that we have the propper frontbuffer tracking support and correct locks on place > > we can enabled this feature by default. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Yay! > > Entire series pulled in, thanks a lot for the testing and review. Is there any chance we can have the output property to know when PSR is available and active on the eDP? -Chris
On Sat, Jul 12, 2014 at 12:42 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jul 12, 2014 at 12:05:13PM +0200, Daniel Vetter wrote: >> On Fri, Jul 11, 2014 at 10:30:19AM -0700, Rodrigo Vivi wrote: >> > Panel Self Refresh is an eDP power saving feature specified by VESA's eDP v1.3, >> > that allows some panel componets to shutdown while you still see static images on >> > the screen. Besides being supported on the platform it must be supported by the >> > eDP panel itself. >> > >> > Now that we have the propper frontbuffer tracking support and correct locks on place >> > we can enabled this feature by default. >> > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> >> Yay! >> >> Entire series pulled in, thanks a lot for the testing and review. > > Is there any chance we can have the output property to know when PSR is > available and active on the eDP? Oh right, forgotten about this. I guess now that the hw stuff is there we can resurrect this. And with the universal planes support we could move the "prefer backbuffer rendering" hint to planes even where it imo belongs (since e.g. fbc is on the plane). Or maybe we don't care about that level of granularity and just have one per pipe. -Daniel
On Sat, Jul 12, 2014 at 01:20:10PM +0200, Daniel Vetter wrote: > On Sat, Jul 12, 2014 at 12:42 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Jul 12, 2014 at 12:05:13PM +0200, Daniel Vetter wrote: > >> On Fri, Jul 11, 2014 at 10:30:19AM -0700, Rodrigo Vivi wrote: > >> > Panel Self Refresh is an eDP power saving feature specified by VESA's eDP v1.3, > >> > that allows some panel componets to shutdown while you still see static images on > >> > the screen. Besides being supported on the platform it must be supported by the > >> > eDP panel itself. > >> > > >> > Now that we have the propper frontbuffer tracking support and correct locks on place > >> > we can enabled this feature by default. > >> > > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> > >> Yay! > >> > >> Entire series pulled in, thanks a lot for the testing and review. > > > > Is there any chance we can have the output property to know when PSR is > > available and active on the eDP? > > Oh right, forgotten about this. I guess now that the hw stuff is there > we can resurrect this. And with the universal planes support we could > move the "prefer backbuffer rendering" hint to planes even where it > imo belongs (since e.g. fbc is on the plane). Or maybe we don't care > about that level of granularity and just have one per pipe. But PSR is definitely an output property... Please-dont-touch-me is indeed a plane property. -Chris
On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > But PSR is definitely an output property... Please-dont-touch-me is > indeed a plane property. Well I guess we also want this for fbc, and fbc isn't an output property. I guess in the end it doesn't really matter that much as long as it's there, but traditional userspace exposes all output properties to userspace (on X), hence why I think we should hide it a bit ;-) -Daniel
On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: > On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > But PSR is definitely an output property... Please-dont-touch-me is > > indeed a plane property. > > Well I guess we also want this for fbc, and fbc isn't an output > property. I guess in the end it doesn't really matter that much as > long as it's there, but traditional userspace exposes all output > properties to userspace (on X), hence why I think we should hide it a > bit ;-) Hence why I want PSR as an output property. I want to see the status exposed in xrandr. -Chris
On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > But PSR is definitely an output property... Please-dont-touch-me is >> > indeed a plane property. >> >> Well I guess we also want this for fbc, and fbc isn't an output >> property. I guess in the end it doesn't really matter that much as >> long as it's there, but traditional userspace exposes all output >> properties to userspace (on X), hence why I think we should hide it a >> bit ;-) > > Hence why I want PSR as an output property. I want to see the status > exposed in xrandr. Hm, why that? I've thought we only want this to give hints to the compositor/ddx? -Daniel
On Sat, Jul 12, 2014 at 08:15:42PM +0200, Daniel Vetter wrote: > On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: > >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > But PSR is definitely an output property... Please-dont-touch-me is > >> > indeed a plane property. > >> > >> Well I guess we also want this for fbc, and fbc isn't an output > >> property. I guess in the end it doesn't really matter that much as > >> long as it's there, but traditional userspace exposes all output > >> properties to userspace (on X), hence why I think we should hide it a > >> bit ;-) > > > > Hence why I want PSR as an output property. I want to see the status > > exposed in xrandr. > > Hm, why that? I've thought we only want this to give hints to the > compositor/ddx? An interesting factoid to present to the user. Also makes it easy for me to check everything works in X. -Chris
On Sat, Jul 12, 2014 at 8:55 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jul 12, 2014 at 08:15:42PM +0200, Daniel Vetter wrote: >> On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: >> >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> > But PSR is definitely an output property... Please-dont-touch-me is >> >> > indeed a plane property. >> >> >> >> Well I guess we also want this for fbc, and fbc isn't an output >> >> property. I guess in the end it doesn't really matter that much as >> >> long as it's there, but traditional userspace exposes all output >> >> properties to userspace (on X), hence why I think we should hide it a >> >> bit ;-) >> > >> > Hence why I want PSR as an output property. I want to see the status >> > exposed in xrandr. >> >> Hm, why that? I've thought we only want this to give hints to the >> compositor/ddx? > > An interesting factoid to present to the user. Also makes it easy for me > to check everything works in X. Hm not sure we want users to know this stuff. Next they ask to adjust it and then we have the same mess as with kernel options ;-) But I don't care strongly enough really either way. In-kernel I still think we want to keep this on the crtc so that fbc and psr work the same way. The connector property would then just chase the connected crtc to read out the property. -Daniel
On Sat, Jul 12, 2014 at 10:48:30PM +0200, Daniel Vetter wrote: > On Sat, Jul 12, 2014 at 8:55 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Jul 12, 2014 at 08:15:42PM +0200, Daniel Vetter wrote: > >> On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: > >> >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> >> > But PSR is definitely an output property... Please-dont-touch-me is > >> >> > indeed a plane property. > >> >> > >> >> Well I guess we also want this for fbc, and fbc isn't an output > >> >> property. I guess in the end it doesn't really matter that much as > >> >> long as it's there, but traditional userspace exposes all output > >> >> properties to userspace (on X), hence why I think we should hide it a > >> >> bit ;-) > >> > > >> > Hence why I want PSR as an output property. I want to see the status > >> > exposed in xrandr. > >> > >> Hm, why that? I've thought we only want this to give hints to the > >> compositor/ddx? > > > > An interesting factoid to present to the user. Also makes it easy for me > > to check everything works in X. > > Hm not sure we want users to know this stuff. Next they ask to adjust > it and then we have the same mess as with kernel options ;-) But I > don't care strongly enough really either way. In-kernel I still think > we want to keep this on the crtc so that fbc and psr work the same > way. The connector property would then just chase the connected crtc > to read out the property. It can't be crtc as it is an connector properrty... And it is immutable. I think it will be one of those useful things that people would like to check infrequently to make sure everything is functioning as intended (like powertop). I think there may be a few properties like this we can expose. And if people ask why PSR isn't active, then we should do a better job at making sure it stays enabled. -Chris
On Sun, Jul 13, 2014 at 8:30 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, Jul 12, 2014 at 10:48:30PM +0200, Daniel Vetter wrote: >> On Sat, Jul 12, 2014 at 8:55 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Sat, Jul 12, 2014 at 08:15:42PM +0200, Daniel Vetter wrote: >> >> On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: >> >> >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> >> > But PSR is definitely an output property... Please-dont-touch-me is >> >> >> > indeed a plane property. >> >> >> >> >> >> Well I guess we also want this for fbc, and fbc isn't an output >> >> >> property. I guess in the end it doesn't really matter that much as >> >> >> long as it's there, but traditional userspace exposes all output >> >> >> properties to userspace (on X), hence why I think we should hide it a >> >> >> bit ;-) >> >> > >> >> > Hence why I want PSR as an output property. I want to see the status >> >> > exposed in xrandr. >> >> >> >> Hm, why that? I've thought we only want this to give hints to the >> >> compositor/ddx? >> > >> > An interesting factoid to present to the user. Also makes it easy for me >> > to check everything works in X. >> >> Hm not sure we want users to know this stuff. Next they ask to adjust >> it and then we have the same mess as with kernel options ;-) But I >> don't care strongly enough really either way. In-kernel I still think >> we want to keep this on the crtc so that fbc and psr work the same >> way. The connector property would then just chase the connected crtc >> to read out the property. > > It can't be crtc as it is an connector properrty... And it is immutable. > I think it will be one of those useful things that people would like to > check infrequently to make sure everything is functioning as intended > (like powertop). I think there may be a few properties like this we can > expose. And if people ask why PSR isn't active, then we should do a > better job at making sure it stays enabled. Oh, you mean just a psr property? I've thought in general terms of discouraging frontbuffer rendering and included fbc. A simple has_self_refresh_display (don't want to exclude dsi) is obviously a connector thing. But I thought you want to use that as a "should I flip or can I frontbuffer render" hint, and I guessed that fbc should be part of that. At least once we switched it to the sw tracking. -Daniel
On Sun, Jul 13, 2014 at 12:16:20PM +0200, Daniel Vetter wrote: > On Sun, Jul 13, 2014 at 8:30 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Sat, Jul 12, 2014 at 10:48:30PM +0200, Daniel Vetter wrote: > >> On Sat, Jul 12, 2014 at 8:55 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > On Sat, Jul 12, 2014 at 08:15:42PM +0200, Daniel Vetter wrote: > >> >> On Sat, Jul 12, 2014 at 7:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> >> > On Sat, Jul 12, 2014 at 07:14:33PM +0200, Daniel Vetter wrote: > >> >> >> On Sat, Jul 12, 2014 at 2:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> >> >> > But PSR is definitely an output property... Please-dont-touch-me is > >> >> >> > indeed a plane property. > >> >> >> > >> >> >> Well I guess we also want this for fbc, and fbc isn't an output > >> >> >> property. I guess in the end it doesn't really matter that much as > >> >> >> long as it's there, but traditional userspace exposes all output > >> >> >> properties to userspace (on X), hence why I think we should hide it a > >> >> >> bit ;-) > >> >> > > >> >> > Hence why I want PSR as an output property. I want to see the status > >> >> > exposed in xrandr. > >> >> > >> >> Hm, why that? I've thought we only want this to give hints to the > >> >> compositor/ddx? > >> > > >> > An interesting factoid to present to the user. Also makes it easy for me > >> > to check everything works in X. > >> > >> Hm not sure we want users to know this stuff. Next they ask to adjust > >> it and then we have the same mess as with kernel options ;-) But I > >> don't care strongly enough really either way. In-kernel I still think > >> we want to keep this on the crtc so that fbc and psr work the same > >> way. The connector property would then just chase the connected crtc > >> to read out the property. > > > > It can't be crtc as it is an connector properrty... And it is immutable. > > I think it will be one of those useful things that people would like to > > check infrequently to make sure everything is functioning as intended > > (like powertop). I think there may be a few properties like this we can > > expose. And if people ask why PSR isn't active, then we should do a > > better job at making sure it stays enabled. > > Oh, you mean just a psr property? I've thought in general terms of > discouraging frontbuffer rendering and included fbc. A simple > has_self_refresh_display (don't want to exclude dsi) is obviously a > connector thing. But I thought you want to use that as a "should I > flip or can I frontbuffer render" hint, and I guessed that fbc should > be part of that. At least once we switched it to the sw tracking. Yes, I was after both. But at the moment, I am interested in having a user-visible flag for showing that your fancy low power display is working. -Chris
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 8145729..bbdee21 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = { .enable_fbc = -1, .enable_hangcheck = true, .enable_ppgtt = -1, - .enable_psr = 0, + .enable_psr = 1, .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), .disable_power_well = 1, .enable_ips = 1, @@ -118,7 +118,7 @@ MODULE_PARM_DESC(enable_ppgtt, "(-1=auto [default], 0=disabled, 1=aliasing, 2=full)"); module_param_named(enable_psr, i915.enable_psr, int, 0600); -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)"); +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)"); module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); MODULE_PARM_DESC(preliminary_hw_support,
Panel Self Refresh is an eDP power saving feature specified by VESA's eDP v1.3, that allows some panel componets to shutdown while you still see static images on the screen. Besides being supported on the platform it must be supported by the eDP panel itself. Now that we have the propper frontbuffer tracking support and correct locks on place we can enabled this feature by default. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)