Re: [PATCH v5 5/9] drm: vkms: Add fb information to `vkms_writeback_job`

On Mon, 4 Apr 2022 17:45:11 -0300 Igor Torrente igormtorrente@gmail.com 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?

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@gmail.com

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?

What about naming them frame_to_line_func and line_to_frame_func?

+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?

• 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?

};

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.

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?

To me it would be more idiomatic to write something like

frame_info->fb = drm_framebuffer_get(fb);

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);