[Intel-gfx] [PATCH 0/8] drm/fb-helper: Use drm_file to get a dumb framebuffer

[Laurent Pinchart]

Hi Noralf,

On Wednesday, 13 September 2017 18:19:22 EEST Noralf Trønnes wrote:
> Den 13.09.2017 07.09, skrev Laurent Pinchart:
> > On Monday, 11 September 2017 17:31:54 EEST Noralf Trønnes wrote:
> >> Hi,
> >> 
> >> I want to start out by saying that this patchset is low priority for me
> >> and if no one has interest or time to review this, that is just fine. I
> >> was in the flow and just typed it out.
> >> 
> >> This patchset adds a way for fbdev emulation code to create a
> >> framebuffer that is backed by a dumb buffer. drm_fb_helper gets a
> >> drm_file to hang the objects on, drm_framebuffer_create_dumb() creates
> >> the framebuffer and drm_fb_helper_fini() destroys it.
> >> I have verified that all cma drivers supports dumb buffers, so
> >> converting the library should be fine for all.
> > 
> > Stupid question, what does this give us ? The series makes the call stack
> > more complex (up to a point where I'm getting trouble just following it),
> > what's the advantage that offsets that ?
> 
> The short answer is that it avoids the need for special fbdev
> _with_funcs() functions in libraries like cma for framebuffers with the
> dirty callback set.
> 
> I'm suprised that you think this more complex, I find it more coherent
> when fbdev userspace is doing things more like DRM userspace, like
> hanging the objects on a drm_file and cleaning up the objects when done.

Maybe moving to a new code base gave me the wrong feeling that the result is 
more complex. The current implementation is certainly not simple.

> The longer and more diffuse answer is that it's annoying me that many
> drivers carry the burden of fbdev emulation just to get a console!

I totally agree with that. Ideally I'd like to remove 100% of fbdev-related 
code from drivers. This includes

- initialization and cleanup of fbdev helpers
- fbdev restore in last_close()
- forwarding of hotplug events to fbdev compatibility layer

In practice we'll likely need to keep one initialization function (or a 
drm_driver flag) to let drivers opt-in to fbdev compatibility, but the rest 
should go away. Or maybe we could even enable fbdev compatibility in all 
drivers unconditionally.

> And an in-kernel drm console, as David Herrmann described it a year ago,
> would allocate a framebuffer using something like what I have done here.
> But maybe it's better to do this the other way around: first get the
> drm console and then let fbdev use the same mechanisms. Or maybe at that
> point, just forget about fbdev and not spend any more time on it :-)

When I mention I'd like fbdev to go away completely people complain that I 
like breaking things :-)

> Sidenote:
> Do you if any DRM drivers is capable of showing kernel oops'es on the
> console?

The last time my kernel oopses the i915 driver certainly didn't show me the 
oops.

> > With the exception of vmwgfx that does weird things I won't even try to
> > understand, all drivers seem to use the drm_file object passed to the
> > .dumb_create() operation just to register the GEM object handle. I wonder
> > whether a better solution to use .dumb_create() for framebuffer emulation
> > wouldn't be to move the GEM object handle registration from the
> > .dumb_create() implementation to its caller in the core.
> 
> Can you elaborate what you mean by this?

Again vmwgfx aside (and that's not a detail, we'd still have to handle that 
driver), the reason why the .dumb_create() operation needs a drm_file pointer 
is to register the GEM object handle for the dumb buffer. We could perform 
that registration in the caller of .dumb_create(), in which case we wouldn't 
need to pass the drm_file to the operation.

> >> A patch by David Herrmann from a year ago made this easy. It was the
> >> last piece in his work to make it possible to create a drm_file for
> >> in-kernel use, but it never got merged.
> >> 
> >> I've cc'ed intel-gfx since that will give CI runs of the core patches if
> >> I understood Daniel right.
> >> 
> >> Noralf.
> >> 
> >> David Herrmann (1):
> >>    drm: provide management functions for drm_file
> >> 
> >> Noralf Trønnes (7):
> >>    drm/framebuffer: Add drm_framebuffer_create_dumb()
> >>    drm/auth: Export drm_dropmaster_ioctl()
> >>    drm/fb-helper: Allocate a drm_file
> >>    drm/fb-cma-helper: Use drm_framebuffer_create_dumb()
> >>    drm/fb-cma-helper: Drop unnecessary fbdev buffer offset
> >>    drm/tinydrm: Use drm_fbdev_cma_init()
> >>    drm/fb-cma-helper: Remove drm_fbdev_cma_init_with_funcs()
> >>   
> >>   drivers/gpu/drm/drm_auth.c                  |   1 +
> >>   drivers/gpu/drm/drm_fb_cma_helper.c         | 111 ++--------
> >>   drivers/gpu/drm/drm_fb_helper.c             |  22 +-
> >>   drivers/gpu/drm/drm_file.c                  | 323 ++++++++++++--------
> >>   drivers/gpu/drm/drm_framebuffer.c           |  61 ++++++
> >>   drivers/gpu/drm/drm_internal.h              |   2 -
> >>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c |   5 +-
> >>   include/drm/drm_auth.h                      |   2 +
> >>   include/drm/drm_fb_helper.h                 |   9 +
> >>   include/drm/drm_file.h                      |   2 +
> >>   include/drm/drm_framebuffer.h               |   4 +
> >>   11 files changed, 305 insertions(+), 237 deletions(-)

-- 
Regards,

Laurent Pinchart