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

[Joe Kniss]

On Tue, Aug 1, 2017 at 1:46 PM, Laurent Pinchart <
laurent.pinch...@ideasonboard.com> wrote:

> 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:
> >
> > 
> > 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.
> > 
> >
> > 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 ?
>
>
Yes, this is basically it.  We identify the crtc to capture, query the
planes associated with it and their properties.  For each plane, we get the
fb, then a FD that we use to import a GBM buffer, which we map and
composite.  It's not ideal, but it's the only thing that will work across
all of our platforms and configs.   We either wait or sleep for consistent
testing captures.  I'm not sure what we can do about the race condition
without a much more substantial change...  We are currently looking into
some platform specific methods, but the current approach won't be going
away anytime soon.


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

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

[Laurent Pinchart]

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.

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

> Signed-off-by: Joe Kniss 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> 
>  #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,
> +
> >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.

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

[Freedreno] [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.

[Joe Kniss]

New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
with addfb2.   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.

Signed-off-by: Joe Kniss 
---
 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(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 39fc388f222a..c77c1cd265a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -566,8 +566,9 @@ static void amdgpu_user_framebuffer_destroy(struct 
drm_framebuffer *fb)
 }
 
 static int amdgpu_user_framebuffer_create_handle(struct drm_framebuffer *fb,
- struct drm_file *file_priv,
- unsigned int *handle)
+unsigned int plane_index,
+struct drm_file *file_priv,
+unsigned int *handle)
 {
struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
 
diff --git a/drivers/gpu/drm/armada/armada_fb.c 
b/drivers/gpu/drm/armada/armada_fb.c
index 2a7eb6817c36..9f237544f6c5 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -23,6 +23,7 @@ static void armada_fb_destroy(struct drm_framebuffer *fb)
 }
 
 static int armada_fb_create_handle(struct drm_framebuffer *fb,
+   unsigned int format_plane_index,
struct drm_file *dfile, unsigned int *handle)
 {
struct armada_framebuffer *dfb = drm_fb_to_armada_fb(fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..ec8d913240fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -170,6 +170,8 @@ int drm_mode_rmfb(struct drm_device *dev,
  void *data, struct drm_file *file_priv);
 int drm_mode_getfb(struct drm_device *dev,
   void *data, struct drm_file *file_priv);
+int drm_mode_getfb2(struct drm_device *dev,
+  void *data, struct drm_file *file_priv);
 int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
   void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
b/drivers/gpu/drm/drm_fb_cma_helper.c
index 596fabf18c3e..5fd7bcc2c6d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -110,13 +110,16 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_fb_cma_destroy);
 
-int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
-   struct drm_file *file_priv, unsigned int *handle)
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+   unsigned int format_plane_index,
+   struct drm_file *file_priv,
+   unsigned int *handle)
 {
struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
+   if (format_plane_index >= 4 || !fb_dma->obj[format_plane_index])
+   return -ENOENT;
return drm_gem_handle_create(file_priv,
-   _cma->obj[0]->base, handle);
+   _cma->obj[format_plane_index]->base, handle);
 }
 EXPORT_SYMBOL(drm_fb_cma_create_handle);
 
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 
 #include 
 #include 
+#include 
 
 #include "drm_crtc_internal.h"
 
@@ -438,7 +439,7 @@ int drm_mode_getfb(struct drm_device *dev,
if (fb->funcs->create_handle) {
if (drm_is_current_master(file_priv) || capable(CAP_SYS_ADMIN) 
||