Re: [PATCH V4 3/3] drm/vkms: Add support for writeback
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira wrote:
> # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
> vkms_composer.o
> +vkms-y := \
> + vkms_drv.o \
> + vkms_plane.o \
> + vkms_output.o \
> + vkms_crtc.o \
> + vkms_gem.o \
> + vkms_composer.o \
> + vkms_writeback.o
>
Nit: sort alphabetically
> @@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> + if (wb_pending)
> + vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(_out, primary_composer, cursor_composer);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.
> if (ret) {
> - if (ret == -EINVAL)
> + if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
> @@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, );
>
> + if (wb_pending) {
> + drm_writeback_signal_completion(>wb_connector, 0);
> + spin_lock_irq(>composer_lock);
> + crtc_state->wb_pending = false;
> + spin_unlock_irq(>composer_lock);
> + return;
> + }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()
if (wb_pending) {
signal()
...
} else {
free()
}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.
> static const struct file_operations vkms_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
>
> #define XRES_MIN20
> #define YRES_MIN20
> @@ -19,6 +20,7 @@
> #define YRES_MAX 8192
>
> extern bool enable_cursor;
> +extern bool enable_writeback;
>
> struct vkms_composer {
> struct drm_framebuffer fb;
> @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> int num_active_planes;
> /* stack of active planes for crc computation, should be in z order */
> struct vkms_plane_state **active_planes;
> + void *active_writeback;
>
> /* below three are protected by vkms_output.composer_lock */
Nit: s/below three/the following four/
> bool crc_pending;
> + bool wb_pending;
> u64 frame_start;
> u64 frame_end;
> };
> @@ -63,6 +67,7 @@ struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> + struct drm_writeback_connector wb_connector;
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> @@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const
> char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
>
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
Don't forget appropriate name spacing - prefix the function with vkms.
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c
> b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int
> index)
> goto err_attach;
> }
>
> + if (enable_writeback) {
> + ret = enable_writeback_connector(vkmsdev);
With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.
> + if (!ret) {
> + output->composer_enabled = true;
> + DRM_INFO("Writeback connector enabled");
> + } else {
> + DRM_ERROR("Failed to init writeback connector\n");
> + }
>
[PATCH V4 3/3] drm/vkms: Add support for writeback
From: Rodrigo Siqueira
This patch implements the necessary functions to add writeback support
for vkms. This feature is useful for testing compositors if you don't
have hardware with writeback support.
Change in V3 (Daniel):
- If writeback is enabled, compose everything into the writeback buffer
instead of CRC private buffer.
- Guarantees that the CRC will match exactly what we have in the
writeback buffer.
Change in V2:
- Rework signal completion (Brian)
- Integrates writeback with active_planes (Daniel)
- Compose cursor (Daniel)
Signed-off-by: Rodrigo Siqueira
---
drivers/gpu/drm/vkms/Makefile | 9 +-
drivers/gpu/drm/vkms/vkms_composer.c | 16 ++-
drivers/gpu/drm/vkms/vkms_drv.c | 4 +
drivers/gpu/drm/vkms/vkms_drv.h | 8 ++
drivers/gpu/drm/vkms/vkms_output.c| 10 ++
drivers/gpu/drm/vkms/vkms_writeback.c | 142 ++
6 files changed, 186 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 0b767d7efa24..333d3cead0e3 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,11 @@
# SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
vkms_composer.o
+vkms-y := \
+ vkms_drv.o \
+ vkms_plane.o \
+ vkms_output.o \
+ vkms_crtc.o \
+ vkms_gem.o \
+ vkms_composer.o \
+ vkms_writeback.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 686d25e7b01d..19849e39f8b9 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -160,16 +160,17 @@ void vkms_composer_worker(struct work_struct *work)
struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
struct vkms_composer *primary_composer = NULL;
struct vkms_composer *cursor_composer = NULL;
+ bool crc_pending, wb_pending;
void *vaddr_out = NULL;
u32 crc32 = 0;
u64 frame_start, frame_end;
- bool crc_pending;
int ret;
spin_lock_irq(>composer_lock);
frame_start = crtc_state->frame_start;
frame_end = crtc_state->frame_end;
crc_pending = crtc_state->crc_pending;
+ wb_pending = crtc_state->wb_pending;
crtc_state->frame_start = 0;
crtc_state->frame_end = 0;
crtc_state->crc_pending = false;
@@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
if (!primary_composer)
return;
+ if (wb_pending)
+ vaddr_out = crtc_state->active_writeback;
+
ret = compose_planes(_out, primary_composer, cursor_composer);
if (ret) {
- if (ret == -EINVAL)
+ if (ret == -EINVAL && !wb_pending)
kfree(vaddr_out);
return;
}
@@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
while (frame_start <= frame_end)
drm_crtc_add_crc_entry(crtc, true, frame_start++, );
+ if (wb_pending) {
+ drm_writeback_signal_completion(>wb_connector, 0);
+ spin_lock_irq(>composer_lock);
+ crtc_state->wb_pending = false;
+ spin_unlock_irq(>composer_lock);
+ return;
+ }
+
kfree(vaddr_out);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 1e8b2169d834..34dd74dc8eb0 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -39,6 +39,10 @@ bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
+bool enable_writeback;
+module_param_named(enable_writeback, enable_writeback, bool, 0444);
+MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
+
static const struct file_operations vkms_driver_fops = {
.owner = THIS_MODULE,
.open = drm_open,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f4036bb0b9a8..35f0b71413c9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#define XRES_MIN20
#define YRES_MIN20
@@ -19,6 +20,7 @@
#define YRES_MAX 8192
extern bool enable_cursor;
+extern bool enable_writeback;
struct vkms_composer {
struct drm_framebuffer fb;
@@ -52,9 +54,11 @@ struct vkms_crtc_state {
int num_active_planes;
/* stack of active planes for crc computation, should be in z order */
struct vkms_plane_state **active_planes;
+ void *active_writeback;
/* below three are protected by vkms_output.composer_lock */
bool crc_pending;
+ bool wb_pending;
u64
