Re: [PATCH 1/2] lib/vsprintf: Add support for generic FourCCs by extending %p4cc

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 13, 2025 at 11:06:54AM +0000, Aditya Garg wrote:
> > On 13 Mar 2025, at 4:18 PM, Petr Mladek <pmladek@suse.com> wrote:
> > On Thu 2025-03-13 09:13:23, Aditya Garg wrote:
> >>> On 13 Mar 2025, at 2:27 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >>> On Thu, Mar 13, 2025 at 08:53:28AM +0000, Aditya Garg wrote:
> >>>>>> On 13 Mar 2025, at 2:19 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >>>>> On Thu, Mar 13, 2025 at 07:26:05AM +0000, Aditya Garg wrote:
> >>>>>>>> On 13 Mar 2025, at 12:58 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >>>>>>> On Wed, Mar 12, 2025 at 07:14:36PM +0000, Aditya Garg wrote:
> >>>>>>>>> On 12 Mar 2025, at 9:05 PM, Sven Peter <sven@svenpeter.dev> wrote:
> >>>>>>>>> On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote:

...

> >>>>>>>>> I don't have a strong opinion either way: for SMC I just need to print
> >>>>>>>>> FourCC keys for debugging / information in a few places.
> >>>>>>>>> 
> >>>>>>>>> I'm preparing the SMC driver for upstreaming again (after a two year delay :-()
> >>>>>>>>> and was just going to use macros to print the SMC FourCC keys similar to
> >>>>>>>>> DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit
> >>>>>>>>> the topic later.
> >>>>>>>>> 
> >>>>>>>>> Right now I have these in my local tree (only compile tested so far):
> >>>>>>>>> 
> >>>>>>>>> #define SMC_KEY_FMT "%c%c%c%c (0x%08x)"
> >>>>>>>>> #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k)
> >>>>>>>> 
> >>>>>>>> That seems to be a nice alternative, which I guess Thomas was also suggesting.
> >>>>>>> 
> >>>>>>> I don't think it's "nice". Each of the approaches has pros and cons.
> >>>>>>> You can start from bloat-o-meter here and compare it with your %p extension.
> >>>>>>> 
> >>>>>>> Also, can you show the bloat-o-meter output for the vsprintf.c?
> >>>>>> 
> >>>>>> Here are your outputs:
> >>>>> 
> >>>>> Thank you!
> >>>>> 
> >>>>>> ---------------------------------------------------------------------
> >>>>>> For appletbdrm:
> >>>>>> 
> >>>>>> aditya@MacBook:~/linux$ ./scripts/bloat-o-meter $P4 $MACRO
> >>>>>> add/remove: 0/0 grow/shrink: 1/1 up/down: 64/-19 (45)
> >>>>>> Function                                     old     new   delta
> >>>>>> appletbdrm_read_response                     395     459     +64
> >>>>>> appletbdrm_probe                            1786    1767     -19
> >>>>>> Total: Before=13418, After=13463, chg +0.34%
> >>>>> 
> >>>>> This is enough, no need to repeat this for every parameter.
> >>>>> 
> >>>>>> ---------------------------------------------------------------------
> >>>>>> For vsprintf:
> >>>>>> 
> >>>>>> aditya@MacBook:~/linux$ ./scripts/bloat-o-meter $OLD $NEW
> >>>>>> add/remove: 0/0 grow/shrink: 1/0 up/down: 220/0 (220)
> >>>>>> Function                                     old     new   delta
> >>>>>> fourcc_string                                479     699    +220
> >>>>>> Total: Before=26454, After=26674, chg +0.83%
> >>>>> 
> >>>>> So, we get +220 bytes vs +43 bytes. It means if we found 5+ users, it worth
> >>>>> doing.
> >>>> 
> >>>> Will it also depend upon the number of times it's being used? In appletbdrm,
> >>>> it is being used 3 times. Probably more in Asahi SMC.
> >>> 
> >>> Right, it depends on the usage count. Also on different architectures it may
> >>> give different results. On 32-bit it probably gives better statistics.
> >> 
> >> Best to go ahead with vsprintf then. Petr, are you still there?
> > 
> > I am here but there were many other things in the queue ;-)
> > 
> > I do not have strong opinion. I am not familiar with the FourCC
> > format and it looks like a magic to me. But it seems that it makes
> > sense for the users.
> > 
> > I personally find the %pcX modifiers a bit less hacky than
> > the two macros SMC_KEY_FMT/SMC_KEY_ARG.
> > 
> > So I am fine with this patch:
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > Tested-by: Petr Mladek <pmladek@suse.com>
> > 
> > 
> > Now, the question is how to get this patch into the mainline.
> > 
> > Normally, it would make perfect sense to queue it via the DRM tree
> > because drivers/gpu/drm/tiny/appletbdrm.c is a new driver...
> > 
> > But this time there is a conflicting patchset which is reworking
> > the entire lib/test_printf.c file, see
> > 20250307-printf-kunit-convert-v6-0-4d85c361c241@gmail.com
> 
> Link seems to be broken

It works fine. Because it's not a link, it's Message-ID, you need to add
https://lore.kernel.org/r/ in front of it.

> > And it will likely be ready for the next merge window as well.
> > I am going to review it right away.
> > 
> > It is even more complicated because the patchset converting
> > the printf test module to KUNIT depends on another changes
> > in Kees' tree (moving kunit test modules to lib/tests/).
> > So it might be easier when it goes via Kees' tree.
> > 
> > And it might be easier when even this patch goes via Kees' tree.
> > 
> > My proposal:
> > 
> > I suggest to separate the fourcc_pointer() test update
> > to a separate patch and add it later after the merge window
> > when things settle down.
> > 
> > I mean to send the vsprintf.c, checkpatch.pl, and doc update
> > via DRM tree together with the new appletbdrm.c driver.
> 
> Sounds good. At least we can get it working. I’ll make sure the self
> tests get updated once 6.15-rc1 gets released, or Kees can share
> his tree, where I can add the tests as well.
> 
> I’ll send a v2 so that Thomas can take them up.
> > 
> > And update the selftest later when both DRM tree and KUNIT
> > update reaches mainline.
> > 
> > How does that sound, please?

To me sounds good, but I'm not a maintainer involved in all this :-)

-- 
With Best Regards,
Andy Shevchenko