[PATCH/RFC 2/2] drm: rcar-du: Allow importing non-contiguous dma-buf with VSP

[Liviu Dudau]

On Tue, Nov 14, 2017 at 05:34:14AM +0200, Laurent Pinchart wrote:
> Hi Liviu,

Hi Laurent,

> 
> On Monday, 13 November 2017 13:53:14 EET Liviu Dudau wrote:
> > On Mon, Nov 13, 2017 at 12:32:28PM +0200, Laurent Pinchart wrote:
> > > When the DU sources its frames from a VSP, it performs no memory access
> > > and thus has no requirements on imported dma-buf memory types. In
> > > particular the DU could import a physically non-contiguous buffer that
> > > would later be mapped contiguously through the VSP IOMMU.
> > > 
> > > This use case isn't supported at the moment as the GEM CMA helpers will
> > > reject any non-contiguous buffer, and the DU isn't connected to an IOMMU
> > > that can make the buffer contiguous for DMA. Fix this by implementing a
> > > custom .gem_prime_import_sg_table() operation that accepts all imported
> > > dma-buf regardless of the number of scatterlist entries.
> > 
> > This patch raises the question of why use CMA at all if you can accept
> > any kind of buffers.
> 
> Both the DU and VSP require DMA contiguous memory. On R-Car Gen2 the DU 
> performs DMA, and thus uses the GEM CMA helpers. On Gen3 it delegates DMA to 
> the external VSP composer, and still uses the GEM CMA helpers to ensure that 
> memory will be DMA contiguous for the VSP. The problem arises when physically 
> non-contiguous dma-buf buffers are imported, they can be mapped contiguously 
> to the VSP (assuming an IOMMU is present) but not to the DU (as there's no 
> IOMMU).
> 
> The situation is particularly messy due to the fact that I have one VSP 
> instance per CRTC, each connected to its own IOMMU channel. The driver thus 
> doesn't know when allocating GEM objects to which struct device they need to 
> be mapped. The DRM helpers don't support delayed mapping of memory. I'd like 
> to know whether that's something I should fix properly (which would likely 
> involve major reworking of the helpers) or hack around it in the DU driver.

I understand. Looks like you have your hands full with an "interesting"
architecture.

> 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas at ideasonboard.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/rcar-du/rcar_du_drv.c |  2 +-
> > >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 39 ++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/rcar-du/rcar_du_kms.h |  7 +++++++
> > >  3 files changed, 47 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 48c166f925a3..d999231f98c7
> > > 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > > @@ -289,7 +289,7 @@ static struct drm_driver rcar_du_driver = {
> > >  	.gem_prime_import	= drm_gem_prime_import,
> > >  	.gem_prime_export	= drm_gem_prime_export,
> > >  	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
> > > -	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> > > +	.gem_prime_import_sg_table = rcar_du_gem_prime_import_sg_table,
> > >  	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
> > >  	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
> > >  	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 566d1a948c8f..2dd0c2ba047d
> > > 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> 
> [snip]
> 
> > > +struct drm_gem_object *rcar_du_gem_prime_import_sg_table(struct
> > > drm_device *dev,
> > > +				struct dma_buf_attachment *attach,
> > > +				struct sg_table *sgt)
> > > +{
> > > +	struct rcar_du_device *rcdu = dev->dev_private;
> > > +	struct drm_gem_cma_object *cma_obj;
> > > +	struct drm_gem_object *gem_obj;
> > > +	int ret;
> > > +
> > > +	if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
> > > +		return drm_gem_cma_prime_import_sg_table(dev, attach, sgt);
> > > +
> > > +	/* Create a CMA GEM buffer. */
> > > +	cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
> > > +	if (!cma_obj)
> > > +		return ERR_PTR(-ENOMEM);
> > > +	gem_obj = &cma_obj->base;
> > > +
> > > +	ret = drm_gem_object_init(dev, gem_obj, attach->dmabuf->size);
> > > +	if (ret)
> > > +		goto error;
> > > +
> > > +	ret = drm_gem_create_mmap_offset(gem_obj);
> > > +	if (ret) {
> > > +		drm_gem_object_release(gem_obj);
> > > +		goto error;
> > > +	}
> > > +
> > > +	cma_obj->paddr = 0;
> > 
> > This is going to break drm_gem_cma_describe() if you are using it
> 
> As far as I can tell drm_gem_cma_describe() will just print paddr = 0, it 
> won't crash.

I'm sorry, didn't meant it will crash, just print (possibly useless)
zero value.

> 
> > plus the rcar_du_plane_setup_scanout() unless I'm missing something besides
> > familiarity with the RCAR driver code :)
> 
> rcar_du_plane_setup_scanout() is only called when !rcar_du_has(rcdu, 
> RCAR_DU_FEATURE_VSP1_SOURCE) (that is on Gen3 hardware, or on Gen2 when I 
> artificially disable all DMA from the DU and use external composers to emulate 
> Gen3 behaviour for testing purpose), in which case this function calls 
> drm_gem_cma_prime_import_sg_table().

Thanks for the explanation!

> 
> > This function looks very similar to what I tried to do for mali-dp to
> > allow the import of contiguous DMA buffers that have more than 1 sgt
> > entries. In the end I gave up as I kept finding issues and went for the
> > drm_gem_cma_prime_import_sg_table() changes. Maybe you need to do a
> > similar change in the function to bypass some requirements if the driver
> > signals that it can accept relaxed requirements?
> 
> As explained above I've considered reworking the helpers (to do it properly I 
> think I'll likely need to rework drm_prime.c too), but I'd first like to know 
> whether there's an interest for that.

Probably if you can express somehow the fact that the DU doesn't do any
memory access it might be interesting to other drivers. Otherwise you're
probably tied to having a driver specific version of the hook.

Best regards,
Liviu

> 
> > > +	cma_obj->sgt = sgt;
> > > +
> > > +	return gem_obj;
> > > +
> > > +error:
> > > +	kfree(cma_obj);
> > > +	return ERR_PTR(ret);
> > > +}
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯