* [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers. [not found] ` <CAOvnNvYKXsy9dvOB0ge1WxQbwubPpzKofd=OGagKyft-yQJ1xw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-07-31 18:29 ` Joe Kniss 2017-08-01 12:09 ` Laurent Pinchart 0 siblings, 1 reply; 5+ messages in thread From: Joe Kniss @ 2017-07-31 18:29 UTC (permalink / raw) To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Cc: seanpaul-hpIqsD4AKlfQT0dZR+AlfA, marcheu-hpIqsD4AKlfQT0dZR+AlfA, alexander.deucher-5C7GfCeVMHo, jy0922.shim-Sze3O3UU22JBDgjK7y7TUQ, sw0312.kim-Sze3O3UU22JBDgjK7y7TUQ, kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ, kgene-DgEjT+Ai2ygdnm+yROfE0A, krzk-DgEjT+Ai2ygdnm+yROfE0A, javier-JPH+aEBZ4P+UEJcrhfAQsw, patrik.r.jakobsson-Re5JQEeQqe8AvxtiuMwx3w, ck.hu-NuS5LvNUpcJWk0Htik3J/w, p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w, robdclark-Re5JQEeQqe8AvxtiuMwx3w, bskeggs-H+wXaHxf7aLQT0dZR+AlfA, tomi.valkeinen-l0cyMroinI0, mark.yao-TNX95d0MmH7DzftRWevZcw, heiko-4mtYJXux2i+zQB+pC5nmwQ, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, swarren-3lzwWm7+Weoh9ZMKESR00Q, gnurou-Re5JQEeQqe8AvxtiuMwx3w, funfunctor-dczkZgxz+BNUPWh3PAxdjQ, Andrey.Grodzovsky-5C7GfCeVMHo, laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, nils.wallmenius-Re5JQEeQqe8AvxtiuMwx3w, michel.daenzer-5C7GfCeVMHo, djmk-hpIqsD4AKlfQT0dZR+AlfA, chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA 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 <djmk-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> --- 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, - &fb_cma->obj[0]->base, handle); + &fb_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 <drm/drmP.h> #include <drm/drm_auth.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #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) || drm_is_control_client(file_priv)) { - ret = fb->funcs->create_handle(fb, file_priv, + ret = fb->funcs->create_handle(fb, 0, file_priv, &r->handle); } else { /* GET_FB() is an unprivileged ioctl so we must not @@ -458,6 +459,82 @@ int drm_mode_getfb(struct drm_device *dev, return ret; } +/** + * 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. */ + 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]); + r->handles[i] = 0; + } + } + + drm_framebuffer_unreference(fb); + + return ret; +} + /** * drm_mode_dirtyfb_ioctl - flush frontbuffer rendering on an FB * @dev: drm device for the ioctl diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index a7c61c23685a..a9b578dc5d17 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -627,6 +627,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index c77a5aced81a..5e8b774dc1af 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -88,13 +88,16 @@ static void exynos_drm_fb_destroy(struct drm_framebuffer *fb) } static int exynos_drm_fb_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle) { struct exynos_drm_fb *exynos_fb = to_exynos_fb(fb); - + if (format_plane_index >= MAX_FB_BUFFER || + !exynos_fb->exynos_gem[format_plane_index]) + return -ENOENT; return drm_gem_handle_create(file_priv, - &exynos_fb->exynos_gem[0]->base, handle); + &exynos_fb->exynos_gem[format_plane_index]->base, handle); } static const struct drm_framebuffer_funcs exynos_drm_fb_funcs = { diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index ffe6b4ffa1a8..c221544b4c6a 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -42,6 +42,7 @@ static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb); static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle); @@ -619,6 +620,7 @@ static void psbfb_output_poll_changed(struct drm_device *dev) * the work for us */ static int psb_user_framebuffer_create_handle(struct drm_framebuffer *fb, + unsigned int plane_index, struct drm_file *file_priv, unsigned int *handle) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3282b0f4b134..54fdd30b0598 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15853,6 +15853,7 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) } static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file, unsigned int *handle) { diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c index d4246c9dceae..8343144ba1cd 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c @@ -44,6 +44,7 @@ struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb) } static int mtk_drm_fb_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle) { diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c index 5cf165c9c3a9..de92fc8a7a1c 100644 --- a/drivers/gpu/drm/msm/msm_fb.c +++ b/drivers/gpu/drm/msm/msm_fb.c @@ -30,12 +30,15 @@ struct msm_framebuffer { static int msm_framebuffer_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle) { struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb); + if (format_plane_index >= MAX_PLANE) + return -ENOENT; return drm_gem_handle_create(file_priv, - msm_fb->planes[0], handle); + msm_fb->planes[format_plane_index], handle); } static void msm_framebuffer_destroy(struct drm_framebuffer *fb) diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 72fdba1a1c5d..bc30c0d3d9cd 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -237,6 +237,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb) static int nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle) { 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]) + return -ENOENT; return drm_gem_handle_create(file_priv, - omap_fb->planes[0].bo, handle); + omap_fb->planes[format_plane_index].bo, handle); } static void omap_framebuffer_destroy(struct drm_framebuffer *fb) diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index aea8b62835a4..2188e6341cd9 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1306,8 +1306,9 @@ static void radeon_user_framebuffer_destroy(struct drm_framebuffer *fb) } static int radeon_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 radeon_framebuffer *radeon_fb = to_radeon_framebuffer(fb); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index c9ccdf8f44bb..206d93249519 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -55,13 +55,15 @@ static void rockchip_drm_fb_destroy(struct drm_framebuffer *fb) } static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb, + unsigned int format_plane_index, struct drm_file *file_priv, unsigned int *handle) { struct rockchip_drm_fb *rockchip_fb = to_rockchip_fb(fb); - + if (format_plane_index >= ROCKCHIP_MAX_FB_BUFFER) + return -ENOENT; return drm_gem_handle_create(file_priv, - rockchip_fb->obj[0], handle); + rockchip_fb->obj[format_plane_index], handle); } static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb, diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c index f142f6a4db25..ff206b143503 100644 --- a/drivers/gpu/drm/tegra/fb.c +++ b/drivers/gpu/drm/tegra/fb.c @@ -81,11 +81,16 @@ static void tegra_fb_destroy(struct drm_framebuffer *framebuffer) } static int tegra_fb_create_handle(struct drm_framebuffer *framebuffer, + unsigned int format_plane_index, struct drm_file *file, unsigned int *handle) { struct tegra_fb *fb = to_tegra_fb(framebuffer); - - return drm_gem_handle_create(file, &fb->planes[0]->gem, handle); + if (format_plane_index >= fb->num_planes || + !fb->planes[format_plane_index]) + return -ENOENT; + return drm_gem_handle_create(file, + &fb->planes[format_plane_index]->gem, + handle); } static const struct drm_framebuffer_funcs tegra_fb_funcs = { diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index dd1e3e99dcff..2fb398cb4646 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -66,6 +66,7 @@ struct drm_framebuffer_funcs { * 0 on success or a negative error code on failure. */ int (*create_handle)(struct drm_framebuffer *fb, + unsigned int plane_index, struct drm_file *file_priv, unsigned int *handle); /** 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) + /** * Device specific ioctls should only be in their respective headers * The device specific ioctl range is from 0x40 to 0x9f. -- 2.14.0.rc0.400.g1c36432dff-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers. 2017-07-31 18:29 ` [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers Joe Kniss @ 2017-08-01 12:09 ` Laurent Pinchart 2017-08-01 17:24 ` Joe Kniss 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2017-08-01 12:09 UTC (permalink / raw) To: Joe Kniss Cc: amd-gfx, heiko, seanpaul, michel.daenzer, dri-devel, linux-tegra, thierry.reding, krzk, djmk, gnurou, linux-samsung-soc, jy0922.shim, linux-rockchip, kyungmin.park, javier, tomi.valkeinen, bskeggs, nouveau, ck.hu, Andrey.Grodzovsky, swarren, linux-arm-msm, intel-gfx, linux-mediatek, matthias.bgg, linux-arm-kernel, mark.yao, marcheu, nils.wallmenius, freedreno, sw0312.kim, linux-kernel, kgene, p.zabel 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 <djmk@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. > + 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. > + 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. > + 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. > /** > * Device specific ioctls should only be in their respective headers > * The device specific ioctl range is from 0x40 to 0x9f. -- Regards, Laurent Pinchart _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers. 2017-08-01 12:09 ` Laurent Pinchart @ 2017-08-01 17:24 ` Joe Kniss [not found] ` <CAOvnNvZoHv8prg2R57cgGx5w7h69=X3hQGrpP5iXaLujhOfzYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Joe Kniss @ 2017-08-01 17:24 UTC (permalink / raw) To: Laurent Pinchart Cc: amd-gfx, heiko, Sean Paul, michel.daenzer, dri-devel, thierry.reding, krzk, gnurou, linux-samsung-soc, jy0922.shim, linux-rockchip, kyungmin.park, javier, tomi.valkeinen, bskeggs, nouveau, ck.hu, Andrey.Grodzovsky, Joe Kniss, swarren, linux-arm-msm, intel-gfx, lin, linux-mediatek, matthias.bgg, linux-arm-kernel, mark.yao, Stéphane Marchesin, nils.wallmenius, freedreno, sw0312.kim [-- Attachment #1.1: Type: text/plain, Size: 9806 bytes --] 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@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@google.com | 1-801-898-7977 <(801)%20898-7977> [-- Attachment #1.2: Type: text/html, Size: 15140 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAOvnNvZoHv8prg2R57cgGx5w7h69=X3hQGrpP5iXaLujhOfzYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers. [not found] ` <CAOvnNvZoHv8prg2R57cgGx5w7h69=X3hQGrpP5iXaLujhOfzYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-08-01 20:46 ` Laurent Pinchart 2017-08-02 0:12 ` Joe Kniss 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2017-08-01 20:46 UTC (permalink / raw) To: Joe Kniss Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, heiko-4mtYJXux2i+zQB+pC5nmwQ, Sean Paul, michel.daenzer-5C7GfCeVMHo, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, krzk-DgEjT+Ai2ygdnm+yROfE0A, gnurou-Re5JQEeQqe8AvxtiuMwx3w, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, jy0922.shim-Sze3O3UU22JBDgjK7y7TUQ, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ, patrik.r.jakobsson-Re5JQEeQqe8AvxtiuMwx3w, javier-JPH+aEBZ4P+UEJcrhfAQsw, tomi.valkeinen-l0cyMroinI0, bskeggs-H+wXaHxf7aLQT0dZR+AlfA, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, ck.hu-NuS5LvNUpcJWk0Htik3J/w, Andrey.Grodzovsky-5C7GfCeVMHo, Joe Kniss, swarren-3lzwWm7+Weoh9ZMKESR00Q, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, lin, linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, mark.yao-TNX95d0MmH7DzftRWevZcw, Stéphane Marchesin, nils.wallmenius-Re5JQEeQqe8AvxtiuMwx3w 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@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 _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers. 2017-08-01 20:46 ` Laurent Pinchart @ 2017-08-02 0:12 ` Joe Kniss 0 siblings, 0 replies; 5+ messages in thread From: Joe Kniss @ 2017-08-02 0:12 UTC (permalink / raw) To: Laurent Pinchart Cc: amd-gfx, Heiko Stübner, Sean Paul, michel.daenzer, dri-devel, Thierry Reding, krzk, Alexandre Courbot, linux-samsung-soc, jy0922.shim, linux-rockchip, Kyungmin Park, javier, tomi.valkeinen, bskeggs, CK HU, Andrey.Grodzovsky, Joe Kniss, Stephen Warren, linux-arm-msm, intel-gfx, lin, linux-mediatek, Matthias Brugger, linux-arm-kernel, long jacob [-- Attachment #1.1: Type: text/plain, Size: 10870 bytes --] On Tue, Aug 1, 2017 at 1:46 PM, Laurent Pinchart < laurent.pinchart@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: > > > > <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 ? > > 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 <djmk@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 :-) > > I am sure this would be fine, I'd be happy to get rid of it. I'll put together a separate patch for this. > > >> + 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. > > Will do. > > > > /** > > > > > > > > * Device specific ioctls should only be in their respective headers > > > > * The device specific ioctl range is from 0x40 to 0x9f. > > -- > Regards, > > Laurent Pinchart > > I'll respond with an updated patch ASAP. -- Dr. Joe Michael Kniss | Google ChromeOS | djmk@google.com | 1-801-898-7977 <(801)%20898-7977> [-- Attachment #1.2: Type: text/html, Size: 16939 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-02 0:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAOvnNvYKXsy9dvOB0ge1WxQbwubPpzKofd=OGagKyft-yQJ1xw@mail.gmail.com>
[not found] ` <CAOvnNvYKXsy9dvOB0ge1WxQbwubPpzKofd=OGagKyft-yQJ1xw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-07-31 18:29 ` [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers Joe Kniss
2017-08-01 12:09 ` Laurent Pinchart
2017-08-01 17:24 ` Joe Kniss
[not found] ` <CAOvnNvZoHv8prg2R57cgGx5w7h69=X3hQGrpP5iXaLujhOfzYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-08-01 20:46 ` Laurent Pinchart
2017-08-02 0:12 ` Joe Kniss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox