[Intel-gfx] [PATCH] drm: Dump mode picture aspect ratio

[Ilia Mirkin]

On Thu, Jun 20, 2019 at 11:46 AM Ville Syrjala
<ville.syrjala at linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Currently the logs show nothing about the mode's aspect ratio.
> Include that information in the normal mode dump.
>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/video/hdmi.c    | 3 ++-
>  include/drm/drm_modes.h | 4 ++--
>  include/linux/hdmi.h    | 3 +++
>  3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index b939bc28d886..bc593fe1c268 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1057,7 +1057,7 @@ static const char *hdmi_colorimetry_get_name(enum hdmi_colorimetry colorimetry)
>         return "Invalid";
>  }
>
> -static const char *
> +const char *
>  hdmi_picture_aspect_get_name(enum hdmi_picture_aspect picture_aspect)
>  {
>         switch (picture_aspect) {
> @@ -1076,6 +1076,7 @@ hdmi_picture_aspect_get_name(enum hdmi_picture_aspect picture_aspect)
>         }
>         return "Invalid";
>  }
> +EXPORT_SYMBOL(hdmi_picture_aspect_get_name);

So this will return "No Data" most of the time (since the DRM_CAP
won't be enabled)? This will look awkward, esp since the person seeing
this print will have no idea what "No Data" is referring to.

>
>  static const char *
>  hdmi_active_aspect_get_name(enum hdmi_active_aspect active_aspect)
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 083f16747369..2b1809c74fbe 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -431,7 +431,7 @@ struct drm_display_mode {
>  /**
>   * DRM_MODE_FMT - printf string for &struct drm_display_mode
>   */
> -#define DRM_MODE_FMT    "\"%s\": %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x"
> +#define DRM_MODE_FMT    "\"%s\": %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x %s"
>
>  /**
>   * DRM_MODE_ARG - printf arguments for &struct drm_display_mode
> @@ -441,7 +441,7 @@ struct drm_display_mode {
>         (m)->name, (m)->vrefresh, (m)->clock, \
>         (m)->hdisplay, (m)->hsync_start, (m)->hsync_end, (m)->htotal, \
>         (m)->vdisplay, (m)->vsync_start, (m)->vsync_end, (m)->vtotal, \
> -       (m)->type, (m)->flags
> +       (m)->type, (m)->flags, hdmi_picture_aspect_get_name((m)->picture_aspect_ratio)

Flags are printed as a literal integer value. Given that AR stuff is
fairly esoteric, I might just print an integer here too. (Why was
picture_aspect_ratio not stuffed into ->flags in the first place?)

>
>  #define obj_to_mode(x) container_of(x, struct drm_display_mode, base)
>
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 9918a6c910c5..de7cbe385dba 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -434,4 +434,7 @@ int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
>  void hdmi_infoframe_log(const char *level, struct device *dev,
>                         const union hdmi_infoframe *frame);
>
> +const char *
> +hdmi_picture_aspect_get_name(enum hdmi_picture_aspect picture_aspect);
> +
>  #endif /* _DRM_HDMI_H */
> --
> 2.21.0
>