This is the V5 version of a series that introduces the writeback support
to VKMS. The first two patches of this series are a pre-work for the
latest patch that adds the writeback connector, this patchset can be seen
in two parts:
* A pre-work that aims to make vkms composer operations a little bit more
generic; these patches try to centralize the vkms framebuffer operations.
* The final patch enables the support for writeback in vkms.
In the previous review, Emil suggested multiple changes in the series. I
tried to apply most of the recommendations except for some suggestions
which I was not able to incorporate due to compilation issues, or other
suggestions that may complicate this series review. I left some changes
for future patches for keeping this patchset simple with the hope of
landing this feature soon in order to support VKMS user's requirements.
Emil, let me know if you want me to change any other thing.
It is important to highlight that from the previous series to the
current version of this patchset we had some changes in the VKMS that
made it unstable. In particular, our previous writeback series stopped
working properly due to changes in our commit tail. Thanks to Melissa
working in the instability issue and her latest fixes to VKMS, I finally
could update writeback and make it work again. The main update in the
latest patch is the use of vkms_set_composer when the writeback work
starts (enable composer) and after the writeback end (disable composer).
Best Regard
Rodrigo Siqueira (3):
drm/vkms: Decouple crc operations from composer
drm/vkms: Compute CRC without change input data
drm/vkms: Add support for writeback
drivers/gpu/drm/vkms/Makefile | 9 +-
drivers/gpu/drm/vkms/vkms_composer.c | 102 +++++++++++-------
drivers/gpu/drm/vkms/vkms_drv.h | 11 +-
drivers/gpu/drm/vkms/vkms_output.c | 4 +
drivers/gpu/drm/vkms/vkms_writeback.c | 143 ++++++++++++++++++++++++++
5 files changed, 231 insertions(+), 38 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
--
2.28.0
The compute_crc() function is responsible for calculating the
framebuffer CRC value; due to the XRGB format, this function has to
ignore the alpha channel during the CRC computation. Therefore,
compute_crc() set zero to the alpha channel directly in the input
framebuffer, which is not a problem since this function receives a copy
of the original buffer. However, if we want to use this function in a
context without a buffer copy, it will change the initial value. This
patch makes compute_crc() calculate the CRC value without modifying the
input framebuffer.
Change in V4 (Emil):
- Move bitmap_clear operation and comments to get_pixel function
Signed-off-by: Rodrigo Siqueira <[email protected]>
---
drivers/gpu/drm/vkms/vkms_composer.c | 38 ++++++++++++++++++----------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 4d8bc04bb6ee..387b0690a64a 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -9,33 +9,43 @@
#include "vkms_drv.h"
+static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
+ const struct vkms_composer *composer)
+{
+ u32 pixel;
+ int src_offset = composer->offset + (y * composer->pitch)
+ + (x * composer->cpp);
+
+ pixel = *(u32 *)&buffer[src_offset];
+ /* XRGB format ignores Alpha channel */
+ bitmap_clear((void *)&pixel, 0, 8);
+
+ return pixel;
+}
+
/**
* compute_crc - Compute CRC value on output frame
*
- * @vaddr_out: address to final framebuffer
+ * @vaddr: address to final framebuffer
* @composer: framebuffer's metadata
*
* returns CRC value computed using crc32 on the visible portion of
* the final framebuffer at vaddr_out
*/
-static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
+static uint32_t compute_crc(const u8 *vaddr,
+ const struct vkms_composer *composer)
{
- int i, j, src_offset;
+ int x, y;
+ u32 crc = 0, pixel = 0;
int x_src = composer->src.x1 >> 16;
int y_src = composer->src.y1 >> 16;
int h_src = drm_rect_height(&composer->src) >> 16;
int w_src = drm_rect_width(&composer->src) >> 16;
- u32 crc = 0;
-
- for (i = y_src; i < y_src + h_src; ++i) {
- for (j = x_src; j < x_src + w_src; ++j) {
- src_offset = composer->offset
- + (i * composer->pitch)
- + (j * composer->cpp);
- /* XRGB format ignores Alpha channel */
- bitmap_clear(vaddr_out + src_offset, 24, 8);
- crc = crc32_le(crc, vaddr_out + src_offset,
- sizeof(u32));
+
+ for (y = y_src; y < y_src + h_src; ++y) {
+ for (x = x_src; x < x_src + w_src; ++x) {
+ pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+ crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
}
}
--
2.28.0
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 V4 (Emil and Melissa):
- Move signal completion above drm_crtc_add_crc_entry()
- Make writeback always available
- Use appropriate namespace
- Drop fb check in vkms_wb_atomic_commit
- Make vkms_set_composer visible for writeback code
- Enable composer operation on prepare_job and disable it on cleanup_job
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 <[email protected]>
---
drivers/gpu/drm/vkms/Makefile | 9 +-
drivers/gpu/drm/vkms/vkms_composer.c | 21 +++-
drivers/gpu/drm/vkms/vkms_drv.h | 11 +-
drivers/gpu/drm/vkms/vkms_output.c | 4 +
drivers/gpu/drm/vkms/vkms_writeback.c | 143 ++++++++++++++++++++++++++
5 files changed, 181 insertions(+), 7 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 387b0690a64a..656085c4ebf3 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -163,16 +163,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(&out->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;
@@ -194,22 +195,32 @@ void vkms_composer_worker(struct work_struct *work)
if (!primary_composer)
return;
+ if (wb_pending)
+ vaddr_out = crtc_state->active_writeback;
+
ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
if (ret) {
- if (ret == -EINVAL)
+ if (ret == -EINVAL && !wb_pending)
kfree(vaddr_out);
return;
}
crc32 = compute_crc(vaddr_out, primary_composer);
+ if (wb_pending) {
+ drm_writeback_signal_completion(&out->wb_connector, 0);
+ spin_lock_irq(&out->composer_lock);
+ crtc_state->wb_pending = false;
+ spin_unlock_irq(&out->composer_lock);
+ } else {
+ kfree(vaddr_out);
+ }
+
/*
* The worker can fall behind the vblank hrtimer, make sure we catch up.
*/
while (frame_start <= frame_end)
drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
-
- kfree(vaddr_out);
}
static const char * const pipe_crc_sources[] = {"auto"};
@@ -252,7 +263,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
return 0;
}
-static void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_set_composer(struct vkms_output *out, bool enabled)
{
bool old_enabled;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f4036bb0b9a8..641d8bc52a3a 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -8,6 +8,7 @@
#include <drm/drm.h>
#include <drm/drm_gem.h>
#include <drm/drm_encoder.h>
+#include <drm/drm_writeback.h>
#define XRES_MIN 20
#define YRES_MIN 20
@@ -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 */
+ /* below four are protected by vkms_output.composer_lock */
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;
@@ -143,5 +148,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
/* Composer Support */
void vkms_composer_worker(struct work_struct *work);
+void vkms_set_composer(struct vkms_output *out, bool enabled);
+
+/* Writeback */
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 85afb77e97f0..4a1848b0318f 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -80,6 +80,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
+ ret = vkms_enable_writeback_connector(vkmsdev);
+ if (ret)
+ DRM_ERROR("Failed to init writeback connector\n");
+
drm_mode_config_reset(dev);
return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
new file mode 100644
index 000000000000..ddcd5ad0d7aa
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+#include <drm/drm_fourcc.h>
+#include <drm/drm_writeback.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static const u32 vkms_wb_formats[] = {
+ DRM_FORMAT_XRGB8888,
+};
+
+static const struct drm_connector_funcs vkms_wb_connector_funcs = {
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = drm_connector_cleanup,
+ .reset = drm_atomic_helper_connector_reset,
+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct drm_framebuffer *fb;
+ const struct drm_display_mode *mode = &crtc_state->mode;
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+ return 0;
+
+ fb = conn_state->writeback_job->fb;
+ if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
+ DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+ fb->width, fb->height);
+ return -EINVAL;
+ }
+
+ if (fb->format->format != vkms_wb_formats[0]) {
+ struct drm_format_name_buf format_name;
+
+ DRM_DEBUG_KMS("Invalid pixel format %s\n",
+ drm_get_format_name(fb->format->format,
+ &format_name));
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
+ .atomic_check = vkms_wb_encoder_atomic_check,
+};
+
+static int vkms_wb_connector_get_modes(struct drm_connector *connector)
+{
+ struct drm_device *dev = connector->dev;
+
+ return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+ dev->mode_config.max_height);
+}
+
+static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
+ struct drm_writeback_job *job)
+{
+ struct vkms_gem_object *vkms_obj;
+ struct drm_gem_object *gem_obj;
+ int ret;
+
+ if (!job->fb)
+ return 0;
+
+ gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+ ret = vkms_gem_vmap(gem_obj);
+ if (ret) {
+ DRM_ERROR("vmap failed: %d\n", ret);
+ return ret;
+ }
+
+ vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+ job->priv = vkms_obj->vaddr;
+
+ return 0;
+}
+
+static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
+ struct drm_writeback_job *job)
+{
+ struct drm_gem_object *gem_obj;
+ struct vkms_device *vkmsdev;
+
+ if (!job->fb)
+ return;
+
+ gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+ vkms_gem_vunmap(gem_obj);
+
+ vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
+ vkms_set_composer(&vkmsdev->output, false);
+}
+
+static void vkms_wb_atomic_commit(struct drm_connector *conn,
+ struct drm_connector_state *state)
+{
+ struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
+ struct vkms_output *output = &vkmsdev->output;
+ struct drm_writeback_connector *wb_conn = &output->wb_connector;
+ struct drm_connector_state *conn_state = wb_conn->base.state;
+ struct vkms_crtc_state *crtc_state = output->composer_state;
+
+ if (!conn_state)
+ return;
+
+ vkms_set_composer(&vkmsdev->output, true);
+
+ spin_lock_irq(&output->composer_lock);
+ crtc_state->active_writeback = conn_state->writeback_job->priv;
+ crtc_state->wb_pending = true;
+ spin_unlock_irq(&output->composer_lock);
+ drm_writeback_queue_job(wb_conn, state);
+}
+
+static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
+ .get_modes = vkms_wb_connector_get_modes,
+ .prepare_writeback_job = vkms_wb_prepare_job,
+ .cleanup_writeback_job = vkms_wb_cleanup_job,
+ .atomic_commit = vkms_wb_atomic_commit,
+};
+
+int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
+{
+ struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+
+ vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
+ drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
+
+ return drm_writeback_connector_init(&vkmsdev->drm, wb,
+ &vkms_wb_connector_funcs,
+ &vkms_wb_encoder_helper_funcs,
+ vkms_wb_formats,
+ ARRAY_SIZE(vkms_wb_formats));
+}
+
--
2.28.0
In the vkms_composer.c, some of the functions related to CRC and compose
have interdependence between each other. This patch reworks some
functions inside vkms_composer to make crc and composer computation
decoupled.
This patch is preparation work for making vkms able to support new
features.
Signed-off-by: Rodrigo Siqueira <[email protected]>
---
drivers/gpu/drm/vkms/vkms_composer.c | 49 ++++++++++++++++------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 4f3b07a32b60..4d8bc04bb6ee 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -108,35 +108,31 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
primary_composer, cursor_composer);
}
-static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
- struct vkms_composer *cursor_composer)
+static int compose_planes(void **vaddr_out,
+ struct vkms_composer *primary_composer,
+ struct vkms_composer *cursor_composer)
{
struct drm_framebuffer *fb = &primary_composer->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
- void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
- u32 crc = 0;
- if (!vaddr_out) {
- DRM_ERROR("Failed to allocate memory for output frame.");
- return 0;
+ if (!*vaddr_out) {
+ *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
+ if (!*vaddr_out) {
+ DRM_ERROR("Cannot allocate memory for output frame.");
+ return -ENOMEM;
+ }
}
- if (WARN_ON(!vkms_obj->vaddr)) {
- kfree(vaddr_out);
- return crc;
- }
+ if (WARN_ON(!vkms_obj->vaddr))
+ return -EINVAL;
- memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
+ memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
if (cursor_composer)
- compose_cursor(cursor_composer, primary_composer, vaddr_out);
+ compose_cursor(cursor_composer, primary_composer, *vaddr_out);
- crc = compute_crc(vaddr_out, primary_composer);
-
- kfree(vaddr_out);
-
- return crc;
+ return 0;
}
/**
@@ -157,9 +153,11 @@ 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;
+ void *vaddr_out = NULL;
u32 crc32 = 0;
u64 frame_start, frame_end;
bool crc_pending;
+ int ret;
spin_lock_irq(&out->composer_lock);
frame_start = crtc_state->frame_start;
@@ -183,14 +181,25 @@ void vkms_composer_worker(struct work_struct *work)
if (crtc_state->num_active_planes == 2)
cursor_composer = crtc_state->active_planes[1]->composer;
- if (primary_composer)
- crc32 = _vkms_get_crc(primary_composer, cursor_composer);
+ if (!primary_composer)
+ return;
+
+ ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
+ if (ret) {
+ if (ret == -EINVAL)
+ kfree(vaddr_out);
+ return;
+ }
+
+ crc32 = compute_crc(vaddr_out, primary_composer);
/*
* The worker can fall behind the vblank hrtimer, make sure we catch up.
*/
while (frame_start <= frame_end)
drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+
+ kfree(vaddr_out);
}
static const char * const pipe_crc_sources[] = {"auto"};
--
2.28.0
On 08/24, Rodrigo Siqueira wrote:
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying the
> input framebuffer.
>
> Change in V4 (Emil):
> - Move bitmap_clear operation and comments to get_pixel function
>
> Signed-off-by: Rodrigo Siqueira <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 38 ++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 4d8bc04bb6ee..387b0690a64a 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -9,33 +9,43 @@
>
> #include "vkms_drv.h"
>
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> + const struct vkms_composer *composer)
> +{
> + u32 pixel;
> + int src_offset = composer->offset + (y * composer->pitch)
> + + (x * composer->cpp);
> +
> + pixel = *(u32 *)&buffer[src_offset];
> + /* XRGB format ignores Alpha channel */
> + bitmap_clear((void *)&pixel, 0, 8);
I noticed a small problem in this bitmap_clear as I recently sent a
patch changing it. The starting point for zeroing the alpha is not 0,
but 24. As in the removed line below:
- bitmap_clear(vaddr_out + src_offset, 24, 8);
It affects the kms_cursor_crc/cursor-alpha-transparent subtest.
> +
> + return pixel;
> +}
> +
> /**
> * compute_crc - Compute CRC value on output frame
> *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
> * @composer: framebuffer's metadata
> *
> * returns CRC value computed using crc32 on the visible portion of
> * the final framebuffer at vaddr_out
> */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> + const struct vkms_composer *composer)
> {
> - int i, j, src_offset;
> + int x, y;
> + u32 crc = 0, pixel = 0;
> int x_src = composer->src.x1 >> 16;
> int y_src = composer->src.y1 >> 16;
> int h_src = drm_rect_height(&composer->src) >> 16;
> int w_src = drm_rect_width(&composer->src) >> 16;
> - u32 crc = 0;
> -
> - for (i = y_src; i < y_src + h_src; ++i) {
> - for (j = x_src; j < x_src + w_src; ++j) {
> - src_offset = composer->offset
> - + (i * composer->pitch)
> - + (j * composer->cpp);
> - /* XRGB format ignores Alpha channel */
> - bitmap_clear(vaddr_out + src_offset, 24, 8);
> - crc = crc32_le(crc, vaddr_out + src_offset,
> - sizeof(u32));
> +
> + for (y = y_src; y < y_src + h_src; ++y) {
> + for (x = x_src; x < x_src + w_src; ++x) {
> + pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> + crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> }
> }
>
> --
> 2.28.0
>
On 08/24, Rodrigo Siqueira wrote:
> 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 V4 (Emil and Melissa):
> - Move signal completion above drm_crtc_add_crc_entry()
> - Make writeback always available
> - Use appropriate namespace
> - Drop fb check in vkms_wb_atomic_commit
> - Make vkms_set_composer visible for writeback code
> - Enable composer operation on prepare_job and disable it on cleanup_job
>
> 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 <[email protected]>
> ---
> drivers/gpu/drm/vkms/Makefile | 9 +-
> drivers/gpu/drm/vkms/vkms_composer.c | 21 +++-
> drivers/gpu/drm/vkms/vkms_drv.h | 11 +-
> drivers/gpu/drm/vkms/vkms_output.c | 4 +
> drivers/gpu/drm/vkms/vkms_writeback.c | 143 ++++++++++++++++++++++++++
> 5 files changed, 181 insertions(+), 7 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 387b0690a64a..656085c4ebf3 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -163,16 +163,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(&out->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;
> @@ -194,22 +195,32 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> + if (wb_pending)
> + vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> if (ret) {
> - if (ret == -EINVAL)
> + if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
>
> crc32 = compute_crc(vaddr_out, primary_composer);
>
> + if (wb_pending) {
> + drm_writeback_signal_completion(&out->wb_connector, 0);
> + spin_lock_irq(&out->composer_lock);
> + crtc_state->wb_pending = false;
> + spin_unlock_irq(&out->composer_lock);
> + } else {
> + kfree(vaddr_out);
> + }
> +
> /*
> * The worker can fall behind the vblank hrtimer, make sure we catch up.
> */
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> -
> - kfree(vaddr_out);
> }
>
> static const char * const pipe_crc_sources[] = {"auto"};
> @@ -252,7 +263,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
> return 0;
> }
>
> -static void vkms_set_composer(struct vkms_output *out, bool enabled)
> +void vkms_set_composer(struct vkms_output *out, bool enabled)
> {
> bool old_enabled;
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..641d8bc52a3a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
>
> #define XRES_MIN 20
> #define YRES_MIN 20
> @@ -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 */
> + /* below four are protected by vkms_output.composer_lock */
> 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;
> @@ -143,5 +148,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
> +void vkms_set_composer(struct vkms_output *out, bool enabled);
> +
> +/* Writeback */
> +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..4a1848b0318f 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> + ret = vkms_enable_writeback_connector(vkmsdev);
> + if (ret)
> + DRM_ERROR("Failed to init writeback connector\n");
> +
> drm_mode_config_reset(dev);
>
> return 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> new file mode 100644
> index 000000000000..ddcd5ad0d7aa
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static const u32 vkms_wb_formats[] = {
> + DRM_FORMAT_XRGB8888,
> +};
> +
> +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_framebuffer *fb;
> + const struct drm_display_mode *mode = &crtc_state->mode;
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> + return 0;
> +
> + fb = conn_state->writeback_job->fb;
> + if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> + fb->width, fb->height);
> + return -EINVAL;
> + }
> +
> + if (fb->format->format != vkms_wb_formats[0]) {
> + struct drm_format_name_buf format_name;
> +
> + DRM_DEBUG_KMS("Invalid pixel format %s\n",
> + drm_get_format_name(fb->format->format,
> + &format_name));
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> + .atomic_check = vkms_wb_encoder_atomic_check,
> +};
> +
> +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> +
> + return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> + dev->mode_config.max_height);
> +}
> +
> +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> + struct drm_writeback_job *job)
> +{
> + struct vkms_gem_object *vkms_obj;
> + struct drm_gem_object *gem_obj;
> + int ret;
> +
> + if (!job->fb)
> + return 0;
> +
> + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> + ret = vkms_gem_vmap(gem_obj);
> + if (ret) {
> + DRM_ERROR("vmap failed: %d\n", ret);
> + return ret;
> + }
> +
> + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> + job->priv = vkms_obj->vaddr;
> +
> + return 0;
> +}
> +
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> + struct drm_writeback_job *job)
> +{
> + struct drm_gem_object *gem_obj;
> + struct vkms_device *vkmsdev;
> +
> + if (!job->fb)
> + return;
> +
> + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> + vkms_gem_vunmap(gem_obj);
> +
> + vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
> + vkms_set_composer(&vkmsdev->output, false);
> +}
> +
> +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> + struct drm_connector_state *state)
> +{
> + struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> + struct vkms_output *output = &vkmsdev->output;
> + struct drm_writeback_connector *wb_conn = &output->wb_connector;
> + struct drm_connector_state *conn_state = wb_conn->base.state;
> + struct vkms_crtc_state *crtc_state = output->composer_state;
> +
> + if (!conn_state)
> + return;
> +
> + vkms_set_composer(&vkmsdev->output, true);
> +
> + spin_lock_irq(&output->composer_lock);
> + crtc_state->active_writeback = conn_state->writeback_job->priv;
> + crtc_state->wb_pending = true;
> + spin_unlock_irq(&output->composer_lock);
> + drm_writeback_queue_job(wb_conn, state);
> +}
> +
> +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> + .get_modes = vkms_wb_connector_get_modes,
> + .prepare_writeback_job = vkms_wb_prepare_job,
> + .cleanup_writeback_job = vkms_wb_cleanup_job,
> + .atomic_commit = vkms_wb_atomic_commit,
> +};
> +
> +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> +{
> + struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +
> + vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> + drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +
> + return drm_writeback_connector_init(&vkmsdev->drm, wb,
> + &vkms_wb_connector_funcs,
> + &vkms_wb_encoder_helper_funcs,
> + vkms_wb_formats,
> + ARRAY_SIZE(vkms_wb_formats));
> +}
> +
Another small fix: there is an extra line above.
> --
> 2.28.0
>
Hi Rodrigo,
Nice work!
I tested this on the kms_writeback set of subtests and works fine.
I have also checked it on the other IGT tests that I usually uses:
- kms_cursor_crc/cursor-alpha-transparent was affected by a small
problem in the bitmap_clear of the get_pixel_from_buffer function,
that I pointed out inline. Please, check it.
- kms_flip and kms_pipe_crc_basic works as expected (was not affected).
Best regards,
Melissa Wen
On 08/24, Rodrigo Siqueira wrote:
> This is the V5 version of a series that introduces the writeback support
> to VKMS. The first two patches of this series are a pre-work for the
> latest patch that adds the writeback connector, this patchset can be seen
> in two parts:
>
> * A pre-work that aims to make vkms composer operations a little bit more
> generic; these patches try to centralize the vkms framebuffer operations.
> * The final patch enables the support for writeback in vkms.
>
> In the previous review, Emil suggested multiple changes in the series. I
> tried to apply most of the recommendations except for some suggestions
> which I was not able to incorporate due to compilation issues, or other
> suggestions that may complicate this series review. I left some changes
> for future patches for keeping this patchset simple with the hope of
> landing this feature soon in order to support VKMS user's requirements.
> Emil, let me know if you want me to change any other thing.
>
> It is important to highlight that from the previous series to the
> current version of this patchset we had some changes in the VKMS that
> made it unstable. In particular, our previous writeback series stopped
> working properly due to changes in our commit tail. Thanks to Melissa
> working in the instability issue and her latest fixes to VKMS, I finally
> could update writeback and make it work again. The main update in the
> latest patch is the use of vkms_set_composer when the writeback work
> starts (enable composer) and after the writeback end (disable composer).
>
> Best Regard
>
> Rodrigo Siqueira (3):
> drm/vkms: Decouple crc operations from composer
> drm/vkms: Compute CRC without change input data
> drm/vkms: Add support for writeback
>
> drivers/gpu/drm/vkms/Makefile | 9 +-
> drivers/gpu/drm/vkms/vkms_composer.c | 102 +++++++++++-------
> drivers/gpu/drm/vkms/vkms_drv.h | 11 +-
> drivers/gpu/drm/vkms/vkms_output.c | 4 +
> drivers/gpu/drm/vkms/vkms_writeback.c | 143 ++++++++++++++++++++++++++
> 5 files changed, 231 insertions(+), 38 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
>
> --
> 2.28.0
>
On Tue, Aug 25, 2020 at 12:45 PM Melissa Wen <[email protected]> wrote:
>
> On 08/24, Rodrigo Siqueira wrote:
> > 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 V4 (Emil and Melissa):
> > - Move signal completion above drm_crtc_add_crc_entry()
> > - Make writeback always available
> > - Use appropriate namespace
> > - Drop fb check in vkms_wb_atomic_commit
> > - Make vkms_set_composer visible for writeback code
> > - Enable composer operation on prepare_job and disable it on cleanup_job
> >
> > 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 <[email protected]>
> > ---
> > drivers/gpu/drm/vkms/Makefile | 9 +-
> > drivers/gpu/drm/vkms/vkms_composer.c | 21 +++-
> > drivers/gpu/drm/vkms/vkms_drv.h | 11 +-
> > drivers/gpu/drm/vkms/vkms_output.c | 4 +
> > drivers/gpu/drm/vkms/vkms_writeback.c | 143 ++++++++++++++++++++++++++
> > 5 files changed, 181 insertions(+), 7 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 387b0690a64a..656085c4ebf3 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -163,16 +163,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(&out->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;
> > @@ -194,22 +195,32 @@ void vkms_composer_worker(struct work_struct *work)
> > if (!primary_composer)
> > return;
> >
> > + if (wb_pending)
> > + vaddr_out = crtc_state->active_writeback;
> > +
> > ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> > if (ret) {
> > - if (ret == -EINVAL)
> > + if (ret == -EINVAL && !wb_pending)
> > kfree(vaddr_out);
> > return;
> > }
> >
> > crc32 = compute_crc(vaddr_out, primary_composer);
> >
> > + if (wb_pending) {
> > + drm_writeback_signal_completion(&out->wb_connector, 0);
> > + spin_lock_irq(&out->composer_lock);
> > + crtc_state->wb_pending = false;
> > + spin_unlock_irq(&out->composer_lock);
> > + } else {
> > + kfree(vaddr_out);
> > + }
> > +
> > /*
> > * The worker can fall behind the vblank hrtimer, make sure we catch up.
> > */
> > while (frame_start <= frame_end)
> > drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > -
> > - kfree(vaddr_out);
> > }
> >
> > static const char * const pipe_crc_sources[] = {"auto"};
> > @@ -252,7 +263,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
> > return 0;
> > }
> >
> > -static void vkms_set_composer(struct vkms_output *out, bool enabled)
> > +void vkms_set_composer(struct vkms_output *out, bool enabled)
> > {
> > bool old_enabled;
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index f4036bb0b9a8..641d8bc52a3a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -8,6 +8,7 @@
> > #include <drm/drm.h>
> > #include <drm/drm_gem.h>
> > #include <drm/drm_encoder.h>
> > +#include <drm/drm_writeback.h>
> >
> > #define XRES_MIN 20
> > #define YRES_MIN 20
> > @@ -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 */
> > + /* below four are protected by vkms_output.composer_lock */
> > 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;
> > @@ -143,5 +148,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >
> > /* Composer Support */
> > void vkms_composer_worker(struct work_struct *work);
> > +void vkms_set_composer(struct vkms_output *out, bool enabled);
> > +
> > +/* Writeback */
> > +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> >
> > #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 85afb77e97f0..4a1848b0318f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -80,6 +80,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > goto err_attach;
> > }
> >
> > + ret = vkms_enable_writeback_connector(vkmsdev);
> > + if (ret)
> > + DRM_ERROR("Failed to init writeback connector\n");
> > +
> > drm_mode_config_reset(dev);
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..ddcd5ad0d7aa
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,143 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const u32 vkms_wb_formats[] = {
> > + DRM_FORMAT_XRGB8888,
> > +};
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > + .fill_modes = drm_helper_probe_single_connector_modes,
> > + .destroy = drm_connector_cleanup,
> > + .reset = drm_atomic_helper_connector_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct drm_framebuffer *fb;
> > + const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > + return 0;
> > +
> > + fb = conn_state->writeback_job->fb;
> > + if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > + fb->width, fb->height);
> > + return -EINVAL;
> > + }
> > +
> > + if (fb->format->format != vkms_wb_formats[0]) {
> > + struct drm_format_name_buf format_name;
> > +
> > + DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > + drm_get_format_name(fb->format->format,
> > + &format_name));
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > + .atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > + struct drm_device *dev = connector->dev;
> > +
> > + return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > + dev->mode_config.max_height);
> > +}
> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > + struct drm_writeback_job *job)
> > +{
> > + struct vkms_gem_object *vkms_obj;
> > + struct drm_gem_object *gem_obj;
> > + int ret;
> > +
> > + if (!job->fb)
> > + return 0;
> > +
> > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > + ret = vkms_gem_vmap(gem_obj);
> > + if (ret) {
> > + DRM_ERROR("vmap failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > + job->priv = vkms_obj->vaddr;
> > +
> > + return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > + struct drm_writeback_job *job)
> > +{
> > + struct drm_gem_object *gem_obj;
> > + struct vkms_device *vkmsdev;
> > +
> > + if (!job->fb)
> > + return;
> > +
> > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > + vkms_gem_vunmap(gem_obj);
> > +
> > + vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
> > + vkms_set_composer(&vkmsdev->output, false);
> > +}
> > +
> > +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> > + struct drm_connector_state *state)
> > +{
> > + struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> > + struct vkms_output *output = &vkmsdev->output;
> > + struct drm_writeback_connector *wb_conn = &output->wb_connector;
> > + struct drm_connector_state *conn_state = wb_conn->base.state;
> > + struct vkms_crtc_state *crtc_state = output->composer_state;
> > +
> > + if (!conn_state)
> > + return;
> > +
> > + vkms_set_composer(&vkmsdev->output, true);
> > +
> > + spin_lock_irq(&output->composer_lock);
> > + crtc_state->active_writeback = conn_state->writeback_job->priv;
> > + crtc_state->wb_pending = true;
> > + spin_unlock_irq(&output->composer_lock);
> > + drm_writeback_queue_job(wb_conn, state);
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > + .get_modes = vkms_wb_connector_get_modes,
> > + .prepare_writeback_job = vkms_wb_prepare_job,
> > + .cleanup_writeback_job = vkms_wb_cleanup_job,
> > + .atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > + struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > + vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > + drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > + return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > + &vkms_wb_connector_funcs,
> > + &vkms_wb_encoder_helper_funcs,
> > + vkms_wb_formats,
> > + ARRAY_SIZE(vkms_wb_formats));
> > +}
> > +
> Another small fix: there is an extra line above.
> > --
> > 2.28.0
> >
Tested-by: Melissa Wen <[email protected]>
On Mon, Aug 24, 2020 at 11:31 PM Rodrigo Siqueira
<[email protected]> wrote:
>
> In the vkms_composer.c, some of the functions related to CRC and compose
> have interdependence between each other. This patch reworks some
> functions inside vkms_composer to make crc and composer computation
> decoupled.
>
> This patch is preparation work for making vkms able to support new
> features.
>
> Signed-off-by: Rodrigo Siqueira <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 49 ++++++++++++++++------------
> 1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 4f3b07a32b60..4d8bc04bb6ee 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -108,35 +108,31 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
> primary_composer, cursor_composer);
> }
>
> -static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
> - struct vkms_composer *cursor_composer)
> +static int compose_planes(void **vaddr_out,
> + struct vkms_composer *primary_composer,
> + struct vkms_composer *cursor_composer)
> {
> struct drm_framebuffer *fb = &primary_composer->fb;
> struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> - void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> - u32 crc = 0;
>
> - if (!vaddr_out) {
> - DRM_ERROR("Failed to allocate memory for output frame.");
> - return 0;
> + if (!*vaddr_out) {
> + *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> + if (!*vaddr_out) {
> + DRM_ERROR("Cannot allocate memory for output frame.");
> + return -ENOMEM;
> + }
> }
>
> - if (WARN_ON(!vkms_obj->vaddr)) {
> - kfree(vaddr_out);
> - return crc;
> - }
> + if (WARN_ON(!vkms_obj->vaddr))
> + return -EINVAL;
>
> - memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> + memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>
> if (cursor_composer)
> - compose_cursor(cursor_composer, primary_composer, vaddr_out);
> + compose_cursor(cursor_composer, primary_composer, *vaddr_out);
>
> - crc = compute_crc(vaddr_out, primary_composer);
> -
> - kfree(vaddr_out);
> -
> - return crc;
> + return 0;
> }
>
> /**
> @@ -157,9 +153,11 @@ 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;
> + void *vaddr_out = NULL;
> u32 crc32 = 0;
> u64 frame_start, frame_end;
> bool crc_pending;
> + int ret;
>
> spin_lock_irq(&out->composer_lock);
> frame_start = crtc_state->frame_start;
> @@ -183,14 +181,25 @@ void vkms_composer_worker(struct work_struct *work)
> if (crtc_state->num_active_planes == 2)
> cursor_composer = crtc_state->active_planes[1]->composer;
>
> - if (primary_composer)
> - crc32 = _vkms_get_crc(primary_composer, cursor_composer);
> + if (!primary_composer)
> + return;
> +
> + ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> + if (ret) {
> + if (ret == -EINVAL)
> + kfree(vaddr_out);
> + return;
> + }
> +
> + crc32 = compute_crc(vaddr_out, primary_composer);
>
> /*
> * The worker can fall behind the vblank hrtimer, make sure we catch up.
> */
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> +
> + kfree(vaddr_out);
> }
>
> static const char * const pipe_crc_sources[] = {"auto"};
> --
> 2.28.0
>
Tested-by: Melissa Wen <[email protected]>