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>
> ---
> Documentation/gpu/drm-kms-helpers.rst | 9 +
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 252 +++++++++++++++++++++++++++
> include/drm/drm_framebuffer.h | 4 +
> include/drm/drm_gem_framebuffer_helper.h | 35 ++++
> 5 files changed, 301 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_gem_framebuffer_helper.c
> create mode 100644 include/drm/drm_gem_framebuffer_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 7c5e254..13dd237 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -296,3 +296,12 @@ Auxiliary Modeset Helpers
>
> .. kernel-doc:: drivers/gpu/drm/drm_modeset_helper.c
> :export:
> +
> +Framebuffer GEM Helper Reference
> +================================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_framebuffer_helper.c
> + :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_framebuffer_helper.c
> + :export:
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 24a066e..a8acc19 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,7 +33,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> drm_simple_kms_helper.o drm_modeset_helper.o \
> - drm_scdc_helper.o
> + drm_scdc_helper.o drm_gem_framebuffer_helper.o
>
> drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> 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..41a506c
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -0,0 +1,252 @@
> +/*
> + * drm gem framebuffer helper functions
> + *
> + * Copyright (C) 2017 Noralf Trønnes
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <linux/dma-fence.h>
> +#include <linux/reservation.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_modeset_helper.h>
> +
> +/**
> + * DOC: overview
> + *
> + * This library provides helpers for drivers that don't subclass
> + * &drm_framebuffer and are backed by &drm_gem_object.
I'd rephrase this as "and use &drm_gem_object for their backing storage".
Maybe also drop a hint for the main entry point for drivers into this
library:
"Drivers without addition needs to validate framebuffers can simply use
drm_gem_fb_create() and everything is wired up automatically. But all
parts can be used individually, too."
> + */
> +
> +/**
> + * drm_gem_fb_get_obj() - Get GEM object for framebuffer
> + * @fb: The framebuffer
> + * @plane: Which plane
> + *
> + * Returns the GEM object for given framebuffer.
> + */
> +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);
> +
> +/**
> + * 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.
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
