On Tue, Nov 05, 2019 at 05:45:01PM -0800, José Roberto de Souza wrote:
> eDP spec states that when sink enconters a problem that prevents it
> to keep PSR running it should set PSR status to internal error and
> set the reason why it happen to PSR_ERROR_STATUS but it is not how it
> was implemented.
> But also I don't want to change this behavior, who knows if there is
> a panel out there that only set the PSR_ERROR_STATUS.
>
> So here refactoring the code a bit to make more easy to read what was
> state above as more checks will be added to this function.
>
> Cc: Gwan-gyeong Mun <gwan-gyeong....@intel.com>
> Signed-off-by: José Roberto de Souza <jose.so...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++----------
> 1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 0d84ea28bc6f..f38da1b9b323 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1386,11 +1386,30 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> mutex_init(&dev_priv->psr.lock);
> }
>
> +static bool psr_get_status_and_error_status(struct intel_dp *intel_dp,
> + u8 *status, u8 *error_status)
This should probably return an integer rather than a bool to match the
kernel coding style guidelines:
"""
If the name of a function is an action or an imperative command,
the function should return an error-code integer. If the name
is a predicate, the function should return a "succeeded"
boolean.
"""
Matt
> +{
> + struct drm_dp_aux *aux = &intel_dp->aux;
> + int r;
> +
> + r = drm_dp_dpcd_readb(aux, DP_PSR_STATUS, status);
> + if (r != 1)
> + return false;
> +
> + r = drm_dp_dpcd_readb(aux, DP_PSR_ERROR_STATUS, error_status);
> + if (r != 1)
> + return false;
> +
> + *status = *status & DP_PSR_SINK_STATE_MASK;
> +
> + return true;
> +}
> +
> void intel_psr_short_pulse(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> struct i915_psr *psr = &dev_priv->psr;
> - u8 val;
> + u8 status, error_status;
> const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
> DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> DP_PSR_LINK_CRC_ERROR;
> @@ -1403,38 +1422,30 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> if (!psr->enabled || psr->dp != intel_dp)
> goto exit;
>
> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
> - DRM_ERROR("PSR_STATUS dpcd read failed\n");
> + if (!psr_get_status_and_error_status(intel_dp, &status, &error_status))
> {
> + DRM_ERROR("Error reading PSR status or error status\n");
> goto exit;
> }
>
> - if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) {
> - DRM_DEBUG_KMS("PSR sink internal error, disabling PSR\n");
> + if (status == DP_PSR_SINK_INTERNAL_ERROR || (error_status & errors)) {
> intel_psr_disable_locked(intel_dp);
> psr->sink_not_reliable = true;
> }
>
> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
> - DRM_ERROR("PSR_ERROR_STATUS dpcd read failed\n");
> - goto exit;
> - }
> -
> - if (val & DP_PSR_RFB_STORAGE_ERROR)
> + if (status == DP_PSR_SINK_INTERNAL_ERROR && !error_status)
> + DRM_DEBUG_KMS("PSR sink internal error, disabling PSR\n");
> + if (error_status & DP_PSR_RFB_STORAGE_ERROR)
> DRM_DEBUG_KMS("PSR RFB storage error, disabling PSR\n");
> - if (val & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> + if (error_status & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> DRM_DEBUG_KMS("PSR VSC SDP uncorrectable error, disabling
> PSR\n");
> - if (val & DP_PSR_LINK_CRC_ERROR)
> + if (error_status & DP_PSR_LINK_CRC_ERROR)
> DRM_DEBUG_KMS("PSR Link CRC error, disabling PSR\n");
>
> - if (val & ~errors)
> + if (error_status & ~errors)
> DRM_ERROR("PSR_ERROR_STATUS unhandled errors %x\n",
> - val & ~errors);
> - if (val & errors) {
> - intel_psr_disable_locked(intel_dp);
> - psr->sink_not_reliable = true;
> - }
> + error_status & ~errors);
> /* clear status register */
> - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, error_status);
> exit:
> mutex_unlock(&psr->lock);
> }
> --
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
