Re: [PATCH v2 21/43] drm/fbdev-dma: Implement damage handling and deferred I/O

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Add support for damage handling and deferred I/O to fbdev-dma. This
> enables fbdev-dma to support all DMA-memory-based DRM drivers, even
> such with a dirty callback in their framebuffers.
>
> The patch adds the code for deferred I/O and also sets a dedicated
> helper for struct fb_ops.fb_mmap that support coherent mappings.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fbdev_dma.c | 65 ++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index 6c9427bb4053b..8ffd072368bca 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -4,6 +4,7 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_fb_dma_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_dma_helper.h>
> @@ -35,6 +36,22 @@ static int drm_fbdev_dma_fb_release(struct fb_info *info, int user)
>  	return 0;
>  }
>  
> +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(drm_fbdev_dma,
> +				   drm_fb_helper_damage_range,
> +				   drm_fb_helper_damage_area);
> +

Shouldn't this be FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS() instead ?

I know that right now the macros are the same but I believe that it was
added it for a reason ?

> +static int drm_fbdev_dma_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_framebuffer *fb = fb_helper->fb;
> +	struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
> +
> +	if (!dma->map_noncoherent)
> +		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);

I noticed that some drivers do:

                 vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));

I see that vm_get_page_prot() is a per-architecture function, but I don't
know about the implications of getting the pgprot_t from the vma->vm_flags
set or just using the current vma->vm_page_prot value...

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat