* [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to vkms_composer
2019-06-18 2:41 [PATCH V2 0/5] drm/vkms: Introduces writeback support Rodrigo Siqueira
@ 2019-06-18 2:42 ` Rodrigo Siqueira
2019-06-18 9:27 ` Daniel Vetter
2019-06-18 2:43 ` [PATCH V2 2/5] drm/vkms: Rename crc_enabled to composer_enabled Rodrigo Siqueira
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 2:42 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser
Cc: dri-devel, linux-kernel
The vkms_crc file has functions related to compose operations which are
not directly associated with CRC. This patch, move those function for a
new file named vkms_composer.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 9 +++-
drivers/gpu/drm/vkms/vkms_composer.c | 69 ++++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_composer.h | 12 +++++
drivers/gpu/drm/vkms/vkms_crc.c | 65 +-------------------------
4 files changed, 90 insertions(+), 65 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_composer.c
create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 89f09bec7b23..b4c040854bd6 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_crc.o
+vkms-y := \
+ vkms_drv.o \
+ vkms_plane.o \
+ vkms_output.o \
+ vkms_crtc.o \
+ vkms_gem.o \
+ vkms_composer.o \
+ vkms_crc.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
new file mode 100644
index 000000000000..3d7c5e316d6e
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+#include "vkms_composer.h"
+#include <drm/drm_gem_framebuffer_helper.h>
+
+/**
+ * blend - belnd value at vaddr_src with value at vaddr_dst
+ * @vaddr_dst: destination address
+ * @vaddr_src: source address
+ * @dest: destination framebuffer's metadata
+ * @src: source framebuffer's metadata
+ *
+ * Blend value at vaddr_src with value at vaddr_dst.
+ * Currently, this function write value at vaddr_src on value
+ * at vaddr_dst using buffer's metadata to locate the new values
+ * from vaddr_src and their distenation at vaddr_dst.
+ *
+ * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
+ * instead of overwriting it.
+ */
+void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
+ struct vkms_crc_data *src)
+{
+ int i, j, j_dst, i_dst;
+ int offset_src, offset_dst;
+
+ int x_src = src->src.x1 >> 16;
+ int y_src = src->src.y1 >> 16;
+
+ int x_dst = src->dst.x1;
+ int y_dst = src->dst.y1;
+ int h_dst = drm_rect_height(&src->dst);
+ int w_dst = drm_rect_width(&src->dst);
+
+ int y_limit = y_src + h_dst;
+ int x_limit = x_src + w_dst;
+
+ for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
+ for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
+ offset_dst = dest->offset
+ + (i_dst * dest->pitch)
+ + (j_dst++ * dest->cpp);
+ offset_src = src->offset
+ + (i * src->pitch)
+ + (j * src->cpp);
+
+ memcpy(vaddr_dst + offset_dst,
+ vaddr_src + offset_src, sizeof(u32));
+ }
+ i_dst++;
+ }
+}
+
+void compose_cursor(struct vkms_crc_data *cursor,
+ struct vkms_crc_data *primary, void *vaddr_out)
+{
+ struct drm_gem_object *cursor_obj;
+ struct vkms_gem_object *cursor_vkms_obj;
+
+ cursor_obj = drm_gem_fb_get_obj(&cursor->fb, 0);
+ cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
+
+ if (WARN_ON(!cursor_vkms_obj->vaddr))
+ return;
+
+ blend(vaddr_out, cursor_vkms_obj->vaddr, primary, cursor);
+}
+
diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
new file mode 100644
index 000000000000..53fdee17a632
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_composer.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _VKMS_COMPOSER_H_
+#define _VKMS_COMPOSER_H_
+
+void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
+ struct vkms_crc_data *src);
+
+void compose_cursor(struct vkms_crc_data *cursor,
+ struct vkms_crc_data *primary, void *vaddr_out);
+
+#endif /* _VKMS_COMPOSER_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 4b3146d83265..3c6a35aba494 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
#include "vkms_drv.h"
+#include "vkms_composer.h"
#include <linux/crc32.h>
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
@@ -39,70 +40,6 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
return crc;
}
-/**
- * blend - belnd value at vaddr_src with value at vaddr_dst
- * @vaddr_dst: destination address
- * @vaddr_src: source address
- * @crc_dst: destination framebuffer's metadata
- * @crc_src: source framebuffer's metadata
- *
- * Blend value at vaddr_src with value at vaddr_dst.
- * Currently, this function write value at vaddr_src on value
- * at vaddr_dst using buffer's metadata to locate the new values
- * from vaddr_src and their distenation at vaddr_dst.
- *
- * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
- * instead of overwriting it.
- */
-static void blend(void *vaddr_dst, void *vaddr_src,
- struct vkms_crc_data *crc_dst,
- struct vkms_crc_data *crc_src)
-{
- int i, j, j_dst, i_dst;
- int offset_src, offset_dst;
-
- int x_src = crc_src->src.x1 >> 16;
- int y_src = crc_src->src.y1 >> 16;
-
- int x_dst = crc_src->dst.x1;
- int y_dst = crc_src->dst.y1;
- int h_dst = drm_rect_height(&crc_src->dst);
- int w_dst = drm_rect_width(&crc_src->dst);
-
- int y_limit = y_src + h_dst;
- int x_limit = x_src + w_dst;
-
- for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
- for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
- offset_dst = crc_dst->offset
- + (i_dst * crc_dst->pitch)
- + (j_dst++ * crc_dst->cpp);
- offset_src = crc_src->offset
- + (i * crc_src->pitch)
- + (j * crc_src->cpp);
-
- memcpy(vaddr_dst + offset_dst,
- vaddr_src + offset_src, sizeof(u32));
- }
- i_dst++;
- }
-}
-
-static void compose_cursor(struct vkms_crc_data *cursor_crc,
- struct vkms_crc_data *primary_crc, void *vaddr_out)
-{
- struct drm_gem_object *cursor_obj;
- struct vkms_gem_object *cursor_vkms_obj;
-
- cursor_obj = drm_gem_fb_get_obj(&cursor_crc->fb, 0);
- cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
-
- if (WARN_ON(!cursor_vkms_obj->vaddr))
- return;
-
- blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
-}
-
static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
struct vkms_crc_data *cursor_crc)
{
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to vkms_composer
2019-06-18 2:42 ` [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to vkms_composer Rodrigo Siqueira
@ 2019-06-18 9:27 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2019-06-18 9:27 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser, dri-devel, linux-kernel
On Mon, Jun 17, 2019 at 11:42:33PM -0300, Rodrigo Siqueira wrote:
> The vkms_crc file has functions related to compose operations which are
> not directly associated with CRC. This patch, move those function for a
> new file named vkms_composer.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
I guess my suggestion wasn't quite clear: I thought we should rename the
entire file from vkms_crc.c to vkms_composor.c. And then adjust functions
and datastructures to use vkms_composer instead of vkms_crc as the prefix.
-Daniel
> ---
> drivers/gpu/drm/vkms/Makefile | 9 +++-
> drivers/gpu/drm/vkms/vkms_composer.c | 69 ++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_composer.h | 12 +++++
> drivers/gpu/drm/vkms/vkms_crc.c | 65 +-------------------------
> 4 files changed, 90 insertions(+), 65 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_composer.c
> create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
>
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..b4c040854bd6 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_crc.o
> +vkms-y := \
> + vkms_drv.o \
> + vkms_plane.o \
> + vkms_output.o \
> + vkms_crtc.o \
> + vkms_gem.o \
> + vkms_composer.o \
> + vkms_crc.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
> new file mode 100644
> index 000000000000..3d7c5e316d6e
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include "vkms_composer.h"
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +/**
> + * blend - belnd value at vaddr_src with value at vaddr_dst
> + * @vaddr_dst: destination address
> + * @vaddr_src: source address
> + * @dest: destination framebuffer's metadata
> + * @src: source framebuffer's metadata
> + *
> + * Blend value at vaddr_src with value at vaddr_dst.
> + * Currently, this function write value at vaddr_src on value
> + * at vaddr_dst using buffer's metadata to locate the new values
> + * from vaddr_src and their distenation at vaddr_dst.
> + *
> + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> + * instead of overwriting it.
> + */
> +void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
> + struct vkms_crc_data *src)
> +{
> + int i, j, j_dst, i_dst;
> + int offset_src, offset_dst;
> +
> + int x_src = src->src.x1 >> 16;
> + int y_src = src->src.y1 >> 16;
> +
> + int x_dst = src->dst.x1;
> + int y_dst = src->dst.y1;
> + int h_dst = drm_rect_height(&src->dst);
> + int w_dst = drm_rect_width(&src->dst);
> +
> + int y_limit = y_src + h_dst;
> + int x_limit = x_src + w_dst;
> +
> + for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
> + for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
> + offset_dst = dest->offset
> + + (i_dst * dest->pitch)
> + + (j_dst++ * dest->cpp);
> + offset_src = src->offset
> + + (i * src->pitch)
> + + (j * src->cpp);
> +
> + memcpy(vaddr_dst + offset_dst,
> + vaddr_src + offset_src, sizeof(u32));
> + }
> + i_dst++;
> + }
> +}
> +
> +void compose_cursor(struct vkms_crc_data *cursor,
> + struct vkms_crc_data *primary, void *vaddr_out)
> +{
> + struct drm_gem_object *cursor_obj;
> + struct vkms_gem_object *cursor_vkms_obj;
> +
> + cursor_obj = drm_gem_fb_get_obj(&cursor->fb, 0);
> + cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> +
> + if (WARN_ON(!cursor_vkms_obj->vaddr))
> + return;
> +
> + blend(vaddr_out, cursor_vkms_obj->vaddr, primary, cursor);
> +}
> +
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
> new file mode 100644
> index 000000000000..53fdee17a632
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_composer.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _VKMS_COMPOSER_H_
> +#define _VKMS_COMPOSER_H_
> +
> +void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
> + struct vkms_crc_data *src);
> +
> +void compose_cursor(struct vkms_crc_data *cursor,
> + struct vkms_crc_data *primary, void *vaddr_out);
> +
> +#endif /* _VKMS_COMPOSER_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index 4b3146d83265..3c6a35aba494 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> #include "vkms_drv.h"
> +#include "vkms_composer.h"
> #include <linux/crc32.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> @@ -39,70 +40,6 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
> return crc;
> }
>
> -/**
> - * blend - belnd value at vaddr_src with value at vaddr_dst
> - * @vaddr_dst: destination address
> - * @vaddr_src: source address
> - * @crc_dst: destination framebuffer's metadata
> - * @crc_src: source framebuffer's metadata
> - *
> - * Blend value at vaddr_src with value at vaddr_dst.
> - * Currently, this function write value at vaddr_src on value
> - * at vaddr_dst using buffer's metadata to locate the new values
> - * from vaddr_src and their distenation at vaddr_dst.
> - *
> - * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> - * instead of overwriting it.
> - */
> -static void blend(void *vaddr_dst, void *vaddr_src,
> - struct vkms_crc_data *crc_dst,
> - struct vkms_crc_data *crc_src)
> -{
> - int i, j, j_dst, i_dst;
> - int offset_src, offset_dst;
> -
> - int x_src = crc_src->src.x1 >> 16;
> - int y_src = crc_src->src.y1 >> 16;
> -
> - int x_dst = crc_src->dst.x1;
> - int y_dst = crc_src->dst.y1;
> - int h_dst = drm_rect_height(&crc_src->dst);
> - int w_dst = drm_rect_width(&crc_src->dst);
> -
> - int y_limit = y_src + h_dst;
> - int x_limit = x_src + w_dst;
> -
> - for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
> - for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
> - offset_dst = crc_dst->offset
> - + (i_dst * crc_dst->pitch)
> - + (j_dst++ * crc_dst->cpp);
> - offset_src = crc_src->offset
> - + (i * crc_src->pitch)
> - + (j * crc_src->cpp);
> -
> - memcpy(vaddr_dst + offset_dst,
> - vaddr_src + offset_src, sizeof(u32));
> - }
> - i_dst++;
> - }
> -}
> -
> -static void compose_cursor(struct vkms_crc_data *cursor_crc,
> - struct vkms_crc_data *primary_crc, void *vaddr_out)
> -{
> - struct drm_gem_object *cursor_obj;
> - struct vkms_gem_object *cursor_vkms_obj;
> -
> - cursor_obj = drm_gem_fb_get_obj(&cursor_crc->fb, 0);
> - cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> -
> - if (WARN_ON(!cursor_vkms_obj->vaddr))
> - return;
> -
> - blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> -}
> -
> static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> struct vkms_crc_data *cursor_crc)
> {
> --
> 2.21.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2 2/5] drm/vkms: Rename crc_enabled to composer_enabled
2019-06-18 2:41 [PATCH V2 0/5] drm/vkms: Introduces writeback support Rodrigo Siqueira
2019-06-18 2:42 ` [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to vkms_composer Rodrigo Siqueira
@ 2019-06-18 2:43 ` Rodrigo Siqueira
2019-06-18 2:45 ` [PATCH V2 3/5] drm/vkms: Rename vkms_crc_data to vkms_data Rodrigo Siqueira
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 2:43 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser
Cc: dri-devel, linux-kernel
Rename crc_enabled to composer_enabled since it does more than just
compute a CRC.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_crc.c | 2 +-
drivers/gpu/drm/vkms/vkms_crtc.c | 2 +-
drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 3c6a35aba494..8b215677581f 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -165,7 +165,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
ret = vkms_crc_parse_source(src_name, &enabled);
spin_lock_irq(&out->lock);
- out->crc_enabled = enabled;
+ out->composer_enabled = enabled;
spin_unlock_irq(&out->lock);
return ret;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 14156ed70415..24a3ff0f7ff1 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -25,7 +25,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
DRM_ERROR("vkms failure on handling vblank");
state = output->crc_state;
- if (state && output->crc_enabled) {
+ if (state && output->composer_enabled) {
u64 frame = drm_crtc_accurate_vblank_count(crtc);
/* update frame_start only if a queued vkms_crc_work_handle()
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 4e7450111d45..a887c05ff70e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -72,7 +72,7 @@ struct vkms_output {
spinlock_t lock;
/* protected by @lock */
- bool crc_enabled;
+ bool composer_enabled;
struct vkms_crtc_state *crc_state;
spinlock_t crc_lock;
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V2 3/5] drm/vkms: Rename vkms_crc_data to vkms_data
2019-06-18 2:41 [PATCH V2 0/5] drm/vkms: Introduces writeback support Rodrigo Siqueira
2019-06-18 2:42 ` [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to vkms_composer Rodrigo Siqueira
2019-06-18 2:43 ` [PATCH V2 2/5] drm/vkms: Rename crc_enabled to composer_enabled Rodrigo Siqueira
@ 2019-06-18 2:45 ` Rodrigo Siqueira
2019-06-18 2:45 ` [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc Rodrigo Siqueira
2019-06-18 2:45 ` [PATCH V2 5/5] drm/vkms: Add support for writeback Rodrigo Siqueira
4 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 2:45 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser
Cc: dri-devel, linux-kernel
Rename the struct vkms_crc_data to vkms_data and also remove the CRC
prefix from variables that use this struct.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_composer.c | 8 +++----
drivers/gpu/drm/vkms/vkms_composer.h | 8 +++----
drivers/gpu/drm/vkms/vkms_crc.c | 14 +++++------
drivers/gpu/drm/vkms/vkms_drv.h | 6 ++---
drivers/gpu/drm/vkms/vkms_plane.c | 36 ++++++++++++++--------------
5 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 3d7c5e316d6e..c636dc672430 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -19,8 +19,8 @@
* Todo: Use the alpha value to blend vaddr_src with vaddr_dst
* instead of overwriting it.
*/
-void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
- struct vkms_crc_data *src)
+void blend(void *vaddr_dst, void *vaddr_src, struct vkms_data *dest,
+ struct vkms_data *src)
{
int i, j, j_dst, i_dst;
int offset_src, offset_dst;
@@ -52,8 +52,8 @@ void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
}
}
-void compose_cursor(struct vkms_crc_data *cursor,
- struct vkms_crc_data *primary, void *vaddr_out)
+void compose_cursor(struct vkms_data *cursor, struct vkms_data *primary,
+ void *vaddr_out)
{
struct drm_gem_object *cursor_obj;
struct vkms_gem_object *cursor_vkms_obj;
diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
index 53fdee17a632..93842b4b2eed 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.h
+++ b/drivers/gpu/drm/vkms/vkms_composer.h
@@ -3,10 +3,10 @@
#ifndef _VKMS_COMPOSER_H_
#define _VKMS_COMPOSER_H_
-void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
- struct vkms_crc_data *src);
+void blend(void *vaddr_dst, void *vaddr_src, struct vkms_data *dest,
+ struct vkms_data *src);
-void compose_cursor(struct vkms_crc_data *cursor,
- struct vkms_crc_data *primary, void *vaddr_out);
+void compose_cursor(struct vkms_data *cursor, struct vkms_data *primary,
+ void *vaddr_out);
#endif /* _VKMS_COMPOSER_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 8b215677581f..69d0decf14af 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -16,7 +16,7 @@
* 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_crc_data *crc_out)
+static uint32_t compute_crc(void *vaddr_out, struct vkms_data *crc_out)
{
int i, j, src_offset;
int x_src = crc_out->src.x1 >> 16;
@@ -40,8 +40,8 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
return crc;
}
-static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
- struct vkms_crc_data *cursor_crc)
+static uint32_t _vkms_get_crc(struct vkms_data *primary_crc,
+ struct vkms_data *cursor_crc)
{
struct drm_framebuffer *fb = &primary_crc->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
@@ -87,8 +87,8 @@ void vkms_crc_work_handle(struct work_struct *work)
crc_work);
struct drm_crtc *crtc = crtc_state->base.crtc;
struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
- struct vkms_crc_data *primary_crc = NULL;
- struct vkms_crc_data *cursor_crc = NULL;
+ struct vkms_data *primary_crc = NULL;
+ struct vkms_data *cursor_crc = NULL;
u32 crc32 = 0;
u64 frame_start, frame_end;
bool crc_pending;
@@ -110,10 +110,10 @@ void vkms_crc_work_handle(struct work_struct *work)
return;
if (crtc_state->num_active_planes >= 1)
- primary_crc = crtc_state->active_planes[0]->crc_data;
+ primary_crc = crtc_state->active_planes[0]->data;
if (crtc_state->num_active_planes == 2)
- cursor_crc = crtc_state->active_planes[1]->crc_data;
+ cursor_crc = crtc_state->active_planes[1]->data;
if (primary_crc)
crc32 = _vkms_get_crc(primary_crc, cursor_crc);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index a887c05ff70e..8d628d18105e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,7 +20,7 @@
extern bool enable_cursor;
-struct vkms_crc_data {
+struct vkms_data {
struct drm_framebuffer fb;
struct drm_rect src, dst;
unsigned int offset;
@@ -31,11 +31,11 @@ struct vkms_crc_data {
/**
* vkms_plane_state - Driver specific plane state
* @base: base plane state
- * @crc_data: data required for CRC computation
+ * @data: data required for compose computation
*/
struct vkms_plane_state {
struct drm_plane_state base;
- struct vkms_crc_data *crc_data;
+ struct vkms_data *data;
};
/**
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0fceb6258422..8cf40297cf56 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -18,20 +18,20 @@ static struct drm_plane_state *
vkms_plane_duplicate_state(struct drm_plane *plane)
{
struct vkms_plane_state *vkms_state;
- struct vkms_crc_data *crc_data;
+ struct vkms_data *data;
vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
if (!vkms_state)
return NULL;
- crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
- if (!crc_data) {
- DRM_DEBUG_KMS("Couldn't allocate crc_data\n");
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ DRM_DEBUG_KMS("Couldn't allocate vkms_data\n");
kfree(vkms_state);
return NULL;
}
- vkms_state->crc_data = crc_data;
+ vkms_state->data = data;
__drm_atomic_helper_plane_duplicate_state(plane,
&vkms_state->base);
@@ -49,12 +49,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
/* dropping the reference we acquired in
* vkms_primary_plane_update()
*/
- if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
- drm_framebuffer_put(&vkms_state->crc_data->fb);
+ if (drm_framebuffer_read_refcount(&vkms_state->data->fb))
+ drm_framebuffer_put(&vkms_state->data->fb);
}
- kfree(vkms_state->crc_data);
- vkms_state->crc_data = NULL;
+ kfree(vkms_state->data);
+ vkms_state->data = NULL;
__drm_atomic_helper_plane_destroy_state(old_state);
kfree(vkms_state);
@@ -91,21 +91,21 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
{
struct vkms_plane_state *vkms_plane_state;
struct drm_framebuffer *fb = plane->state->fb;
- struct vkms_crc_data *crc_data;
+ struct vkms_data *data;
if (!plane->state->crtc || !fb)
return;
vkms_plane_state = to_vkms_plane_state(plane->state);
- crc_data = vkms_plane_state->crc_data;
- memcpy(&crc_data->src, &plane->state->src, sizeof(struct drm_rect));
- memcpy(&crc_data->dst, &plane->state->dst, sizeof(struct drm_rect));
- memcpy(&crc_data->fb, fb, sizeof(struct drm_framebuffer));
- drm_framebuffer_get(&crc_data->fb);
- crc_data->offset = fb->offsets[0];
- crc_data->pitch = fb->pitches[0];
- crc_data->cpp = fb->format->cpp[0];
+ data = vkms_plane_state->data;
+ memcpy(&data->src, &plane->state->src, sizeof(struct drm_rect));
+ memcpy(&data->dst, &plane->state->dst, sizeof(struct drm_rect));
+ memcpy(&data->fb, fb, sizeof(struct drm_framebuffer));
+ drm_framebuffer_get(&data->fb);
+ data->offset = fb->offsets[0];
+ data->pitch = fb->pitches[0];
+ data->cpp = fb->format->cpp[0];
}
static int vkms_plane_atomic_check(struct drm_plane *plane,
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc
2019-06-18 2:41 [PATCH V2 0/5] drm/vkms: Introduces writeback support Rodrigo Siqueira
` (2 preceding siblings ...)
2019-06-18 2:45 ` [PATCH V2 3/5] drm/vkms: Rename vkms_crc_data to vkms_data Rodrigo Siqueira
@ 2019-06-18 2:45 ` Rodrigo Siqueira
2019-06-18 7:56 ` Simon Ser
2019-06-18 9:29 ` Daniel Vetter
2019-06-18 2:45 ` [PATCH V2 5/5] drm/vkms: Add support for writeback Rodrigo Siqueira
4 siblings, 2 replies; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 2:45 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser
Cc: dri-devel, linux-kernel
When vkms calls drm_universal_plane_init(), it sets 0 for the
possible_crtcs parameter which works well for a single encoder and
connector; however, this approach is not flexible and does not fit well
for vkms. This commit adds an index parameter for vkms_plane_init()
which makes code flexible and enables vkms to support other DRM features.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
drivers/gpu/drm/vkms/vkms_drv.h | 4 ++--
drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
drivers/gpu/drm/vkms/vkms_plane.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index cc53ef88a331..966b3d653189 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -127,7 +127,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
dev->mode_config.preferred_depth = 24;
dev->mode_config.helper_private = &vkms_mode_config_helpers;
- return vkms_output_init(vkmsdev);
+ return vkms_output_init(vkmsdev, 0);
}
static int __init vkms_init(void)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 8d628d18105e..ad63dbe5e994 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -115,10 +115,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
int *max_error, ktime_t *vblank_time,
bool in_vblank_irq);
-int vkms_output_init(struct vkms_device *vkmsdev);
+int vkms_output_init(struct vkms_device *vkmsdev, int index);
struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type);
+ enum drm_plane_type type, int index);
/* Gem stuff */
struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 56fb5c2a2315..fb1941a6522c 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -35,7 +35,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
.get_modes = vkms_conn_get_modes,
};
-int vkms_output_init(struct vkms_device *vkmsdev)
+int vkms_output_init(struct vkms_device *vkmsdev, int index)
{
struct vkms_output *output = &vkmsdev->output;
struct drm_device *dev = &vkmsdev->drm;
@@ -45,12 +45,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
struct drm_plane *primary, *cursor = NULL;
int ret;
- primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
+ primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
if (IS_ERR(primary))
return PTR_ERR(primary);
if (enable_cursor) {
- cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
+ cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
if (IS_ERR(cursor)) {
ret = PTR_ERR(cursor);
goto err_cursor;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 8cf40297cf56..3f8e8b53f3bb 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -176,7 +176,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
};
struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type)
+ enum drm_plane_type type, int index)
{
struct drm_device *dev = &vkmsdev->drm;
const struct drm_plane_helper_funcs *funcs;
@@ -198,7 +198,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
funcs = &vkms_primary_helper_funcs;
}
- ret = drm_universal_plane_init(dev, plane, 0,
+ ret = drm_universal_plane_init(dev, plane, 1 << index,
&vkms_plane_funcs,
formats, nformats,
NULL, type, NULL);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc
2019-06-18 2:45 ` [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc Rodrigo Siqueira
@ 2019-06-18 7:56 ` Simon Ser
2019-06-18 11:09 ` Daniel Vetter
2019-06-18 9:29 ` Daniel Vetter
1 sibling, 1 reply; 12+ messages in thread
From: Simon Ser @ 2019-06-18 7:56 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Interestingly, even with the previous code, possible_crtcs=1 was
exposed to userspace [1]. I think this is because of a safeguard in
drm_crtc_init_with_planes (drm_crtc.c:284) which sets the primary and
cursor plane's possible_crtcs to the first CRTC if zero.
If we want to warn on possible_crtcs=0, we should probably remove this
safeguard. Checking first whether this safeguard is used by any driver
is probably a good idea.
[1]: https://drmdb.emersion.fr/devices/f218d1242714
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc
2019-06-18 7:56 ` Simon Ser
@ 2019-06-18 11:09 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2019-06-18 11:09 UTC (permalink / raw)
To: Simon Ser
Cc: Rodrigo Siqueira, Brian Starkey, Liviu Dudau, Daniel Vetter,
Haneen Mohammed, dri-devel@lists.freedesktop.org,
linux-kernel@vger.kernel.org
On Tue, Jun 18, 2019 at 07:56:23AM +0000, Simon Ser wrote:
> Interestingly, even with the previous code, possible_crtcs=1 was
> exposed to userspace [1]. I think this is because of a safeguard in
> drm_crtc_init_with_planes (drm_crtc.c:284) which sets the primary and
> cursor plane's possible_crtcs to the first CRTC if zero.
>
> If we want to warn on possible_crtcs=0, we should probably remove this
> safeguard. Checking first whether this safeguard is used by any driver
> is probably a good idea.
>
> [1]: https://drmdb.emersion.fr/devices/f218d1242714
Yeaht it's a bit a mess, that's why I've suggested we should bite this
bullet and fix it for real. There's a bunch others such bitmasks that many
drivers seem to not set correctly.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc
2019-06-18 2:45 ` [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc Rodrigo Siqueira
2019-06-18 7:56 ` Simon Ser
@ 2019-06-18 9:29 ` Daniel Vetter
1 sibling, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2019-06-18 9:29 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser, dri-devel, linux-kernel
On Mon, Jun 17, 2019 at 11:45:29PM -0300, Rodrigo Siqueira wrote:
> When vkms calls drm_universal_plane_init(), it sets 0 for the
> possible_crtcs parameter which works well for a single encoder and
> connector; however, this approach is not flexible and does not fit well
> for vkms. This commit adds an index parameter for vkms_plane_init()
> which makes code flexible and enables vkms to support other DRM features.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 4 ++--
> drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
> drivers/gpu/drm/vkms/vkms_plane.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index cc53ef88a331..966b3d653189 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -127,7 +127,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> dev->mode_config.preferred_depth = 24;
> dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
> - return vkms_output_init(vkmsdev);
> + return vkms_output_init(vkmsdev, 0);
Having to hard-code the index because for the crtc you need the planes,
but for the planes you need drm_crtc_index of their crtc is annoying, but
not really a better way :-/
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> }
>
> static int __init vkms_init(void)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8d628d18105e..ad63dbe5e994 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -115,10 +115,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> int *max_error, ktime_t *vblank_time,
> bool in_vblank_irq);
>
> -int vkms_output_init(struct vkms_device *vkmsdev);
> +int vkms_output_init(struct vkms_device *vkmsdev, int index);
>
> struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type);
> + enum drm_plane_type type, int index);
>
> /* Gem stuff */
> struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 56fb5c2a2315..fb1941a6522c 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -35,7 +35,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -int vkms_output_init(struct vkms_device *vkmsdev)
> +int vkms_output_init(struct vkms_device *vkmsdev, int index)
> {
> struct vkms_output *output = &vkmsdev->output;
> struct drm_device *dev = &vkmsdev->drm;
> @@ -45,12 +45,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> struct drm_plane *primary, *cursor = NULL;
> int ret;
>
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> + primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> if (enable_cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> + cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> if (IS_ERR(cursor)) {
> ret = PTR_ERR(cursor);
> goto err_cursor;
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 8cf40297cf56..3f8e8b53f3bb 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -176,7 +176,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> };
>
> struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type)
> + enum drm_plane_type type, int index)
> {
> struct drm_device *dev = &vkmsdev->drm;
> const struct drm_plane_helper_funcs *funcs;
> @@ -198,7 +198,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> funcs = &vkms_primary_helper_funcs;
> }
>
> - ret = drm_universal_plane_init(dev, plane, 0,
> + ret = drm_universal_plane_init(dev, plane, 1 << index,
> &vkms_plane_funcs,
> formats, nformats,
> NULL, type, NULL);
> --
> 2.21.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2 5/5] drm/vkms: Add support for writeback
2019-06-18 2:41 [PATCH V2 0/5] drm/vkms: Introduces writeback support Rodrigo Siqueira
` (3 preceding siblings ...)
2019-06-18 2:45 ` [PATCH V2 4/5] drm/vkms: Use index instead of 0 in possible crtc Rodrigo Siqueira
@ 2019-06-18 2:45 ` Rodrigo Siqueira
2019-06-18 9:33 ` Daniel Vetter
4 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 2:45 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser
Cc: dri-devel, linux-kernel
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 V2:
- Rework signal completion (Brian)
- Integrates writeback with active_planes (Daniel)
- Compose cursor (Daniel)
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 3 +-
drivers/gpu/drm/vkms/vkms_drv.c | 7 ++
drivers/gpu/drm/vkms/vkms_drv.h | 6 +
drivers/gpu/drm/vkms/vkms_output.c | 6 +
drivers/gpu/drm/vkms/vkms_writeback.c | 166 ++++++++++++++++++++++++++
5 files changed, 187 insertions(+), 1 deletion(-)
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 b4c040854bd6..091e6fa643d1 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -6,6 +6,7 @@ vkms-y := \
vkms_crtc.o \
vkms_gem.o \
vkms_composer.o \
- vkms_crc.o
+ vkms_crc.o \
+ vkms_writeback.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 966b3d653189..d870779abf9d 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -30,6 +30,10 @@ bool enable_cursor;
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,
@@ -158,6 +162,9 @@ static int __init vkms_init(void)
goto out_fini;
}
+ if (enable_writeback)
+ DRM_INFO("Writeback connector enabled");
+
ret = vkms_modeset_init(vkms_device);
if (ret)
goto out_fini;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index ad63dbe5e994..bf3fa737b3d7 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -7,6 +7,7 @@
#include <drm/drm.h>
#include <drm/drm_gem.h>
#include <drm/drm_encoder.h>
+#include <drm/drm_writeback.h>
#include <linux/hrtimer.h>
#define XRES_MIN 20
@@ -19,6 +20,7 @@
#define YRES_MAX 8192
extern bool enable_cursor;
+extern bool enable_writeback;
struct vkms_data {
struct drm_framebuffer fb;
@@ -63,6 +65,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,4 +146,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
size_t *values_cnt);
void vkms_crc_work_handle(struct work_struct *work);
+/* Writeback */
+int 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 fb1941a6522c..3b093ae8f373 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -84,6 +84,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
+ if (enable_writeback) {
+ ret = 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..56632eb393cb
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+#include "vkms_composer.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 != DRM_FORMAT_XRGB8888) {
+ 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;
+
+ if (!job->fb)
+ return;
+
+ gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+ vkms_gem_vunmap(gem_obj);
+}
+
+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 vkms_crtc_state *crtc_state = output->crc_state;
+ struct drm_writeback_connector *wb_conn = &output->wb_connector;
+ struct drm_connector_state *conn_state = wb_conn->base.state;
+ void *priv_data = conn_state->writeback_job->priv;
+ struct vkms_data *primary_data = NULL;
+ struct vkms_data *cursor_data = NULL;
+ struct drm_framebuffer *fb = NULL;
+ struct vkms_gem_object *vkms_obj;
+ struct drm_gem_object *gem_obj;
+
+ if (!conn_state)
+ return;
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
+ DRM_DEBUG_DRIVER("Disable writeback\n");
+ return;
+ }
+
+ if (crtc_state->num_active_planes >= 1)
+ primary_data = crtc_state->active_planes[0]->data;
+
+ if (crtc_state->num_active_planes == 2)
+ cursor_data = crtc_state->active_planes[1]->data;
+
+ if (!primary_data)
+ return;
+
+ fb = &primary_data->fb;
+ gem_obj = drm_gem_fb_get_obj(fb, 0);
+ vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+
+ if (!vkms_obj->vaddr || !priv_data)
+ return;
+
+ drm_writeback_queue_job(wb_conn, state);
+
+ memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
+ if (cursor_data)
+ compose_cursor(cursor_data, primary_data, priv_data);
+
+ drm_writeback_signal_completion(wb_conn, 0);
+}
+
+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 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.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V2 5/5] drm/vkms: Add support for writeback
2019-06-18 2:45 ` [PATCH V2 5/5] drm/vkms: Add support for writeback Rodrigo Siqueira
@ 2019-06-18 9:33 ` Daniel Vetter
2019-06-18 22:10 ` Rodrigo Siqueira
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2019-06-18 9:33 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Haneen Mohammed,
Simon Ser, dri-devel, linux-kernel
On Mon, Jun 17, 2019 at 11:45:55PM -0300, 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 V2:
> - Rework signal completion (Brian)
> - Integrates writeback with active_planes (Daniel)
> - Compose cursor (Daniel)
Not quite what I had in mind ... my idea was to reuse the crc worker
(hence renaming all that stuff to vkms_composer). The problem here is that
now we have the blend/compose code duplicated, at least parts of it. Plus
if you enable both crc and writeback, then we compose 2x, which is a bit
too much.
Rough sketch of an implementation:
- add writeback_pending, like we have crc_pending already. Difference is
that writeback is one-shot, i.e. set it in atomic_check somewhere, clear
it in the crc_worker (well, composer_worker now). vblank hrtimer will
not re-enable that one (unlike crc_pending).
- in the crc worker, if writeback_pending is set, then compose everything
into the writeback buffer instead of into our private crc buffer. Except
that we use different memory, crc computation will be done exactly the
same. For subsequent frames with the same crtc_state we will again use
the normal crc buffer to compose & compute the crc.
The benefit here is that we'll only have one place in vkms that does
composing, so if we add lots more features in the future, it'll be much
easier to maintain. Also since this guarantees that the crc will match
exactly what we've written back, we can use the crc to help validate our
writeback code.
Cheers, Daniel
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> drivers/gpu/drm/vkms/Makefile | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 7 ++
> drivers/gpu/drm/vkms/vkms_drv.h | 6 +
> drivers/gpu/drm/vkms/vkms_output.c | 6 +
> drivers/gpu/drm/vkms/vkms_writeback.c | 166 ++++++++++++++++++++++++++
> 5 files changed, 187 insertions(+), 1 deletion(-)
> 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 b4c040854bd6..091e6fa643d1 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -6,6 +6,7 @@ vkms-y := \
> vkms_crtc.o \
> vkms_gem.o \
> vkms_composer.o \
> - vkms_crc.o
> + vkms_crc.o \
> + vkms_writeback.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 966b3d653189..d870779abf9d 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -30,6 +30,10 @@ bool enable_cursor;
> 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,
> @@ -158,6 +162,9 @@ static int __init vkms_init(void)
> goto out_fini;
> }
>
> + if (enable_writeback)
> + DRM_INFO("Writeback connector enabled");
> +
> ret = vkms_modeset_init(vkms_device);
> if (ret)
> goto out_fini;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index ad63dbe5e994..bf3fa737b3d7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -7,6 +7,7 @@
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
> #include <linux/hrtimer.h>
>
> #define XRES_MIN 20
> @@ -19,6 +20,7 @@
> #define YRES_MAX 8192
>
> extern bool enable_cursor;
> +extern bool enable_writeback;
>
> struct vkms_data {
> struct drm_framebuffer fb;
> @@ -63,6 +65,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,4 +146,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> size_t *values_cnt);
> void vkms_crc_work_handle(struct work_struct *work);
>
> +/* Writeback */
> +int 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 fb1941a6522c..3b093ae8f373 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -84,6 +84,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> + if (enable_writeback) {
> + ret = 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..56632eb393cb
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include "vkms_composer.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 != DRM_FORMAT_XRGB8888) {
> + 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;
> +
> + if (!job->fb)
> + return;
> +
> + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> + vkms_gem_vunmap(gem_obj);
> +}
> +
> +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 vkms_crtc_state *crtc_state = output->crc_state;
> + struct drm_writeback_connector *wb_conn = &output->wb_connector;
> + struct drm_connector_state *conn_state = wb_conn->base.state;
> + void *priv_data = conn_state->writeback_job->priv;
> + struct vkms_data *primary_data = NULL;
> + struct vkms_data *cursor_data = NULL;
> + struct drm_framebuffer *fb = NULL;
> + struct vkms_gem_object *vkms_obj;
> + struct drm_gem_object *gem_obj;
> +
> + if (!conn_state)
> + return;
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> + DRM_DEBUG_DRIVER("Disable writeback\n");
> + return;
> + }
> +
> + if (crtc_state->num_active_planes >= 1)
> + primary_data = crtc_state->active_planes[0]->data;
> +
> + if (crtc_state->num_active_planes == 2)
> + cursor_data = crtc_state->active_planes[1]->data;
> +
> + if (!primary_data)
> + return;
> +
> + fb = &primary_data->fb;
> + gem_obj = drm_gem_fb_get_obj(fb, 0);
> + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +
> + if (!vkms_obj->vaddr || !priv_data)
> + return;
> +
> + drm_writeback_queue_job(wb_conn, state);
> +
> + memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> + if (cursor_data)
> + compose_cursor(cursor_data, primary_data, priv_data);
> +
> + drm_writeback_signal_completion(wb_conn, 0);
> +}
> +
> +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 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.21.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V2 5/5] drm/vkms: Add support for writeback
2019-06-18 9:33 ` Daniel Vetter
@ 2019-06-18 22:10 ` Rodrigo Siqueira
0 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18 22:10 UTC (permalink / raw)
To: Rodrigo Siqueira, Brian Starkey, Liviu Dudau, Haneen Mohammed,
Simon Ser, dri-devel, Linux Kernel Mailing List
Cc: Daniel Vetter
On Tue, Jun 18, 2019 at 6:34 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 17, 2019 at 11:45:55PM -0300, 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 V2:
> > - Rework signal completion (Brian)
> > - Integrates writeback with active_planes (Daniel)
> > - Compose cursor (Daniel)
>
> Not quite what I had in mind ... my idea was to reuse the crc worker
> (hence renaming all that stuff to vkms_composer). The problem here is that
> now we have the blend/compose code duplicated, at least parts of it. Plus
> if you enable both crc and writeback, then we compose 2x, which is a bit
> too much.
>
> Rough sketch of an implementation:
>
> - add writeback_pending, like we have crc_pending already. Difference is
> that writeback is one-shot, i.e. set it in atomic_check somewhere, clear
> it in the crc_worker (well, composer_worker now). vblank hrtimer will
> not re-enable that one (unlike crc_pending).
>
> - in the crc worker, if writeback_pending is set, then compose everything
> into the writeback buffer instead of into our private crc buffer. Except
> that we use different memory, crc computation will be done exactly the
> same. For subsequent frames with the same crtc_state we will again use
> the normal crc buffer to compose & compute the crc.
>
> The benefit here is that we'll only have one place in vkms that does
> composing, so if we add lots more features in the future, it'll be much
> easier to maintain. Also since this guarantees that the crc will match
> exactly what we've written back, we can use the crc to help validate our
> writeback code.
Thanks for your review and comments, I think that I understood all of
them. I’ll prepare a V3.
> Cheers, Daniel
>
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> > drivers/gpu/drm/vkms/Makefile | 3 +-
> > drivers/gpu/drm/vkms/vkms_drv.c | 7 ++
> > drivers/gpu/drm/vkms/vkms_drv.h | 6 +
> > drivers/gpu/drm/vkms/vkms_output.c | 6 +
> > drivers/gpu/drm/vkms/vkms_writeback.c | 166 ++++++++++++++++++++++++++
> > 5 files changed, 187 insertions(+), 1 deletion(-)
> > 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 b4c040854bd6..091e6fa643d1 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -6,6 +6,7 @@ vkms-y := \
> > vkms_crtc.o \
> > vkms_gem.o \
> > vkms_composer.o \
> > - vkms_crc.o
> > + vkms_crc.o \
> > + vkms_writeback.o
> >
> > obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 966b3d653189..d870779abf9d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -30,6 +30,10 @@ bool enable_cursor;
> > 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,
> > @@ -158,6 +162,9 @@ static int __init vkms_init(void)
> > goto out_fini;
> > }
> >
> > + if (enable_writeback)
> > + DRM_INFO("Writeback connector enabled");
> > +
> > ret = vkms_modeset_init(vkms_device);
> > if (ret)
> > goto out_fini;
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index ad63dbe5e994..bf3fa737b3d7 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,7 @@
> > #include <drm/drm.h>
> > #include <drm/drm_gem.h>
> > #include <drm/drm_encoder.h>
> > +#include <drm/drm_writeback.h>
> > #include <linux/hrtimer.h>
> >
> > #define XRES_MIN 20
> > @@ -19,6 +20,7 @@
> > #define YRES_MAX 8192
> >
> > extern bool enable_cursor;
> > +extern bool enable_writeback;
> >
> > struct vkms_data {
> > struct drm_framebuffer fb;
> > @@ -63,6 +65,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,4 +146,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > size_t *values_cnt);
> > void vkms_crc_work_handle(struct work_struct *work);
> >
> > +/* Writeback */
> > +int 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 fb1941a6522c..3b093ae8f373 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -84,6 +84,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > goto err_attach;
> > }
> >
> > + if (enable_writeback) {
> > + ret = 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..56632eb393cb
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,166 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include "vkms_composer.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 != DRM_FORMAT_XRGB8888) {
> > + 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;
> > +
> > + if (!job->fb)
> > + return;
> > +
> > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > + vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +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 vkms_crtc_state *crtc_state = output->crc_state;
> > + struct drm_writeback_connector *wb_conn = &output->wb_connector;
> > + struct drm_connector_state *conn_state = wb_conn->base.state;
> > + void *priv_data = conn_state->writeback_job->priv;
> > + struct vkms_data *primary_data = NULL;
> > + struct vkms_data *cursor_data = NULL;
> > + struct drm_framebuffer *fb = NULL;
> > + struct vkms_gem_object *vkms_obj;
> > + struct drm_gem_object *gem_obj;
> > +
> > + if (!conn_state)
> > + return;
> > +
> > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > + DRM_DEBUG_DRIVER("Disable writeback\n");
> > + return;
> > + }
> > +
> > + if (crtc_state->num_active_planes >= 1)
> > + primary_data = crtc_state->active_planes[0]->data;
> > +
> > + if (crtc_state->num_active_planes == 2)
> > + cursor_data = crtc_state->active_planes[1]->data;
> > +
> > + if (!primary_data)
> > + return;
> > +
> > + fb = &primary_data->fb;
> > + gem_obj = drm_gem_fb_get_obj(fb, 0);
> > + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +
> > + if (!vkms_obj->vaddr || !priv_data)
> > + return;
> > +
> > + drm_writeback_queue_job(wb_conn, state);
> > +
> > + memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
> > + if (cursor_data)
> > + compose_cursor(cursor_data, primary_data, priv_data);
> > +
> > + drm_writeback_signal_completion(wb_conn, 0);
> > +}
> > +
> > +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 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.21.0
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Rodrigo Siqueira
https://siqueira.tech
^ permalink raw reply [flat|nested] 12+ messages in thread