| Message ID | 1395172267-9203-1-git-send-email-sivachandra@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Mar 2014 12:51:07 -0700 Siva Chandra <sivachandra@chromium.org> wrote: > This property helps one turn PSR "on" and "off" via xrandr. > The default value is same as that of the module param i915.enable_psr. > > Signed-off-by: Siva Chandra <sivachandra@google.com> > --- So are you using this in Chromium for disabling PSR in cases where it doesn't work? Or to optimize power consumption when the kernel driver gets it wrong? Or just for debug? Seems potentially useful, just curious what motivated you guys. Thanks,
On Tue, Mar 18, 2014 at 1:06 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 18 Mar 2014 12:51:07 -0700 > Siva Chandra <sivachandra@chromium.org> wrote: > >> This property helps one turn PSR "on" and "off" via xrandr. >> The default value is same as that of the module param i915.enable_psr. >> >> Signed-off-by: Siva Chandra <sivachandra@google.com> >> --- > > So are you using this in Chromium for disabling PSR in cases where it > doesn't work? Or to optimize power consumption when the kernel driver > gets it wrong? Or just for debug? We are testing a few PSR panels; Having a knob to turn PSR on and off would be of great convenience for manual testing and for test scripts. Thanks, Siva Chandra
On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: > On Tue, Mar 18, 2014 at 1:06 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Tue, 18 Mar 2014 12:51:07 -0700 > > Siva Chandra <sivachandra@chromium.org> wrote: > > > >> This property helps one turn PSR "on" and "off" via xrandr. > >> The default value is same as that of the module param i915.enable_psr. > >> > >> Signed-off-by: Siva Chandra <sivachandra@google.com> > >> --- > > > > So are you using this in Chromium for disabling PSR in cases where it > > doesn't work? Or to optimize power consumption when the kernel driver > > gets it wrong? Or just for debug? > > We are testing a few PSR panels; Having a knob to turn PSR on and off > would be of great convenience for manual testing and for test scripts. Is the module param not good enough for that? Iirc we recheck that every time ... -Daniel
On Wed, Mar 19, 2014 at 09:44:38AM +0100, Daniel Vetter wrote: > On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: > > On Tue, Mar 18, 2014 at 1:06 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Tue, 18 Mar 2014 12:51:07 -0700 > > > Siva Chandra <sivachandra@chromium.org> wrote: > > > > > >> This property helps one turn PSR "on" and "off" via xrandr. > > >> The default value is same as that of the module param i915.enable_psr. > > >> > > >> Signed-off-by: Siva Chandra <sivachandra@google.com> > > >> --- > > > > > > So are you using this in Chromium for disabling PSR in cases where it > > > doesn't work? Or to optimize power consumption when the kernel driver > > > gets it wrong? Or just for debug? > > > > We are testing a few PSR panels; Having a knob to turn PSR on and off > > would be of great convenience for manual testing and for test scripts. > > Is the module param not good enough for that? Iirc we recheck that every > time ... (the module parameter is accessible through a file in sysfs: /sys/module/i915/parameters/enable_psr) Alternatively, doesn't it look like something that belongs to debugfs? ie not an API with a stability guarantee?
On Wed, 19 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: >> On Tue, Mar 18, 2014 at 1:06 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > On Tue, 18 Mar 2014 12:51:07 -0700 >> > Siva Chandra <sivachandra@chromium.org> wrote: >> > >> >> This property helps one turn PSR "on" and "off" via xrandr. >> >> The default value is same as that of the module param i915.enable_psr. >> >> >> >> Signed-off-by: Siva Chandra <sivachandra@google.com> >> >> --- >> > >> > So are you using this in Chromium for disabling PSR in cases where it >> > doesn't work? Or to optimize power consumption when the kernel driver >> > gets it wrong? Or just for debug? >> >> We are testing a few PSR panels; Having a knob to turn PSR on and off >> would be of great convenience for manual testing and for test scripts. > > Is the module param not good enough for that? Iirc we recheck that every > time ... The only thing missing is that changing the module parameter does not actually change the psr state. So you need to change the parameter and then cause intel_edp_psr_update to happen. Jani. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 19, 2014 at 8:32 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 19 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: >>> We are testing a few PSR panels; Having a knob to turn PSR on and off >>> would be of great convenience for manual testing and for test scripts. >> >> Is the module param not good enough for that? Iirc we recheck that every >> time ... > > The only thing missing is that changing the module parameter does not > actually change the psr state. So you need to change the parameter and > then cause intel_edp_psr_update to happen. Yes. The module parameter is not active, as in it does not have any effect if we write to its sysfs file. I did consider making the param active. However, since being able to call intel_edp_psr_update requires one to have a handle to the drm_device, we will have to bring in an ugly global. Thanks, Siva Chandra
On Wed, Mar 19, 2014 at 8:03 AM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Wed, Mar 19, 2014 at 09:44:38AM +0100, Daniel Vetter wrote: >> On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: >> > We are testing a few PSR panels; Having a knob to turn PSR on and off >> > would be of great convenience for manual testing and for test scripts. >> >> Is the module param not good enough for that? Iirc we recheck that every >> time ... > > (the module parameter is accessible through a file in sysfs: > /sys/module/i915/parameters/enable_psr) As I mentioned in the other mail, this param is not active and I couldn't think of any neat solution to make it active. > Alternatively, doesn't it look like something that belongs to debugfs? > ie not an API with a stability guarantee? PSR is part of the eDP standard now. Also, we are only bringing in an on/off knob. Is really going to cause us stability issues? Thanks, Siva Chandra
On Wed, 19 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: > On Wed, Mar 19, 2014 at 8:03 AM, Damien Lespiau > <damien.lespiau@intel.com> wrote: >> On Wed, Mar 19, 2014 at 09:44:38AM +0100, Daniel Vetter wrote: >>> On Tue, Mar 18, 2014 at 01:53:56PM -0700, Siva Chandra wrote: >>> > We are testing a few PSR panels; Having a knob to turn PSR on and off >>> > would be of great convenience for manual testing and for test scripts. >>> >>> Is the module param not good enough for that? Iirc we recheck that every >>> time ... >> >> (the module parameter is accessible through a file in sysfs: >> /sys/module/i915/parameters/enable_psr) > > As I mentioned in the other mail, this param is not active and I > couldn't think of any neat solution to make it active. > >> Alternatively, doesn't it look like something that belongs to debugfs? >> ie not an API with a stability guarantee? > > PSR is part of the eDP standard now. Also, we are only bringing in an > on/off knob. Is really going to cause us stability issues? The point is, if we add this, we need to support and maintain it until the end of time, i.e. the ABI must remain stable. It is quite a commitment to make. If this is for testing only, and the module parameter does not quite cut it, please add it to debugfs. That we can pretty much change at will. BR, Jani. > > Thanks, > Siva Chandra > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Mar 20, 2014 at 3:52 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > If this is for testing only, and the module parameter does not quite cut > it, please add it to debugfs. That we can pretty much change at will. If I understand correctly, you are suggesting that the on/off knob live in debugfs. So, should the module param in sysfs mirror this debugfs knob?
On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: > On Thu, Mar 20, 2014 at 3:52 AM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: >> If this is for testing only, and the module parameter does not quite cut >> it, please add it to debugfs. That we can pretty much change at will. > > If I understand correctly, you are suggesting that the on/off knob > live in debugfs. So, should the module param in sysfs mirror this > debugfs knob? I guess the simplest thing to move forward would be something to update psr state after the module parameter has been changed, although it's perhaps a bit clumsy to use. Jani.
On Thu, Mar 20, 2014 at 6:25 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: >> On Thu, Mar 20, 2014 at 3:52 AM, Jani Nikula >> <jani.nikula@linux.intel.com> wrote: >>> If this is for testing only, and the module parameter does not quite cut >>> it, please add it to debugfs. That we can pretty much change at will. >> >> If I understand correctly, you are suggesting that the on/off knob >> live in debugfs. So, should the module param in sysfs mirror this >> debugfs knob? > > I guess the simplest thing to move forward would be something to update > psr state after the module parameter has been changed, although it's > perhaps a bit clumsy to use. Couldn't understand. May be an example to illustrate what you are saying? Thanks, Siva Chandra
On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: > On Thu, Mar 20, 2014 at 6:25 AM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: >> On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: >>> On Thu, Mar 20, 2014 at 3:52 AM, Jani Nikula >>> <jani.nikula@linux.intel.com> wrote: >>>> If this is for testing only, and the module parameter does not quite cut >>>> it, please add it to debugfs. That we can pretty much change at will. >>> >>> If I understand correctly, you are suggesting that the on/off knob >>> live in debugfs. So, should the module param in sysfs mirror this >>> debugfs knob? >> >> I guess the simplest thing to move forward would be something to update >> psr state after the module parameter has been changed, although it's >> perhaps a bit clumsy to use. > > Couldn't understand. May be an example to illustrate what you are saying? echo 1 > /sys/module/i915/parameters/enable_psr echo something > sys/kernel/debug/dri/0/i915_edp_psr_update where that debugfs file essentially calls intel_edp_psr_update() on update. I guess it could be i915_edp_psr_enable and echoing there would modify the module parameter and call update. With debugfs I'm not so fussy. Jani. > > Thanks, > Siva Chandra
On Thu, Mar 20, 2014 at 04:49:30PM +0200, Jani Nikula wrote: > On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: > > On Thu, Mar 20, 2014 at 6:25 AM, Jani Nikula > > <jani.nikula@linux.intel.com> wrote: > >> On Thu, 20 Mar 2014, Siva Chandra <sivachandra@google.com> wrote: > >>> On Thu, Mar 20, 2014 at 3:52 AM, Jani Nikula > >>> <jani.nikula@linux.intel.com> wrote: > >>>> If this is for testing only, and the module parameter does not quite cut > >>>> it, please add it to debugfs. That we can pretty much change at will. > >>> > >>> If I understand correctly, you are suggesting that the on/off knob > >>> live in debugfs. So, should the module param in sysfs mirror this > >>> debugfs knob? > >> > >> I guess the simplest thing to move forward would be something to update > >> psr state after the module parameter has been changed, although it's > >> perhaps a bit clumsy to use. > > > > Couldn't understand. May be an example to illustrate what you are saying? > > echo 1 > /sys/module/i915/parameters/enable_psr > echo something > sys/kernel/debug/dri/0/i915_edp_psr_update > > where that debugfs file essentially calls intel_edp_psr_update() on > update. > > I guess it could be i915_edp_psr_enable and echoing there would modify > the module parameter and call update. With debugfs I'm not so fussy. Whatever the interface I think it might be nice to allow you to force the mode of PSR operation (main link off vs. standby).
On Thu, Mar 20, 2014 at 7:49 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > echo 1 > /sys/module/i915/parameters/enable_psr > echo something > sys/kernel/debug/dri/0/i915_edp_psr_update > > where that debugfs file essentially calls intel_edp_psr_update() on > update. > > I guess it could be i915_edp_psr_enable and echoing there would modify > the module parameter and call update. I am OK with this, but your last sentence is confusing. If echoing to the debugfs file modifies the module param, why is the echo to the module param required at all? One last note before I stop dragging this thread: We are dealing with few panels which support PSR but in non-standard ways. So, we will likely use this knob for PSR control beyond just testing. While I am OK with debugfs, I would prefer to have a more standard way. Do you think debugfs is the best place to keep this knob? Thanks, Siva Chandra
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 70fbe90..83e6303 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1607,6 +1607,7 @@ typedef struct drm_i915_private { struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property; + struct drm_property *psr_property; uint32_t hw_context_size; struct list_head context_list; @@ -1661,6 +1662,11 @@ enum hdmi_force_audio { HDMI_AUDIO_ON, /* force turn on HDMI audio */ }; +enum psr_state { + EDP_PSR_ON, + EDP_PSR_OFF +}; + #define I915_GTT_OFFSET_NONE ((u32)-1) struct drm_i915_gem_object_ops { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 59ee4dc..c4546fa 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3197,6 +3197,33 @@ static int intel_dp_get_modes(struct drm_connector *connector) return 0; } +static const struct drm_prop_enum_list psr_names[] = { + { EDP_PSR_ON, "on" }, + { EDP_PSR_OFF, "off" } +}; + +static void intel_attach_psr_property(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_property *prop; + + prop = dev_priv->psr_property; + if (prop == NULL) { + prop = drm_property_create_enum( + dev, + i915.enable_psr ? EDP_PSR_ON : EDP_PSR_OFF, + "psr", + psr_names, + ARRAY_SIZE(psr_names)); + if (prop == NULL) + return; + + dev_priv->psr_property = prop; + } + drm_object_attach_property(&connector->base, prop, 0); +} + static bool intel_dp_detect_audio(struct drm_connector *connector) { @@ -3302,6 +3329,15 @@ intel_dp_set_property(struct drm_connector *connector, goto done; } + if (is_edp(intel_dp) && property == dev_priv->psr_property) { + if (val == EDP_PSR_ON) + intel_edp_psr_enable(intel_dp); + else + intel_edp_psr_disable(intel_dp); + + return 0; + } + return -EINVAL; done: @@ -3424,6 +3460,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect { struct intel_connector *intel_connector = to_intel_connector(connector); + intel_attach_psr_property(connector); intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); intel_dp->color_range_auto = true;
This property helps one turn PSR "on" and "off" via xrandr. The default value is same as that of the module param i915.enable_psr. Signed-off-by: Siva Chandra <sivachandra@google.com> --- drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)