[PATCH v3 01/22] drm: Add GEM backed framebuffer library
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 16 20:37:54 UTC 2017
Ni Noralf,
Thank you for the patch.
On Sunday 13 Aug 2017 15:31:44 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>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> Documentation/gpu/drm-kms-helpers.rst | 9 +
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 283 ++++++++++++++++++++++++
> include/drm/drm_framebuffer.h | 7 +
> include/drm/drm_gem_framebuffer_helper.h | 37 ++++
> 5 files changed, 337 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_gem_framebuffer_helper.c
> create mode 100644 include/drm/drm_gem_framebuffer_helper.h
[snip]
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c new file mode 100644
> index 0000000..068a630
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -0,0 +1,283 @@
[snip]
> +/**
> + * DOC: overview
> + *
> + * This library provides helpers for drivers that don't subclass
> + * &drm_framebuffer and and use &drm_gem_object for their backing storage.
s/and and/and/
> + *
> + * Drivers without additional needs to validate framebuffers can simply use
> + * drm_gem_fb_create() and everything is wired up automatically. But all
> + * parts can be used individually.
A sentence should not start by "but". How about "Other drivers can use all
parts independently." ?
> + */
> +
> +/**
> + * drm_gem_fb_get_obj() - Get GEM object for framebuffer
> + * @fb: The framebuffer
> + * @plane: Which plane
> + *
> + * Returns the GEM object for given framebuffer.
You might want to mention that no reference is taken on the GEM object.
> + */
> +struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
> + unsigned int plane)
> +{
> + if (plane >= 4)
> + return NULL;
> +
> + return fb->obj[plane];
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
> +
> +static 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;
i is never negative, you can make it an unsigned int.
> +
> + 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;
> +}
> +
> +/**
> + * drm_gem_fb_destroy - Free GEM backed framebuffer
> + * @fb: DRM framebuffer
> + *
> + * Frees a GEM backed framebuffer with it's backing buffer(s) and the
s/it's/its/
> structure
> + * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
> + * callback.
> + */
> +void drm_gem_fb_destroy(struct drm_framebuffer *fb)
> +{
> + int i;
i is never negative, you can make it an unsigned int.
> +
> + for (i = 0; i < 4; 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
Why does the fb have a DRM and the file a drm ? :-) I would pick one and stick
to it for the whole 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
This looks like data to me, not metadata (data about the data). Same for the
next function.
> + * @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.
> + * The function does buffer size validation.
> + */
> +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;
i is never negative (except in the error path, but see below), you can make it
an unsigned int.
> + 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");
s/GEM/GEM object/ ?
As this is a condition that can easily be triggered by userspace I would make
it a debug message, not an error message.
> + ret = -ENOENT;
> + goto err_gem_object_put;
> + }
> +
> + min_size = (height - 1) * mode_cmd->pitches[i]
> + + width * info->cpp[i]
> + + mode_cmd->offsets[i];
That might be a bit of an overoptimization, I think I would have required the
last line to match the pitch of the rest of the image, but it's too late to
change that as the CMA FB helpers have the same check in place.
> + 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]);
You can write this as
for (; i > 0; i--)
drm_gem_object_put_unlocked(objs[i-1]);
to restrict i to positive values. Up to you.
> +
> + 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,
> +};
Nitpicking, I find it easier to navigate code when the ops structure is
located right after the ops functions. You could just move this one one
function up. Up to you.
> +/**
> + * 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. The function does buffer size
> + * validation. 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
Good English that not be :-)
> + *
> + * This can be used as the &drm_plane_helper_funcs.prepare_fb hook.
> + *
> + * This function checks if the plane FB has an dma-buf attached, extracts
s/has an/has a/
> + * the exclusive fence and attaches it to plane state for the atomic helper
> + * to wait on.
> + *
> + * 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)
No need for the inner parentheses.
> + 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);
> +
> +/**
> + * 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);
[snip]
> diff --git a/include/drm/drm_gem_framebuffer_helper.h
> b/include/drm/drm_gem_framebuffer_helper.h new file mode 100644
> index 0000000..db9cfa0
> --- /dev/null
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -0,0 +1,37 @@
> +#ifndef __DRM_GEM_FB_HELPER_H__
> +#define __DRM_GEM_FB_HELPER_H__
> +
> +struct drm_device;
> +struct drm_file;
> +struct drm_fb_helper_surface_size;
Nitpicking, in alphabetical order drm_fb_helper_surface_size goes before
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);
> +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);
> +
> +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);
> +
> +#endif
--
Regards,
Laurent Pinchart
More information about the dri-devel
mailing list