Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Fri, 2019-12-13 at 23:36 +0200, Ville Syrjälä wrote:
> On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> > From: Daniel Stone
> >
> > getfb2 allows us to pass multiple planes and modifiers, just like
> > addfb2
> > over addfb.
> >
> > Changes since v1:
> > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> > - update ioctl number
> >
> > Signed-off-by: Daniel Stone
> > Signed-off-by: Juston Li
> > ---
> > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > drivers/gpu/drm/drm_framebuffer.c | 110
> >
> > drivers/gpu/drm/drm_ioctl.c | 1 +
> > include/uapi/drm/drm.h | 2 +
> > 4 files changed, 115 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> > b/drivers/gpu/drm/drm_crtc_internal.h
> > index c7d5e4c21423..16f2413403aa 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c
> > index 57564318ceea..6db54f177443 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
> >
> > out:
> > drm_framebuffer_put(fb);
> > + return ret;
> > +}
> > +
> > +/**
> > + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> > + void *data, struct drm_file *file_priv)
> > +{
> > + struct drm_mode_fb_cmd2 *r = data;
> > + struct drm_framebuffer *fb;
> > + unsigned int i;
> > + int ret;
> > +
> > + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > + return -EINVAL;
> > +
> > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> > + if (!fb)
> > + return -ENOENT;
> > +
> > + /* For multi-plane framebuffers, we require the driver to place
> > the
> > +* GEM objects directly in the drm_framebuffer. For single-
> > plane
> > +* framebuffers, we can fall back to create_handle.
> > +*/
> > + if (!fb->obj[0] &&
> > + (fb->format->num_planes > 1 || !fb->funcs->create_handle))
> > {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + r->height = fb->height;
> > + r->width = fb->width;
> > + r->pixel_format = fb->format->format;
> > +
> > + r->flags = 0;
> > + if (dev->mode_config.allow_fb_modifiers)
> > + r->flags |= DRM_MODE_FB_MODIFIERS;
> > +
> > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> > + r->handles[i] = 0;
> > + r->pitches[i] = 0;
> > + r->offsets[i] = 0;
> > + r->modifier[i] = 0;
> > + }
> >
> > + for (i = 0; i < fb->format->num_planes; i++) {
> > + int j;
> > +
> > + r->pitches[i] = fb->pitches[i];
> > + r->offsets[i] = fb->offsets[i];
> > + if (dev->mode_config.allow_fb_modifiers)
> > + r->modifier[i] = fb->modifier;
> > +
> > + /* If we reuse the same object for multiple planes,
> > also
> > +* return the same handle.
> > +*/
> > + for (j = 0; j < i; j++) {
> > + if (fb->obj[i] == fb->obj[j]) {
> > + r->handles[i] = r->handles[j];
> > + break;
> > + }
> > + }
> > +
> > + if (r->handles[i])
> > + continue;
> > +
> > + if (fb->obj[i]) {
> > + ret = drm_gem_handle_create(file_priv, fb-
> > >obj[i],
> > + >handles[i]);
> > + } else {
> > + WARN_ON(i > 0);
> > + ret = fb->funcs->create_handle(fb, file_priv,
> > + >handles[i]);
> > + }
>
> getfb1 doesn't allow non-master/root to see the handles. Here we
> don't
> seem to have that same protection?
Hmm yeah sorry I missed the protections handling.
I think we can just
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> From: Daniel Stone
>
> getfb2 allows us to pass multiple planes and modifiers, just like addfb2
> over addfb.
>
> Changes since v1:
> - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> - update ioctl number
>
> Signed-off-by: Daniel Stone
> Signed-off-by: Juston Li
> ---
> drivers/gpu/drm/drm_crtc_internal.h | 2 +
> drivers/gpu/drm/drm_framebuffer.c | 110
> drivers/gpu/drm/drm_ioctl.c | 1 +
> include/uapi/drm/drm.h | 2 +
> 4 files changed, 115 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> b/drivers/gpu/drm/drm_crtc_internal.h
> index c7d5e4c21423..16f2413403aa 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> index 57564318ceea..6db54f177443 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
>
> out:
> drm_framebuffer_put(fb);
> + return ret;
> +}
> +
> +/**
> + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + struct drm_mode_fb_cmd2 *r = data;
> + struct drm_framebuffer *fb;
> + unsigned int i;
> + int ret;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> + if (!fb)
> + return -ENOENT;
> +
> + /* For multi-plane framebuffers, we require the driver to place the
> + * GEM objects directly in the drm_framebuffer. For single-plane
> + * framebuffers, we can fall back to create_handle.
> + */
> + if (!fb->obj[0] &&
> + (fb->format->num_planes > 1 || !fb->funcs->create_handle)) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + r->height = fb->height;
> + r->width = fb->width;
> + r->pixel_format = fb->format->format;
> +
> + r->flags = 0;
> + if (dev->mode_config.allow_fb_modifiers)
> + r->flags |= DRM_MODE_FB_MODIFIERS;
> +
> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> + r->handles[i] = 0;
> + r->pitches[i] = 0;
> + r->offsets[i] = 0;
> + r->modifier[i] = 0;
> + }
>
> + for (i = 0; i < fb->format->num_planes; i++) {
> + int j;
> +
> + r->pitches[i] = fb->pitches[i];
> + r->offsets[i] = fb->offsets[i];
> + if (dev->mode_config.allow_fb_modifiers)
> + r->modifier[i] = fb->modifier;
> +
> + /* If we reuse the same object for multiple planes, also
> + * return the same handle.
> + */
> + for (j = 0; j < i; j++) {
> + if (fb->obj[i] == fb->obj[j]) {
> + r->handles[i] = r->handles[j];
> + break;
> + }
> + }
> +
> + if (r->handles[i])
> + continue;
> +
> + if (fb->obj[i]) {
> + ret = drm_gem_handle_create(file_priv, fb->obj[i],
> + >handles[i]);
> + } else {
> + WARN_ON(i > 0);
> + ret = fb->funcs->create_handle(fb, file_priv,
> +>handles[i]);
> + }
getfb1 doesn't allow non-master/root to see the handles. Here we don't
seem to have that same protection?
> +
> + if (ret != 0)
> + goto out;
Could be just 'break;' and then we wouldn't even need the label.
Rest lgtm.
> + }
> +
> +out:
> + if (ret != 0) {
> + /* Delete any previously-created handles on failure. */
> + for (i = 0; i <
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Thu, 2019-11-21 at 10:28 +0100, Daniel Vetter wrote: > On Thu, Nov 21, 2019 at 9:26 AM Timo Aaltonen > wrote: > > On 9.10.2019 18.50, Daniel Vetter wrote: > > > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote: > > > > From: Daniel Stone > > > > > > > > getfb2 allows us to pass multiple planes and modifiers, just > > > > like addfb2 > > > > over addfb. > > > > > > > > Changes since v1: > > > > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID > > > > - update ioctl number > > > > > > > > Signed-off-by: Daniel Stone > > > > Signed-off-by: Juston Li > > > > > > Looks all good from a very quick glance (kernel, libdrm, igt), > > > but where's > > > the userspace? Link to weston/drm_hwc/whatever MR good enough. > > > -Daniel > > > > xserver too > > > > https://lists.x.org/archives/xorg-devel/2018-March/056363.html > > > > to fix > > > > https://gitlab.freedesktop.org/xorg/xserver/issues/33 > > > > which forces Ubuntu to disable CCS compression, and I'd like to get > > rid > > of that patch. > > > > Thanks Juston for pushing this! > > Acked-by: Daniel Vetter > > ... but someone needs to review all the patches and make sure kernel > patch + igt work and pass intel CI and then push it all, and given > the > pile of committers we have that's not me I think. Just in case people > expect me to push this forward, I only jumped in here to make sure > new > uapi is done by the book and checks all the boxes. > -Daniel Thanks for clarifying Daniel, I'll try to find someone to review. Juston ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Thu, Nov 21, 2019 at 9:26 AM Timo Aaltonen wrote: > > On 9.10.2019 18.50, Daniel Vetter wrote: > > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote: > >> From: Daniel Stone > >> > >> getfb2 allows us to pass multiple planes and modifiers, just like addfb2 > >> over addfb. > >> > >> Changes since v1: > >> - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID > >> - update ioctl number > >> > >> Signed-off-by: Daniel Stone > >> Signed-off-by: Juston Li > > > > Looks all good from a very quick glance (kernel, libdrm, igt), but where's > > the userspace? Link to weston/drm_hwc/whatever MR good enough. > > -Daniel > > xserver too > > https://lists.x.org/archives/xorg-devel/2018-March/056363.html > > to fix > > https://gitlab.freedesktop.org/xorg/xserver/issues/33 > > which forces Ubuntu to disable CCS compression, and I'd like to get rid > of that patch. > > Thanks Juston for pushing this! Acked-by: Daniel Vetter ... but someone needs to review all the patches and make sure kernel patch + igt work and pass intel CI and then push it all, and given the pile of committers we have that's not me I think. Just in case people expect me to push this forward, I only jumped in here to make sure new uapi is done by the book and checks all the boxes. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On 9.10.2019 18.50, Daniel Vetter wrote: > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote: >> From: Daniel Stone >> >> getfb2 allows us to pass multiple planes and modifiers, just like addfb2 >> over addfb. >> >> Changes since v1: >> - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID >> - update ioctl number >> >> Signed-off-by: Daniel Stone >> Signed-off-by: Juston Li > > Looks all good from a very quick glance (kernel, libdrm, igt), but where's > the userspace? Link to weston/drm_hwc/whatever MR good enough. > -Daniel xserver too https://lists.x.org/archives/xorg-devel/2018-March/056363.html to fix https://gitlab.freedesktop.org/xorg/xserver/issues/33 which forces Ubuntu to disable CCS compression, and I'd like to get rid of that patch. Thanks Juston for pushing this! -- t ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Mon, Oct 14, 2019 at 1:51 PM Daniel Vetter wrote:
>
> On Mon, Oct 14, 2019 at 6:21 PM Li, Juston wrote:
> >
> > On Wed, 2019-10-09 at 17:50 +0200, Daniel Vetter wrote:
> > > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> > > > From: Daniel Stone
> > > >
> > > > getfb2 allows us to pass multiple planes and modifiers, just like
> > > > addfb2
> > > > over addfb.
> > > >
> > > > Changes since v1:
> > > > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> > > > - update ioctl number
> > > >
> > > > Signed-off-by: Daniel Stone
> > > > Signed-off-by: Juston Li
> > >
> > > Looks all good from a very quick glance (kernel, libdrm, igt), but
> > > where's
> > > the userspace? Link to weston/drm_hwc/whatever MR good enough.
> > > -Daniel
> >
> > My use case is a screenshot utility in chromiuomos that breaks with y-
> > tiled ccs enabled.
> > Review linked is just WIP hack for now since it depends on this
> > merging:
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1815146
>
> Adding Sean & Daniele to confirm this is real cros.
> -Daniel
Yes, this is useful for cros and not only for the screenshot utility.
It is also useful for the transition from the splash screen to Chrome
login without flashing the screen black.
Thank you!
>
> >
> > Thanks
> > Juston
> >
> > > > ---
> > > > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > > > drivers/gpu/drm/drm_framebuffer.c | 110
> > > >
> > > > drivers/gpu/drm/drm_ioctl.c | 1 +
> > > > include/uapi/drm/drm.h | 2 +
> > > > 4 files changed, 115 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> > > > b/drivers/gpu/drm/drm_crtc_internal.h
> > > > index c7d5e4c21423..16f2413403aa 100644
> > > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > > @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> > > > b/drivers/gpu/drm/drm_framebuffer.c
> > > > index 57564318ceea..6db54f177443 100644
> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > @@ -31,6 +31,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
> > > >
> > > > out:
> > > > drm_framebuffer_put(fb);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> > > > + void *data, struct drm_file *file_priv)
> > > > +{
> > > > + struct drm_mode_fb_cmd2 *r = data;
> > > > + struct drm_framebuffer *fb;
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > > + return -EINVAL;
> > > > +
> > > > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> > > > + if (!fb)
> > > > + return -ENOENT;
> > > > +
> > > > + /* For multi-plane framebuffers, we require the driver to place
> > > > the
> > > > +* GEM objects directly in the drm_framebuffer. For single-
> > > > plane
> > > > +* framebuffers, we can fall back to create_handle.
> > > > +*/
> > > > + if (!fb->obj[0] &&
> > > > + (fb->format->num_planes > 1 || !fb->funcs->create_handle))
> > > > {
> > > > + ret = -ENODEV;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + r->height = fb->height;
> > > > + r->width = fb->width;
> > > > + r->pixel_format = fb->format->format;
> > > > +
> > > > + r->flags = 0;
> > > > + if (dev->mode_config.allow_fb_modifiers)
> > > > + r->flags |= DRM_MODE_FB_MODIFIERS;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> > > > + r->handles[i] = 0;
> > > > + r->pitches[i] = 0;
> > > > + r->offsets[i] = 0;
> > > > + r->modifier[i] = 0;
> > > > + }
> > > >
> > > > + for (i = 0; i < fb->format->num_planes; i++) {
> > > > +
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Mon, 2019-10-14 at 19:51 +0200, Daniel Vetter wrote:
> On Mon, Oct 14, 2019 at 6:21 PM Li, Juston
> wrote:
> > On Wed, 2019-10-09 at 17:50 +0200, Daniel Vetter wrote:
> > > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> > > > From: Daniel Stone
> > > >
> > > > getfb2 allows us to pass multiple planes and modifiers, just
> > > > like
> > > > addfb2
> > > > over addfb.
> > > >
> > > > Changes since v1:
> > > > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> > > > - update ioctl number
> > > >
> > > > Signed-off-by: Daniel Stone
> > > > Signed-off-by: Juston Li
> > >
> > > Looks all good from a very quick glance (kernel, libdrm, igt),
> > > but
> > > where's
> > > the userspace? Link to weston/drm_hwc/whatever MR good enough.
> > > -Daniel
> >
> > My use case is a screenshot utility in chromiuomos that breaks with
> > y-
> > tiled ccs enabled.
> > Review linked is just WIP hack for now since it depends on this
> > merging:
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1815146
>
> Adding Sean & Daniele to confirm this is real cros.
> -Daniel
+Mark, who I've been talking about enabling render buffer compression.
This patch allows me to fix a regression w/ screenshot utility when
enabling RBC:
https://chromium-review.googlesource.com/c/chromiumos/platform/minigbm/+/1759155
Thanks
Juston
> > > > ---
> > > > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > > > drivers/gpu/drm/drm_framebuffer.c | 110
> > > >
> > > > drivers/gpu/drm/drm_ioctl.c | 1 +
> > > > include/uapi/drm/drm.h | 2 +
> > > > 4 files changed, 115 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> > > > b/drivers/gpu/drm/drm_crtc_internal.h
> > > > index c7d5e4c21423..16f2413403aa 100644
> > > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > > @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> > > > b/drivers/gpu/drm/drm_framebuffer.c
> > > > index 57564318ceea..6db54f177443 100644
> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > @@ -31,6 +31,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device
> > > > *dev,
> > > >
> > > > out:
> > > > drm_framebuffer_put(fb);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> > > > + void *data, struct drm_file *file_priv)
> > > > +{
> > > > + struct drm_mode_fb_cmd2 *r = data;
> > > > + struct drm_framebuffer *fb;
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > > + return -EINVAL;
> > > > +
> > > > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> > > > + if (!fb)
> > > > + return -ENOENT;
> > > > +
> > > > + /* For multi-plane framebuffers, we require the driver to
> > > > place
> > > > the
> > > > +* GEM objects directly in the drm_framebuffer. For single-
> > > > plane
> > > > +* framebuffers, we can fall back to create_handle.
> > > > +*/
> > > > + if (!fb->obj[0] &&
> > > > + (fb->format->num_planes > 1 || !fb->funcs-
> > > > >create_handle))
> > > > {
> > > > + ret = -ENODEV;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + r->height = fb->height;
> > > > + r->width = fb->width;
> > > > + r->pixel_format = fb->format->format;
> > > > +
> > > > + r->flags = 0;
> > > > + if (dev->mode_config.allow_fb_modifiers)
> > > > + r->flags |= DRM_MODE_FB_MODIFIERS;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> > > > + r->handles[i] = 0;
> > > > + r->pitches[i] = 0;
> > > > + r->offsets[i] = 0;
> > > > + r->modifier[i] = 0;
> > > > +
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Mon, Oct 14, 2019 at 6:21 PM Li, Juston wrote:
>
> On Wed, 2019-10-09 at 17:50 +0200, Daniel Vetter wrote:
> > On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> > > From: Daniel Stone
> > >
> > > getfb2 allows us to pass multiple planes and modifiers, just like
> > > addfb2
> > > over addfb.
> > >
> > > Changes since v1:
> > > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> > > - update ioctl number
> > >
> > > Signed-off-by: Daniel Stone
> > > Signed-off-by: Juston Li
> >
> > Looks all good from a very quick glance (kernel, libdrm, igt), but
> > where's
> > the userspace? Link to weston/drm_hwc/whatever MR good enough.
> > -Daniel
>
> My use case is a screenshot utility in chromiuomos that breaks with y-
> tiled ccs enabled.
> Review linked is just WIP hack for now since it depends on this
> merging:
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1815146
Adding Sean & Daniele to confirm this is real cros.
-Daniel
>
> Thanks
> Juston
>
> > > ---
> > > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > > drivers/gpu/drm/drm_framebuffer.c | 110
> > >
> > > drivers/gpu/drm/drm_ioctl.c | 1 +
> > > include/uapi/drm/drm.h | 2 +
> > > 4 files changed, 115 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> > > b/drivers/gpu/drm/drm_crtc_internal.h
> > > index c7d5e4c21423..16f2413403aa 100644
> > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> > > b/drivers/gpu/drm/drm_framebuffer.c
> > > index 57564318ceea..6db54f177443 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -31,6 +31,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
> > >
> > > out:
> > > drm_framebuffer_put(fb);
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> > > + void *data, struct drm_file *file_priv)
> > > +{
> > > + struct drm_mode_fb_cmd2 *r = data;
> > > + struct drm_framebuffer *fb;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > + return -EINVAL;
> > > +
> > > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> > > + if (!fb)
> > > + return -ENOENT;
> > > +
> > > + /* For multi-plane framebuffers, we require the driver to place
> > > the
> > > +* GEM objects directly in the drm_framebuffer. For single-
> > > plane
> > > +* framebuffers, we can fall back to create_handle.
> > > +*/
> > > + if (!fb->obj[0] &&
> > > + (fb->format->num_planes > 1 || !fb->funcs->create_handle))
> > > {
> > > + ret = -ENODEV;
> > > + goto out;
> > > + }
> > > +
> > > + r->height = fb->height;
> > > + r->width = fb->width;
> > > + r->pixel_format = fb->format->format;
> > > +
> > > + r->flags = 0;
> > > + if (dev->mode_config.allow_fb_modifiers)
> > > + r->flags |= DRM_MODE_FB_MODIFIERS;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> > > + r->handles[i] = 0;
> > > + r->pitches[i] = 0;
> > > + r->offsets[i] = 0;
> > > + r->modifier[i] = 0;
> > > + }
> > >
> > > + for (i = 0; i < fb->format->num_planes; i++) {
> > > + int j;
> > > +
> > > + r->pitches[i] = fb->pitches[i];
> > > + r->offsets[i] = fb->offsets[i];
> > > + if (dev->mode_config.allow_fb_modifiers)
> > > + r->modifier[i] = fb->modifier;
> > > +
> > > + /* If we reuse the same object for multiple planes,
> > > also
> > > +* return the same handle.
> > > +*/
> > > + for (j = 0; j < i; j++) {
> > > + if (fb->obj[i] == fb->obj[j]) {
> > > +
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Wed, 2019-10-09 at 17:50 +0200, Daniel Vetter wrote:
> On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> > From: Daniel Stone
> >
> > getfb2 allows us to pass multiple planes and modifiers, just like
> > addfb2
> > over addfb.
> >
> > Changes since v1:
> > - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> > - update ioctl number
> >
> > Signed-off-by: Daniel Stone
> > Signed-off-by: Juston Li
>
> Looks all good from a very quick glance (kernel, libdrm, igt), but
> where's
> the userspace? Link to weston/drm_hwc/whatever MR good enough.
> -Daniel
My use case is a screenshot utility in chromiuomos that breaks with y-
tiled ccs enabled.
Review linked is just WIP hack for now since it depends on this
merging:
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1815146
Thanks
Juston
> > ---
> > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > drivers/gpu/drm/drm_framebuffer.c | 110
> >
> > drivers/gpu/drm/drm_ioctl.c | 1 +
> > include/uapi/drm/drm.h | 2 +
> > 4 files changed, 115 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> > b/drivers/gpu/drm/drm_crtc_internal.h
> > index c7d5e4c21423..16f2413403aa 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c
> > index 57564318ceea..6db54f177443 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
> >
> > out:
> > drm_framebuffer_put(fb);
> > + return ret;
> > +}
> > +
> > +/**
> > + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> > + void *data, struct drm_file *file_priv)
> > +{
> > + struct drm_mode_fb_cmd2 *r = data;
> > + struct drm_framebuffer *fb;
> > + unsigned int i;
> > + int ret;
> > +
> > + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > + return -EINVAL;
> > +
> > + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> > + if (!fb)
> > + return -ENOENT;
> > +
> > + /* For multi-plane framebuffers, we require the driver to place
> > the
> > +* GEM objects directly in the drm_framebuffer. For single-
> > plane
> > +* framebuffers, we can fall back to create_handle.
> > +*/
> > + if (!fb->obj[0] &&
> > + (fb->format->num_planes > 1 || !fb->funcs->create_handle))
> > {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + r->height = fb->height;
> > + r->width = fb->width;
> > + r->pixel_format = fb->format->format;
> > +
> > + r->flags = 0;
> > + if (dev->mode_config.allow_fb_modifiers)
> > + r->flags |= DRM_MODE_FB_MODIFIERS;
> > +
> > + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> > + r->handles[i] = 0;
> > + r->pitches[i] = 0;
> > + r->offsets[i] = 0;
> > + r->modifier[i] = 0;
> > + }
> >
> > + for (i = 0; i < fb->format->num_planes; i++) {
> > + int j;
> > +
> > + r->pitches[i] = fb->pitches[i];
> > + r->offsets[i] = fb->offsets[i];
> > + if (dev->mode_config.allow_fb_modifiers)
> > + r->modifier[i] = fb->modifier;
> > +
> > + /* If we reuse the same object for multiple planes,
> > also
> > +* return the same handle.
> > +*/
> > + for (j = 0; j < i; j++) {
> > + if (fb->obj[i] == fb->obj[j]) {
> > + r->handles[i] = r->handles[j];
> > + break;
> > + }
> > + }
> > +
> > + if (r->handles[i])
> > + continue;
> > +
> > + if (fb->obj[i]) {
> > + ret = drm_gem_handle_create(file_priv, fb-
> > >obj[i],
> > +
Re: [RESEND PATCH v2] drm: Add getfb2 ioctl
On Thu, Oct 03, 2019 at 11:31:25AM -0700, Juston Li wrote:
> From: Daniel Stone
>
> getfb2 allows us to pass multiple planes and modifiers, just like addfb2
> over addfb.
>
> Changes since v1:
> - unused modifiers set to 0 instead of DRM_FORMAT_MOD_INVALID
> - update ioctl number
>
> Signed-off-by: Daniel Stone
> Signed-off-by: Juston Li
Looks all good from a very quick glance (kernel, libdrm, igt), but where's
the userspace? Link to weston/drm_hwc/whatever MR good enough.
-Daniel
> ---
> drivers/gpu/drm/drm_crtc_internal.h | 2 +
> drivers/gpu/drm/drm_framebuffer.c | 110
> drivers/gpu/drm/drm_ioctl.c | 1 +
> include/uapi/drm/drm.h | 2 +
> 4 files changed, 115 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> b/drivers/gpu/drm/drm_crtc_internal.h
> index c7d5e4c21423..16f2413403aa 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -216,6 +216,8 @@ int drm_mode_rmfb_ioctl(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_ioctl(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_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> index 57564318ceea..6db54f177443 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -548,7 +549,116 @@ int drm_mode_getfb(struct drm_device *dev,
>
> out:
> drm_framebuffer_put(fb);
> + return ret;
> +}
> +
> +/**
> + * drm_mode_getfb2 - get extended 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_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv)
> +{
> + struct drm_mode_fb_cmd2 *r = data;
> + struct drm_framebuffer *fb;
> + unsigned int i;
> + int ret;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + fb = drm_framebuffer_lookup(dev, file_priv, r->fb_id);
> + if (!fb)
> + return -ENOENT;
> +
> + /* For multi-plane framebuffers, we require the driver to place the
> + * GEM objects directly in the drm_framebuffer. For single-plane
> + * framebuffers, we can fall back to create_handle.
> + */
> + if (!fb->obj[0] &&
> + (fb->format->num_planes > 1 || !fb->funcs->create_handle)) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + r->height = fb->height;
> + r->width = fb->width;
> + r->pixel_format = fb->format->format;
> +
> + r->flags = 0;
> + if (dev->mode_config.allow_fb_modifiers)
> + r->flags |= DRM_MODE_FB_MODIFIERS;
> +
> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> + r->handles[i] = 0;
> + r->pitches[i] = 0;
> + r->offsets[i] = 0;
> + r->modifier[i] = 0;
> + }
>
> + for (i = 0; i < fb->format->num_planes; i++) {
> + int j;
> +
> + r->pitches[i] = fb->pitches[i];
> + r->offsets[i] = fb->offsets[i];
> + if (dev->mode_config.allow_fb_modifiers)
> + r->modifier[i] = fb->modifier;
> +
> + /* If we reuse the same object for multiple planes, also
> + * return the same handle.
> + */
> + for (j = 0; j < i; j++) {
> + if (fb->obj[i] == fb->obj[j]) {
> + r->handles[i] = r->handles[j];
> + break;
> + }
> + }
> +
> + if (r->handles[i])
> + continue;
> +
> + if (fb->obj[i]) {
> + ret = drm_gem_handle_create(file_priv, fb->obj[i],
> + >handles[i]);
> + } else {
> + WARN_ON(i > 0);
> + ret = fb->funcs->create_handle(fb, file_priv,
> +>handles[i]);
> + }
> +
> + if (ret != 0)
> + goto out;
> + }
> +
> +out:
> + if (ret != 0) {
> + /* Delete any previously-created handles on failure. */
> + for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
> +
