Thanks for the quick review!=C2=A0

On Tue, Aug 1, 2017 at 5:09 AM, Laure= nt 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<= br> > with addfb2.

What's the use case for this ? We haven't needed such an ioc= tl 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 origina= l email.=C2=A0 Here it is:=C2=A0

<snip>

=C2=A0I am a recent addition to Google's Chrome= OS gfx team. =C2=A0 I am currently working on display testing and reporting= . =C2=A0 An important part of this is our screen capture tool, which works = by querying drm for crtcs, planes, and fbs.=C2=A0 Unfortunately, there is o= nly limited information available via drmModeGetFB(), which often wrong inf= ormation when drmModeAddFB2() was used to create the fbs. =C2=A0 For exampl= e, if the pixel format is NV12 or YUV420, the fb returned knows nothing abo= ut the additional buffer planes required by these formats. =C2=A0 Ideally, = we would like a function (e.g. drmModeGetFB2) to return information symmetr= ic with drmModeAddFB2 including the pixel format id, buffer plane informati= on etc. =C2=A0=C2=A0

<= /snip>

ChromeOS has needed this functionality from the = start, for both testing and error reporting.=C2=A0 We got away with guessin= g the buffer's format (32bit xrgb) until now.=C2=A0 We are now enabling= overlays and more formats including multi-planar (e.g. NV12).=C2=A0 Curren= t getfb() reports neither the pixel format nor =C2=A0planar information.=C2= =A0 Without this information, going forward, our gfx testing is going to br= eak.=C2=A0 It would be great if we had access to higher level buffer struct= s (like gbm), but we generally don't since they would be created by oth= er apps (chrome browser, android apps, etc...).

=C2=A0

> 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 su= pport multi-planar formats, but they don't appear to have them implemen= ted yet (except perhaps as offsets into a single buffer).=C2=A0 I will cert= ainly be looking into this soon, but any changes will come in future patche= s. =C2=A0

=C2=A0

> Signed-off-by: Joe Kniss <djmk@google.com>
> ---
>=C2=A0 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |=C2=A0 5 +- >=C2=A0 drivers/gpu/drm/armada/armada_fb.c=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 |=C2=A0 1 +
>=C2=A0 drivers/gpu/drm/drm_crtc_internal.h=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0|=C2=A0 2 +
>=C2=A0 drivers/gpu/drm/drm_fb_cma_helper.c=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0| 11 ++--
>=C2=A0 drivers/gpu/drm/drm_framebuffer.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0| 79 +++++++++++++++++++++++++-
>=C2=A0 drivers/gpu/drm/drm_ioctl.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 1 +
>=C2=A0 drivers/gpu/drm/exynos/exynos_drm_fb.c=C2=A0 =C2=A0 =C2=A0 = |=C2=A0 7 ++-
>=C2=A0 drivers/gpu/drm/gma500/framebuffer.c=C2=A0 =C2=A0 =C2=A0 = =C2=A0 |=C2=A0 2 +
>=C2=A0 drivers/gpu/drm/i915/intel_display.c=C2=A0 =C2=A0 =C2=A0 = =C2=A0 |=C2=A0 1 +
>=C2=A0 drivers/gpu/drm/mediatek/mtk_drm_fb.c=C2=A0 =C2=A0 =C2=A0 = =C2=A0|=C2=A0 1 +
>=C2=A0 drivers/gpu/drm/msm/msm_fb.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 |=C2=A0 5 +-
>=C2=A0 drivers/gpu/drm/nouveau/nouveau_display.c=C2=A0 =C2=A0|=C2= =A0 1 +
>=C2=A0 drivers/gpu/drm/omapdrm/omap_fb.c=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0|=C2=A0 5 +-
>=C2=A0 drivers/gpu/drm/radeon/radeon_display.c=C2=A0 =C2=A0 =C2=A0= |=C2=A0 5 +-
>=C2=A0 drivers/gpu/drm/rockchip/rockchip_drm_fb.c=C2=A0 |=C2=A0 6 = ++-
>=C2=A0 drivers/gpu/drm/tegra/fb.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 9 +++-
>=C2=A0 include/drm/drm_framebuffer.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0|=C2=A0 1 +
>=C2=A0 include/uapi/drm/drm.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 2 +
>=C2=A0 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..67b3be1be= dbc 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -24,6 +24,7 @@
>=C2=A0 #include <drm/drmP.h>
>=C2=A0 #include <drm/drm_auth.h>
>=C2=A0 #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem.h>
>
>=C2=A0 #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,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 void *data, s= truct drm_file *file_priv)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct drm_mode_fb_cmd2 *r =3D data;
> +=C2=A0 =C2=A0 =C2=A0struct drm_framebuffer *fb;
> +=C2=A0 =C2=A0 =C2=A0int ret, i;
> +
> +=C2=A0 =C2=A0 =C2=A0if (!drm_core_check_feature(dev, DRIVER_MODESET))=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> +
> +=C2=A0 =C2=A0 =C2=A0fb =3D drm_framebuffer_lookup(dev, r->fb_id);<= br> > +=C2=A0 =C2=A0 =C2=A0if (!fb)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOENT;
> +
> +=C2=A0 =C2=A0 =C2=A0r->height =3D fb->height;
> +=C2=A0 =C2=A0 =C2=A0r->width =3D fb->width;
> +=C2=A0 =C2=A0 =C2=A0r->pixel_format =3D fb->format->format;<= br> > +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < 4; ++i) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r->pitches[i] =3D = fb->pitches[i];
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r->offsets[i] =3D = fb->offsets[i];
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r->modifier[i] =3D= fb->modifier;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r->handles[i] =3D = 0;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < fb->format->num_planes= ; ++i) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (fb->funcs->= create_handle) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (drm_is_current_master(file_priv) ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0capable(CAP_SYS_ADMIN) ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0drm_is_control_client(file_priv)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D fb->funcs->create_handle(f= b, i,
> file_priv,
> +
> &r->handles[i]);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* GET_FB() is an unprivileged ioctl so = we
> must
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * not return a buffer-handle to non-mas= ter
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * processes! For backwards-compatibilit= y
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * reasons, we cannot make GET_FB()
> privileged,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * so just return an invalid handle for<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * non-masters. */

There's no backward compatibility to handle here, just make= it privileged if
it has to be.

Sure thing, sounds good.

<= /div>

=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r->handles[i] =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D -ENODEV;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0break;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0/* If handle creation failed, delete/dereference = any that were made.
*/
> +=C2=A0 =C2=A0 =C2=A0if (ret) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < = 4; ++i) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (r->handles[i])
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0drm_gem_handle_delete(file_priv, r-=
>handles[i]);

My initial reaction to this was to reply that not all drivers use GE= M, but
after some investigation it seems they actually do. If that's really th= e 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 c= ases, but creating the handle isn't consistent across the drivers.=C2= =A0 I didn't see a clean way to simplify this.

=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0r->handles[i] =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0drm_framebuffer_unreference(fb);
> +
> +=C2=A0 =C2=A0 =C2=A0return ret;
> +}

[snip]

> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index 29dc677dd4d3..a982c72a7= 73e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -90,12 +90,15 @@ struct omap_framebuffer {
>=C2=A0 };
>
>=C2=A0 static int omap_framebuffer_create_handle(struct drm_frameb= uffer *fb,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int format_p= lane_index,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_file = *file_priv,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int *ha= ndle)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct omap_framebuffer *omap_fb =3D to_omap= _framebuffer(fb);
> +=C2=A0 =C2=A0 =C2=A0if (format_plane_index >=3D 4 || !omap_fb->= planes[format_plane_index])

The first check shouldn't be needed, the core should never call = this operation
with an index >=3D 4.

I'll remove the check then.

=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOENT;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return drm_gem_handle_create(file_priv,=
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0omap_fb->planes[0].bo, handle);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0omap_fb->planes[format_plane_index].bo, handle);
>=C2=A0 }

[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" {
>=C2=A0 #define DRM_IOCTL_MODE_CREATEPROPBLOB=C2=A0 =C2=A0 =C2=A0 =C2=A0= DRM_IOWR(0xBD, struct
> drm_mode_create_blob)
> #define DRM_IOCTL_MODE_DESTROYPROPBLOB=C2=A0 =C2=A0 =C2=A0 =C2=A0= DRM_IOWR(0xBE, struct
> drm_mode_destroy_blob)
>
> +#define DRM_IOCTL_MODE_GETFB2=C2=A0 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-fligh= t from last month that used 0xC3 (https://lists.freedesktop.org/archi= ves/amd-gfx/2017-June/010153.html). =C2=A0 Naturally, I'll set this= to whatever is appropriate.

=C2=A0

>=C2=A0 /**
>=C2=A0 =C2=A0* Device specific ioctls should only be in their respectiv= e headers
>=C2=A0 =C2=A0* The device specific ioctl range is from 0x40 to 0x9f.
--
Regards,

Laurent Pinchart

Thanks! =C2=A0

-j

--
<= div class=3D"gmail-m_-3711990965528987190gmail_signature">

Dr. Joe Michael Kniss |=

=C2=A0Google=C2=A0Chr= omeOS=C2=A0|

=C2=A0djmk@google.com=C2=A0=C2= =A0

| <= /a>1-801-898-7977=