| Message ID | 20180705003121.2478-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 04, 2018 at 05:31:21PM -0700, Dhinakaran Pandiyan wrote: > This allows to read i915_edp_psr_status from tests without triggering > any AUX communication. Take this opportunity to move this under the > eDP-1 connector directory as the status we print is of the sink. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++---------------- > 1 file changed, 39 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index f6142d78ede4..5069d5dedafe 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2592,6 +2592,41 @@ static const struct file_operations i915_guc_log_relay_fops = { > .release = i915_guc_log_relay_release, > }; > > +static int i915_psr_sink_status_show(struct seq_file *m, void *data) > +{ > + u8 val; > + static const char * const sink_status[] = { > + "inactive", > + "transition to active, capture and display", > + "active, display from RFB", > + "active, capture and display on sink device timings", > + "transition to inactive, capture and display, timing re-sync", > + "reserved", > + "reserved", > + "sink internal error", > + }; > + struct drm_connector *connector = m->private; > + struct intel_dp *intel_dp = > + enc_to_intel_dp(&intel_attached_encoder(connector)->base); > + > + if (connector->status != connector_status_connected) > + return -ENODEV; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { > + const char *str = "unknown"; > + > + val &= DP_PSR_SINK_STATE_MASK; > + if (val < ARRAY_SIZE(sink_status)) > + str = sink_status[val]; > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > + } else { > + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); > + } > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > + > static void > psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > { > @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown"); > } > > -static const char *psr_sink_status(u8 val) > -{ > - static const char * const sink_status[] = { > - "inactive", > - "transition to active, capture and display", > - "active, display from RFB", > - "active, capture and display on sink device timings", > - "transition to inactive, capture and display, timing re-sync", > - "reserved", > - "reserved", > - "sink internal error" > - }; > - > - val &= DP_PSR_SINK_STATE_MASK; > - if (val < ARRAY_SIZE(sink_status)) > - return sink_status[val]; > - > - return "unknown"; > -} > - > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > } > > psr_source_status(dev_priv, m); > - > - if (dev_priv->psr.enabled) { > - struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux; > - u8 val; > - > - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1) > - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > - psr_sink_status(val)); > - } > mutex_unlock(&dev_priv->psr.lock); I wonder if we shouldn't get this lock there to make sure that no PSR transition from our driver is getting called. > > if (READ_ONCE(dev_priv->psr.debug)) { > @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct drm_connector *connector) > debugfs_create_file("i915_dpcd", S_IRUGO, root, > connector, &i915_dpcd_fops); > > - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { it seems that we should also move the other status file here to this block... > debugfs_create_file("i915_panel_timings", S_IRUGO, root, > connector, &i915_panel_fops); > + debugfs_create_file("i915_psr_sink_status", S_IRUGO, root, > + connector, &i915_psr_sink_status_fops); ... and also rename the other 2 entries to be in pair with this new one: or all i915_edp_psr or all i915_psr... > + } > > return 0; > } > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thursday, July 5, 2018 2:04:18 PM PDT Rodrigo Vivi wrote: > On Wed, Jul 04, 2018 at 05:31:21PM -0700, Dhinakaran Pandiyan wrote: > > This allows to read i915_edp_psr_status from tests without triggering > > any AUX communication. Take this opportunity to move this under the > > eDP-1 connector directory as the status we print is of the sink. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 69 > > +++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), > > 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c index f6142d78ede4..5069d5dedafe > > 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2592,6 +2592,41 @@ static const struct file_operations > > i915_guc_log_relay_fops = {> > > .release = i915_guc_log_relay_release, > > > > }; > > > > +static int i915_psr_sink_status_show(struct seq_file *m, void *data) > > +{ > > + u8 val; > > + static const char * const sink_status[] = { > > + "inactive", > > + "transition to active, capture and display", > > + "active, display from RFB", > > + "active, capture and display on sink device timings", > > + "transition to inactive, capture and display, timing re-sync", > > + "reserved", > > + "reserved", > > + "sink internal error", > > + }; > > + struct drm_connector *connector = m->private; > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(&intel_attached_encoder(connector)->base); > > + > > + if (connector->status != connector_status_connected) > > + return -ENODEV; > > + > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { > > + const char *str = "unknown"; > > + > > + val &= DP_PSR_SINK_STATE_MASK; > > + if (val < ARRAY_SIZE(sink_status)) > > + str = sink_status[val]; > > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > > + } else { > > + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); > > + } > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > + > > > > static void > > psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > > { > > > > @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m)> > > seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown"); > > > > } > > > > -static const char *psr_sink_status(u8 val) > > -{ > > - static const char * const sink_status[] = { > > - "inactive", > > - "transition to active, capture and display", > > - "active, display from RFB", > > - "active, capture and display on sink device timings", > > - "transition to inactive, capture and display, timing re-sync", > > - "reserved", > > - "reserved", > > - "sink internal error" > > - }; > > - > > - val &= DP_PSR_SINK_STATE_MASK; > > - if (val < ARRAY_SIZE(sink_status)) > > - return sink_status[val]; > > - > > - return "unknown"; > > -} > > - > > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > { > > > > struct drm_i915_private *dev_priv = node_to_i915(m->private); > > > > @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct seq_file *m, > > void *data)> > > } > > > > psr_source_status(dev_priv, m); > > > > - > > - if (dev_priv->psr.enabled) { > > - struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux; > > - u8 val; > > - > > - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1) > > - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > > - psr_sink_status(val)); > > - } > > > > mutex_unlock(&dev_priv->psr.lock); > > I wonder if we shouldn't get this lock there to make sure taat no > PSR transition from our driver is getting called. > This lock was useful as we wanted dev_priv->psr.enabled == intel_dp to be valid. I don't see why we have to serialize sink DPCD reads w.r.t the driver's PSR state transitions. > > if (READ_ONCE(dev_priv->psr.debug)) { > > > > @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct drm_connector > > *connector)> > > debugfs_create_file("i915_dpcd", S_IRUGO, root, > > > > connector, &i915_dpcd_fops); > > > > - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) > > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > > it seems that we should also move the other status file here to this > block... Do you mean i915_edp_psr_status? As I see it, that is a feature specific debugfs node that includes source information as well. The ones under $debugfs/eDP-1/ are all panel related. > > debugfs_create_file("i915_panel_timings", S_IRUGO, root, > > > > connector, &i915_panel_fops); > > > > + debugfs_create_file("i915_psr_sink_status", S_IRUGO, root, > > + connector, &i915_psr_sink_status_fops); > > ... and also rename the other 2 entries to be in pair with this new one: > > or all i915_edp_psr or all i915_psr... > I wrote this as i915_psr and realized that it might get confusing since we also have i915_edp_psr_status for feature status. And "_edp_" is redundant because all the nodes are under the eDP-1 directory. But, yeah I can change it to "i915_psr" if you think that is better. > > + } > > > > return 0; > > > > }
On Thu, Jul 05, 2018 at 02:27:46PM -0700, Dhinakaran Pandiyan wrote: > On Thursday, July 5, 2018 2:04:18 PM PDT Rodrigo Vivi wrote: > > On Wed, Jul 04, 2018 at 05:31:21PM -0700, Dhinakaran Pandiyan wrote: > > > This allows to read i915_edp_psr_status from tests without triggering > > > any AUX communication. Take this opportunity to move this under the > > > eDP-1 connector directory as the status we print is of the sink. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 69 > > > +++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), > > > 30 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c index f6142d78ede4..5069d5dedafe > > > 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2592,6 +2592,41 @@ static const struct file_operations > > > i915_guc_log_relay_fops = {> > > > .release = i915_guc_log_relay_release, > > > > > > }; > > > > > > +static int i915_psr_sink_status_show(struct seq_file *m, void *data) > > > +{ > > > + u8 val; > > > + static const char * const sink_status[] = { > > > + "inactive", > > > + "transition to active, capture and display", > > > + "active, display from RFB", > > > + "active, capture and display on sink device timings", > > > + "transition to inactive, capture and display, timing re-sync", > > > + "reserved", > > > + "reserved", > > > + "sink internal error", > > > + }; > > > + struct drm_connector *connector = m->private; > > > + struct intel_dp *intel_dp = > > > + enc_to_intel_dp(&intel_attached_encoder(connector)->base); > > > + > > > + if (connector->status != connector_status_connected) > > > + return -ENODEV; > > > + > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { > > > + const char *str = "unknown"; > > > + > > > + val &= DP_PSR_SINK_STATE_MASK; > > > + if (val < ARRAY_SIZE(sink_status)) > > > + str = sink_status[val]; > > > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > > > + } else { > > > + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); > > > + } > > > + > > > + return 0; > > > +} > > > +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > > + > > > > > > static void > > > psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > > > { > > > > > > @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private > > > *dev_priv, struct seq_file *m)> > > > seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, > "unknown"); > > > > > > } > > > > > > -static const char *psr_sink_status(u8 val) > > > -{ > > > - static const char * const sink_status[] = { > > > - "inactive", > > > - "transition to active, capture and display", > > > - "active, display from RFB", > > > - "active, capture and display on sink device timings", > > > - "transition to inactive, capture and display, timing re-sync", > > > - "reserved", > > > - "reserved", > > > - "sink internal error" > > > - }; > > > - > > > - val &= DP_PSR_SINK_STATE_MASK; > > > - if (val < ARRAY_SIZE(sink_status)) > > > - return sink_status[val]; > > > - > > > - return "unknown"; > > > -} > > > - > > > > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > > { > > > > > > struct drm_i915_private *dev_priv = node_to_i915(m->private); > > > > > > @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct seq_file *m, > > > void *data)> > > > } > > > > > > psr_source_status(dev_priv, m); > > > > > > - > > > - if (dev_priv->psr.enabled) { > > > - struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux; > > > - u8 val; > > > - > > > - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1) > > > - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > > > - psr_sink_status(val)); > > > - } > > > > > > mutex_unlock(&dev_priv->psr.lock); > > > > I wonder if we shouldn't get this lock there to make sure taat no > > PSR transition from our driver is getting called. > > > This lock was useful as we wanted dev_priv->psr.enabled == intel_dp to be > valid. I don't see why we have to serialize sink DPCD reads w.r.t the driver's > PSR state transitions. hm... makes sense... > > > > if (READ_ONCE(dev_priv->psr.debug)) { > > > > > > @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct drm_connector > > > *connector)> > > > debugfs_create_file("i915_dpcd", S_IRUGO, root, > > > > > > connector, &i915_dpcd_fops); > > > > > > - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) > > > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > > > > it seems that we should also move the other status file here to this > > block... > > Do you mean i915_edp_psr_status? As I see it, that is a feature specific > debugfs node that includes source information as well. The ones under > $debugfs/eDP-1/ are all panel related. oh good point... > > > > debugfs_create_file("i915_panel_timings", S_IRUGO, root, > > > > > > connector, &i915_panel_fops); > > > > > > + debugfs_create_file("i915_psr_sink_status", S_IRUGO, root, > > > + connector, &i915_psr_sink_status_fops); > > > > ... and also rename the other 2 entries to be in pair with this new one: > > > > or all i915_edp_psr or all i915_psr... > > > I wrote this as i915_psr and realized that it might get confusing since we > also have i915_edp_psr_status for feature status. And "_edp_" is redundant > because all the nodes are under the eDP-1 directory. But, yeah I can change > it to "i915_psr" if you think that is better. well, the i915_edp_psr_status is before we had this directory I think and the idea of having the "_edp_" was to name it as eDP-1 when we had multiple cases... So, whatever we decide for that... move or not to eDP-1, remove or not "_edp_" I think we should do in a separated patch... So, for this one here: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > + } > > > > > > return 0; > > > > > > } > >
On Wed, Jul 04, 2018 at 05:31:21PM -0700, Dhinakaran Pandiyan wrote: > This allows to read i915_edp_psr_status from tests without triggering > any AUX communication. Take this opportunity to move this under the > eDP-1 connector directory as the status we print is of the sink. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++---------------- > 1 file changed, 39 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index f6142d78ede4..5069d5dedafe 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2592,6 +2592,41 @@ static const struct file_operations i915_guc_log_relay_fops = { > .release = i915_guc_log_relay_release, > }; > > +static int i915_psr_sink_status_show(struct seq_file *m, void *data) > +{ > + u8 val; > + static const char * const sink_status[] = { > + "inactive", > + "transition to active, capture and display", > + "active, display from RFB", > + "active, capture and display on sink device timings", > + "transition to inactive, capture and display, timing re-sync", > + "reserved", > + "reserved", > + "sink internal error", > + }; > + struct drm_connector *connector = m->private; > + struct intel_dp *intel_dp = > + enc_to_intel_dp(&intel_attached_encoder(connector)->base); > + > + if (connector->status != connector_status_connected) > + return -ENODEV; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { > + const char *str = "unknown"; > + > + val &= DP_PSR_SINK_STATE_MASK; > + if (val < ARRAY_SIZE(sink_status)) > + str = sink_status[val]; > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > + } else { > + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); > + } > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > + > static void > psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > { > @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) > seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown"); > } > > -static const char *psr_sink_status(u8 val) > -{ > - static const char * const sink_status[] = { > - "inactive", > - "transition to active, capture and display", > - "active, display from RFB", > - "active, capture and display on sink device timings", > - "transition to inactive, capture and display, timing re-sync", > - "reserved", > - "reserved", > - "sink internal error" > - }; > - > - val &= DP_PSR_SINK_STATE_MASK; > - if (val < ARRAY_SIZE(sink_status)) > - return sink_status[val]; > - > - return "unknown"; > -} > - > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > } > > psr_source_status(dev_priv, m); > - > - if (dev_priv->psr.enabled) { > - struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux; > - u8 val; > - > - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1) > - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > - psr_sink_status(val)); > - } > mutex_unlock(&dev_priv->psr.lock); > > if (READ_ONCE(dev_priv->psr.debug)) { > @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct drm_connector *connector) > debugfs_create_file("i915_dpcd", S_IRUGO, root, > connector, &i915_dpcd_fops); > > - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > debugfs_create_file("i915_panel_timings", S_IRUGO, root, > connector, &i915_panel_fops); > + debugfs_create_file("i915_psr_sink_status", S_IRUGO, root, > + connector, &i915_psr_sink_status_fops); my OCD on names here about i915_psr vs i915_edp_psr is still a real thing although they are on different places ;) We could probably remove "_edp" from the others, because psr is an edp only after all.... Anyways let's not block the progress here ;) Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + } > > return 0; > } > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-07-12 at 16:26 -0700, Rodrigo Vivi wrote: > On Wed, Jul 04, 2018 at 05:31:21PM -0700, Dhinakaran Pandiyan wrote: > > > > This allows to read i915_edp_psr_status from tests without > > triggering > > any AUX communication. Take this opportunity to move this under the > > eDP-1 connector directory as the status we print is of the sink. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++-- > > -------------- > > 1 file changed, 39 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index f6142d78ede4..5069d5dedafe 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2592,6 +2592,41 @@ static const struct file_operations > > i915_guc_log_relay_fops = { > > .release = i915_guc_log_relay_release, > > }; > > > > +static int i915_psr_sink_status_show(struct seq_file *m, void > > *data) > > +{ > > + u8 val; > > + static const char * const sink_status[] = { > > + "inactive", > > + "transition to active, capture and display", > > + "active, display from RFB", > > + "active, capture and display on sink device > > timings", > > + "transition to inactive, capture and display, > > timing re-sync", > > + "reserved", > > + "reserved", > > + "sink internal error", > > + }; > > + struct drm_connector *connector = m->private; > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(&intel_attached_encoder(connector) > > ->base); > > + > > + if (connector->status != connector_status_connected) > > + return -ENODEV; > > + > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) > > == 1) { > > + const char *str = "unknown"; > > + > > + val &= DP_PSR_SINK_STATE_MASK; > > + if (val < ARRAY_SIZE(sink_status)) > > + str = sink_status[val]; > > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > > str); > > + } else { > > + DRM_ERROR("dpcd read (at %u) failed\n", > > DP_PSR_STATUS); > > + } > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > + > > static void > > psr_source_status(struct drm_i915_private *dev_priv, struct > > seq_file *m) > > { > > @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m) > > seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > psr_status, "unknown"); > > } > > > > -static const char *psr_sink_status(u8 val) > > -{ > > - static const char * const sink_status[] = { > > - "inactive", > > - "transition to active, capture and display", > > - "active, display from RFB", > > - "active, capture and display on sink device > > timings", > > - "transition to inactive, capture and display, > > timing re-sync", > > - "reserved", > > - "reserved", > > - "sink internal error" > > - }; > > - > > - val &= DP_PSR_SINK_STATE_MASK; > > - if (val < ARRAY_SIZE(sink_status)) > > - return sink_status[val]; > > - > > - return "unknown"; > > -} > > - > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > { > > struct drm_i915_private *dev_priv = node_to_i915(m- > > >private); > > @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct > > seq_file *m, void *data) > > } > > > > psr_source_status(dev_priv, m); > > - > > - if (dev_priv->psr.enabled) { > > - struct drm_dp_aux *aux = &dev_priv->psr.enabled- > > >aux; > > - u8 val; > > - > > - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == > > 1) > > - seq_printf(m, "Sink PSR status: 0x%x > > [%s]\n", val, > > - psr_sink_status(val)); > > - } > > mutex_unlock(&dev_priv->psr.lock); > > > > if (READ_ONCE(dev_priv->psr.debug)) { > > @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct > > drm_connector *connector) > > debugfs_create_file("i915_dpcd", S_IRUGO, root, > > connector, &i915_dpcd_fops); > > > > - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) > > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > > debugfs_create_file("i915_panel_timings", S_IRUGO, > > root, > > connector, &i915_panel_fops); > > + debugfs_create_file("i915_psr_sink_status", > > S_IRUGO, root, > > + connector, > > &i915_psr_sink_status_fops); > my OCD on names here about i915_psr vs i915_edp_psr is still a real > thing although > they are on different places ;) > > We could probably remove "_edp" from the others, because psr is an > edp only after all.... > > Anyways let's not block the progress here ;) > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thank you, I pushed this to -queued. > > > > > > > + } > > > > return 0; > > }
Quoting Dhinakaran Pandiyan (2018-07-05 01:31:21) > This allows to read i915_edp_psr_status from tests without triggering > any AUX communication. Take this opportunity to move this under the > eDP-1 connector directory as the status we print is of the sink. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++---------------- > 1 file changed, 39 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index f6142d78ede4..5069d5dedafe 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2592,6 +2592,41 @@ static const struct file_operations i915_guc_log_relay_fops = { > .release = i915_guc_log_relay_release, > }; > > +static int i915_psr_sink_status_show(struct seq_file *m, void *data) > +{ > + u8 val; > + static const char * const sink_status[] = { > + "inactive", > + "transition to active, capture and display", > + "active, display from RFB", > + "active, capture and display on sink device timings", > + "transition to inactive, capture and display, timing re-sync", > + "reserved", > + "reserved", > + "sink internal error", > + }; > + struct drm_connector *connector = m->private; > + struct intel_dp *intel_dp = > + enc_to_intel_dp(&intel_attached_encoder(connector)->base); > + > + if (connector->status != connector_status_connected) > + return -ENODEV; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { > + const char *str = "unknown"; > + > + val &= DP_PSR_SINK_STATE_MASK; > + if (val < ARRAY_SIZE(sink_status)) > + str = sink_status[val]; > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > + } else { > + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); Why is this common occurrence (any DP that doesn't support PSR) an error? Why report it via a backchannel when you are already directly communicating with the user?!!! Please fix this mess. -Chris
On Thu, Jul 19, 2018 at 12:18:39AM -0700, Dhinakaran Pandiyan wrote: > On Thu, 2018-07-19 at 07:18 +0100, Chris Wilson wrote: > > Quoting Dhinakaran Pandiyan (2018-07-05 01:31:21) > > > > > > This allows to read i915_edp_psr_status from tests without > > > triggering > > > any AUX communication. Take this opportunity to move this under the > > > eDP-1 connector directory as the status we print is of the sink. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++-- > > > -------------- > > > 1 file changed, 39 insertions(+), 30 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index f6142d78ede4..5069d5dedafe 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2592,6 +2592,41 @@ static const struct file_operations > > > i915_guc_log_relay_fops = { > > > .release = i915_guc_log_relay_release, > > > }; > > > > > > +static int i915_psr_sink_status_show(struct seq_file *m, void > > > *data) > > > +{ > > > + u8 val; > > > + static const char * const sink_status[] = { > > > + "inactive", > > > + "transition to active, capture and display", > > > + "active, display from RFB", > > > + "active, capture and display on sink device > > > timings", > > > + "transition to inactive, capture and display, > > > timing re-sync", > > > + "reserved", > > > + "reserved", > > > + "sink internal error", > > > + }; > > > + struct drm_connector *connector = m->private; > > > + struct intel_dp *intel_dp = > > > + enc_to_intel_dp(&intel_attached_encoder(connector)- > > > >base); > > > + > > > + if (connector->status != connector_status_connected) > > > + return -ENODEV; > > > + > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) > > > == 1) { > > > + const char *str = "unknown"; > > > + > > > + val &= DP_PSR_SINK_STATE_MASK; > > > + if (val < ARRAY_SIZE(sink_status)) > > > + str = sink_status[val]; > > > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > > > str); > > > + } else { > > > + DRM_ERROR("dpcd read (at %u) failed\n", > > > DP_PSR_STATUS); > > Why is this common occurrence (any DP that doesn't support PSR) an > > error? > We create this file only for eDPs and I did not think dpcd_read would > return an error if the panel did not support PSR. Do you happen to have > any logs? > > > Why report it via a backchannel when you are already directly > > communicating with the user?!!! > > > You are right, i915_dpcd_show() also has the same issue. How about > returning the error to the user? > > @@ -2595,6 +2595,7 @@ static const struct file_operations > i915_guc_log_relay_fops = { > static int i915_psr_sink_status_show(struct seq_file *m, void *data) > { > u8 val; > + int ret; > static const char * const sink_status[] = { > "inactive", > "transition to active, capture and display", > @@ -2605,6 +2606,7 @@ static int i915_psr_sink_status_show(struct > seq_file *m, void *data) > "reserved", > "sink internal error", > }; > + const char *str = "unknown"; > struct drm_connector *connector = m->private; > struct intel_dp *intel_dp = > enc_to_intel_dp(&intel_attached_encoder(connector)- > >base); > @@ -2612,17 +2614,16 @@ static int i915_psr_sink_status_show(struct > seq_file *m, void *data) > if (connector->status != connector_status_connected) > return -ENODEV; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == > 1) { > - const char *str = "unknown"; > + ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val); > > + if (ret < 0) { > + return ret; > + } else if (ret == 1) { > val &= DP_PSR_SINK_STATE_MASK; > if (val < ARRAY_SIZE(sink_status)) > str = sink_status[val]; > - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > str); > - } else { > - DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); > } > - > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); > return 0; > } ops, just saw this after sending the patch... I think this should work as well. However I believe it is better not even read the dpcd if psr is not supported. > > > Please fix this mess. > > -Chris
On Thu, 2018-07-19 at 07:18 +0100, Chris Wilson wrote: > Quoting Dhinakaran Pandiyan (2018-07-05 01:31:21) > > > > This allows to read i915_edp_psr_status from tests without > > triggering > > any AUX communication. Take this opportunity to move this under the > > eDP-1 connector directory as the status we print is of the sink. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++-- > > -------------- > > 1 file changed, 39 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index f6142d78ede4..5069d5dedafe 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2592,6 +2592,41 @@ static const struct file_operations > > i915_guc_log_relay_fops = { > > .release = i915_guc_log_relay_release, > > }; > > > > +static int i915_psr_sink_status_show(struct seq_file *m, void > > *data) > > +{ > > + u8 val; > > + static const char * const sink_status[] = { > > + "inactive", > > + "transition to active, capture and display", > > + "active, display from RFB", > > + "active, capture and display on sink device > > timings", > > + "transition to inactive, capture and display, > > timing re-sync", > > + "reserved", > > + "reserved", > > + "sink internal error", > > + }; > > + struct drm_connector *connector = m->private; > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(&intel_attached_encoder(connector)- > > >base); > > + > > + if (connector->status != connector_status_connected) > > + return -ENODEV; > > + > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) > > == 1) { > > + const char *str = "unknown"; > > + > > + val &= DP_PSR_SINK_STATE_MASK; > > + if (val < ARRAY_SIZE(sink_status)) > > + str = sink_status[val]; > > + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, > > str); > > + } else { > > + DRM_ERROR("dpcd read (at %u) failed\n", > > DP_PSR_STATUS); > Why is this common occurrence (any DP that doesn't support PSR) an > error? We create this file only for eDPs and I did not think dpcd_read would return an error if the panel did not support PSR. Do you happen to have any logs? > Why report it via a backchannel when you are already directly > communicating with the user?!!! > You are right, i915_dpcd_show() also has the same issue. How about returning the error to the user? @@ -2595,6 +2595,7 @@ static const struct file_operations i915_guc_log_relay_fops = { static int i915_psr_sink_status_show(struct seq_file *m, void *data) { u8 val; + int ret; static const char * const sink_status[] = { "inactive", "transition to active, capture and display", @@ -2605,6 +2606,7 @@ static int i915_psr_sink_status_show(struct seq_file *m, void *data) "reserved", "sink internal error", }; + const char *str = "unknown"; struct drm_connector *connector = m->private; struct intel_dp *intel_dp = enc_to_intel_dp(&intel_attached_encoder(connector)- >base); @@ -2612,17 +2614,16 @@ static int i915_psr_sink_status_show(struct seq_file *m, void *data) if (connector->status != connector_status_connected) return -ENODEV; - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { - const char *str = "unknown"; + ret = drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val); + if (ret < 0) { + return ret; + } else if (ret == 1) { val &= DP_PSR_SINK_STATE_MASK; if (val < ARRAY_SIZE(sink_status)) str = sink_status[val]; - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); - } else { - DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); } - + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); return 0; } > Please fix this mess. > -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index f6142d78ede4..5069d5dedafe 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2592,6 +2592,41 @@ static const struct file_operations i915_guc_log_relay_fops = { .release = i915_guc_log_relay_release, }; +static int i915_psr_sink_status_show(struct seq_file *m, void *data) +{ + u8 val; + static const char * const sink_status[] = { + "inactive", + "transition to active, capture and display", + "active, display from RFB", + "active, capture and display on sink device timings", + "transition to inactive, capture and display, timing re-sync", + "reserved", + "reserved", + "sink internal error", + }; + struct drm_connector *connector = m->private; + struct intel_dp *intel_dp = + enc_to_intel_dp(&intel_attached_encoder(connector)->base); + + if (connector->status != connector_status_connected) + return -ENODEV; + + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) == 1) { + const char *str = "unknown"; + + val &= DP_PSR_SINK_STATE_MASK; + if (val < ARRAY_SIZE(sink_status)) + str = sink_status[val]; + seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, str); + } else { + DRM_ERROR("dpcd read (at %u) failed\n", DP_PSR_STATUS); + } + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); + static void psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) { @@ -2643,26 +2678,6 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown"); } -static const char *psr_sink_status(u8 val) -{ - static const char * const sink_status[] = { - "inactive", - "transition to active, capture and display", - "active, display from RFB", - "active, capture and display on sink device timings", - "transition to inactive, capture and display, timing re-sync", - "reserved", - "reserved", - "sink internal error" - }; - - val &= DP_PSR_SINK_STATE_MASK; - if (val < ARRAY_SIZE(sink_status)) - return sink_status[val]; - - return "unknown"; -} - static int i915_edp_psr_status(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -2706,15 +2721,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) } psr_source_status(dev_priv, m); - - if (dev_priv->psr.enabled) { - struct drm_dp_aux *aux = &dev_priv->psr.enabled->aux; - u8 val; - - if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1) - seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val, - psr_sink_status(val)); - } mutex_unlock(&dev_priv->psr.lock); if (READ_ONCE(dev_priv->psr.debug)) { @@ -4971,9 +4977,12 @@ int i915_debugfs_connector_add(struct drm_connector *connector) debugfs_create_file("i915_dpcd", S_IRUGO, root, connector, &i915_dpcd_fops); - if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { debugfs_create_file("i915_panel_timings", S_IRUGO, root, connector, &i915_panel_fops); + debugfs_create_file("i915_psr_sink_status", S_IRUGO, root, + connector, &i915_psr_sink_status_fops); + } return 0; }
This allows to read i915_edp_psr_status from tests without triggering any AUX communication. Take this opportunity to move this under the eDP-1 connector directory as the status we print is of the sink. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 69 +++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 30 deletions(-)