Re: [Freedreno] [PATCH 1/2] drm: Pretty print mode flags
On Thu, Jun 20, 2019 at 10:25:42PM +0200, Sam Ravnborg wrote:
> Hi Ville.
>
> On Thu, Jun 20, 2019 at 09:50:48PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Decode the mode flags when printing the modeline so that I
> > no longer have to decode the hex number myself.
> You are extending the current way to print mode flags,
> but I would anyway like to point out a different approach.
>
> When I need to print a fourcc code it is as simple as:
>
> {
> struct drm_format_name_buf fbuf;
>
> printk("My format: %s\n", drm_get_format_name(format, );
> }
>
> This way to handle this feels more straightforward
> than the current approach used for modes.
>
> Maybe bikeshedding, as your mileage may vary.
You're suggesting to print the whole mode to a temp buffer, and then
print that with just %s? I thought about that, but it would waste
even more stack so didn't think it was all that great.
>
> A middle ground could be to introduce a struct for the buf so we know
> the callers do it right.
I'm not a fan of that struct approach because it forces you to use
a dedicated buffer for the thing. With just a raw char[] you can
print to anything, so it could be used more like strcat() as well.
Also I think it makes it more clear how much stack space you're
blowing away. And it looks a bit like snprintf() so it's more
clear what's happening on. Although in this case that last point
is kinda lost due to the macro thing.
>
> Most of the code would be the same, but all call sites would need to be
> updated.
> What do you think?
>
> Sam
>
>
> >
> > To do this neatly I made the caller provide a temporary
> > on stack buffer where we can produce the results. I choce 64
> > bytes as a reasonable size for this. The worst case I think
> > is > 100 bytes but that kind of mode would be nonsense anyway
> > so I figured correct decoding isn't as important in such
> > cases.
> >
> > Cc: Russell King
> > Cc: Neil Armstrong
> > Cc: Rob Clark
> > Cc: Tomi Valkeinen
> > Cc: Thierry Reding
> > Cc: Sam Ravnborg
> > Cc: Benjamin Gaignard
> > Cc: Vincent Abriou
> > Cc: linux-amlo...@lists.infradead.org
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > Cc: Ilia Mirkin
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/armada/armada_crtc.c | 3 +-
> > drivers/gpu/drm/drm_atomic.c | 3 +-
> > drivers/gpu/drm/drm_modes.c | 116 +-
> > drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
> > drivers/gpu/drm/meson/meson_dw_hdmi.c | 3 +-
> > drivers/gpu/drm/meson/meson_venc.c| 4 +-
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 3 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 +-
> > drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +-
> > drivers/gpu/drm/msm/edp/edp_bridge.c | 3 +-
> > drivers/gpu/drm/omapdrm/omap_connector.c | 5 +-
> > drivers/gpu/drm/omapdrm/omap_crtc.c | 3 +-
> > drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 3 +-
> > drivers/gpu/drm/sti/sti_crtc.c| 3 +-
> > include/drm/drm_modes.h | 14 ++-
> > 20 files changed, 165 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> > b/drivers/gpu/drm/armada/armada_crtc.c
> > index ba4a3fab7745..ce9335682bd2 100644
> > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > @@ -262,6 +262,7 @@ static void armada_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> > unsigned long flags;
> > unsigned i;
> > bool interlaced = !!(adj->flags & DRM_MODE_FLAG_INTERLACE);
> > + char buf[DRM_MODE_FLAGS_BUF_LEN];
> >
> > i = 0;
> > rm = adj->crtc_hsync_start - adj->crtc_hdisplay;
> > @@ -270,7 +271,7 @@ static void armada_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> > tm = adj->crtc_vtotal - adj->crtc_vsync_end;
> >
> > DRM_DEBUG_KMS("[CRTC:%d:%s] mode " DRM_MODE_FMT "\n",
> > - crtc->base.id, crtc->name, DRM_MODE_ARG(adj));
> > + crtc->base.id, crtc->name, DRM_MODE_ARG(adj, buf));
> > DRM_DEBUG_KMS("lm %d rm %d tm %d bm %d\n", lm, rm, tm, bm);
> >
> > /* Now compute the divider for real */
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 419381abbdd1..81caf91fbd72 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -380,6 +380,7 @@ static void drm_atomic_crtc_print_state(struct
> > drm_printer *p,
> > const struct drm_crtc_state *state)
> > {
> > struct drm_crtc
Re: [Freedreno] [PATCH 1/2] drm: Pretty print mode flags
Hi Ville.
On Thu, Jun 20, 2019 at 09:50:48PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Decode the mode flags when printing the modeline so that I
> no longer have to decode the hex number myself.
You are extending the current way to print mode flags,
but I would anyway like to point out a different approach.
When I need to print a fourcc code it is as simple as:
{
struct drm_format_name_buf fbuf;
printk("My format: %s\n", drm_get_format_name(format, );
}
This way to handle this feels more straightforward
than the current approach used for modes.
Maybe bikeshedding, as your mileage may vary.
A middle ground could be to introduce a struct for the buf so we know
the callers do it right.
Most of the code would be the same, but all call sites would need to be
updated.
What do you think?
Sam
>
> To do this neatly I made the caller provide a temporary
> on stack buffer where we can produce the results. I choce 64
> bytes as a reasonable size for this. The worst case I think
> is > 100 bytes but that kind of mode would be nonsense anyway
> so I figured correct decoding isn't as important in such
> cases.
>
> Cc: Russell King
> Cc: Neil Armstrong
> Cc: Rob Clark
> Cc: Tomi Valkeinen
> Cc: Thierry Reding
> Cc: Sam Ravnborg
> Cc: Benjamin Gaignard
> Cc: Vincent Abriou
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: Ilia Mirkin
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/armada/armada_crtc.c | 3 +-
> drivers/gpu/drm/drm_atomic.c | 3 +-
> drivers/gpu/drm/drm_modes.c | 116 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 3 +-
> drivers/gpu/drm/meson/meson_venc.c| 4 +-
> drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +-
> .../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c | 3 +-
> .../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c | 3 +-
> .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 3 +-
> .../gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 +-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 3 +-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 +-
> drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +-
> drivers/gpu/drm/msm/edp/edp_bridge.c | 3 +-
> drivers/gpu/drm/omapdrm/omap_connector.c | 5 +-
> drivers/gpu/drm/omapdrm/omap_crtc.c | 3 +-
> drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 3 +-
> drivers/gpu/drm/sti/sti_crtc.c| 3 +-
> include/drm/drm_modes.h | 14 ++-
> 20 files changed, 165 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> b/drivers/gpu/drm/armada/armada_crtc.c
> index ba4a3fab7745..ce9335682bd2 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -262,6 +262,7 @@ static void armada_drm_crtc_mode_set_nofb(struct drm_crtc
> *crtc)
> unsigned long flags;
> unsigned i;
> bool interlaced = !!(adj->flags & DRM_MODE_FLAG_INTERLACE);
> + char buf[DRM_MODE_FLAGS_BUF_LEN];
>
> i = 0;
> rm = adj->crtc_hsync_start - adj->crtc_hdisplay;
> @@ -270,7 +271,7 @@ static void armada_drm_crtc_mode_set_nofb(struct drm_crtc
> *crtc)
> tm = adj->crtc_vtotal - adj->crtc_vsync_end;
>
> DRM_DEBUG_KMS("[CRTC:%d:%s] mode " DRM_MODE_FMT "\n",
> - crtc->base.id, crtc->name, DRM_MODE_ARG(adj));
> + crtc->base.id, crtc->name, DRM_MODE_ARG(adj, buf));
> DRM_DEBUG_KMS("lm %d rm %d tm %d bm %d\n", lm, rm, tm, bm);
>
> /* Now compute the divider for real */
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 419381abbdd1..81caf91fbd72 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -380,6 +380,7 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> const struct drm_crtc_state *state)
> {
> struct drm_crtc *crtc = state->crtc;
> + char buf[DRM_MODE_FLAGS_BUF_LEN];
>
> drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name);
> drm_printf(p, "\tenable=%d\n", state->enable);
> @@ -393,7 +394,7 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> drm_printf(p, "\tplane_mask=%x\n", state->plane_mask);
> drm_printf(p, "\tconnector_mask=%x\n", state->connector_mask);
> drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask);
> - drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(>mode));
> + drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(>mode,
> buf));
>
> if (crtc->funcs->atomic_print_state)
> crtc->funcs->atomic_print_state(p, state);
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 57e6408288c8..3d15c600295a 100644
> ---
[Freedreno] [PATCH 1/2] drm: Pretty print mode flags
From: Ville Syrjälä
Decode the mode flags when printing the modeline so that I
no longer have to decode the hex number myself.
To do this neatly I made the caller provide a temporary
on stack buffer where we can produce the results. I choce 64
bytes as a reasonable size for this. The worst case I think
is > 100 bytes but that kind of mode would be nonsense anyway
so I figured correct decoding isn't as important in such
cases.
Cc: Russell King
Cc: Neil Armstrong
Cc: Rob Clark
Cc: Tomi Valkeinen
Cc: Thierry Reding
Cc: Sam Ravnborg
Cc: Benjamin Gaignard
Cc: Vincent Abriou
Cc: linux-amlo...@lists.infradead.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: Ilia Mirkin
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/armada/armada_crtc.c | 3 +-
drivers/gpu/drm/drm_atomic.c | 3 +-
drivers/gpu/drm/drm_modes.c | 116 +-
drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
drivers/gpu/drm/meson/meson_dw_hdmi.c | 3 +-
drivers/gpu/drm/meson/meson_venc.c| 4 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +-
.../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c | 3 +-
.../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c | 3 +-
.../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 3 +-
.../gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 3 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 +-
drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +-
drivers/gpu/drm/msm/edp/edp_bridge.c | 3 +-
drivers/gpu/drm/omapdrm/omap_connector.c | 5 +-
drivers/gpu/drm/omapdrm/omap_crtc.c | 3 +-
drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 3 +-
drivers/gpu/drm/sti/sti_crtc.c| 3 +-
include/drm/drm_modes.h | 14 ++-
20 files changed, 165 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_crtc.c
b/drivers/gpu/drm/armada/armada_crtc.c
index ba4a3fab7745..ce9335682bd2 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -262,6 +262,7 @@ static void armada_drm_crtc_mode_set_nofb(struct drm_crtc
*crtc)
unsigned long flags;
unsigned i;
bool interlaced = !!(adj->flags & DRM_MODE_FLAG_INTERLACE);
+ char buf[DRM_MODE_FLAGS_BUF_LEN];
i = 0;
rm = adj->crtc_hsync_start - adj->crtc_hdisplay;
@@ -270,7 +271,7 @@ static void armada_drm_crtc_mode_set_nofb(struct drm_crtc
*crtc)
tm = adj->crtc_vtotal - adj->crtc_vsync_end;
DRM_DEBUG_KMS("[CRTC:%d:%s] mode " DRM_MODE_FMT "\n",
- crtc->base.id, crtc->name, DRM_MODE_ARG(adj));
+ crtc->base.id, crtc->name, DRM_MODE_ARG(adj, buf));
DRM_DEBUG_KMS("lm %d rm %d tm %d bm %d\n", lm, rm, tm, bm);
/* Now compute the divider for real */
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 419381abbdd1..81caf91fbd72 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -380,6 +380,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer
*p,
const struct drm_crtc_state *state)
{
struct drm_crtc *crtc = state->crtc;
+ char buf[DRM_MODE_FLAGS_BUF_LEN];
drm_printf(p, "crtc[%u]: %s\n", crtc->base.id, crtc->name);
drm_printf(p, "\tenable=%d\n", state->enable);
@@ -393,7 +394,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer
*p,
drm_printf(p, "\tplane_mask=%x\n", state->plane_mask);
drm_printf(p, "\tconnector_mask=%x\n", state->connector_mask);
drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask);
- drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(>mode));
+ drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(>mode,
buf));
if (crtc->funcs->atomic_print_state)
crtc->funcs->atomic_print_state(p, state);
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 57e6408288c8..3d15c600295a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -45,6 +45,118 @@
#include "drm_crtc_internal.h"
+static char *snprint_cont(char *buf, int *len,
+ const char *str, bool last)
+{
+ int r;
+
+ r = snprintf(buf, *len, "%s%s", str, last ? "" : ",");
+ if (r >= *len)
+ return buf;
+
+ *len -= r;
+ buf += r;
+
+ return buf;
+}
+
+#define MODE_STR(x) { .name = #x, .flag = DRM_MODE_FLAG_ ## x, }
+
+static const struct {
+ const char *name;
+ u32 flag;
+} mode_flags[] = {
+ MODE_STR(PHSYNC),
+ MODE_STR(NHSYNC),
+ MODE_STR(PVSYNC),
+ MODE_STR(NVSYNC),
+ MODE_STR(INTERLACE),
+ MODE_STR(CSYNC),
+ MODE_STR(PCSYNC),
+ MODE_STR(NCSYNC),
+ MODE_STR(DBLSCAN),
+ MODE_STR(HSKEW),
+ MODE_STR(DBLCLK),
+
