Re: [RESEND PATCH v2] drm: Add getfb2 ioctl

["Li, Juston" <juston.li@intel.com>]

On Mon, 2019-10-14 at 19:51 +0200, Daniel Vetter wrote:
> On Mon, Oct 14, 2019 at 6:21 PM Li, Juston <juston.li@intel.com>
> 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 <daniels@collabora.com>
> > > > 
> > > > 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 <daniels@collabora.com>
> > > > Signed-off-by: Juston Li <juston.li@intel.com>
> > > 
> > > 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 <drm/drm_file.h>
> > > >  #include <drm/drm_fourcc.h>
> > > >  #include <drm/drm_framebuffer.h>
> > > > +#include <drm/drm_gem.h>
> > > >  #include <drm/drm_print.h>
> > > >  #include <drm/drm_util.h>
> > > > 
> > > > @@ -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],
> > > > +                                               &r-
> > > > >handles[i]);
> > > > +           } else {
> > > > +                   WARN_ON(i > 0);
> > > > +                   ret = fb->funcs->create_handle(fb,
> > > > file_priv,
> > > > +                                                  &r-
> > > > >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++) {
> > > > +                   int j;
> > > > +
> > > > +                   if (r->handles[i])
> > > > +                           drm_gem_handle_delete(file_priv, r-
> > > > > handles[i]);
> > > > +
> > > > +                   /* Zero out any handles identical to the
> > > > one we
> > > > just
> > > > +                    * deleted.
> > > > +                    */
> > > > +                   for (j = i + 1; j < ARRAY_SIZE(r->handles);
> > > > j++) {
> > > > +                           if (r->handles[j] == r->handles[i])
> > > > +                                   r->handles[j] = 0;
> > > > +                   }
> > > > +           }
> > > > +   }
> > > > +
> > > > +   drm_framebuffer_put(fb);
> > > >     return ret;
> > > >  }
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_ioctl.c
> > > > b/drivers/gpu/drm/drm_ioctl.c
> > > > index fcd728d7cf72..b1fafce3ad8c 100644
> > > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > @@ -671,6 +671,7 @@ static const struct drm_ioctl_desc
> > > > drm_ioctls[]
> > > > = {
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY,
> > > > drm_connector_property_set_ioctl, DRM_MASTER),
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB,
> > > > drm_mode_getblob_ioctl, 0),
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, 0),
> > > > +   DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB2, drm_mode_getfb2_ioctl,
> > > > 0),
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb_ioctl,
> > > > 0),
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2_ioctl,
> > > > 0),
> > > >     DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb_ioctl, 0),
> > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > index 8a5b2f8f8eb9..021f33675ba2 100644
> > > > --- a/include/uapi/drm/drm.h
> > > > +++ b/include/uapi/drm/drm.h
> > > > @@ -947,6 +947,8 @@ extern "C" {
> > > >  #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct
> > > > drm_syncobj_transfer)
> > > >  #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL  DRM_IOWR(0xCD,
> > > > struct
> > > > drm_syncobj_timeline_array)
> > > > 
> > > > +#define DRM_IOCTL_MODE_GETFB2              DRM_IOWR(0xCE,
> > > > 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.21.0
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx