| Message ID | 20241127131838.3268735-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dp: use seq buf for printing rates | expand |
Quoting Jani Nikula (2024-11-27 10:18:38-03:00) >Hand rolling the buffer overflow handling with snprintf() is a bit >tedious. The seq_buf interface is made for this. Switch to it. Cool! Today I learned a new kernel interface. :-) > >Use struct intel_display while at it. > >Signed-off-by: Jani Nikula <jani.nikula@intel.com> >--- > drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >index 053a9a4182e7..4471c8fcd478 100644 >--- a/drivers/gpu/drm/i915/display/intel_dp.c >+++ b/drivers/gpu/drm/i915/display/intel_dp.c >@@ -28,6 +28,7 @@ > #include <linux/export.h> > #include <linux/i2c.h> > #include <linux/notifier.h> >+#include <linux/seq_buf.h> > #include <linux/slab.h> > #include <linux/sort.h> > #include <linux/string_helpers.h> >@@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) > return DISPLAY_VER(i915) >= 10; > } > >-static void snprintf_int_array(char *str, size_t len, >- const int *array, int nelem) >+static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) > { > int i; > >- str[0] = '\0'; >- >- for (i = 0; i < nelem; i++) { >- int r = snprintf(str, len, "%s%d", i ? ", " : "", array[i]); >- if (r >= len) >- return; >- str += r; >- len -= r; >- } >+ for (i = 0; i < nelem; i++) >+ seq_buf_printf(s, "%s%d", i ? ", " : "", array[i]); > } > > static void intel_dp_print_rates(struct intel_dp *intel_dp) > { >- struct drm_i915_private *i915 = dp_to_i915(intel_dp); >- char str[128]; /* FIXME: too big for stack? */ >+ struct intel_display *display = to_intel_display(intel_dp); >+ DECLARE_SEQ_BUF(s, 128); /* FIXME: too big for stack? */ I wonder if just using drm_dbg_printer() would make things simpler, without requiring a buffer. Anyway, the patch looks good to me, so: Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> > > if (!drm_debug_enabled(DRM_UT_KMS)) > return; > >- snprintf_int_array(str, sizeof(str), >- intel_dp->source_rates, intel_dp->num_source_rates); >- drm_dbg_kms(&i915->drm, "source rates: %s\n", str); >+ seq_buf_print_array(&s, intel_dp->source_rates, intel_dp->num_source_rates); >+ drm_dbg_kms(display->drm, "source rates: %s\n", seq_buf_str(&s)); > >- snprintf_int_array(str, sizeof(str), >- intel_dp->sink_rates, intel_dp->num_sink_rates); >- drm_dbg_kms(&i915->drm, "sink rates: %s\n", str); >+ seq_buf_clear(&s); >+ seq_buf_print_array(&s, intel_dp->sink_rates, intel_dp->num_sink_rates); >+ drm_dbg_kms(display->drm, "sink rates: %s\n", seq_buf_str(&s)); > >- snprintf_int_array(str, sizeof(str), >- intel_dp->common_rates, intel_dp->num_common_rates); >- drm_dbg_kms(&i915->drm, "common rates: %s\n", str); >+ seq_buf_clear(&s); >+ seq_buf_print_array(&s, intel_dp->common_rates, intel_dp->num_common_rates); >+ drm_dbg_kms(display->drm, "common rates: %s\n", seq_buf_str(&s)); > } > > static int forced_link_rate(struct intel_dp *intel_dp) >-- >2.39.5 >
On Wed, Nov 27, 2024 at 03:18:38PM +0200, Jani Nikula wrote: > Hand rolling the buffer overflow handling with snprintf() is a bit > tedious. The seq_buf interface is made for this. Switch to it. > > Use struct intel_display while at it. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 053a9a4182e7..4471c8fcd478 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -28,6 +28,7 @@ > #include <linux/export.h> > #include <linux/i2c.h> > #include <linux/notifier.h> > +#include <linux/seq_buf.h> > #include <linux/slab.h> > #include <linux/sort.h> > #include <linux/string_helpers.h> > @@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) > return DISPLAY_VER(i915) >= 10; > } > > -static void snprintf_int_array(char *str, size_t len, > - const int *array, int nelem) > +static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) Perhaps with more users worth introducing as part of core lib? Andy and I did something similar with kmemdup. https://lore.kernel.org/linux-gpio/20241126172240.6044-1-raag.jadav@intel.com/ Raag
On Wed, 27 Nov 2024, Raag Jadav <raag.jadav@intel.com> wrote: > On Wed, Nov 27, 2024 at 03:18:38PM +0200, Jani Nikula wrote: >> Hand rolling the buffer overflow handling with snprintf() is a bit >> tedious. The seq_buf interface is made for this. Switch to it. >> >> Use struct intel_display while at it. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- >> 1 file changed, 14 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 053a9a4182e7..4471c8fcd478 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -28,6 +28,7 @@ >> #include <linux/export.h> >> #include <linux/i2c.h> >> #include <linux/notifier.h> >> +#include <linux/seq_buf.h> >> #include <linux/slab.h> >> #include <linux/sort.h> >> #include <linux/string_helpers.h> >> @@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) >> return DISPLAY_VER(i915) >= 10; >> } >> >> -static void snprintf_int_array(char *str, size_t len, >> - const int *array, int nelem) >> +static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) > > Perhaps with more users worth introducing as part of core lib? Maybe later; right now I have neither the time or energy, just dumped this off my local branch. BR, Jani. > > Andy and I did something similar with kmemdup. > https://lore.kernel.org/linux-gpio/20241126172240.6044-1-raag.jadav@intel.com/ > > Raag
On Wed, 27 Nov 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote: > Quoting Jani Nikula (2024-11-27 10:18:38-03:00) >>Hand rolling the buffer overflow handling with snprintf() is a bit >>tedious. The seq_buf interface is made for this. Switch to it. > > Cool! Today I learned a new kernel interface. :-) \o/ >> >>Use struct intel_display while at it. >> >>Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>--- >> drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- >> 1 file changed, 14 insertions(+), 22 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >>index 053a9a4182e7..4471c8fcd478 100644 >>--- a/drivers/gpu/drm/i915/display/intel_dp.c >>+++ b/drivers/gpu/drm/i915/display/intel_dp.c >>@@ -28,6 +28,7 @@ >> #include <linux/export.h> >> #include <linux/i2c.h> >> #include <linux/notifier.h> >>+#include <linux/seq_buf.h> >> #include <linux/slab.h> >> #include <linux/sort.h> >> #include <linux/string_helpers.h> >>@@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) >> return DISPLAY_VER(i915) >= 10; >> } >> >>-static void snprintf_int_array(char *str, size_t len, >>- const int *array, int nelem) >>+static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) >> { >> int i; >> >>- str[0] = '\0'; >>- >>- for (i = 0; i < nelem; i++) { >>- int r = snprintf(str, len, "%s%d", i ? ", " : "", array[i]); >>- if (r >= len) >>- return; >>- str += r; >>- len -= r; >>- } >>+ for (i = 0; i < nelem; i++) >>+ seq_buf_printf(s, "%s%d", i ? ", " : "", array[i]); >> } >> >> static void intel_dp_print_rates(struct intel_dp *intel_dp) >> { >>- struct drm_i915_private *i915 = dp_to_i915(intel_dp); >>- char str[128]; /* FIXME: too big for stack? */ >>+ struct intel_display *display = to_intel_display(intel_dp); >>+ DECLARE_SEQ_BUF(s, 128); /* FIXME: too big for stack? */ > > I wonder if just using drm_dbg_printer() would make things simpler, > without requiring a buffer. Mmh, that's always line based, isn't it? It would result in each rate getting printed on its own line, which is too spammy. > Anyway, the patch looks good to me, so: > > Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> Thanks! > >> >> if (!drm_debug_enabled(DRM_UT_KMS)) >> return; >> >>- snprintf_int_array(str, sizeof(str), >>- intel_dp->source_rates, intel_dp->num_source_rates); >>- drm_dbg_kms(&i915->drm, "source rates: %s\n", str); >>+ seq_buf_print_array(&s, intel_dp->source_rates, intel_dp->num_source_rates); >>+ drm_dbg_kms(display->drm, "source rates: %s\n", seq_buf_str(&s)); >> >>- snprintf_int_array(str, sizeof(str), >>- intel_dp->sink_rates, intel_dp->num_sink_rates); >>- drm_dbg_kms(&i915->drm, "sink rates: %s\n", str); >>+ seq_buf_clear(&s); >>+ seq_buf_print_array(&s, intel_dp->sink_rates, intel_dp->num_sink_rates); >>+ drm_dbg_kms(display->drm, "sink rates: %s\n", seq_buf_str(&s)); >> >>- snprintf_int_array(str, sizeof(str), >>- intel_dp->common_rates, intel_dp->num_common_rates); >>- drm_dbg_kms(&i915->drm, "common rates: %s\n", str); >>+ seq_buf_clear(&s); >>+ seq_buf_print_array(&s, intel_dp->common_rates, intel_dp->num_common_rates); >>+ drm_dbg_kms(display->drm, "common rates: %s\n", seq_buf_str(&s)); >> } >> >> static int forced_link_rate(struct intel_dp *intel_dp) >>-- >>2.39.5 >>
Quoting Jani Nikula (2024-11-27 14:24:58-03:00) >On Wed, 27 Nov 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote: >> Quoting Jani Nikula (2024-11-27 10:18:38-03:00) >>>Hand rolling the buffer overflow handling with snprintf() is a bit >>>tedious. The seq_buf interface is made for this. Switch to it. >> >> Cool! Today I learned a new kernel interface. :-) > >\o/ > >>> >>>Use struct intel_display while at it. >>> >>>Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>>--- >>> drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- >>> 1 file changed, 14 insertions(+), 22 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >>>index 053a9a4182e7..4471c8fcd478 100644 >>>--- a/drivers/gpu/drm/i915/display/intel_dp.c >>>+++ b/drivers/gpu/drm/i915/display/intel_dp.c >>>@@ -28,6 +28,7 @@ >>> #include <linux/export.h> >>> #include <linux/i2c.h> >>> #include <linux/notifier.h> >>>+#include <linux/seq_buf.h> >>> #include <linux/slab.h> >>> #include <linux/sort.h> >>> #include <linux/string_helpers.h> >>>@@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) >>> return DISPLAY_VER(i915) >= 10; >>> } >>> >>>-static void snprintf_int_array(char *str, size_t len, >>>- const int *array, int nelem) >>>+static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) >>> { >>> int i; >>> >>>- str[0] = '\0'; >>>- >>>- for (i = 0; i < nelem; i++) { >>>- int r = snprintf(str, len, "%s%d", i ? ", " : "", array[i]); >>>- if (r >= len) >>>- return; >>>- str += r; >>>- len -= r; >>>- } >>>+ for (i = 0; i < nelem; i++) >>>+ seq_buf_printf(s, "%s%d", i ? ", " : "", array[i]); >>> } >>> >>> static void intel_dp_print_rates(struct intel_dp *intel_dp) >>> { >>>- struct drm_i915_private *i915 = dp_to_i915(intel_dp); >>>- char str[128]; /* FIXME: too big for stack? */ >>>+ struct intel_display *display = to_intel_display(intel_dp); >>>+ DECLARE_SEQ_BUF(s, 128); /* FIXME: too big for stack? */ >> >> I wonder if just using drm_dbg_printer() would make things simpler, >> without requiring a buffer. > >Mmh, that's always line based, isn't it? It would result in each rate >getting printed on its own line, which is too spammy. Oh... I thought we could keep the "\n" to be emitted at the end. However, now that you mentioned, I'm not sure if that really works and/or that it is race-free. Looking at results of git grep -W -e 'drm_printf(.*",' --and --not -e '\\n' , I see that ivpu_bo_print_info(), drm_buddy_print(), bridges_show() and drm_print_bits() are some examples of the existing cases that are similar to what I was thinking. -- Gustavo Sousa > >> Anyway, the patch looks good to me, so: >> >> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> > >Thanks! > >> >>> >>> if (!drm_debug_enabled(DRM_UT_KMS)) >>> return; >>> >>>- snprintf_int_array(str, sizeof(str), >>>- intel_dp->source_rates, intel_dp->num_source_rates); >>>- drm_dbg_kms(&i915->drm, "source rates: %s\n", str); >>>+ seq_buf_print_array(&s, intel_dp->source_rates, intel_dp->num_source_rates); >>>+ drm_dbg_kms(display->drm, "source rates: %s\n", seq_buf_str(&s)); >>> >>>- snprintf_int_array(str, sizeof(str), >>>- intel_dp->sink_rates, intel_dp->num_sink_rates); >>>- drm_dbg_kms(&i915->drm, "sink rates: %s\n", str); >>>+ seq_buf_clear(&s); >>>+ seq_buf_print_array(&s, intel_dp->sink_rates, intel_dp->num_sink_rates); >>>+ drm_dbg_kms(display->drm, "sink rates: %s\n", seq_buf_str(&s)); >>> >>>- snprintf_int_array(str, sizeof(str), >>>- intel_dp->common_rates, intel_dp->num_common_rates); >>>- drm_dbg_kms(&i915->drm, "common rates: %s\n", str); >>>+ seq_buf_clear(&s); >>>+ seq_buf_print_array(&s, intel_dp->common_rates, intel_dp->num_common_rates); >>>+ drm_dbg_kms(display->drm, "common rates: %s\n", seq_buf_str(&s)); >>> } >>> >>> static int forced_link_rate(struct intel_dp *intel_dp) >>>-- >>>2.39.5 >>> > >-- >Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 053a9a4182e7..4471c8fcd478 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -28,6 +28,7 @@ #include <linux/export.h> #include <linux/i2c.h> #include <linux/notifier.h> +#include <linux/seq_buf.h> #include <linux/slab.h> #include <linux/sort.h> #include <linux/string_helpers.h> @@ -1506,41 +1507,32 @@ bool intel_dp_source_supports_tps4(struct drm_i915_private *i915) return DISPLAY_VER(i915) >= 10; } -static void snprintf_int_array(char *str, size_t len, - const int *array, int nelem) +static void seq_buf_print_array(struct seq_buf *s, const int *array, int nelem) { int i; - str[0] = '\0'; - - for (i = 0; i < nelem; i++) { - int r = snprintf(str, len, "%s%d", i ? ", " : "", array[i]); - if (r >= len) - return; - str += r; - len -= r; - } + for (i = 0; i < nelem; i++) + seq_buf_printf(s, "%s%d", i ? ", " : "", array[i]); } static void intel_dp_print_rates(struct intel_dp *intel_dp) { - struct drm_i915_private *i915 = dp_to_i915(intel_dp); - char str[128]; /* FIXME: too big for stack? */ + struct intel_display *display = to_intel_display(intel_dp); + DECLARE_SEQ_BUF(s, 128); /* FIXME: too big for stack? */ if (!drm_debug_enabled(DRM_UT_KMS)) return; - snprintf_int_array(str, sizeof(str), - intel_dp->source_rates, intel_dp->num_source_rates); - drm_dbg_kms(&i915->drm, "source rates: %s\n", str); + seq_buf_print_array(&s, intel_dp->source_rates, intel_dp->num_source_rates); + drm_dbg_kms(display->drm, "source rates: %s\n", seq_buf_str(&s)); - snprintf_int_array(str, sizeof(str), - intel_dp->sink_rates, intel_dp->num_sink_rates); - drm_dbg_kms(&i915->drm, "sink rates: %s\n", str); + seq_buf_clear(&s); + seq_buf_print_array(&s, intel_dp->sink_rates, intel_dp->num_sink_rates); + drm_dbg_kms(display->drm, "sink rates: %s\n", seq_buf_str(&s)); - snprintf_int_array(str, sizeof(str), - intel_dp->common_rates, intel_dp->num_common_rates); - drm_dbg_kms(&i915->drm, "common rates: %s\n", str); + seq_buf_clear(&s); + seq_buf_print_array(&s, intel_dp->common_rates, intel_dp->num_common_rates); + drm_dbg_kms(display->drm, "common rates: %s\n", seq_buf_str(&s)); } static int forced_link_rate(struct intel_dp *intel_dp)
Hand rolling the buffer overflow handling with snprintf() is a bit tedious. The seq_buf interface is made for this. Switch to it. Use struct intel_display while at it. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 36 ++++++++++--------------- 1 file changed, 14 insertions(+), 22 deletions(-)