Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Joe,

On Tuesday 01 Aug 2017 10:24:25 Joe Kniss wrote:
> On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart wrote:
> > On Monday 31 Jul 2017 11:29:13 Joe Kniss wrote:
> >> New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
> >> with addfb2.
> > 
> > What's the use case for this ? We haven't needed such an ioctl for so long
> > that it seemed to me that userspace doesn't really need it, but I could be
> > wrong.
> 
> Sorry, I failed to reference the original email.  Here it is:
> 
> <snip>
> I am a recent addition to Google's ChromeOS gfx team.   I am currently
> working on display testing and reporting.   An important part of this is
> our screen capture tool, which works by querying drm for crtcs, planes, and
> fbs.  Unfortunately, there is only limited information available via
> drmModeGetFB(), which often wrong information when drmModeAddFB2() was used
> to create the fbs.   For example, if the pixel format is NV12 or YUV420,
> the fb returned knows nothing about the additional buffer planes required
> by these formats.   Ideally, we would like a function (e.g. drmModeGetFB2)
> to return information symmetric with drmModeAddFB2 including the pixel
> format id, buffer plane information etc.
> </snip>
> 
> ChromeOS has needed this functionality from the start, for both testing and
> error reporting.  We got away with guessing the buffer's format (32bit
> xrgb) until now.  We are now enabling overlays and more formats including
> multi-planar (e.g. NV12).  Current getfb() reports neither the pixel format
> nor  planar information.  Without this information, going forward, our gfx
> testing is going to break.  It would be great if we had access to higher
> level buffer structs (like gbm), but we generally don't since they would be
> created by other apps (chrome browser, android apps, etc...).

How is screen capture implemented ? Do you enumerate the framebuffers being 
scanned out, dump their contents and compose them manually based on the active 
plane configuration ? If so, isn't this very race-prone ?

> >> Also modifies *_fb_create_handle() calls to accept a
> >> format_plane_index so that handles for each plane can be generated.
> >> Previously, many *_fb_create_handle() calls simply defaulted to plane 0
> >> only.
> > 
> > And with this patch the amd/amdgpu, armada, gma500, i915, mediatek, msm,
> > nouveau and radeon drivers still do. Do none of them support multi-planar
> > formats ?
>  
> I would imagine that some of these do support multi-planar formats, but
> they don't appear to have them implemented yet (except perhaps as offsets
> into a single buffer).  I will certainly be looking into this soon, but any
> changes will come in future patches.
> 
> >> Signed-off-by: Joe Kniss <djmk@google.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |  5 +-
> >>  drivers/gpu/drm/armada/armada_fb.c          |  1 +
> >>  drivers/gpu/drm/drm_crtc_internal.h         |  2 +
> >>  drivers/gpu/drm/drm_fb_cma_helper.c         | 11 ++--
> >>  drivers/gpu/drm/drm_framebuffer.c           | 79 +++++++++++++++++++-
> >>  drivers/gpu/drm/drm_ioctl.c                 |  1 +
> >>  drivers/gpu/drm/exynos/exynos_drm_fb.c      |  7 ++-
> >>  drivers/gpu/drm/gma500/framebuffer.c        |  2 +
> >>  drivers/gpu/drm/i915/intel_display.c        |  1 +
> >>  drivers/gpu/drm/mediatek/mtk_drm_fb.c       |  1 +
> >>  drivers/gpu/drm/msm/msm_fb.c                |  5 +-
> >>  drivers/gpu/drm/nouveau/nouveau_display.c   |  1 +
> >>  drivers/gpu/drm/omapdrm/omap_fb.c           |  5 +-
> >>  drivers/gpu/drm/radeon/radeon_display.c     |  5 +-
> >>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  6 ++-
> >>  drivers/gpu/drm/tegra/fb.c                  |  9 +++-
> >>  include/drm/drm_framebuffer.h               |  1 +
> >>  include/uapi/drm/drm.h                      |  2 +
> >>  18 files changed, 127 insertions(+), 17 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> >> b/drivers/gpu/drm/drm_framebuffer.c index 28a0108a1ab8..67b3be1bedbc
> >> 100644
> >> --- a/drivers/gpu/drm/drm_framebuffer.c
> >> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >> @@ -24,6 +24,7 @@

[snip]

> >> +/**
> >> + * drm_mode_getfb2 - get FB info
> >> + * @dev: drm device for the ioctl
> >> + * @data: data pointer for the ioctl
> >> + * @file_priv: drm file for the ioctl call
> >> + *
> >> + * Lookup the FB given its ID and return info about it.
> >> + *
> >> + * Called by the user via ioctl.
> >> + *
> >> + * Returns:
> >> + * Zero on success, negative errno on failure.
> >> + */
> >> +int drm_mode_getfb2(struct drm_device *dev,
> >> +                void *data, struct drm_file *file_priv)
> >> +{
> >> +     struct drm_mode_fb_cmd2 *r = data;
> >> +     struct drm_framebuffer *fb;
> >> +     int ret, i;
> >> +
> >> +     if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >> +             return -EINVAL;
> >> +
> >> +     fb = drm_framebuffer_lookup(dev, r->fb_id);
> >> +     if (!fb)
> >> +             return -ENOENT;
> >> +
> >> +     r->height = fb->height;
> >> +     r->width = fb->width;
> >> +     r->pixel_format = fb->format->format;
> >> +     for (i = 0; i < 4; ++i) {
> >> +             r->pitches[i] = fb->pitches[i];
> >> +             r->offsets[i] = fb->offsets[i];
> >> +             r->modifier[i] = fb->modifier;
> >> +             r->handles[i] = 0;
> >> +     }
> >> +
> >> +     for (i = 0; i < fb->format->num_planes; ++i) {
> >> +             if (fb->funcs->create_handle) {
> >> +                     if (drm_is_current_master(file_priv) ||
> >> +                         capable(CAP_SYS_ADMIN) ||
> >> +                         drm_is_control_client(file_priv)) {
> >> +                             ret = fb->funcs->create_handle(fb, i,
> >> file_priv,
> >> +
> >> &r->handles[i]);
> >> +                             if (ret)
> >> +                                     break;
> >> +                     } else {
> >> +                             /* GET_FB() is an unprivileged ioctl so we
> >> must
> >> +                              * not return a buffer-handle to
> >> non-master
> >> +                              * processes! For backwards-compatibility
> >> +                              * reasons, we cannot make GET_FB()
> >> privileged,
> >> +                              * so just return an invalid handle for
> >> +                              * non-masters. */
> > 
> > There's no backward compatibility to handle here, just make it privileged
> > if it has to be.
> 
> Sure thing, sounds good.
> 
> > > +                             r->handles[i] = 0;
> > > +                             ret = 0;
> > > +                     }
> > > +             } else {
> > > +                     ret = -ENODEV;
> > > +                     break;
> > > +             }
> > > +     }
> > > +
> > > +     /* If handle creation failed, delete/dereference any that were
> > 
> > made.
> > */
> > 
> > > +     if (ret) {
> > > +             for (i = 0; i < 4; ++i) {
> > > +                     if (r->handles[i])
> > > +                             drm_gem_handle_delete(file_priv, r-
> > >handles[i]);
> > 
> > My initial reaction to this was to reply that not all drivers use GEM, but
> > after some investigation it seems they actually do. If that's really the
> > case, we could get rid of the .create_handle() operation completely, which
> > should simplify the implementation of both DRM_IOCTL_MODE_GETFB and
> > DRM_IOCTL_MODE_GETFB2.
> 
> Deleting the handle is the same in all cases, but creating the handle
> isn't consistent across the drivers.  I didn't see a clean way to simplify
> this.

Creating the handle seems to be fairly consistent, except for a very small 
number of special cases. If we stored the GEM object pointers in the 
drm_framebuffer structure, the common case would be simplified. Or maybe I've 
missed something :-)

> >> +                     r->handles[i] = 0;
> >> +             }
> >> +     }
> >> +
> >> +     drm_framebuffer_unreference(fb);
> >> +
> >> +     return ret;
> >> +}

[snip]

> >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >> index b2c52843bc70..c81c75335cca 100644
> >> --- a/include/uapi/drm/drm.h
> >> +++ b/include/uapi/drm/drm.h
> >> @@ -814,6 +814,8 @@ extern "C" {
> >> 
> >>  #define DRM_IOCTL_MODE_CREATEPROPBLOB        DRM_IOWR(0xBD, struct
> >> drm_mode_create_blob)
> >> #define DRM_IOCTL_MODE_DESTROYPROPBLOB        DRM_IOWR(0xBE, struct
> >> drm_mode_destroy_blob)
> >> 
> >> +#define DRM_IOCTL_MODE_GETFB2  DRM_IOWR(0xC4, struct drm_mode_fb_cmd2)
> >> +
> > 
> > Which tree is this based on ? The last defined ioctl
> > (DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE) is 0xC2 in drm-next and drm-misc-next.
> 
> I just did a quick search for "DRM_IOWR 0xC*" and picked the first unused
> one, there was a patch in-flight from last month that used 0xC3 (
> https://lists.freedesktop.org/archives/amd-gfx/2017-June/010153.html).
> Naturally, I'll set this to whatever is appropriate.

The best is to base your patch on top of the drm-misc-next branch of the drm-
misc tree. Whoever is merged first will win the ioctl number :-) Of course it 
means rebasing if someone else wins the race, but that shouldn't be a big 
deal.

> > >  /**
> > >  
> > >   * Device specific ioctls should only be in their respective headers
> > >   * The device specific ioctl range is from 0x40 to 0x9f.

-- 
Regards,

Laurent Pinchart

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno