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

[Ville Syrjälä]

On Thu, Jun 20, 2019 at 11:59:37AM -0400, Ilia Mirkin wrote:
> 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.

I suppose we could do 
picture_aspet_ratio ? hdmi_picture_aspect_get_name(picture_aspet_ratio) : ""

> 
> >
> >  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.

I hate those thing. I can't even remember which is one the type
(absolutely useless) and which on is the flags. And I always end up
going through the headers to figure out which bit is what sync polarity.
So I'd rather like to decode those too, just been too lazy to do it.

> (Why was
> picture_aspect_ratio not stuffed into ->flags in the first place?)

It's also in there. I think the reason for having this odd duplication
was that we didn't have the flags initially, and just had the prop.
I've not looked how hard it would be to get rid of that.


> 
> >
> >  #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
> >

-- 
Ville Syrjälä
Intel