Re: [PATCH v2] drm/exynos: use prime helpers

[Aaron Plattner <aplattner@nvidia.com>]

On 12/06/2012 10:36 PM, Inki Dae wrote:
>
> Hi,
>
> CCing media guys.
>
> I agree with you but we should consider one issue released to v4l2.
>
> As you may know, V4L2-based driver uses vb2 as buffer manager and the
> vb2 includes dmabuf feature>(import and export) And v4l2 uses streaming
> concept>(qbuf and dqbuf)
> With dmabuf and iommu, generally qbuf imports a fd into its own buffer
> and maps it with its own iommu table calling dma_buf_map_attachment().
> And dqbuf calls dma_buf_unmap_attachment() to unmap that buffer from its
> own iommu table.
> But now vb2's unmap_dma_buf callback is nothing to do. I think that the
> reason is the below issue,
>
> If qbuf maps buffer with iomm table and dqbuf unmaps it from iommu table
> then it has performance deterioration because qbuf and dqbuf are called
> repeatedly.
> And this means map/unmap are repeated also. So I think media guys moved
> dma_unmap_sg call from its own unmap_dma_buf callback to detach callback
> instead.
> For this, you can refer to vb2_dc_dmabuf_ops_unmap and
> vb2_dc_dmabuf_ops_detach function.
>
> So I added the below patch to avoid that performance deterioration and
> am testing it now.(this patch is derived from videobuf2-dma-contig.c)
> http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git;a=commit;h=576b1c3de8b90cf1570b8418b60afd1edaae4e30
>
> Thus, I'm not sure that your common set could cover all the cases
> including other frameworks. Please give me any opinions.

It seems like this adjustment would make perfect sense to add to the 
helper layer I suggested.  E.g., instead of having an exynos_attach 
structure that caches the sgt, there'd be a struct drm_gem_prime_attach 
that would do the same thing, and save the sgt it gets from 
driver->gem_prime_get_sg.  Then it would benefit nouveau and radeon, too.

Alternatively, patch #4 could be dropped and Exynos can continue to 
reimplement all of this core functionality, since the helpers are 
optional, but I don't see anything about this change that should make it 
Exynos-specific, unless I'm missing something.

--
Aaron