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@xxxxxxxxxx>
> >> ---
> >>
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
