[PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Noralf Trønnes]

Den 23.11.2022 09.22, skrev Javier Martinez Canillas:
> Hello Noralf,
> 
> On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
>> From: Noralf Trønnes <noralf at tronnes.org>
>>
>> Complete the shadow fb access functions by also preparing imported buffers
>> for CPU access. Update the affected drivers that currently use
>> drm_gem_fb_begin_cpu_access().
>>
>> Through this change the following SHMEM drivers will now also make sure
>> their imported buffers are prepared for CPU access: cirrus, hyperv,
>> mgag200, vkms
>>
> 
> [...]
> 
>> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_sta
>>  {
>>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>  	struct drm_framebuffer *fb = plane_state->fb;
>> +	int ret;
>>  
>>  	if (!fb)
>>  		return 0;
>>  
>> -	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
>> +	ret = drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +	if (ret)
>> +		drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> +
>> +	return ret;
> 
> Makes sense to me to have the CPU access prepare here too.
> 
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index 53464afc2b9a..58a2f0113f24 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>>  	struct iosys_map dst;
>>  	unsigned int dst_pitch;
>> -	int ret = 0;
>>  	u8 *buf = NULL;
>>  
>>  	/* Align y to display page boundaries */
>> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>  	if (!buf)
>>  		return -ENOMEM;
>>  
>> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -	if (ret)
>> -		goto out_free;
>> -
>>  	iosys_map_set_vaddr(&dst, buf);
>>  	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
>>  
>> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -
> 
> The only thing I'm not sure about is that drivers used to call the begin/end
> CPU access only during the window while the BO data was accessed but now the
> windows will be slightly bigger if that happens in .{begin,end}_fb_access.
> 

I didn't think that could be an issue since userspace isn't touching the
buffer while the commit is ongoing anyways, but it's a complicated
machinery. We'll see what Daniel has to say.

Noralf.

> If that's not an issue then, I agree with your patch since it simplifies the
> logic in the drivers and gets rid of duplicated code.
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>