| Message ID | 20180322214848.28022-10-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza wrote: please add some justification on why this is useful.... > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 54 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 16f9977995df..0a0642c61cd0 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val) > return "unknown"; > } > > +static const char *psr_sink_self_refresh_status(u8 val) > +{ > + static const char * const sink_status[] = { > + "inactive", > + "transitioning to active", > + "active", > + "active receiving selective update", > + "transitioning to inactive", > + "reserved", > + "reserved", > + "sink internal error" > + }; > + > + val &= DP_PSR_SINK_STATE_MASK; > + if (val < ARRAY_SIZE(sink_status)) > + return sink_status[val]; > + > + return "unknown"; > +} > + > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val) > +{ > + if (val & DP_PSR_STATE_BIT) > + seq_puts(m, "\tPSR active\n"); I'm a bit confused here why we are printing status here again if we are adding the sink_status char * array with some status definition up there. Any simplification possible? > + if (val & DP_UPDATE_RFB_BIT) > + seq_puts(m, "\tUpdate RFB\n"); > + if (val & DP_CRC_VALID_BIT) > + seq_puts(m, "\tCRC valid\n"); > + if (val & DP_SU_VALID) > + seq_puts(m, "\tSU valid\n"); > + if (val & DP_FIRST_SCAN_LINE_SU_REGION) > + seq_puts(m, "\tFirst scan line of the SU region\n"); > + if (val & DP_LAST_SCAN_LINE_SU_REGION) > + seq_puts(m, "\tLast scan line of the SU region\n"); > + if (val & DP_Y_COORDINATE_VALID) > + seq_puts(m, "\tY-Coordinate valid\n"); > +} > + > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n", > psr2, psr2_live_status(psr2)); > } > + > + 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 self refresh status: 0x%x [%s]\n", > + val, psr_sink_self_refresh_status(val)); > + > + if (drm_dp_dpcd_readb(aux, DP_LAST_RECEIVED_PSR_SDP, &val) > + == 1) { > + seq_printf(m, "Sink last received PSR SDP: 0x%x\n", > + val); > + psr_sink_last_received_psr_sdp_sprintf(m, val); > + } > + } > mutex_unlock(&dev_priv->psr.lock); > > intel_runtime_pm_put(dev_priv); > -- > 2.16.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote: > On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza > wrote: > > please add some justification on why this is useful.... Okay something like this should be fine? IGT tests could be improved with sink status, knowing for sure that hardware have activate PSR before get the CRC. This is also userful to check if hardware is really doing PSR2 selective update with the y-coordinate. > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 54 > > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 16f9977995df..0a0642c61cd0 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val) > > return "unknown"; > > } > > > > +static const char *psr_sink_self_refresh_status(u8 val) > > +{ > > + static const char * const sink_status[] = { > > + "inactive", > > + "transitioning to active", > > + "active", > > + "active receiving selective update", > > + "transitioning to inactive", > > + "reserved", > > + "reserved", > > + "sink internal error" > > + }; > > + > > + val &= DP_PSR_SINK_STATE_MASK; > > + if (val < ARRAY_SIZE(sink_status)) > > + return sink_status[val]; > > + > > + return "unknown"; > > +} > > + > > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file > > *m, u32 val) > > +{ > > + if (val & DP_PSR_STATE_BIT) > > + seq_puts(m, "\tPSR active\n"); > > I'm a bit confused here why we are printing status here again if we > are adding the > sink_status char * array with some status definition up there. > > Any simplification possible? Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the status of the sink to change, so I this 2 can be removed. > > > + if (val & DP_UPDATE_RFB_BIT) > > + seq_puts(m, "\tUpdate RFB\n"); > > + if (val & DP_CRC_VALID_BIT) > > + seq_puts(m, "\tCRC valid\n"); > > + if (val & DP_SU_VALID) > > + seq_puts(m, "\tSU valid\n"); > > + if (val & DP_FIRST_SCAN_LINE_SU_REGION) > > + seq_puts(m, "\tFirst scan line of the SU > > region\n"); > > + if (val & DP_LAST_SCAN_LINE_SU_REGION) > > + seq_puts(m, "\tLast scan line of the SU > > region\n"); > > + if (val & DP_Y_COORDINATE_VALID) > > + seq_puts(m, "\tY-Coordinate valid\n"); > > +} > > + > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > { > > struct drm_i915_private *dev_priv = node_to_i915(m- > > >private); > > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct > > seq_file *m, void *data) > > seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n", > > psr2, psr2_live_status(psr2)); > > } > > + > > + 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 self refresh status: > > 0x%x [%s]\n", > > + val, > > psr_sink_self_refresh_status(val)); > > + > > + if (drm_dp_dpcd_readb(aux, > > DP_LAST_RECEIVED_PSR_SDP, &val) > > + == 1) { > > + seq_printf(m, "Sink last received PSR SDP: > > 0x%x\n", > > + val); > > + psr_sink_last_received_psr_sdp_sprintf(m, > > val); > > + } > > + } > > mutex_unlock(&dev_priv->psr.lock); > > > > intel_runtime_pm_put(dev_priv); > > -- > > 2.16.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Mar 22, 2018 at 05:06:13PM -0700, Souza, Jose wrote: > On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote: > > On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza > > wrote: > > > > please add some justification on why this is useful.... > > Okay something like this should be fine? > > IGT tests could be improved with sink status, knowing for sure that > hardware have activate PSR before get the CRC. > This is also userful to check if hardware is really doing PSR2 > selective update with the y-coordinate. I think so. Although for the tests I'd like you sync with DK on this versus the Interrupt approach. > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 54 > > > +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 54 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 16f9977995df..0a0642c61cd0 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val) > > > return "unknown"; > > > } > > > > > > +static const char *psr_sink_self_refresh_status(u8 val) > > > +{ > > > + static const char * const sink_status[] = { > > > + "inactive", > > > + "transitioning to active", > > > + "active", > > > + "active receiving selective update", > > > + "transitioning to inactive", > > > + "reserved", > > > + "reserved", > > > + "sink internal error" > > > + }; > > > + > > > + val &= DP_PSR_SINK_STATE_MASK; > > > + if (val < ARRAY_SIZE(sink_status)) > > > + return sink_status[val]; > > > + > > > + return "unknown"; > > > +} > > > + > > > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file > > > *m, u32 val) > > > +{ > > > + if (val & DP_PSR_STATE_BIT) > > > + seq_puts(m, "\tPSR active\n"); > > > > I'm a bit confused here why we are printing status here again if we > > are adding the > > sink_status char * array with some status definition up there. > > > > Any simplification possible? > > Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the > status of the sink to change, so I this 2 can be removed. > > > > > > + if (val & DP_UPDATE_RFB_BIT) > > > + seq_puts(m, "\tUpdate RFB\n"); > > > + if (val & DP_CRC_VALID_BIT) > > > + seq_puts(m, "\tCRC valid\n"); > > > + if (val & DP_SU_VALID) > > > + seq_puts(m, "\tSU valid\n"); > > > + if (val & DP_FIRST_SCAN_LINE_SU_REGION) > > > + seq_puts(m, "\tFirst scan line of the SU > > > region\n"); > > > + if (val & DP_LAST_SCAN_LINE_SU_REGION) > > > + seq_puts(m, "\tLast scan line of the SU > > > region\n"); > > > + if (val & DP_Y_COORDINATE_VALID) > > > + seq_puts(m, "\tY-Coordinate valid\n"); > > > +} > > > + > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > > { > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > >private); > > > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct > > > seq_file *m, void *data) > > > seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n", > > > psr2, psr2_live_status(psr2)); > > > } > > > + > > > + 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 self refresh status: > > > 0x%x [%s]\n", > > > + val, > > > psr_sink_self_refresh_status(val)); > > > + > > > + if (drm_dp_dpcd_readb(aux, > > > DP_LAST_RECEIVED_PSR_SDP, &val) > > > + == 1) { > > > + seq_printf(m, "Sink last received PSR SDP: > > > 0x%x\n", > > > + val); > > > + psr_sink_last_received_psr_sdp_sprintf(m, > > > val); > > > + } > > > + } > > > mutex_unlock(&dev_priv->psr.lock); > > > > > > intel_runtime_pm_put(dev_priv); > > > -- > > > 2.16.2 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2018-03-23 at 00:06 +0000, Souza, Jose wrote: > On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote: > > On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza > > wrote: > > > > please add some justification on why this is useful.... > The plan is to remove sink crc and use these bits :) > Okay something like this should be fine? > > IGT tests could be improved with sink status, knowing for sure that > hardware have activate PSR before get the CRC. > This is also userful to check if hardware is really doing PSR2 > selective update with the y-coordinate. > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 54 > > > +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 54 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 16f9977995df..0a0642c61cd0 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val) > > > return "unknown"; > > > } > > > > > > +static const char *psr_sink_self_refresh_status(u8 val) > > > +{ > > > + static const char * const sink_status[] = { > > > + "inactive", > > > + "transitioning to active", > > > + "active", > > > + "active receiving selective update", > > > + "transitioning to inactive", > > > + "reserved", > > > + "reserved", > > > + "sink internal error" > > > + }; > > > + > > > + val &= DP_PSR_SINK_STATE_MASK; > > > + if (val < ARRAY_SIZE(sink_status)) > > > + return sink_status[val]; > > > + > > > + return "unknown"; > > > +} > > > + > > > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file > > > *m, u32 val) > > > +{ > > > + if (val & DP_PSR_STATE_BIT) > > > + seq_puts(m, "\tPSR active\n"); > > > > I'm a bit confused here why we are printing status here again if we > > are adding the > > sink_status char * array with some status definition up there. > > > > Any simplification possible? These values are what the sink received in the SDP, the one from DP_PSR_STATUS is the current status of the sink. > > Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the > status of the sink to change, so I this 2 can be removed. This should be split as two patches. And more importantly I can't think of a use case for printing SDP status. The update might be too fast for making any sense of it. Printing DP_PSR_STATUS should be sufficient for writing tests. > > > > > > + if (val & DP_UPDATE_RFB_BIT) > > > + seq_puts(m, "\tUpdate RFB\n"); > > > + if (val & DP_CRC_VALID_BIT) > > > + seq_puts(m, "\tCRC valid\n"); > > > + if (val & DP_SU_VALID) > > > + seq_puts(m, "\tSU valid\n"); > > > + if (val & DP_FIRST_SCAN_LINE_SU_REGION) > > > + seq_puts(m, "\tFirst scan line of the SU > > > region\n"); > > > + if (val & DP_LAST_SCAN_LINE_SU_REGION) > > > + seq_puts(m, "\tLast scan line of the SU > > > region\n"); > > > + if (val & DP_Y_COORDINATE_VALID) > > > + seq_puts(m, "\tY-Coordinate valid\n"); > > > +} > > > + > > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > > { > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > >private); > > > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct > > > seq_file *m, void *data) > > > seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n", > > > psr2, psr2_live_status(psr2)); > > > } > > > + > > > + 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 self refresh status: > > > 0x%x [%s]\n", > > > + val, > > > psr_sink_self_refresh_status(val)); > > > + > > > + if (drm_dp_dpcd_readb(aux, > > > DP_LAST_RECEIVED_PSR_SDP, &val) > > > + == 1) { > > > + seq_printf(m, "Sink last received PSR SDP: > > > 0x%x\n", > > > + val); > > > + psr_sink_last_received_psr_sdp_sprintf(m, > > > val); > > > + } > > > + } > > > mutex_unlock(&dev_priv->psr.lock); > > > > > > intel_runtime_pm_put(dev_priv); > > > -- > > > 2.16.2 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 16f9977995df..0a0642c61cd0 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val) return "unknown"; } +static const char *psr_sink_self_refresh_status(u8 val) +{ + static const char * const sink_status[] = { + "inactive", + "transitioning to active", + "active", + "active receiving selective update", + "transitioning to inactive", + "reserved", + "reserved", + "sink internal error" + }; + + val &= DP_PSR_SINK_STATE_MASK; + if (val < ARRAY_SIZE(sink_status)) + return sink_status[val]; + + return "unknown"; +} + +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file *m, u32 val) +{ + if (val & DP_PSR_STATE_BIT) + seq_puts(m, "\tPSR active\n"); + if (val & DP_UPDATE_RFB_BIT) + seq_puts(m, "\tUpdate RFB\n"); + if (val & DP_CRC_VALID_BIT) + seq_puts(m, "\tCRC valid\n"); + if (val & DP_SU_VALID) + seq_puts(m, "\tSU valid\n"); + if (val & DP_FIRST_SCAN_LINE_SU_REGION) + seq_puts(m, "\tFirst scan line of the SU region\n"); + if (val & DP_LAST_SCAN_LINE_SU_REGION) + seq_puts(m, "\tLast scan line of the SU region\n"); + if (val & DP_Y_COORDINATE_VALID) + seq_puts(m, "\tY-Coordinate valid\n"); +} + static int i915_edp_psr_status(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n", psr2, psr2_live_status(psr2)); } + + 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 self refresh status: 0x%x [%s]\n", + val, psr_sink_self_refresh_status(val)); + + if (drm_dp_dpcd_readb(aux, DP_LAST_RECEIVED_PSR_SDP, &val) + == 1) { + seq_printf(m, "Sink last received PSR SDP: 0x%x\n", + val); + psr_sink_last_received_psr_sdp_sprintf(m, val); + } + } mutex_unlock(&dev_priv->psr.lock); intel_runtime_pm_put(dev_priv);
Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 54 +++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)