Re: [Intel-gfx] [RESEND PATCH v2] drm: Add getfb2 ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 13, 2019 at 10:52:03PM +0000, Li, Juston wrote:
> 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 <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>
> > > ---
> > >  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]);
> > > +		}
> > 
> > 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 set the getfb2 ioctl flags as
> DRM_MASTER|DRM_ROOT_ONLY

Since this was missed, can you pls extend the igt to test for this? Iirc
the getfb igt makes sure non-root doesn't get this ...

Also for consistency I think doing the same as getfb would be good. Fewer
surprises.
-Daniel

> 
> > > +
> > > +		if (ret != 0)
> > > +			goto out;
> > 
> > Could be just 'break;' and then we wouldn't even need the label.
> 
> Will do.
> 
> > 
> > Rest lgtm.
> > 
> 
> Much appreciated
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel