Re: [PATCH v3 2/2] drm/virtio: New fence for every plane update

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 10/22/24 07:44, Kasireddy, Vivek wrote:
>>  		virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle,
>> x, y,
>> -					      width, height, objs, vgfb->fence);
>> +					      width, height, objs,
>> +					      vgplane_st->fence);
>>  		virtio_gpu_notify(vgdev);
>> -
>> -		dma_fence_wait_timeout(&vgfb->fence->f, true,
>> +		dma_fence_wait_timeout(&vgplane_st->fence->f, true,
>>  				       msecs_to_jiffies(50));
>> -		dma_fence_put(&vgfb->fence->f);
>> -		vgfb->fence = NULL;
> Not sure if it makes any difference but would there be a problem if you unref
> the fence here (existing behavior) instead of deferring it until cleanup?

Previously fence was a part of virtio-gpu framebuffer, which was kind of
a hack. Maybe there was no better option back then, when this code was
written initially.

Now fence is a part of plane's atomic state, like it should be. We
shouldn't change atomic state at the commit time.

...
>> @@ -326,11 +348,9 @@ static void virtio_gpu_cursor_plane_update(struct
>> drm_plane *plane,
>>  			(vgdev, 0,
>>  			 plane->state->crtc_w,
>>  			 plane->state->crtc_h,
>> -			 0, 0, objs, vgfb->fence);
>> +			 0, 0, objs, vgplane_st->fence);
>>  		virtio_gpu_notify(vgdev);
>> -		dma_fence_wait(&vgfb->fence->f, true);
>> -		dma_fence_put(&vgfb->fence->f);
>> -		vgfb->fence = NULL;
> Same comment as above.
> Regardless, the patch LGTM.
> 
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

Thanks for the review :)

-- 
Best regards,
Dmitry