On Thu, Mar 13, 2025 at 11:06:54AM +0000, Aditya Garg wrote: > > On 13 Mar 2025, at 4:18 PM, Petr Mladek <pmladek@xxxxxxxx> wrote: > > On Thu 2025-03-13 09:13:23, Aditya Garg wrote: > >>> On 13 Mar 2025, at 2:27 PM, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxx> > > Tested-by: Petr Mladek <pmladek@xxxxxxxx> > > > > > > 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@xxxxxxxxx > > 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
