Hi,
On 08/30, Rodrigo Siqueira wrote:
> In the vkms_composer.c, some of the functions related to CRC and compose
> have interdependence between each other. This patch reworks some
> functions inside vkms_composer to make crc and composer computation
> decoupled.
>
> This patch is preparation work for making vkms able to support new
> features.
>
> Tested-by: Melissa Wen
> Signed-off-by: Rodrigo Siqueira
> ---
> 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 eaecc5a6c5db..f67d1baf1942 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -131,35 +131,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 = _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;
> }
>
> /**
> @@ -180,9 +176,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(>composer_lock);
> frame_start = crtc_state->frame_start;
> @@ -206,14 +204,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(_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++, );
> +
> + kfree(vaddr_out);
> }
>
> static const char * const pipe_crc_sources[] = {"auto"};
> --
> 2.28.0
>
I noticed two improvements: for a pre-existing op (alloc), but that
could affect the logic of the next patch; and another one (kfree -
release vaddr_out earlier, when it’s no longer needed) that you also
adjusted in the next one and is harmless here.
So, looking as a preparation for the writeback patch, I tested and
consider that this rework keeps operations stable here.
Reviewed-by: Melissa Wen
