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
