Re: [Intel-gfx] [PATCH v3] drm: move allocation out of drm_get_format_name()

[Laurent Pinchart]

Hi Jani,

On Thursday 10 Nov 2016 12:30:09 Jani Nikula wrote:
> On Thu, 10 Nov 2016, Laurent Pinchart wrote:
> > On Wednesday 09 Nov 2016 16:59:31 Eric Engestrom wrote:
> >> On Wednesday, 2016-11-09 14:13:40 +0100, Daniel Vetter wrote:
> >>> On Wed, Nov 9, 2016 at 12:42 PM, Eric Engestrom wrote:
> >>>>> Well, had to drop it again since it didn't compile:
> >>>>>   CC [M]  drivers/gpu/drm/drm_blend.o
> >>>>> 
> >>>>> drivers/gpu/drm/drm_atomic.c: In function
> >>>>> âdrm_atomic_plane_print_stateâ:
> >>>>> drivers/gpu/drm/drm_atomic.c:920:5: error: too few arguments to
> >>>>> function âdrm_get_format_nameâ> >>
> >>>>>      drm_get_format_name(fb->pixel_format));
> >>>>>      ^~~~~~~~~~~~~~~~~~~
> >>>>> In file included from ./include/drm/drmP.h:71:0,
> >>>>>                  from drivers/gpu/drm/drm_atomic.c:29:
> >>>>> ./include/drm/drm_fourcc.h:65:7: note: declared here
> >>>>>  char *drm_get_format_name(uint32_t format, struct
> >>>>>  drm_format_name_buf *buf);
> >>>>>        ^~~~~~~~~~~~~~~~~~~
> >>>>> 
> >>>>> Can you pls rebase onto drm-misc or linux-next or something?
> >>>> 
> >>>> That was based on airlied/drm-next (last fetched on Sunday I think),
> >>>> I can rebase it on drm-misc if it helps, but it seems older than
> >>>> drm-next. Should I just rebase on top of current head of drm-next?
> >>> 
> >>> It needs to be drm-misc (linux-next doesn't have it yet) due to the
> >>> new atomic debug work that we just landed. I'm working on drm-tip as a
> >>> drm local integration tree to ease pains like these a bit, but that
> >>> doesn't really exist yet.
> >> 
> >> I'm confused as to how the different trees and branches merge back to
> >> Torvalds' tree (I'm interested in particular in drm), and I'm not sure
> >> which branch you want me to rebase on in the drm-misc tree [1],
> >> especially since all of them are older than drm-next [2].
> >> 
> >> I'll try to rebase on drm-misc-fixes (currently at 4da5caa6a6f82cda3193)
> >> as it sounds about right, but it doesn't apply at all, so it'll take
> >> a little while.
> > 
> > While at it, could you make the function return a const char * ?
> 
> I thought I mentioned that too, though I didn't insist.

You did, and I think Eric agreed to change that.

> > By the way, while this is an improvement over the current situation in
> > that it fixes the missing kfree() issue, I wonder whether the problem
> > we're trying to solve should be addressed at a more global level.
> 
> Maybe, but let's not block this one!

Sure, that wasn't the intent of my e-mail.

> > The issue here is that printk can't format the fourcc as a string by
> > itself. There's a bunch of places in the kernel where a similar
> > formatting problem occurs. In a few occasions it has been solved by
> > extending printk with additional format specifiers (such as for MAC/IP
> > addresses, GUIDs, various kind of device names, ...). DRM fourccs are
> > probably too DRM specific to be worth a format specifier, but I wonder
> > whether we could introduce a new specifier that takes a function pointer
> > as a formatting helper. Another similarly crazy option would be a format
> > specifier for strings that would free the passed pointer after printing
> > it.
> 
> I think there are too many non-standard format specifiers already. I
> can't review the non-standard format strings without looking at
> Documentation/prink-formats.txt first. The formatting hook would be a
> generic alternative, but that's more than a little scary from the
> security standpoint. And what if the hook has to allocate memory? Can't
> do that in atomic contexts.

There are lots of details to sort out obviously and I don't have an answer to 
all questions yet. I think it would be worth researching this, as the problem 
isn't specific to DRM/KMS.

> >> Could you give me a quick explanation or point me to a doc/page that
> >> explains how the various trees and branches get merged?
> >> I googled a bit and found this doc [4] by Jani, but it doesn't mention
> >> drm-misc for instance, so I'm not sure how up-to-date and
> >> non-intel-specific it is.
> >> 
> >> Looking at this page, something just occurred to me: did you mean
> >> drm-fixes [3], instead of one of the branches on drm-misc?
> >> 
> >> Cheers,
> >> 
> >>   Eric
> >> 
> >> [1] git://anongit.freedesktop.org/drm/drm-misc
> >> [2] git://people.freedesktop.org/~airlied/linux drm-next
> >> [2] git://people.freedesktop.org/~airlied/linux drm-fixes
> >> [3] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html

-- 
Regards,

Laurent Pinchart