Re: [PATCH] drm/i915/dp: use seq buf for printing rates

[Gustavo Sousa <gustavo.sousa@intel.com>]

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