Re: [PATCH v2 5/6] drm/dp: Add helper to dump an LTTPR PHY descriptor

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

LGTM

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 7/9/2024 12:30 AM, Imre Deak wrote:
> Add a helper to dump the DPCD descriptor for an LTTPR PHY. This is based
> on [1] and [2] moving the helper to DRM core as suggested by Ville.
>
> [1] https://lore.kernel.org/all/20240703155937.1674856-5-imre.deak@intel.com
> [2] https://lore.kernel.org/all/20240703155937.1674856-6-imre.deak@intel.com
>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/display/drm_dp_helper.c | 66 +++++++++++++++++++++----
>   include/drm/display/drm_dp.h            |  4 ++
>   include/drm/display/drm_dp_helper.h     |  2 +
>   3 files changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index d4c34f3641400..6ee51003de3ce 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2328,6 +2328,31 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch)
>   #undef DEVICE_ID_ANY
>   #undef DEVICE_ID
>   
> +static int drm_dp_read_ident(struct drm_dp_aux *aux, unsigned int offset,
> +			     struct drm_dp_dpcd_ident *ident)
> +{
> +	int ret;
> +
> +	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static void drm_dp_dump_desc(struct drm_dp_aux *aux,
> +			     const char *device_name, const struct drm_dp_desc *desc)
> +{
> +	const struct drm_dp_dpcd_ident *ident = &desc->ident;
> +
> +	drm_dbg_kms(aux->drm_dev,
> +		    "%s: %s: OUI %*phD dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
> +		    aux->name, device_name,
> +		    (int)sizeof(ident->oui), ident->oui,
> +		    (int)strnlen(ident->device_id, sizeof(ident->device_id)), ident->device_id,
> +		    ident->hw_rev >> 4, ident->hw_rev & 0xf,
> +		    ident->sw_major_rev, ident->sw_minor_rev,
> +		    desc->quirks);
> +}
> +
>   /**
>    * drm_dp_read_desc - read sink/branch descriptor from DPCD
>    * @aux: DisplayPort AUX channel
> @@ -2344,27 +2369,48 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
>   {
>   	struct drm_dp_dpcd_ident *ident = &desc->ident;
>   	unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI;
> -	int ret, dev_id_len;
> +	int ret;
>   
> -	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
> +	ret = drm_dp_read_ident(aux, offset, ident);
>   	if (ret < 0)
>   		return ret;
>   
>   	desc->quirks = drm_dp_get_quirks(ident, is_branch);
>   
> -	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
> -
> -	drm_dbg_kms(aux->drm_dev,
> -		    "%s: DP %s: OUI %*phD dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
> -		    aux->name, is_branch ? "branch" : "sink",
> -		    (int)sizeof(ident->oui), ident->oui, dev_id_len,
> -		    ident->device_id, ident->hw_rev >> 4, ident->hw_rev & 0xf,
> -		    ident->sw_major_rev, ident->sw_minor_rev, desc->quirks);
> +	drm_dp_dump_desc(aux, is_branch ? "DP branch" : "DP sink", desc);
>   
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_dp_read_desc);
>   
> +/**
> + * drm_dp_dump_lttpr_desc - read and dump the DPCD descriptor for an LTTPR PHY
> + * @aux: DisplayPort AUX channel
> + * @dp_phy: LTTPR PHY instance
> + *
> + * Read the DPCD LTTPR PHY descriptor for @dp_phy and print a debug message
> + * with its details to dmesg.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_dump_lttpr_desc(struct drm_dp_aux *aux, enum drm_dp_phy dp_phy)
> +{
> +	struct drm_dp_desc desc = {};
> +	int ret;
> +
> +	if (drm_WARN_ON(aux->drm_dev, dp_phy < DP_PHY_LTTPR1 || dp_phy > DP_MAX_LTTPR_COUNT))
> +		return -EINVAL;
> +
> +	ret = drm_dp_read_ident(aux, DP_OUI_PHY_REPEATER(dp_phy), &desc.ident);
> +	if (ret < 0)
> +		return ret;
> +
> +	drm_dp_dump_desc(aux, drm_dp_phy_name(dp_phy), &desc);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dump_lttpr_desc);
> +
>   /**
>    * drm_dp_dsc_sink_bpp_incr() - Get bits per pixel increment
>    * @dsc_dpcd: DSC capabilities from DPCD
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index 173548c6473a9..a6f8b098c56f1 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -1543,6 +1543,10 @@ enum drm_dp_phy {
>   #define DP_SYMBOL_ERROR_COUNT_LANE2_PHY_REPEATER1	    0xf0039 /* 1.3 */
>   #define DP_SYMBOL_ERROR_COUNT_LANE3_PHY_REPEATER1	    0xf003b /* 1.3 */
>   
> +#define DP_OUI_PHY_REPEATER1				    0xf003d /* 1.3 */
> +#define DP_OUI_PHY_REPEATER(dp_phy) \
> +	DP_LTTPR_REG(dp_phy, DP_OUI_PHY_REPEATER1)
> +
>   #define __DP_FEC1_BASE					    0xf0290 /* 1.4 */
>   #define __DP_FEC2_BASE					    0xf0298 /* 1.4 */
>   #define DP_FEC_BASE(dp_phy) \
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ea03e1dd26ba7..bbb1cdc4fc68d 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -657,6 +657,8 @@ struct drm_dp_desc {
>   int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
>   		     bool is_branch);
>   
> +int drm_dp_dump_lttpr_desc(struct drm_dp_aux *aux, enum drm_dp_phy dp_phy);
> +
>   /**
>    * enum drm_dp_quirk - Display Port sink/branch device specific quirks
>    *