Re: [PATCH v2 01/22] drm: Add GEM backed framebuffer library

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 10, 2017 at 08:55:59PM +0200, Noralf Trønnes wrote:
> 
> Den 09.08.2017 22.04, skrev Daniel Vetter:
> > On Wed, Aug 09, 2017 at 12:11:04PM +0200, Noralf Trønnes wrote:
> > > This library provides helpers for drivers that don't subclass
> > > drm_framebuffer and are backed by drm_gem_object. The code is
> > > taken from drm_fb_cma_helper.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > lgtm, a few nits below to polish the documentation. With those addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> 
> [...]
> 
> > > +/**
> > > + * drm_gem_fb_alloc - Allocate GEM backed framebuffer
> > > + * @dev: DRM device
> > > + * @mode_cmd: metadata from the userspace fb creation request
> > > + * @obj: GEM object(s) backing the framebuffer
> > > + * @num_planes: Number of planes
> > > + * @funcs: vtable to be used for the new framebuffer object
> > This kinda puts all the warnings from drm_framebuffer_init() under the
> > rug, so not enough text for such a tricky function. Proposal
> > 
> > "This allocates a &struct drm_framebuffer and publishes it by calling
> > drm_framebuffer_init().
> > 
> > IMPORTANT:
> > All checking and validation must be done before calling this function.
> > Framebuffers are supposed to be invariant over their lifetime, which means
> > evil userspace could otherwise trick the kernel into using unvalidated
> > framebuffer objects."
> > 
> > Not sure whether that kills your use-cases for this, in which case we
> > probably need to open-code this in all callers. Or remove the call to
> > drm_framebuffer_init() and place the driver's validation code between the
> > call to _alloc() and _init().
> > 
> > Maybe we should even change _init() to be called _register() to avoid this
> > bug in the future.
> 
> The only reason drm_gem_fb_alloc() is exported is to add framebuffers
> for fbdev emulation. So maybe it's better to hide that function and make
> an explicit one:

Yeah if you can just use this one here instead of the other, that looks
like the safer interface.

Aside: Do we really need to export this if it's only used for fbdev
allocation? Or is your plan that drivers will call this (like the cma
helpers already do)?

Anyway, my r-b stands if you add this function and unexport the alloc one
(and then I don't think it needs kerneldoc).
-Daniel

> 
> /**
>  * drm_gem_fbdev_fb_create - Create a drm_framebuffer for fbdev emulation
>  * @dev: DRM device
>  * @sizes: fbdev size description
>  * @pitch_align: optional pitch alignment
>  * @obj: GEM object backing the framebuffer
>  * @funcs: vtable to be used for the new framebuffer object
>  *
>  * This function creates a framebuffer for use with fbdev emulation.
>  *
>  * Returns:
>  * Pointer to a drm_framebuffer on success or an error pointer on failure.
>  */
> struct drm_framebuffer *
> drm_gem_fbdev_fb_create(struct drm_device *dev,
>             struct drm_fb_helper_surface_size *sizes,
>             unsigned int pitch_align, struct drm_gem_object *obj,
>             const struct drm_framebuffer_funcs *funcs)
> {
>     struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> 
>     mode_cmd.width = sizes->surface_width;
>     mode_cmd.height = sizes->surface_height;
>     mode_cmd.pitches[0] = sizes->surface_width *
>                   DIV_ROUND_UP(sizes->surface_bpp, 8);
>     if (pitch_align)
>         mode_cmd.pitches[0] = roundup(mode_cmd.pitches[0],
>                           pitch_align);
>     mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>                             sizes->surface_depth);
>     if (obj->size < mode_cmd.pitches[0] * mode_cmd.height)
>         return ERR_PTR(-EINVAL);
> 
>     return drm_gem_fb_alloc(dev, &mode_cmd, &obj, 1, funcs);
> }
> EXPORT_SYMBOL(drm_gem_fbdev_fb_create);
> 
> 
> Noralf.
> 
> > But that's just all aside, from your create_with_funcs example below it
> > all looks good.
> > 
> > > + *
> > > + * Returns:
> > > + * Allocated struct drm_framebuffer * or error encoded pointer.
> > > + */
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_alloc(struct drm_device *dev,
> > > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > > +		 const struct drm_framebuffer_funcs *funcs)
> > > +{
> > > +	struct drm_framebuffer *fb;
> > > +	int ret, i;
> > > +
> > > +	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> > > +	if (!fb)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
> > > +
> > > +	for (i = 0; i < num_planes; i++)
> > > +		fb->obj[i] = obj[i];
> > > +
> > > +	ret = drm_framebuffer_init(dev, fb, funcs);
> > > +	if (ret) {
> > > +		DRM_DEV_ERROR(dev->dev, "Failed to init framebuffer: %d\n",
> > > +			      ret);
> > > +		kfree(fb);
> > > +		return ERR_PTR(ret);
> > > +	}
> > > +
> > > +	return fb;
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_fb_alloc);
> > > +
> > > +/**
> > > + * drm_gem_fb_destroy - Free GEM backed framebuffer
> > > + * @fb: DRM framebuffer
> > > + *
> > > + * Frees a GEM backed framebuffer with it's backing buffer(s) and the structure
> > > + * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
> > > + * callback.
> > > + */
> > > +void drm_gem_fb_destroy(struct drm_framebuffer *fb)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < 4; i++) {
> > > +		if (fb->obj[i])
> > > +			drm_gem_object_put_unlocked(fb->obj[i]);
> > > +	}
> > > +
> > > +	drm_framebuffer_cleanup(fb);
> > > +	kfree(fb);
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_fb_destroy);
> > > +
> > > +/**
> > > + * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer
> > > + * @fb: DRM framebuffer
> > > + * @file: drm file
> > > + * @handle: handle created
> > > + *
> > > + * Drivers can use this as their &drm_framebuffer_funcs->create_handle
> > > + * callback.
> > > + *
> > > + * Returns:
> > > + * 0 on success or a negative error code on failure.
> > > + */
> > > +int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> > > +			     unsigned int *handle)
> > > +{
> > > +	return drm_gem_handle_create(file, fb->obj[0], handle);
> > > +}
> > > +EXPORT_SYMBOL(drm_gem_fb_create_handle);
> > > +
> > > +/**
> > > + * drm_gem_fb_create_with_funcs() - helper function for the
> > > + *                                  &drm_mode_config_funcs.fb_create
> > > + *                                  callback
> > > + * @dev: DRM device
> > > + * @file: drm file for the ioctl call
> > > + * @mode_cmd: metadata from the userspace fb creation request
> > > + * @funcs: vtable to be used for the new framebuffer object
> > > + *
> > > + * This can be used to set &drm_framebuffer_funcs for drivers that need the
> > > + * &drm_framebuffer_funcs.dirty callback. Use drm_gem_fb_create() if you don't
> > > + * need to change &drm_framebuffer_funcs.
> > Maybe mention here and below that this already validates against the
> > buffer size (which is what most drivers need to do beyond the basic checks
> > the drm core already does).
> > 
> > One thing this doesn't do is validate the pixel format and modifiers. We
> > should only accept framebuffers which can be used for this driver (on any
> > drm_plane). Not sure this is something current drivers get right even, and
> > maybe we should try to check that in the core perhaps ... So different
> > patch series, just something that crossed my mind.
> > 
> > > + */
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +			     const struct drm_framebuffer_funcs *funcs)
> > > +{
> > > +	const struct drm_format_info *info;
> > > +	struct drm_gem_object *objs[4];
> > > +	struct drm_framebuffer *fb;
> > > +	int ret, i;
> > > +
> > > +	info = drm_get_format_info(dev, mode_cmd);
> > > +	if (!info)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	for (i = 0; i < info->num_planes; i++) {
> > > +		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> > > +		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> > > +		unsigned int min_size;
> > > +
> > > +		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
> > > +		if (!objs[i]) {
> > > +			DRM_DEV_ERROR(dev->dev, "Failed to lookup GEM\n");
> > > +			ret = -ENOENT;
> > > +			goto err_gem_object_put;
> > > +		}
> > > +
> > > +		min_size = (height - 1) * mode_cmd->pitches[i]
> > > +			 + width * info->cpp[i]
> > > +			 + mode_cmd->offsets[i];
> > > +
> > > +		if (objs[i]->size < min_size) {
> > > +			drm_gem_object_put_unlocked(objs[i]);
> > > +			ret = -EINVAL;
> > > +			goto err_gem_object_put;
> > > +		}
> > > +	}
> > > +
> > > +	fb = drm_gem_fb_alloc(dev, mode_cmd, objs, i, funcs);
> > > +	if (IS_ERR(fb)) {
> > > +		ret = PTR_ERR(fb);
> > > +		goto err_gem_object_put;
> > > +	}
> > > +
> > > +	return fb;
> > > +
> > > +err_gem_object_put:
> > > +	for (i--; i >= 0; i--)
> > > +		drm_gem_object_put_unlocked(objs[i]);
> > > +
> > > +	return ERR_PTR(ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_funcs);
> > > +
> > > +static const struct drm_framebuffer_funcs drm_gem_fb_funcs = {
> > > +	.destroy	= drm_gem_fb_destroy,
> > > +	.create_handle	= drm_gem_fb_create_handle,
> > > +};
> > > +
> > > +/**
> > > + * drm_gem_fb_create() - &drm_mode_config_funcs.fb_create callback function
> > > + * @dev: DRM device
> > > + * @file: drm file for the ioctl call
> > > + * @mode_cmd: metadata from the userspace fb creation request
> > > + *
> > > + * If your hardware has special alignment or pitch requirements these should be
> > > + * checked before calling this function. Use drm_gem_fb_create_with_funcs() if
> > > + * you need to set &drm_framebuffer_funcs.dirty.
> > > + */
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
> > > +		  const struct drm_mode_fb_cmd2 *mode_cmd)
> > > +{
> > > +	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> > > +					    &drm_gem_fb_funcs);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_create);
> > > +
> > > +/**
> > > + * drm_gem_fb_prepare_fb() - Prepare gem framebuffer
> > > + * @plane: Which plane
> > > + * @state: Plane state attach fence to
> > > + *
> > > + * This should be set as the &struct drm_plane_helper_funcs.prepare_fb hook.
> > We tend to not use the struct label when referencing members, so just
> > &drm_plane_helper_funcs.prepare_fb hook.
> > 
> > > + *
> > > + * This function checks if the plane FB has an dma-buf attached, extracts
> > > + * the exclusive fence and attaches it to plane state for the atomic helper
> > > + * to wait on.
> > > + *
> > > + * There is no need for cleanup_fb for gem based framebuffer drivers.
> > A bit too simple:
> > 
> > "There is no need for &drm_plane_helper_funcs.cleanup_fb hook for simple
> > gem based framebuffer drivers which have their buffers always pinned in
> > memory."
> > 
> > 
> > > + */
> > > +int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> > > +			  struct drm_plane_state *state)
> > > +{
> > > +	struct dma_buf *dma_buf;
> > > +	struct dma_fence *fence;
> > > +
> > > +	if ((plane->state->fb == state->fb) || !state->fb)
> > > +		return 0;
> > > +
> > > +	dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
> > > +	if (dma_buf) {
> > > +		fence = reservation_object_get_excl_rcu(dma_buf->resv);
> > > +		drm_atomic_set_fence_for_plane(state, fence);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_fb_prepare_fb);
> > > diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> > > index 5244f05..50def22 100644
> > > --- a/include/drm/drm_framebuffer.h
> > > +++ b/include/drm/drm_framebuffer.h
> > > @@ -190,6 +190,10 @@ struct drm_framebuffer {
> > >   	 * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock.
> > >   	 */
> > >   	struct list_head filp_head;
> > > +	/**
> > > +	 * @obj: GEM objects backing the framebuffer, one per plane (optional).
> > I'd elaborate a bit more:
> > 
> > "This is used by the GEM framebuffer helpers, see e.g.
> > drm_gem_fb_create()."
> > 
> > > +	 */
> > > +	struct drm_gem_object *obj[4];
> > >   };
> > >   #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
> > > diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> > > new file mode 100644
> > > index 0000000..af3d1ee
> > > --- /dev/null
> > > +++ b/include/drm/drm_gem_framebuffer_helper.h
> > > @@ -0,0 +1,35 @@
> > > +#ifndef __DRM_GEM_FB_HELPER_H__
> > > +#define __DRM_GEM_FB_HELPER_H__
> > > +
> > > +struct drm_device;
> > > +struct drm_file;
> > > +struct drm_framebuffer;
> > > +struct drm_framebuffer_funcs;
> > > +struct drm_gem_object;
> > > +struct drm_mode_fb_cmd2;
> > > +struct drm_plane;
> > > +struct drm_plane_state;
> > > +
> > > +struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> > > +					  unsigned int plane);
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_alloc(struct drm_device *dev,
> > > +		 const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +		 struct drm_gem_object **obj, unsigned int num_planes,
> > > +		 const struct drm_framebuffer_funcs *funcs);
> > > +void drm_gem_fb_destroy(struct drm_framebuffer *fb);
> > > +int drm_gem_fb_create_handle(struct drm_framebuffer *fb, struct drm_file *file,
> > > +			     unsigned int *handle);
> > > +
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> > > +			     const struct drm_mode_fb_cmd2 *mode_cmd,
> > > +			     const struct drm_framebuffer_funcs *funcs);
> > > +struct drm_framebuffer *
> > > +drm_gem_fb_create(struct drm_device *dev, struct drm_file *file,
> > > +		  const struct drm_mode_fb_cmd2 *mode_cmd);
> > > +
> > > +int drm_gem_fb_prepare_fb(struct drm_plane *plane,
> > > +			  struct drm_plane_state *state);
> > > +
> > > +#endif
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux