diff mbox series

drm: Dump mode picture aspect ratio

Message ID 20190620154608.16239-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm: Dump mode picture aspect ratio | expand

Commit Message

Ville Syrjälä June 20, 2019, 3:46 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@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@alum.mit.edu>
Signed-off-by: Ville Syrjälä <ville.syrjala@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(-)

Comments

Ilia Mirkin June 20, 2019, 3:59 p.m. UTC | #1 
On Thu, Jun 20, 2019 at 11:46 AM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@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@alum.mit.edu>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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
>
Ville Syrjälä June 20, 2019, 4:11 p.m. UTC | #2 
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@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@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@alum.mit.edu>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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
> >
diff mbox series

Patch

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);
 
 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)
 
 #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 */