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.
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>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
