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@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>
