Hi Pekka,
On 4/20/22 08:23, Pekka Paalanen wrote:
On Mon, 4 Apr 2022 17:45:11 -0300
Igor Torrente <igormtorrente@xxxxxxxxx> wrote:
This commit is the groundwork to introduce new formats to the planes and
writeback buffer. As part of it, a new buffer metadata field is added to
`vkms_writeback_job`, this metadata is represented by the `vkms_composer`
struct.
Hi,
should this be talking about vkms_frame_info struct instead?
Yes it should. I will fix this. Thanks.
Also adds two new function pointers (`{wb,plane}_format_transform_func`)
are defined to handle format conversion to/from internal format.
These things will allow us, in the future, to have different compositing
and wb format types.
V2: Change the code to get the drm_framebuffer reference and not copy its
contents(Thomas Zimmermann).
V3: Drop the refcount in the wb code(Thomas Zimmermann).
V5: Add {wb,plane}_format_transform_func to vkms_writeback_job
and vkms_plane_state (Pekka Paalanen)
Signed-off-by: Igor Torrente <igormtorrente@xxxxxxxxx>
---
drivers/gpu/drm/vkms/vkms_composer.c | 4 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 31 +++++++++++++++++++++------
drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++-----
drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++++++++++++++---
4 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 2d946368a561..95029d2ebcac 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -153,7 +153,7 @@ static void compose_plane(struct vkms_frame_info *primary_plane_info,
struct vkms_frame_info *plane_frame_info,
void *vaddr_out)
{
- struct drm_framebuffer *fb = &plane_frame_info->fb;
+ struct drm_framebuffer *fb = plane_frame_info->fb;
void *vaddr;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
@@ -175,7 +175,7 @@ static int compose_active_planes(void **vaddr_out,
struct vkms_frame_info *primary_plane_info,
struct vkms_crtc_state *crtc_state)
{
- struct drm_framebuffer *fb = &primary_plane_info->fb;
+ struct drm_framebuffer *fb = primary_plane_info->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
const void *vaddr;
int i;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 2e6342164bef..2704cfb6904b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -22,13 +22,8 @@
#define NUM_OVERLAY_PLANES 8
-struct vkms_writeback_job {
- struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
- struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
-};
-
struct vkms_frame_info {
- struct drm_framebuffer fb;
+ struct drm_framebuffer *fb;
struct drm_rect src, dst;
struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
@@ -36,6 +31,29 @@ struct vkms_frame_info {
unsigned int cpp;
};
+struct pixel_argb_u16 {
+ u16 a, r, g, b;
+};
+
+struct line_buffer {
+ size_t n_pixels;
+ struct pixel_argb_u16 *pixels;
+};
+
+typedef void
+(*wb_format_transform_func)(struct vkms_frame_info *frame_info,
+ const struct line_buffer *buffer, int y);
+
+typedef void
+(*plane_format_transform_func)(struct line_buffer *buffer,
+ const struct vkms_frame_info *frame_info, int y);
It wasn't immediately obvious to me in which direction these function
types work from their names. The arguments are not wb and plane but
vkms_frame_info and line_buffer in both. The implementations of these
functions would have nothing specific to a wb or a plane either, would
they?
No, there's nothing specific.
Do you think adding {dst_,src_} would be enough?
(*wb_format_transform_func)(struct vkms_frame_info *dst_frame_info,
const struct line_buffer *src_buffer
(*plane_format_transform_func)(struct line_buffer *dst_buffer,
const struct vkms_frame_info *src_frame_info,
What about naming them frame_to_line_func and line_to_frame_func?
Sounds good. I will rename it.
+
+struct vkms_writeback_job {
+ struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
+ struct vkms_frame_info frame_info;
Which frame_info is this? Should the field be called wb_frame_info?
Considering it's already in the writeback_job struct do you think this
necessary?
In other words, what kind of misudertanding do you think can happen if
this variable stay without the `wb_` prefix?
I spent a few minutes trying to find a case, but nothing came to my
mind.
+ wb_format_transform_func format_func;
line_to_frame_func wb_write;
perhaps? The type explains the general type of the function, and the
field name refers to what it is used for.
+};
+
/**
* vkms_plane_state - Driver specific plane state
* @base: base plane state
@@ -44,6 +62,7 @@ struct vkms_frame_info {
struct vkms_plane_state {
struct drm_shadow_plane_state base;
struct vkms_frame_info *frame_info;
+ plane_format_transform_func format_func;
Similarly here, maybe
frame_to_line_func plane_read;
perhaps?
Yeah, sure.
};
struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index a56b0f76eddd..28752af0118c 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -50,12 +50,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
struct drm_crtc *crtc = vkms_state->base.base.crtc;
- if (crtc) {
+ if (crtc && vkms_state->frame_info->fb) {
/* dropping the reference we acquired in
* vkms_primary_plane_update()
*/
- if (drm_framebuffer_read_refcount(&vkms_state->frame_info->fb))
- drm_framebuffer_put(&vkms_state->frame_info->fb);
+ if (drm_framebuffer_read_refcount(vkms_state->frame_info->fb))
+ drm_framebuffer_put(vkms_state->frame_info->fb);
}
kfree(vkms_state->frame_info);
@@ -110,9 +110,9 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
frame_info = vkms_plane_state->frame_info;
memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
- memcpy(&frame_info->fb, fb, sizeof(struct drm_framebuffer));
+ frame_info->fb = fb;
This change, replacing the memcpy with storing a pointer, seems to be
another major point of this patch. Should it be a separate patch?
It doesn't seem to fit with the current commit message.
I have no idea what kind of locking or referencing a drm_framebuffer
would need, and I suspect that would be easier to review if it was a
patch of its own.
Makes sense. I will do that.
memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
- drm_framebuffer_get(&frame_info->fb);
+ drm_framebuffer_get(frame_info->fb);
Does drm_framebuffer_get() not return anything?
No, it doesn't actually. This function increments the ref count of this
struct and doesn't return anything.
To me it would be more idiomatic to write something like
frame_info->fb = drm_framebuffer_get(fb);
I spend few minutes trying to find a case but nothing comes to my mind.
I don't know if that pattern is used in the kernel, but I use it in
userspace to emphasise that frame_info owns a new reference rather than
borrowing someone else's.
Thanks,
pq
frame_info->offset = fb->offsets[0];
frame_info->pitch = fb->pitches[0];
frame_info->cpp = fb->format->cpp[0];
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 746cb0abc6ec..ad4bb1fb37ca 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -74,12 +74,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
if (!vkmsjob)
return -ENOMEM;
- ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data);
+ ret = drm_gem_fb_vmap(job->fb, vkmsjob->frame_info.map, vkmsjob->data);
if (ret) {
DRM_ERROR("vmap failed: %d\n", ret);
goto err_kfree;
}
+ vkmsjob->frame_info.fb = job->fb;
+ drm_framebuffer_get(vkmsjob->frame_info.fb);
+
job->priv = vkmsjob;
return 0;
@@ -98,7 +101,9 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
if (!job->fb)
return;
- drm_gem_fb_vunmap(job->fb, vkmsjob->map);
+ drm_gem_fb_vunmap(job->fb, vkmsjob->frame_info.map);
+
+ drm_framebuffer_put(vkmsjob->frame_info.fb);
vkmsdev = drm_device_to_vkms_device(job->fb->dev);
vkms_set_composer(&vkmsdev->output, false);
@@ -115,14 +120,23 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
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;
+ struct drm_framebuffer *fb = connector_state->writeback_job->fb;
+ struct vkms_writeback_job *active_wb;
+ struct vkms_frame_info *wb_frame_info;
if (!conn_state)
return;
vkms_set_composer(&vkmsdev->output, true);
+ active_wb = conn_state->writeback_job->priv;
+ wb_frame_info = &active_wb->frame_info;
+
spin_lock_irq(&output->composer_lock);
- crtc_state->active_writeback = conn_state->writeback_job->priv;
+ crtc_state->active_writeback = active_wb;
+ wb_frame_info->offset = fb->offsets[0];
+ wb_frame_info->pitch = fb->pitches[0];
+ wb_frame_info->cpp = fb->format->cpp[0];
crtc_state->wb_pending = true;
spin_unlock_irq(&output->composer_lock);
drm_writeback_queue_job(wb_conn, connector_state);
