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

[Laurent Pinchart]

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 ?

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

Re: [Nouveau] [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