* [PATCH V4 0/3] drm/vkms: Introduces writeback support
@ 2020-05-11 11:55 ` Rodrigo Siqueira
0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: linux-kernel, dri-devel
This is the V4 version of a series that introduces basic writeback
support to VKMS. This patchset starts with a pre-work that aims to make
VKMS composer operations ready for getting the writeback support; it has
updates on the CRC functions and reworks a small part of the VKMS
framebuffer operations. Finally, the latest patch introduces the
writeback support in VKMS.
The previous series was reviewed and tested, this patchset only rebases
the code on the latest version of drm-misc-next (I also tested it). It
is important to highlight, that we have an IGT test for validating
writeback feature as can be seen at:
IGT writeback tests: https://patchwork.freedesktop.org/series/68352/
Best Regards
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 | 94 +++++++++++------
drivers/gpu/drm/vkms/vkms_drv.c | 4 +
drivers/gpu/drm/vkms/vkms_drv.h | 8 ++
drivers/gpu/drm/vkms/vkms_output.c | 10 ++
drivers/gpu/drm/vkms/vkms_writeback.c | 142 ++++++++++++++++++++++++++
6 files changed, 233 insertions(+), 34 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
--
2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-05-11 11:55 ` Rodrigo Siqueira
-1 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: linux-kernel, dri-devel, Rodrigo Siqueira
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
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 <rodrigosiqueiramelo@gmail.com>
---
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 4af2f19480f4..258e659ecfba 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.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer
@ 2020-05-11 11:55 ` Rodrigo Siqueira
0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: Rodrigo Siqueira, linux-kernel, dri-devel
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
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 <rodrigosiqueiramelo@gmail.com>
---
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 4af2f19480f4..258e659ecfba 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.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-06-02 11:19 ` Emil Velikov
-1 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 11:19 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike, Rodrigo Siqueira,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi Rodrigo,
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> -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);
It would be clearer if you keep the to alloc (or not for wb) in the
caller. As-is it's very subtle and error prone.
> + 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;
> +
This early return changes the functionality. Namely the
drm_crtc_add_crc_entry(.... 0) is now missing. I don't recall much
about the crc to judge if that's a genuine bugfix, or newly introduced
bug.
In the former case, it should be a separate patch.
> + 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);
Nit: move the free() just after compute_crc() - it's not needed for
the drm_crtc_add_crc_entry().
-Emil
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V4 1/3] drm/vkms: Decouple crc operations from composer
@ 2020-06-02 11:19 ` Emil Velikov
0 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 11:19 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Rodrigo Siqueira, Liviu Dudau, Linux-Kernel@Vger. Kernel. Org,
Leandro Ribeiro, Helen Koike, ML dri-devel
Hi Rodrigo,
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> -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);
It would be clearer if you keep the to alloc (or not for wb) in the
caller. As-is it's very subtle and error prone.
> + 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;
> +
This early return changes the functionality. Namely the
drm_crtc_add_crc_entry(.... 0) is now missing. I don't recall much
about the crc to judge if that's a genuine bugfix, or newly introduced
bug.
In the former case, it should be a separate patch.
> + 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);
Nit: move the free() just after compute_crc() - it's not needed for
the drm_crtc_add_crc_entry().
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-05-11 11:55 ` Rodrigo Siqueira
-1 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: linux-kernel, dri-devel, Rodrigo Siqueira
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
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.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 258e659ecfba..686d25e7b01d 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -9,33 +9,40 @@
#include "vkms_drv.h"
+static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
+ const struct vkms_composer *composer)
+{
+ int src_offset = composer->offset + (y * composer->pitch)
+ + (x * composer->cpp);
+
+ return *(u32 *)&buffer[src_offset];
+}
+
/**
* 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;
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;
+ u32 crc = 0, pixel = 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);
+ for (y = y_src; y < y_src + h_src; ++y) {
+ for (x = x_src; x < x_src + w_src; ++x) {
/* XRGB format ignores Alpha channel */
- memset(vaddr_out + src_offset + 24, 0, 8);
- crc = crc32_le(crc, vaddr_out + src_offset,
- sizeof(u32));
+ pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+ bitmap_clear((void *)&pixel, 0, 8);
+ crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
@ 2020-05-11 11:55 ` Rodrigo Siqueira
0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: Rodrigo Siqueira, linux-kernel, dri-devel
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
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.
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 258e659ecfba..686d25e7b01d 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -9,33 +9,40 @@
#include "vkms_drv.h"
+static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
+ const struct vkms_composer *composer)
+{
+ int src_offset = composer->offset + (y * composer->pitch)
+ + (x * composer->cpp);
+
+ return *(u32 *)&buffer[src_offset];
+}
+
/**
* 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;
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;
+ u32 crc = 0, pixel = 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);
+ for (y = y_src; y < y_src + h_src; ++y) {
+ for (x = x_src; x < x_src + w_src; ++x) {
/* XRGB format ignores Alpha channel */
- memset(vaddr_out + src_offset + 24, 0, 8);
- crc = crc32_le(crc, vaddr_out + src_offset,
- sizeof(u32));
+ pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+ bitmap_clear((void *)&pixel, 0, 8);
+ crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
}
}
--
2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-05-12 11:34 ` Emil Velikov
-1 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-05-12 11:34 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike, Rodrigo Siqueira,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi Rodrigo,
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
>
> From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>
> 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.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 258e659ecfba..686d25e7b01d 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -9,33 +9,40 @@
>
> #include "vkms_drv.h"
>
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> + const struct vkms_composer *composer)
> +{
> + int src_offset = composer->offset + (y * composer->pitch)
> + + (x * composer->cpp);
> +
> + return *(u32 *)&buffer[src_offset];
> +}
> +
> /**
> * 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;
> 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;
> + u32 crc = 0, pixel = 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);
> + for (y = y_src; y < y_src + h_src; ++y) {
> + for (x = x_src; x < x_src + w_src; ++x) {
> /* XRGB format ignores Alpha channel */
> - memset(vaddr_out + src_offset + 24, 0, 8);
> - crc = crc32_le(crc, vaddr_out + src_offset,
> - sizeof(u32));
> + pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> + bitmap_clear((void *)&pixel, 0, 8);
> + crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> }
> }
>
IMHO using something like the following makes the code far simpler and clearer.
offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);
for (i = 0; i < h_src; i++, offset += composer->pitch) {
for (j = 0; j < w_src; j++, offset += composer->cpp) {
pixel = get_pixel_from_buffer(vaddr, offset);
crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed
}
}
With the bitmap_clear() and related comment moved into get_pixel_from_buffer().
-Emil
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
@ 2020-05-12 11:34 ` Emil Velikov
0 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-05-12 11:34 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Rodrigo Siqueira, Liviu Dudau, Linux-Kernel@Vger. Kernel. Org,
Leandro Ribeiro, Helen Koike, ML dri-devel
Hi Rodrigo,
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
>
> From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>
> 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.
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 258e659ecfba..686d25e7b01d 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -9,33 +9,40 @@
>
> #include "vkms_drv.h"
>
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> + const struct vkms_composer *composer)
> +{
> + int src_offset = composer->offset + (y * composer->pitch)
> + + (x * composer->cpp);
> +
> + return *(u32 *)&buffer[src_offset];
> +}
> +
> /**
> * 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;
> 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;
> + u32 crc = 0, pixel = 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);
> + for (y = y_src; y < y_src + h_src; ++y) {
> + for (x = x_src; x < x_src + w_src; ++x) {
> /* XRGB format ignores Alpha channel */
> - memset(vaddr_out + src_offset + 24, 0, 8);
> - crc = crc32_le(crc, vaddr_out + src_offset,
> - sizeof(u32));
> + pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> + bitmap_clear((void *)&pixel, 0, 8);
> + crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> }
> }
>
IMHO using something like the following makes the code far simpler and clearer.
offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);
for (i = 0; i < h_src; i++, offset += composer->pitch) {
for (j = 0; j < w_src; j++, offset += composer->cpp) {
pixel = get_pixel_from_buffer(vaddr, offset);
crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed
}
}
With the bitmap_clear() and related comment moved into get_pixel_from_buffer().
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
2020-05-12 11:34 ` Emil Velikov
@ 2020-06-02 11:24 ` Emil Velikov
-1 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 11:24 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike, Rodrigo Siqueira,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On Tue, 12 May 2020 at 12:34, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Rodrigo,
>
> On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> >
> > From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >
> > 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.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> > drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 258e659ecfba..686d25e7b01d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -9,33 +9,40 @@
> >
> > #include "vkms_drv.h"
> >
> > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > + const struct vkms_composer *composer)
> > +{
> > + int src_offset = composer->offset + (y * composer->pitch)
> > + + (x * composer->cpp);
> > +
> > + return *(u32 *)&buffer[src_offset];
> > +}
> > +
> > /**
> > * 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;
> > 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;
> > + u32 crc = 0, pixel = 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);
> > + for (y = y_src; y < y_src + h_src; ++y) {
> > + for (x = x_src; x < x_src + w_src; ++x) {
> > /* XRGB format ignores Alpha channel */
> > - memset(vaddr_out + src_offset + 24, 0, 8);
> > - crc = crc32_le(crc, vaddr_out + src_offset,
> > - sizeof(u32));
> > + pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > + bitmap_clear((void *)&pixel, 0, 8);
> > + crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> > }
> > }
> >
> IMHO using something like the following makes the code far simpler and clearer.
>
> offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);
>
> for (i = 0; i < h_src; i++, offset += composer->pitch) {
> for (j = 0; j < w_src; j++, offset += composer->cpp) {
> pixel = get_pixel_from_buffer(vaddr, offset);
> crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed
> }
> }
>
> With the bitmap_clear() and related comment moved into get_pixel_from_buffer().
>
If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop
the cast (unless I'm missing something and it's really needed) for
crc32_le() this patch is:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
I would suggest (but it's not a requirement) that you simplify the
loop/offset calculation as separate patch in v5.
-Emil
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V4 2/3] drm/vkms: Compute CRC without change input data
@ 2020-06-02 11:24 ` Emil Velikov
0 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 11:24 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Rodrigo Siqueira, Liviu Dudau, Linux-Kernel@Vger. Kernel. Org,
Leandro Ribeiro, Helen Koike, ML dri-devel
On Tue, 12 May 2020 at 12:34, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Rodrigo,
>
> On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> >
> > From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >
> > 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.
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> > drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 258e659ecfba..686d25e7b01d 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -9,33 +9,40 @@
> >
> > #include "vkms_drv.h"
> >
> > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > + const struct vkms_composer *composer)
> > +{
> > + int src_offset = composer->offset + (y * composer->pitch)
> > + + (x * composer->cpp);
> > +
> > + return *(u32 *)&buffer[src_offset];
> > +}
> > +
> > /**
> > * 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;
> > 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;
> > + u32 crc = 0, pixel = 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);
> > + for (y = y_src; y < y_src + h_src; ++y) {
> > + for (x = x_src; x < x_src + w_src; ++x) {
> > /* XRGB format ignores Alpha channel */
> > - memset(vaddr_out + src_offset + 24, 0, 8);
> > - crc = crc32_le(crc, vaddr_out + src_offset,
> > - sizeof(u32));
> > + pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > + bitmap_clear((void *)&pixel, 0, 8);
> > + crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> > }
> > }
> >
> IMHO using something like the following makes the code far simpler and clearer.
>
> offset = composer->offset + (y_src * composer->pitch) + (x_src * composer->cpp);
>
> for (i = 0; i < h_src; i++, offset += composer->pitch) {
> for (j = 0; j < w_src; j++, offset += composer->cpp) {
> pixel = get_pixel_from_buffer(vaddr, offset);
> crc = crc32_le(crc, &pixel, sizeof(u32); // cast should not be needed
> }
> }
>
> With the bitmap_clear() and related comment moved into get_pixel_from_buffer().
>
If you fold the bitmap_clear() in get_pixel_from_buffer(), and drop
the cast (unless I'm missing something and it's really needed) for
crc32_le() this patch is:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
I would suggest (but it's not a requirement) that you simplify the
loop/offset calculation as separate patch in v5.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V4 3/3] drm/vkms: Add support for writeback
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-05-11 11:55 ` Rodrigo Siqueira
-1 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: linux-kernel, dri-devel, Rodrigo Siqueira
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
This patch implements the necessary functions to add writeback support
for vkms. This feature is useful for testing compositors if you don't
have hardware with writeback support.
Change in V3 (Daniel):
- If writeback is enabled, compose everything into the writeback buffer
instead of CRC private buffer.
- Guarantees that the CRC will match exactly what we have in the
writeback buffer.
Change in V2:
- Rework signal completion (Brian)
- Integrates writeback with active_planes (Daniel)
- Compose cursor (Daniel)
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 9 +-
drivers/gpu/drm/vkms/vkms_composer.c | 16 ++-
drivers/gpu/drm/vkms/vkms_drv.c | 4 +
drivers/gpu/drm/vkms/vkms_drv.h | 8 ++
drivers/gpu/drm/vkms/vkms_output.c | 10 ++
drivers/gpu/drm/vkms/vkms_writeback.c | 142 ++++++++++++++++++++++++++
6 files changed, 186 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 0b767d7efa24..333d3cead0e3 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,11 @@
# SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
+vkms-y := \
+ vkms_drv.o \
+ vkms_plane.o \
+ vkms_output.o \
+ vkms_crtc.o \
+ vkms_gem.o \
+ vkms_composer.o \
+ vkms_writeback.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 686d25e7b01d..19849e39f8b9 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -160,16 +160,17 @@ void vkms_composer_worker(struct work_struct *work)
struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
struct vkms_composer *primary_composer = NULL;
struct vkms_composer *cursor_composer = NULL;
+ bool crc_pending, wb_pending;
void *vaddr_out = NULL;
u32 crc32 = 0;
u64 frame_start, frame_end;
- bool crc_pending;
int ret;
spin_lock_irq(&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;
@@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
if (!primary_composer)
return;
+ if (wb_pending)
+ vaddr_out = crtc_state->active_writeback;
+
ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
if (ret) {
- if (ret == -EINVAL)
+ if (ret == -EINVAL && !wb_pending)
kfree(vaddr_out);
return;
}
@@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
while (frame_start <= frame_end)
drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+ 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);
+ return;
+ }
+
kfree(vaddr_out);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 1e8b2169d834..34dd74dc8eb0 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -39,6 +39,10 @@ bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
+bool enable_writeback;
+module_param_named(enable_writeback, enable_writeback, bool, 0444);
+MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
+
static const struct file_operations vkms_driver_fops = {
.owner = THIS_MODULE,
.open = drm_open,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f4036bb0b9a8..35f0b71413c9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -8,6 +8,7 @@
#include <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 */
bool crc_pending;
+ bool wb_pending;
u64 frame_start;
u64 frame_end;
};
@@ -63,6 +67,7 @@ struct vkms_output {
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
+ struct drm_writeback_connector wb_connector;
struct hrtimer vblank_hrtimer;
ktime_t period_ns;
struct drm_pending_vblank_event *event;
@@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
/* Composer Support */
void vkms_composer_worker(struct work_struct *work);
+/* Writeback */
+int enable_writeback_connector(struct vkms_device *vkmsdev);
+
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 85afb77e97f0..19ffc67affec 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
+ if (enable_writeback) {
+ ret = enable_writeback_connector(vkmsdev);
+ if (!ret) {
+ output->composer_enabled = true;
+ DRM_INFO("Writeback connector enabled");
+ } else {
+ 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..868f0d15ca9f
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,142 @@
+// 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 != 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 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;
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
+ DRM_DEBUG_DRIVER("Disable writeback\n");
+ return;
+ }
+
+ 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 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.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH V4 3/3] drm/vkms: Add support for writeback
@ 2020-05-11 11:55 ` Rodrigo Siqueira
0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Siqueira @ 2020-05-11 11:55 UTC (permalink / raw)
To: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike
Cc: Rodrigo Siqueira, linux-kernel, dri-devel
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
This patch implements the necessary functions to add writeback support
for vkms. This feature is useful for testing compositors if you don't
have hardware with writeback support.
Change in V3 (Daniel):
- If writeback is enabled, compose everything into the writeback buffer
instead of CRC private buffer.
- Guarantees that the CRC will match exactly what we have in the
writeback buffer.
Change in V2:
- Rework signal completion (Brian)
- Integrates writeback with active_planes (Daniel)
- Compose cursor (Daniel)
Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 9 +-
drivers/gpu/drm/vkms/vkms_composer.c | 16 ++-
drivers/gpu/drm/vkms/vkms_drv.c | 4 +
drivers/gpu/drm/vkms/vkms_drv.h | 8 ++
drivers/gpu/drm/vkms/vkms_output.c | 10 ++
drivers/gpu/drm/vkms/vkms_writeback.c | 142 ++++++++++++++++++++++++++
6 files changed, 186 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 0b767d7efa24..333d3cead0e3 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,11 @@
# SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
+vkms-y := \
+ vkms_drv.o \
+ vkms_plane.o \
+ vkms_output.o \
+ vkms_crtc.o \
+ vkms_gem.o \
+ vkms_composer.o \
+ vkms_writeback.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 686d25e7b01d..19849e39f8b9 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -160,16 +160,17 @@ void vkms_composer_worker(struct work_struct *work)
struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
struct vkms_composer *primary_composer = NULL;
struct vkms_composer *cursor_composer = NULL;
+ bool crc_pending, wb_pending;
void *vaddr_out = NULL;
u32 crc32 = 0;
u64 frame_start, frame_end;
- bool crc_pending;
int ret;
spin_lock_irq(&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;
@@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
if (!primary_composer)
return;
+ if (wb_pending)
+ vaddr_out = crtc_state->active_writeback;
+
ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
if (ret) {
- if (ret == -EINVAL)
+ if (ret == -EINVAL && !wb_pending)
kfree(vaddr_out);
return;
}
@@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
while (frame_start <= frame_end)
drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+ 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);
+ return;
+ }
+
kfree(vaddr_out);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 1e8b2169d834..34dd74dc8eb0 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -39,6 +39,10 @@ bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
+bool enable_writeback;
+module_param_named(enable_writeback, enable_writeback, bool, 0444);
+MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
+
static const struct file_operations vkms_driver_fops = {
.owner = THIS_MODULE,
.open = drm_open,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f4036bb0b9a8..35f0b71413c9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -8,6 +8,7 @@
#include <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 */
bool crc_pending;
+ bool wb_pending;
u64 frame_start;
u64 frame_end;
};
@@ -63,6 +67,7 @@ struct vkms_output {
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
+ struct drm_writeback_connector wb_connector;
struct hrtimer vblank_hrtimer;
ktime_t period_ns;
struct drm_pending_vblank_event *event;
@@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
/* Composer Support */
void vkms_composer_worker(struct work_struct *work);
+/* Writeback */
+int enable_writeback_connector(struct vkms_device *vkmsdev);
+
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 85afb77e97f0..19ffc67affec 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
+ if (enable_writeback) {
+ ret = enable_writeback_connector(vkmsdev);
+ if (!ret) {
+ output->composer_enabled = true;
+ DRM_INFO("Writeback connector enabled");
+ } else {
+ 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..868f0d15ca9f
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,142 @@
+// 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 != 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 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;
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
+ DRM_DEBUG_DRIVER("Disable writeback\n");
+ return;
+ }
+
+ 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 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.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH V4 3/3] drm/vkms: Add support for writeback
2020-05-11 11:55 ` Rodrigo Siqueira
@ 2020-06-02 12:14 ` Emil Velikov
-1 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 12:14 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Brian Starkey, Liviu Dudau, Daniel Vetter, Simon Ser,
Leandro Ribeiro, Helen Koike, Rodrigo Siqueira,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> +vkms-y := \
> + vkms_drv.o \
> + vkms_plane.o \
> + vkms_output.o \
> + vkms_crtc.o \
> + vkms_gem.o \
> + vkms_composer.o \
> + vkms_writeback.o
>
Nit: sort alphabetically
> @@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> + if (wb_pending)
> + vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.
> if (ret) {
> - if (ret == -EINVAL)
> + if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
> @@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
>
> + 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);
> + return;
> + }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()
if (wb_pending) {
signal()
...
} else {
free()
}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.
> static const struct file_operations vkms_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
> #include <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 */
Nit: s/below three/the following four/
> bool crc_pending;
> + bool wb_pending;
> u64 frame_start;
> u64 frame_end;
> };
> @@ -63,6 +67,7 @@ struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> + struct drm_writeback_connector wb_connector;
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> @@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
>
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
Don't forget appropriate name spacing - prefix the function with vkms.
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> + if (enable_writeback) {
> + ret = enable_writeback_connector(vkmsdev);
With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.
> + if (!ret) {
> + output->composer_enabled = true;
> + DRM_INFO("Writeback connector enabled");
> + } else {
> + DRM_ERROR("Failed to init writeback connector\n");
> + }
> + }
> +
> 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..868f0d15ca9f
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,142 @@
> +// 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>
> +
Nit: sort includes alphabetically.
> +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)
Drop the fb check.
> + 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) {
Use the vkms_wb_formats[], regardless if it's one entry or not.
> + 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;
Thinking out loud:
This function should be a helper that drivers can reuse and build
upon. The format might be tricky.
It's already in drm_writeback_connector::pixel_formats_blob_ptr, while
the function takes drm_writeback::encoder as argument.
But that for another patch series.
> +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;
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
Drop the fb check.
-Emil
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH V4 3/3] drm/vkms: Add support for writeback
@ 2020-06-02 12:14 ` Emil Velikov
0 siblings, 0 replies; 16+ messages in thread
From: Emil Velikov @ 2020-06-02 12:14 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Rodrigo Siqueira, Liviu Dudau, Linux-Kernel@Vger. Kernel. Org,
Leandro Ribeiro, Helen Koike, ML dri-devel
On Mon, 11 May 2020 at 12:55, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote:
> # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> +vkms-y := \
> + vkms_drv.o \
> + vkms_plane.o \
> + vkms_output.o \
> + vkms_crtc.o \
> + vkms_gem.o \
> + vkms_composer.o \
> + vkms_writeback.o
>
Nit: sort alphabetically
> @@ -191,9 +192,12 @@ void vkms_composer_worker(struct work_struct *work)
> if (!primary_composer)
> return;
>
> + if (wb_pending)
> + vaddr_out = crtc_state->active_writeback;
> +
> ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
Forgot to mention earlier - with the allocation happening in the
caller, compose_planes() can take void *vaddr_out.
> if (ret) {
> - if (ret == -EINVAL)
> + if (ret == -EINVAL && !wb_pending)
> kfree(vaddr_out);
> return;
> }
> @@ -206,6 +210,14 @@ void vkms_composer_worker(struct work_struct *work)
> while (frame_start <= frame_end)
> drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
>
> + 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);
> + return;
> + }
> +
Just like the free() move this above the drm_crtc_add_crc_entry()
if (wb_pending) {
signal()
...
} else {
free()
}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 1e8b2169d834..34dd74dc8eb0 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -39,6 +39,10 @@ bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
Why is this a module parameter? Let's always enable it and leave it to
userspace whether to use the wb or not.
> static const struct file_operations vkms_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f4036bb0b9a8..35f0b71413c9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -8,6 +8,7 @@
> #include <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 */
Nit: s/below three/the following four/
> bool crc_pending;
> + bool wb_pending;
> u64 frame_start;
> u64 frame_end;
> };
> @@ -63,6 +67,7 @@ struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> struct drm_connector connector;
> + struct drm_writeback_connector wb_connector;
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> @@ -144,4 +149,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
>
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
Don't forget appropriate name spacing - prefix the function with vkms.
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 85afb77e97f0..19ffc67affec 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -80,6 +80,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> + if (enable_writeback) {
> + ret = enable_writeback_connector(vkmsdev);
With wb connector always enabled, you can kill off the
vkms_output::composer_enabled all together. Plus the info/error
warnings (below) can go as well.
> + if (!ret) {
> + output->composer_enabled = true;
> + DRM_INFO("Writeback connector enabled");
> + } else {
> + DRM_ERROR("Failed to init writeback connector\n");
> + }
> + }
> +
> 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..868f0d15ca9f
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,142 @@
> +// 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>
> +
Nit: sort includes alphabetically.
> +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)
Drop the fb check.
> + 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) {
Use the vkms_wb_formats[], regardless if it's one entry or not.
> + 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;
Thinking out loud:
This function should be a helper that drivers can reuse and build
upon. The format might be tricky.
It's already in drm_writeback_connector::pixel_formats_blob_ptr, while
the function takes drm_writeback::encoder as argument.
But that for another patch series.
> +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;
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
Drop the fb check.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread