Re: [PATCH] drm/vkms: use a bitmask instead of a bool for composer users

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/vkms: vkms_wb_cleanup_job() clears VKMS_COMPOSER_WB for
  pending job
--

commit 881f833de91536e1dc29f1f083a56309194fa1be
Author: Oarora Etimis <oaroraetimis@gmail.com>

drm/vkms: use a bitmask instead of a bool for composer users

This commit replaces the single boolean composer_enabled flag with a
per-user bitmask (composer_users) and replaces vkms_set_composer() with
vkms_composer_get() and vkms_composer_put() so CRC capture and writeback
do not disable the composer for each other.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 0fc915a954bad..59f1dd50b5bd5 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -711,19 +711,34 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
>  	return 0;
>  }
>  
> -void vkms_set_composer(struct vkms_output *out, bool enabled)
> +void vkms_composer_get(struct vkms_output *out, u8 user)
>  {
> -	bool old_enabled;
> +	bool was_enabled;
>  
> -	if (enabled)
> -		drm_crtc_vblank_get(&out->crtc);
> +	if (drm_crtc_vblank_get(&out->crtc))
> +		return;
> +
> +	spin_lock_irq(&out->lock);
> +	was_enabled = out->composer_users;
> +	out->composer_users |= user;
> +	spin_unlock_irq(&out->lock);
> +
> +	/* Another user already holds the reference for the composer. */
> +	if (was_enabled)
> +		drm_crtc_vblank_put(&out->crtc);
> +}
> +
> +void vkms_composer_put(struct vkms_output *out, u8 user)
> +{
> +	bool was_set, enabled;
>  
>  	spin_lock_irq(&out->lock);
> -	old_enabled = out->composer_enabled;
> -	out->composer_enabled = enabled;
> +	was_set = out->composer_users & user;
> +	out->composer_users &= ~user;
> +	enabled = out->composer_users;
>  	spin_unlock_irq(&out->lock);
>  
> -	if (old_enabled)
> +	if (was_set && !enabled)
>  		drm_crtc_vblank_put(&out->crtc);
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 858df1f89f660..7e2f82e44b392 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -118,7 +118,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  
>  	drm_framebuffer_put(vkmsjob->wb_frame_info.fb);
>  
> -	vkms_set_composer(vkms_output, false);
> +	vkms_composer_put(vkms_output, VKMS_COMPOSER_WB);
>  	kfree(vkmsjob);
>  }
>  
> @@ -141,7 +141,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>  	if (!conn_state)
>  		return;
>  
> -	vkms_set_composer(output, true);
> +	vkms_composer_get(output, VKMS_COMPOSER_WB);
>  
>  	active_wb = conn_state->writeback_job->priv;
>  	wb_frame_info = &active_wb->wb_frame_info;

[Severity: Medium]
Can vkms_wb_cleanup_job() from a finished writeback job clear
VKMS_COMPOSER_WB while the next writeback job is already pending?

drm_writeback_signal_completion() signals the job's out_fence synchronously
before queueing job->cleanup_work on system_long_wq:

drivers/gpu/drm/drm_writeback.c:drm_writeback_signal_completion() {
    ...
		dma_fence_signal(out_fence);
		dma_fence_put(out_fence);
		job->out_fence = NULL;
	}

	INIT_WORK(&job->cleanup_work, cleanup_work);
	queue_work(system_long_wq, &job->cleanup_work);
}

Because VKMS_COMPOSER_WB is a single bit rather than a count, a second
writeback commit submitted after out_fence signals can overlap with the
first job's deferred cleanup:

CPU0                                  CPU1 (system_long_wq)
vkms_composer_worker()
  drm_writeback_signal_completion()
    dma_fence_signal(job1->out_fence)
    queue_work(..., &job1->cleanup_work)

/* userspace commits job2 */
vkms_wb_atomic_commit()
  vkms_composer_get(out, VKMS_COMPOSER_WB)
    /* VKMS_COMPOSER_WB already set */
                                      cleanup_work()
                                        ...
                                          vkms_wb_cleanup_job()
                                            vkms_composer_put(out,
                                                VKMS_COMPOSER_WB)
                                              out->composer_users &= ~user;
                                              drm_crtc_vblank_put(&out->crtc)

Once out->composer_users is cleared to 0, vkms_crtc_handle_vblank_timeout()
stops queuing vkms_composer_worker(), leaving job2 uncomposed and its
out_fence unsignaled.  Wait-for-fences interruption or prepare_fb failure in
drm_atomic_helper_commit() after vkms_wb_prepare_job() also runs
vkms_wb_cleanup_job() without a matching vkms_composer_get(), which similarly
clears VKMS_COMPOSER_WB for an in-flight job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-composer-v1-1-9d5fa94656f8@gmail.com?part=1