| Message ID | 20171219052659.2355-4-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote: > DPCD read for the eDP is complete by the time intel_psr_init() is > called, which means we can avoid initializing PSR structures and state > if there is no sink support. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++- > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 64e5a263458c..1a7b28f62570 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > u32 stat[3]; > enum pipe pipe; > bool enabled = false; > + bool sink_support; > > if (!HAS_PSR(dev_priv)) > return -ENODEV; > > + sink_support = dev_priv->psr.sink_support; > + seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > + if (!sink_support) > + return 0; > + > intel_runtime_pm_get(dev_priv); > > mutex_lock(&dev_priv->psr.lock); > - seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); > seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 76339cf387cb..095e0a5a8574 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > if (!crtc_state->has_psr) > return; > > + if (WARN_ON(!CAN_PSR(dev_priv))) > + return; hmm... I believe we will see this warning sooner than later... has_psr is not the same as CAN_PSR. also, btw I didn't like all this crtc_state has_psr x has_psr2. :/ probably this series could also unify that and clean it up. to many has_psr like cases. > + > WARN_ON(dev_priv->drrs.dp); > mutex_lock(&dev_priv->psr.lock); > if (dev_priv->psr.enabled) { > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp, > if (!old_crtc_state->has_psr) > return; > > + if (WARN_ON(!CAN_PSR(dev_priv))) > + return; > + > mutex_lock(&dev_priv->psr.lock); > if (!dev_priv->psr.enabled) { > mutex_unlock(&dev_priv->psr.lock); > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > + if (!dev_priv->psr.sink_support) > + return; > + Why not use CAN_PSR here? > /* Per platform default: all disabled. */ > if (i915_modparams.enable_psr == -1) > i915_modparams.enable_psr = 0; > -- > 2.11.0 >
On Tue, 2017-12-19 at 13:29 -0800, Rodrigo Vivi wrote: > On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote: > > DPCD read for the eDP is complete by the time intel_psr_init() is > > called, which means we can avoid initializing PSR structures and state > > if there is no sink support. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++- > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++++ > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 64e5a263458c..1a7b28f62570 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > u32 stat[3]; > > enum pipe pipe; > > bool enabled = false; > > + bool sink_support; > > > > if (!HAS_PSR(dev_priv)) > > return -ENODEV; > > > > + sink_support = dev_priv->psr.sink_support; > > + seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > + if (!sink_support) > > + return 0; > > + > > intel_runtime_pm_get(dev_priv); > > > > mutex_lock(&dev_priv->psr.lock); > > - seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); > > seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 76339cf387cb..095e0a5a8574 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > if (!crtc_state->has_psr) > > return; > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > + return; > > hmm... I believe we will see this warning sooner than later... > > has_psr is not the same as CAN_PSR. has_psr should not be set in psr_compute_config() unless both source and sink have PSR. So, the warn_on is if we mess up the state preparation. > > also, btw I didn't like all this crtc_state has_psr x has_psr2. :/ > > probably this series could also unify that and clean it up. > to many has_psr like cases. > > > + > > WARN_ON(dev_priv->drrs.dp); > > mutex_lock(&dev_priv->psr.lock); > > if (dev_priv->psr.enabled) { > > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > if (!old_crtc_state->has_psr) > > return; > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > + return; > > + > > mutex_lock(&dev_priv->psr.lock); > > if (!dev_priv->psr.enabled) { > > mutex_unlock(&dev_priv->psr.lock); > > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > > > + if (!dev_priv->psr.sink_support) > > + return; > > + > > Why not use CAN_PSR here? So that we have the right MMIO's in case we want to probe the HW with no sink support. I wasn't what would happen if we reg_read() on PSR registers without the correct MMIO base. > > > /* Per platform default: all disabled. */ > > if (i915_modparams.enable_psr == -1) > > i915_modparams.enable_psr = 0; > > -- > > 2.11.0 > >
first of all sorry for not getting back sooner on this... On Tue, Dec 19, 2017 at 09:40:01PM +0000, Pandiyan, Dhinakaran wrote: > > > > On Tue, 2017-12-19 at 13:29 -0800, Rodrigo Vivi wrote: > > On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote: > > > DPCD read for the eDP is complete by the time intel_psr_init() is > > > called, which means we can avoid initializing PSR structures and state > > > if there is no sink support. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++- > > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++++ > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 64e5a263458c..1a7b28f62570 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > > u32 stat[3]; > > > enum pipe pipe; > > > bool enabled = false; > > > + bool sink_support; > > > > > > if (!HAS_PSR(dev_priv)) > > > return -ENODEV; > > > > > > + sink_support = dev_priv->psr.sink_support; > > > + seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > > + if (!sink_support) > > > + return 0; > > > + > > > intel_runtime_pm_get(dev_priv); > > > > > > mutex_lock(&dev_priv->psr.lock); > > > - seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > > > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); > > > seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > > > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > index 76339cf387cb..095e0a5a8574 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > > if (!crtc_state->has_psr) > > > return; > > > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > > + return; > > > > hmm... I believe we will see this warning sooner than later... > > > > has_psr is not the same as CAN_PSR. > > has_psr should not be set in psr_compute_config() unless both source and > sink have PSR. So, the warn_on is if we mess up the state preparation. > > > > also, btw I didn't like all this crtc_state has_psr x has_psr2. :/ > > > > probably this series could also unify that and clean it up. > > to many has_psr like cases. > > > > > + > > > WARN_ON(dev_priv->drrs.dp); > > > mutex_lock(&dev_priv->psr.lock); > > > if (dev_priv->psr.enabled) { > > > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > > if (!old_crtc_state->has_psr) > > > return; > > > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > > + return; > > > + > > > mutex_lock(&dev_priv->psr.lock); > > > if (!dev_priv->psr.enabled) { > > > mutex_unlock(&dev_priv->psr.lock); > > > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > > > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > > > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > > > > > + if (!dev_priv->psr.sink_support) > > > + return; > > > + > > > > Why not use CAN_PSR here? > > > So that we have the right MMIO's in case we want to probe the HW with no > sink support. I wasn't what would happen if we reg_read() on PSR > registers without the correct MMIO base. But isn't the goal of CAN_PSR to avoid any extra calls on platforms that either doesn't have support nor the panel? In this case we should never being touching any register. > > > > > > /* Per platform default: all disabled. */ > > > if (i915_modparams.enable_psr == -1) > > > i915_modparams.enable_psr = 0; > > > -- > > > 2.11.0 > > >
On Wed, 2018-01-03 at 13:59 -0800, Rodrigo Vivi wrote: > first of all sorry for not getting back sooner on this... > > On Tue, Dec 19, 2017 at 09:40:01PM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Tue, 2017-12-19 at 13:29 -0800, Rodrigo Vivi wrote: > > > On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote: > > > > DPCD read for the eDP is complete by the time intel_psr_init() is > > > > called, which means we can avoid initializing PSR structures and state > > > > if there is no sink support. > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++- > > > > drivers/gpu/drm/i915/intel_psr.c | 9 +++++++++ > > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > > > index 64e5a263458c..1a7b28f62570 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > > > > u32 stat[3]; > > > > enum pipe pipe; > > > > bool enabled = false; > > > > + bool sink_support; > > > > > > > > if (!HAS_PSR(dev_priv)) > > > > return -ENODEV; > > > > > > > > + sink_support = dev_priv->psr.sink_support; > > > > + seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > > > + if (!sink_support) > > > > + return 0; > > > > + > > > > intel_runtime_pm_get(dev_priv); > > > > > > > > mutex_lock(&dev_priv->psr.lock); > > > > - seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > > > > seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); > > > > seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); > > > > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > > index 76339cf387cb..095e0a5a8574 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > > > if (!crtc_state->has_psr) > > > > return; > > > > > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > > > + return; > > > > > > hmm... I believe we will see this warning sooner than later... > > > > > > has_psr is not the same as CAN_PSR. > > > > has_psr should not be set in psr_compute_config() unless both source and > > sink have PSR. So, the warn_on is if we mess up the state preparation. > > > > > > also, btw I didn't like all this crtc_state has_psr x has_psr2. :/ > > > > > > probably this series could also unify that and clean it up. > > > to many has_psr like cases. > > > > > > > + > > > > WARN_ON(dev_priv->drrs.dp); > > > > mutex_lock(&dev_priv->psr.lock); > > > > if (dev_priv->psr.enabled) { > > > > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > > > if (!old_crtc_state->has_psr) > > > > return; > > > > > > > > + if (WARN_ON(!CAN_PSR(dev_priv))) > > > > + return; > > > > + > > > > mutex_lock(&dev_priv->psr.lock); > > > > if (!dev_priv->psr.enabled) { > > > > mutex_unlock(&dev_priv->psr.lock); > > > > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > > > > dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? > > > > HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; > > > > > > > > + if (!dev_priv->psr.sink_support) > > > > + return; > > > > + > > > > > > Why not use CAN_PSR here? > > > > > > So that we have the right MMIO's in case we want to probe the HW with no > > sink support. I wasn't what would happen if we reg_read() on PSR > > registers without the correct MMIO base. > > But isn't the goal of CAN_PSR to avoid any extra calls on platforms > that either doesn't have support nor the panel? In this case we should > never being touching any register. We should not be writing to any PSR related registers if PSR cannot be enabled. But I also think reading any hardware register should be decoupled from a feature's enablement status. > > > > > > > > > /* Per platform default: all disabled. */ > > > > if (i915_modparams.enable_psr == -1) > > > > i915_modparams.enable_psr = 0; > > > > -- > > > > 2.11.0 > > > >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 64e5a263458c..1a7b28f62570 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) u32 stat[3]; enum pipe pipe; bool enabled = false; + bool sink_support; if (!HAS_PSR(dev_priv)) return -ENODEV; + sink_support = dev_priv->psr.sink_support; + seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); + if (!sink_support) + return 0; + intel_runtime_pm_get(dev_priv); mutex_lock(&dev_priv->psr.lock); - seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active)); seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 76339cf387cb..095e0a5a8574 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, if (!crtc_state->has_psr) return; + if (WARN_ON(!CAN_PSR(dev_priv))) + return; + WARN_ON(dev_priv->drrs.dp); mutex_lock(&dev_priv->psr.lock); if (dev_priv->psr.enabled) { @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp, if (!old_crtc_state->has_psr) return; + if (WARN_ON(!CAN_PSR(dev_priv))) + return; + mutex_lock(&dev_priv->psr.lock); if (!dev_priv->psr.enabled) { mutex_unlock(&dev_priv->psr.lock); @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; + if (!dev_priv->psr.sink_support) + return; + /* Per platform default: all disabled. */ if (i915_modparams.enable_psr == -1) i915_modparams.enable_psr = 0;
DPCD read for the eDP is complete by the time intel_psr_init() is called, which means we can avoid initializing PSR structures and state if there is no sink support. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++- drivers/gpu/drm/i915/intel_psr.c | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-)