Nouveau - Aug 2017 - [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

Thanks for the quick review!

On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart <laurent.pinchart@
ideasonboard.com> wrote:
> Hi Joe,
>
> Thank you for the patch.
>
> 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...).

> > 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 at 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 @@
> >  #include <drm/drmP.h>
> >  #include <drm/drm_auth.h>
> >  #include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem.h>
> >
> >  #include "drm_crtc_internal.h"
>
> [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.

> > +                     r->handles[i] = 0;
> > +             }
> > +     }
> > +
> > +     drm_framebuffer_unreference(fb);
> > +
> > +     return ret;
> > +}
>
> [snip]
>
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> > b/drivers/gpu/drm/omapdrm/omap_fb.c index 29dc677dd4d3..a982c72a773e
> 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> > @@ -90,12 +90,15 @@ struct omap_framebuffer {
> >  };
> >
> >  static int omap_framebuffer_create_handle(struct drm_framebuffer *fb,
> > +             unsigned int format_plane_index,
> >               struct drm_file *file_priv,
> >               unsigned int *handle)
> >  {
> >       struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
> > +     if (format_plane_index >= 4 ||
!omap_fb->planes[format_plane_
> index])
>
> The first check shouldn't be needed, the core should never call this
> operation
> with an index >= 4.
>
> I'll remove the check then.
> > +             return -ENOENT;
> >       return drm_gem_handle_create(file_priv,
> > -                     omap_fb->planes[0].bo, handle);
> > +                     omap_fb->planes[format_plane_index].bo,
handle);
> >  }
>
> [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.

> >  /**
> >   * Device specific ioctls should only be in their respective headers
> >   * The device specific ioctl range is from 0x40 to 0x9f.
>
> --
> Regards,
>
> Laurent Pinchart
>
>
Thanks!
-j


-- 
Dr. Joe Michael Kniss |  Google ChromeOS |  djmk at google.com   |
1-801-898-7977 <(801)%20898-7977>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20170801/9d2822ff/attachment-0001.html>

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 at 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