On Thu, 23 Nov 2023, Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx> wrote:
> Add structure representing Adaptive Sync Secondary Data
> Packet (AS SDP). Also, add Adaptive Sync SDP logging in
> drm_dp_helper.c to facilitate debugging.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 15 +++++++++++++
> include/drm/display/drm_dp.h | 1 +
> include/drm/display/drm_dp_helper.h | 30 +++++++++++++++++++++++++
> 3 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index d72b6f9a352c..a205e14a6681 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2917,6 +2917,21 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> }
> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>
> +void drm_dp_as_sdp_log(const char *level, struct device *dev,
> + const struct drm_dp_as_sdp *async)
> +{
> +#define DP_SDP_LOG(fmt, ...) dev_printk(level, dev, fmt, ##__VA_ARGS__)
> + DP_SDP_LOG("DP SDP: %s, revision %u, length %u\n", "VSC",
> + async->revision, async->length);
> + DP_SDP_LOG(" vmin: %d vmax: %d\n", async->vmin, async->vmax);
> + DP_SDP_LOG(" target_rr: %s\n", async->target_rr);
> + DP_SDP_LOG(" duration_incr_ms: %u\n", async->duration_incr_ms);
> + DP_SDP_LOG(" duration_decr_ms: %u\n", async->duration_decr_ms);
> + DP_SDP_LOG(" operation_mode: %u\n", async->operation_mode);
> +#undef DP_SDP_LOG
> +}
> +EXPORT_SYMBOL(drm_dp_as_sdp_log);
This inspired me to resurrect a drm logging series I've had [1], in
particular patch [2]. Please let's do this properly.
Also, throughout the series, please don't use "async" for "adaptive
sync". It's bound to confuse people. Async means asynchronous, period.
[1] https://patchwork.freedesktop.org/series/126873/
[2] https://patchwork.freedesktop.org/patch/msgid/95f1e3981fa3c5304f3c74e82330f12983d35735.1700829750.git.jani.nikula@xxxxxxxxx
> +
> /**
> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> * @dpcd: DisplayPort configuration data
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index 83d2039c018b..0575ab8ea088 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -1578,6 +1578,7 @@ enum drm_dp_phy {
> #define DP_SDP_PPS 0x10 /* DP 1.4 */
> #define DP_SDP_VSC_EXT_VESA 0x20 /* DP 1.4 */
> #define DP_SDP_VSC_EXT_CEA 0x21 /* DP 1.4 */
> +#define DP_SDP_ADAPTIVE_SYNC 0x22 /* DP 1.4 */
This is completely unrelated to everything else in the patch.
> /* 0x80+ CEA-861 infoframe types */
>
> #define DP_SDP_AUDIO_INFOFRAME_HB2 0x1b
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 863b2e7add29..63b6bef3f21d 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -98,6 +98,36 @@ struct drm_dp_vsc_sdp {
> enum dp_content_type content_type;
> };
>
> +/**
> + * struct drm_dp_as_sdp - drm DP Adaptive Sync SDP
> + *
> + * This structure represents a DP AS SDP of drm
> + * It is based on DP 2.1 spec [Table 2-126: Adaptive-Sync SDP Header Bytes] and
> + * [Table 2-127: Adaptive-Sync SDP Payload for DB0 through DB8]
> + *
> + * @sdp_type: secondary-data packet type
> + * @length: number of valid data bytes
> + * @vmin: minimum vtotal
> + * @vmax: maximum vtotal
> + * @duration_incr_ms: Successive frame duration increase
> + * @duration_decr_ms: Successive frame duration decrease
> + * @operation_mode: Adaptive Sync Operation Mode
> + */
> +
> +struct drm_dp_as_sdp {
> + unsigned char sdp_type;
> + unsigned char revision;
> + unsigned char length;
Why unsigned char here...
> + u16 vmin, vmax;
> + u16 target_rr;
> + u8 duration_incr_ms;
> + u8 duration_decr_ms;
> + u8 operation_mode;
...but u8 here?
> +};
> +
> +void drm_dp_as_sdp_log(const char *level, struct device *dev,
> + const struct drm_dp_as_sdp *async);
> +
> void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> const struct drm_dp_vsc_sdp *vsc);
--
Jani Nikula, Intel
