(was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
Functions in the atomic commit tail are not allowed to acquire the dmabuf's reservation lock. So we cannot legally call the GEM object's vmap functionality in atomic_update.
But, much better, we can use drm_shadow_plane_state to do all the mapping for us. Patch 1 exports the helpers for shadow-buffered planes from the DRM KMS helper library and adds documentation on how to use them. Patch 2 replaces the vmap code in vbox' cursor update code with a the helpers for shadow-buffered planes.
Thomas Zimmermann (2): drm/gem: Export helpers for shadow-buffered planes drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 ++--- include/drm/drm_gem_atomic_helper.h | 32 +++++ 3 files changed, 181 insertions(+), 27 deletions(-)
-- 2.30.0
Export the helpers for shadow-buffered planes. These will be used by several drivers.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- include/drm/drm_gem_atomic_helper.h | 32 +++++ 2 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e27762cef360..79b4d3f0495a 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -14,13 +14,101 @@ * functions for drivers that use GEM objects. Currently, it provides * plane state and framebuffer BO mappings for planes with shadow * buffers. + * + * A driver using a shadow buffer copies the content of the shadow buffers + * into the HW's framebuffer memory during an atomic update. This requires + * a mapping of the shadow buffer into kernel address space. The mappings + * cannot be established by commit-tail functions, such as atomic_update, + * as this would violate locking rules vmap. + * + * The helpers for shadow-buffered planes establish and release mappings, + * and provide struct drm_shadow_plane_state, which stores the plane's mapping + * for commit-tail functons. + * + * Shadow-buffered planes can easily be enabled by using the provided macros + * DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS. + * These macros set up the plane and plane-helper callbacks to point to the + * shadow-buffer helpers. + * + * .. code-block:: c + * + * #include <drm/drm/gem_atomic_helper.h> + * + * struct drm_plane_funcs driver_plane_funcs = { + * ..., + * DRM_GEM_SHADOW_PLANE_FUNCS, + * }; + * + * struct drm_plane_helper_funcs driver_plane_helper_funcs = { + * ..., + * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, + * }; + * + * In the driver's atomic-update function, shadow-buffer mappings are available + * from the plane state. Use to_drm_shadow_plane_state() to upcast from + * struct drm_plane_state. + * + * .. code-block:: c + * + * void driver_plane_atomic_update(struct drm_plane *plane, + * struct drm_plane_state *old_plane_state) + * { + * struct drm_plane_state *plane_state = plane->state; + * struct drm_shadow_plane_state *shadow_plane_state = + * to_drm_shadow_plane_state(plane_state); + * + * // access shadow buffer via shadow_plane_state->map + * } + * + * A mapping address for each of the framebuffer's buffer object is stored in + * struct drm_shadow_plane_state.map. The mappings are valid while the state + * is being used. + * + * Drivers that use struct drm_simple_display_pipe can use + * DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp + * callbacks. Access to shadow-buffer mappings is similar to regular + * atomic_update. + * + * .. code-block:: c + * + * struct drm_simple_display_pipe_funcs driver_pipe_funcs = { + * ..., + * DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS, + * }; + * + * void driver_pipe_enable(struct drm_simple_display_pipe *pipe, + * struct drm_crtc_state *crtc_state, + * struct drm_plane_state *plane_state) + * { + * struct drm_shadow_plane_state *shadow_plane_state = + * to_drm_shadow_plane_state(plane_state); + * + * // access shadow buffer via shadow_plane_state->map + * } */
/* * Shadow-buffered Planes */
-static struct drm_plane_state * +/** + * drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state + * @plane: the plane + * + * This function implements struct drm_plane_funcs.atomic_duplicate_state for + * shadow-buffered planes. It assumes the existing state to be of type + * struct drm_shadow_plane_state and it allocates the new state to be of this + * type. + * + * The function does not duplicate existing mappings of the shadow buffers. + * Mappings are maintained during the atomic commit by the plane's prepare_fb + * and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb() + * for corresponding helpers. + * + * Returns: + * A pointer to a new plane state on success, or NULL otherwise. + */ +struct drm_plane_state * drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane) { struct drm_plane_state *plane_state = plane->state; @@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
return &new_shadow_plane_state->base; } +EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
-static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, - struct drm_plane_state *plane_state) +/** + * drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state + * @plane: the plane + * @plane_state: the plane state of type struct drm_shadow_plane_state + * + * This function implements struct drm_plane_funcs.atomic_destroy_state + * for shadow-buffered planes. It expects that mappings of shadow buffers + * have been released already. + */ +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, + struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); @@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, __drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base); kfree(shadow_plane_state); } +EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
-static void drm_gem_reset_shadow_plane(struct drm_plane *plane) +/** + * drm_gem_reset_shadow_plane - resets a shadow-buffered plane + * @plane: the plane + * + * This function implements struct drm_plane_funcs.reset_plane for + * shadow-buffered planes. It assumes the current plane state to be + * of type struct drm_shadow_plane and it allocates the new state of + * this type. + */ +void drm_gem_reset_shadow_plane(struct drm_plane *plane) { struct drm_shadow_plane_state *shadow_plane_state;
@@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane) return; __drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base); } +EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
-static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/** + * drm_gem_prepare_shadow_fb - prepares shadow framebuffers + * @plane: the plane + * @plane_state: the plane state of type struct drm_shadow_plane_state + * + * This function implements struct drm_plane_helper_funcs.prepare_fb. It + * maps all buffer objects of the plane's framebuffer into kernel address + * space and stores them in struct drm_shadow_plane_state.map. The + * framebuffer will be synchronized as part of the atomic commit. + * + * See drm_gem_cleanup_shadow_fb() for cleanup. + * + * Returns: + * 0 on success, or a negative errno code otherwise. + */ +int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s } return ret; } +EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
-static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/** + * drm_gem_cleanup_shadow_fb - releases shadow framebuffers + * @plane: the plane + * @plane_state: the plane state of type struct drm_shadow_plane_state + * + * This function implements struct drm_plane_helper_funcs.cleanup_fb. + * This function unmaps all buffer objects of the plane's framebuffer. + * + * See drm_gem_prepare_shadow_fb() for more inforamtion. + */ +void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_ drm_gem_vunmap(obj, &shadow_plane_state->map[i]); } } +EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
/** * drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h index 08b96ccea325..7abf40bdab3d 100644 --- a/include/drm/drm_gem_atomic_helper.h +++ b/include/drm/drm_gem_atomic_helper.h @@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state) return container_of(state, struct drm_shadow_plane_state, base); }
+void drm_gem_reset_shadow_plane(struct drm_plane *plane); +struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane); +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, + struct drm_plane_state *plane_state); + +/** + * DRM_GEM_SHADOW_PLANE_FUNCS - + * Initializes struct drm_plane_funcs for shadow-buffered planes + * + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This + * macro initializes struct drm_plane_funcs to use the rsp helper functions. + */ +#define DRM_GEM_SHADOW_PLANE_FUNCS \ + .reset = drm_gem_reset_shadow_plane, \ + .atomic_duplicate_state = drm_gem_duplicate_shadow_plane_state, \ + .atomic_destroy_state = drm_gem_destroy_shadow_plane_state + +int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state); +void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state); + +/** + * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS - + * Initializes struct drm_plane_helper_funcs for shadow-buffered planes + * + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This + * macro initializes struct drm_plane_helper_funcs to use the rsp helper + * functions. + */ +#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \ + .prepare_fb = drm_gem_prepare_shadow_fb, \ + .cleanup_fb = drm_gem_cleanup_shadow_fb + int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe, struct drm_plane_state *plane_state); void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
On Mon, Feb 08, 2021 at 02:50:43PM +0100, Thomas Zimmermann wrote:
Export the helpers for shadow-buffered planes. These will be used by several drivers.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- include/drm/drm_gem_atomic_helper.h | 32 +++++ 2 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e27762cef360..79b4d3f0495a 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -14,13 +14,101 @@
- functions for drivers that use GEM objects. Currently, it provides
- plane state and framebuffer BO mappings for planes with shadow
- buffers.
- A driver using a shadow buffer copies the content of the shadow buffers
- into the HW's framebuffer memory during an atomic update. This requires
- a mapping of the shadow buffer into kernel address space. The mappings
- cannot be established by commit-tail functions, such as atomic_update,
- as this would violate locking rules vmap.
"... locking rules around dma_buf_vmap()"?
- The helpers for shadow-buffered planes establish and release mappings,
- and provide struct drm_shadow_plane_state, which stores the plane's mapping
- for commit-tail functons.
- Shadow-buffered planes can easily be enabled by using the provided macros
- DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS.
I think for hyperlinks/highlights we need %CONSTANT? Maybe check what works.
- These macros set up the plane and plane-helper callbacks to point to the
- shadow-buffer helpers.
- .. code-block:: c
- #include <drm/drm/gem_atomic_helper.h>
- struct drm_plane_funcs driver_plane_funcs = {
...,
DRM_GEM_SHADOW_PLANE_FUNCS,
- };
- struct drm_plane_helper_funcs driver_plane_helper_funcs = {
...,
DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
- };
- In the driver's atomic-update function, shadow-buffer mappings are available
- from the plane state. Use to_drm_shadow_plane_state() to upcast from
- struct drm_plane_state.
- .. code-block:: c
- void driver_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_plane_state)
- {
struct drm_plane_state *plane_state = plane->state;
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
// access shadow buffer via shadow_plane_state->map
- }
- A mapping address for each of the framebuffer's buffer object is stored in
- struct drm_shadow_plane_state.map. The mappings are valid while the state
- is being used.
- Drivers that use struct drm_simple_display_pipe can use
- DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp
- callbacks. Access to shadow-buffer mappings is similar to regular
- atomic_update.
- .. code-block:: c
- struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
...,
DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
- };
- void driver_pipe_enable(struct drm_simple_display_pipe *pipe,
struct drm_crtc_state *crtc_state,
struct drm_plane_state *plane_state)
- {
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
// access shadow buffer via shadow_plane_state->map*/
- }
/*
- Shadow-buffered Planes
*/
-static struct drm_plane_state * +/**
- drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
- @plane: the plane
- This function implements struct drm_plane_funcs.atomic_duplicate_state for
Does this hyperlink automatically? I didn't know it works since for members I just always use &struct.member myself.
- shadow-buffered planes. It assumes the existing state to be of type
- struct drm_shadow_plane_state and it allocates the new state to be of this
- type.
- The function does not duplicate existing mappings of the shadow buffers.
- Mappings are maintained during the atomic commit by the plane's prepare_fb
- and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb()
- for corresponding helpers.
- Returns:
- A pointer to a new plane state on success, or NULL otherwise.
- */
+struct drm_plane_state * drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane) { struct drm_plane_state *plane_state = plane->state; @@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
return &new_shadow_plane_state->base; } +EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
-static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state)+/**
- drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_funcs.atomic_destroy_state
- for shadow-buffered planes. It expects that mappings of shadow buffers
- have been released already.
- */
+void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state){ struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); @@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, __drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base); kfree(shadow_plane_state); } +EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
-static void drm_gem_reset_shadow_plane(struct drm_plane *plane) +/**
- drm_gem_reset_shadow_plane - resets a shadow-buffered plane
- @plane: the plane
- This function implements struct drm_plane_funcs.reset_plane for
- shadow-buffered planes. It assumes the current plane state to be
- of type struct drm_shadow_plane and it allocates the new state of
- this type.
- */
+void drm_gem_reset_shadow_plane(struct drm_plane *plane) { struct drm_shadow_plane_state *shadow_plane_state;
@@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane) return; __drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base); } +EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
-static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/**
- drm_gem_prepare_shadow_fb - prepares shadow framebuffers
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_helper_funcs.prepare_fb. It
- maps all buffer objects of the plane's framebuffer into kernel address
- space and stores them in struct drm_shadow_plane_state.map. The
- framebuffer will be synchronized as part of the atomic commit.
- See drm_gem_cleanup_shadow_fb() for cleanup.
- Returns:
- 0 on success, or a negative errno code otherwise.
- */
+int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s } return ret; } +EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
-static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/**
- drm_gem_cleanup_shadow_fb - releases shadow framebuffers
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_helper_funcs.cleanup_fb.
- This function unmaps all buffer objects of the plane's framebuffer.
- See drm_gem_prepare_shadow_fb() for more inforamtion.
- */
+void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_ drm_gem_vunmap(obj, &shadow_plane_state->map[i]); } } +EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
/**
- drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h index 08b96ccea325..7abf40bdab3d 100644 --- a/include/drm/drm_gem_atomic_helper.h +++ b/include/drm/drm_gem_atomic_helper.h @@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state) return container_of(state, struct drm_shadow_plane_state, base); }
+void drm_gem_reset_shadow_plane(struct drm_plane *plane); +struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane); +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state);+/**
- DRM_GEM_SHADOW_PLANE_FUNCS -
- Initializes struct drm_plane_funcs for shadow-buffered planes
- Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
- macro initializes struct drm_plane_funcs to use the rsp helper functions.
- */
+#define DRM_GEM_SHADOW_PLANE_FUNCS \
- .reset = drm_gem_reset_shadow_plane, \
- .atomic_duplicate_state = drm_gem_duplicate_shadow_plane_state, \
- .atomic_destroy_state = drm_gem_destroy_shadow_plane_state
+int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state); +void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state);
+/**
- DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
- Initializes struct drm_plane_helper_funcs for shadow-buffered planes
- Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
- macro initializes struct drm_plane_helper_funcs to use the rsp helper
- functions.
- */
+#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
- .prepare_fb = drm_gem_prepare_shadow_fb, \
- .cleanup_fb = drm_gem_cleanup_shadow_fb
int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe, struct drm_plane_state *plane_state); void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
Very nice and thoroughly explained docs!
Thanks, Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
-- 2.30.0
Hi
Am 09.02.21 um 10:44 schrieb Daniel Vetter:
On Mon, Feb 08, 2021 at 02:50:43PM +0100, Thomas Zimmermann wrote:
Export the helpers for shadow-buffered planes. These will be used by several drivers.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- include/drm/drm_gem_atomic_helper.h | 32 +++++ 2 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e27762cef360..79b4d3f0495a 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -14,13 +14,101 @@
- functions for drivers that use GEM objects. Currently, it provides
- plane state and framebuffer BO mappings for planes with shadow
- buffers.
- A driver using a shadow buffer copies the content of the shadow buffers
- into the HW's framebuffer memory during an atomic update. This requires
- a mapping of the shadow buffer into kernel address space. The mappings
- cannot be established by commit-tail functions, such as atomic_update,
- as this would violate locking rules vmap.
"... locking rules around dma_buf_vmap()"?
- The helpers for shadow-buffered planes establish and release mappings,
- and provide struct drm_shadow_plane_state, which stores the plane's mapping
- for commit-tail functons.
- Shadow-buffered planes can easily be enabled by using the provided macros
- DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS.
I think for hyperlinks/highlights we need %CONSTANT? Maybe check what works.
- These macros set up the plane and plane-helper callbacks to point to the
- shadow-buffer helpers.
- .. code-block:: c
- #include <drm/drm/gem_atomic_helper.h>
- struct drm_plane_funcs driver_plane_funcs = {
...,
DRM_GEM_SHADOW_PLANE_FUNCS,
- };
- struct drm_plane_helper_funcs driver_plane_helper_funcs = {
...,
DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
- };
- In the driver's atomic-update function, shadow-buffer mappings are available
- from the plane state. Use to_drm_shadow_plane_state() to upcast from
- struct drm_plane_state.
- .. code-block:: c
- void driver_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_plane_state)
- {
struct drm_plane_state *plane_state = plane->state;
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
// access shadow buffer via shadow_plane_state->map
- }
- A mapping address for each of the framebuffer's buffer object is stored in
- struct drm_shadow_plane_state.map. The mappings are valid while the state
- is being used.
- Drivers that use struct drm_simple_display_pipe can use
- DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp
- callbacks. Access to shadow-buffer mappings is similar to regular
- atomic_update.
- .. code-block:: c
- struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
...,
DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
- };
- void driver_pipe_enable(struct drm_simple_display_pipe *pipe,
struct drm_crtc_state *crtc_state,
struct drm_plane_state *plane_state)
- {
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
// access shadow buffer via shadow_plane_state->map
- }
*/
/*
- Shadow-buffered Planes
*/
-static struct drm_plane_state * +/**
- drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
- @plane: the plane
- This function implements struct drm_plane_funcs.atomic_duplicate_state for
Does this hyperlink automatically? I didn't know it works since for members I just always use &struct.member myself.
Ah, ok. Fixed. This work with struct &name.field. The % only adds formatting to constants.
Best regards Thomas
- shadow-buffered planes. It assumes the existing state to be of type
- struct drm_shadow_plane_state and it allocates the new state to be of this
- type.
- The function does not duplicate existing mappings of the shadow buffers.
- Mappings are maintained during the atomic commit by the plane's prepare_fb
- and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb()
- for corresponding helpers.
- Returns:
- A pointer to a new plane state on success, or NULL otherwise.
- */
+struct drm_plane_state * drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane) { struct drm_plane_state *plane_state = plane->state; @@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
return &new_shadow_plane_state->base; } +EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
-static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state)+/**
- drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_funcs.atomic_destroy_state
- for shadow-buffered planes. It expects that mappings of shadow buffers
- have been released already.
- */
+void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state)@@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane, __drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base); kfree(shadow_plane_state); } +EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
-static void drm_gem_reset_shadow_plane(struct drm_plane *plane) +/**
- drm_gem_reset_shadow_plane - resets a shadow-buffered plane
- @plane: the plane
- This function implements struct drm_plane_funcs.reset_plane for
- shadow-buffered planes. It assumes the current plane state to be
- of type struct drm_shadow_plane and it allocates the new state of
- this type.
- */
+void drm_gem_reset_shadow_plane(struct drm_plane *plane) { struct drm_shadow_plane_state *shadow_plane_state;
@@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane) return; __drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base); } +EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
-static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/**
- drm_gem_prepare_shadow_fb - prepares shadow framebuffers
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_helper_funcs.prepare_fb. It
- maps all buffer objects of the plane's framebuffer into kernel address
- space and stores them in struct drm_shadow_plane_state.map. The
- framebuffer will be synchronized as part of the atomic commit.
- See drm_gem_cleanup_shadow_fb() for cleanup.
- Returns:
- 0 on success, or a negative errno code otherwise.
- */
+int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s } return ret; } +EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
-static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) +/**
- drm_gem_cleanup_shadow_fb - releases shadow framebuffers
- @plane: the plane
- @plane_state: the plane state of type struct drm_shadow_plane_state
- This function implements struct drm_plane_helper_funcs.cleanup_fb.
- This function unmaps all buffer objects of the plane's framebuffer.
- See drm_gem_prepare_shadow_fb() for more inforamtion.
- */
+void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; @@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_ drm_gem_vunmap(obj, &shadow_plane_state->map[i]); } } +EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
/**
- drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h index 08b96ccea325..7abf40bdab3d 100644 --- a/include/drm/drm_gem_atomic_helper.h +++ b/include/drm/drm_gem_atomic_helper.h @@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state) return container_of(state, struct drm_shadow_plane_state, base); }
+void drm_gem_reset_shadow_plane(struct drm_plane *plane); +struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane); +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
struct drm_plane_state *plane_state);+/**
- DRM_GEM_SHADOW_PLANE_FUNCS -
- Initializes struct drm_plane_funcs for shadow-buffered planes
- Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
- macro initializes struct drm_plane_funcs to use the rsp helper functions.
- */
+#define DRM_GEM_SHADOW_PLANE_FUNCS \
- .reset = drm_gem_reset_shadow_plane, \
- .atomic_duplicate_state = drm_gem_duplicate_shadow_plane_state, \
- .atomic_destroy_state = drm_gem_destroy_shadow_plane_state
+int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state); +void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state);
+/**
- DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
- Initializes struct drm_plane_helper_funcs for shadow-buffered planes
- Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
- macro initializes struct drm_plane_helper_funcs to use the rsp helper
- functions.
- */
+#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
- .prepare_fb = drm_gem_prepare_shadow_fb, \
- .cleanup_fb = drm_gem_cleanup_shadow_fb
- int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe, struct drm_plane_state *plane_state); void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
Very nice and thoroughly explained docs!
Thanks, Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
-- 2.30.0
Functions in the atomic commit tail are not allowed to acquire the dmabuf's reservation lock. So we cannot legally call the GEM object's vmap functionality in atomic_update.
Instead use struct drm_shadow_plane_state and friends. It vmaps the framebuffer BOs in prepare_fb and vunmaps them in cleanup_fb. The cursor plane state stores the mapping's address. The pinning of the BO is implicitly done by vmap.
As an extra benefit, there's no source of runtime errors left in atomic_update.
v2: * rebase patch onto struct drm_shadow_plane_state
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 +++++++-------------------- 1 file changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c index dbc0dd53c69e..6e4ad966be71 100644 --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c @@ -17,6 +17,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_fourcc.h> +#include <drm/drm_gem_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> @@ -381,14 +382,14 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, container_of(plane->dev, struct vbox_private, ddev); struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc); struct drm_framebuffer *fb = plane->state->fb; - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(fb->obj[0]); u32 width = plane->state->crtc_w; u32 height = plane->state->crtc_h; + struct drm_shadow_plane_state *shadow_plane_state = + to_drm_shadow_plane_state(plane->state); + struct dma_buf_map map = shadow_plane_state->map[0]; + u8 *src = map.vaddr; /* TODO: Use mapping abstraction properly */ size_t data_size, mask_size; u32 flags; - struct dma_buf_map map; - int ret; - u8 *src;
/* * VirtualBox uses the host windowing system to draw the cursor so @@ -401,17 +402,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
vbox_crtc->cursor_enabled = true;
- ret = drm_gem_vram_vmap(gbo, &map); - if (ret) { - /* - * BUG: we should have pinned the BO in prepare_fb(). - */ - mutex_unlock(&vbox->hw_mutex); - DRM_WARN("Could not map cursor bo, skipping update\n"); - return; - } - src = map.vaddr; /* TODO: Use mapping abstraction properly */ - /* * The mask must be calculated based on the alpha * channel, one bit per ARGB word, and must be 32-bit @@ -421,7 +411,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, data_size = width * height * 4 + mask_size;
copy_cursor_image(src, vbox->cursor_data, width, height, mask_size); - drm_gem_vram_vunmap(gbo, &map);
flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE | VBOX_MOUSE_POINTER_ALPHA; @@ -466,17 +455,14 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = { .atomic_check = vbox_cursor_atomic_check, .atomic_update = vbox_cursor_atomic_update, .atomic_disable = vbox_cursor_atomic_disable, - .prepare_fb = drm_gem_vram_plane_helper_prepare_fb, - .cleanup_fb = drm_gem_vram_plane_helper_cleanup_fb, + DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, };
static const struct drm_plane_funcs vbox_cursor_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = drm_primary_helper_destroy, - .reset = drm_atomic_helper_plane_reset, - .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, + DRM_GEM_SHADOW_PLANE_FUNCS, };
static const u32 vbox_primary_plane_formats[] = {
Hi,
On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
(was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
Functions in the atomic commit tail are not allowed to acquire the dmabuf's reservation lock. So we cannot legally call the GEM object's vmap functionality in atomic_update.
But, much better, we can use drm_shadow_plane_state to do all the mapping for us. Patch 1 exports the helpers for shadow-buffered planes from the DRM KMS helper library and adds documentation on how to use them. Patch 2 replaces the vmap code in vbox' cursor update code with a the helpers for shadow-buffered planes.
Thomas Zimmermann (2): drm/gem: Export helpers for shadow-buffered planes drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state
I've given this a test spin in a virtualbox vm using VboxSVGA graphics and I've not found any problems:
Tested-by: Hans de Goede hdegoede@redhat.com
Regards,
Hans
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 ++--- include/drm/drm_gem_atomic_helper.h | 32 +++++ 3 files changed, 181 insertions(+), 27 deletions(-)
-- 2.30.0
Hi
Am 08.02.21 um 18:43 schrieb Hans de Goede:
Hi,
On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
(was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
Functions in the atomic commit tail are not allowed to acquire the dmabuf's reservation lock. So we cannot legally call the GEM object's vmap functionality in atomic_update.
But, much better, we can use drm_shadow_plane_state to do all the mapping for us. Patch 1 exports the helpers for shadow-buffered planes from the DRM KMS helper library and adds documentation on how to use them. Patch 2 replaces the vmap code in vbox' cursor update code with a the helpers for shadow-buffered planes.
Thomas Zimmermann (2): drm/gem: Export helpers for shadow-buffered planes drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state
I've given this a test spin in a virtualbox vm using VboxSVGA graphics and I've not found any problems:
Tested-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Can I add your Acked-by as well?
Best regards Thomas
Regards,
Hans
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 ++--- include/drm/drm_gem_atomic_helper.h | 32 +++++ 3 files changed, 181 insertions(+), 27 deletions(-)
-- 2.30.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On 2/9/21 9:13 AM, Thomas Zimmermann wrote:
Hi
Am 08.02.21 um 18:43 schrieb Hans de Goede:
Hi,
On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
(was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
Functions in the atomic commit tail are not allowed to acquire the dmabuf's reservation lock. So we cannot legally call the GEM object's vmap functionality in atomic_update.
But, much better, we can use drm_shadow_plane_state to do all the mapping for us. Patch 1 exports the helpers for shadow-buffered planes from the DRM KMS helper library and adds documentation on how to use them. Patch 2 replaces the vmap code in vbox' cursor update code with a the helpers for shadow-buffered planes.
Thomas Zimmermann (2): drm/gem: Export helpers for shadow-buffered planes drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state
I've given this a test spin in a virtualbox vm using VboxSVGA graphics and I've not found any problems:
Tested-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Can I add your Acked-by as well?
Patch 1/2 is a bit outside of my area of expertise, but I see you already have a Reviewed-by from Daniel there, so that one should be good to go.
I've just reviewed 2/2 and it looks good to me, so for 2/2 you may add my:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Regards,
Hans
drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++- drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 ++--- include/drm/drm_gem_atomic_helper.h | 32 +++++ 3 files changed, 181 insertions(+), 27 deletions(-)
-- 2.30.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Hans de Goede -
Thomas Zimmermann