[PATCH v2 01/22] drm: Add GEM backed framebuffer library
Noralf Trønnes
noralf at tronnes.org
Thu Aug 10 18:55:59 UTC 2017
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 at tronnes.org>
> lgtm, a few nits below to polish the documentation. With those addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
[...]
>> +/**
>> + * 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:
/**
* 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list