[PATCH 2/2] drm/omap: add OMAP_BO flags to affect buffer allocation

[Laurent Pinchart]

Hi Tomi,

CC'ing Daniel.

On Monday 14 Aug 2017 14:02:16 Tomi Valkeinen wrote:
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> On 11/08/17 17:23, Laurent Pinchart wrote:
> > On Monday 08 May 2017 11:51:22 Tomi Valkeinen wrote:
> >> On SoCs with TILER, we have to ways to allocate buffers: normal
> > 
> > s/to ways/two ways/
> > 
> >> dma_alloc or via TILER (which basically functions as an IOMMU). TILER
> >> can map 128MB at a time, and we only map the TILER buffers when they are
> >> used (i.e. not at alloc time). If TILER is present, omapdrm always
> >> uses TILER.
> >> 
> >> There are use cases that require lots of big buffers that are being used
> >> at the same time by different IPs. At the moment the userspace has a
> >> hard maximum of 128MB.
> >> 
> >> This patch adds three new flags that can be used by the userspace to
> >> solve the situation:
> >> 
> >> OMAP_BO_MEM_CONTIG: The driver will use dma_alloc to get the memory.
> >> This can be used to avoid TILER if the userspace knows it needs more
> >> than 128M of memory at the same time.
> >> 
> >> OMAP_BO_MEM_TILER: The driver will use TILER to get the memory. There's
> >> nto much use for this flag at the moment, but it's here for
> > 
> > s/nto/not/
> > 
> >> completeness.
> > 
> > I assume the OMAP_BO_MEM_CONTIG and OMAP_BO_MEM_TILER flags are supposed
> > to be mutually exclusive ?
> 
> That is true.
> 
> >> OMAP_BO_MEM_PIN: The driver will pin the memory at alloc time, and keep
> >> it pinned. This can be used to 1) get an error at alloc time if TILER
> >> space is full, and 2) get rid of the constant pin/unpin operations which
> >> may have some effect on performance.
> >> 
> >> If none of the flags are given, the behavior is the same as currently.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 23 ++++++++++++++++++++++-
> >>  include/uapi/drm/omap_drm.h        |  3 +++
> >>  2 files changed, 25 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 5d73dccc1383..90ae8615f6c6
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -1292,6 +1292,9 @@ void omap_gem_free_object(struct drm_gem_object
> >> *obj)
> >> 
> >>  	list_del(&omap_obj->mm_list);
> >>  	spin_unlock(&priv->list_lock);
> >> 
> >> +	if (omap_obj->flags & OMAP_BO_MEM_PIN)
> >> +		omap_gem_put_paddr_locked(obj);
> >> +
> >>  	/* this means the object is still pinned.. which really should
> >>  	 * not happen.  I think..
> >>  	 */
> >> @@ -1338,6 +1341,11 @@ struct drm_gem_object *omap_gem_new(struct
> >> drm_device *dev,
> >>  			return NULL;
> >>  		}
> >> 
> >> +		if (flags & OMAP_BO_MEM_CONTIG) {
> >> +			dev_err(dev->dev, "Tiled buffers require TILER
> > memory\n");
> > 
> > I think we need more sanity checks, only part of the faulty cases are
> > caught. The semantics of the new flags need to be defined more precisely
> > and properly documented (with kerneldoc for all BO flags for instance).
> > In particular,
> 
> Ok.
> 
> > interactions between OMAP_BO_MEM_TILER and the existing OMAP_BO_TILED_*
> > flags are not clear to me. I wonder whether we really should introduce
> > OMAP_BO_MEM_TILER, relying on OMAP_BO_TILED seems enough to me.
> 
> TILED_* is for "real" tiled modes, i.e. the so called 2D mode. TILER
> just means to use the 1D mode. However, I think that perhaps it's better
> to use some other word than TILER there, as, if I'm not mistaken, the 1D
> mode doesn't really use TILER (even if it's often referred to as TILER
> 1D mode). It is handled by the same driver, though. Probably
> OMAP_BO_MEM_DMM or OMAP_BO_MEM_PAT would be better.

That's a good idea, yes. It's a kind of OMAP_BO_MEM_IOMMU, except that the 
hardware isn't a real IOMMU.

> > In addition to this, I think there's a rule that new userspace APIs need a
> > userspace implementation, and not just in libdrm, but in Xorg, Weston,
> > Android or a similar project.
> 
> Yes, unfortunately that is not going to happen. I don't see how this
> could be used in a generic system like Weston or X. It's meant for very
> specific use cases, where you know exactly the requirements of your
> application and the capabilities of the whole system, and optimize based
> on that.

That's a very good point. Daniel, what do you think ?

> >> +			return NULL;
> >> +		}
> >> +
> >>  		/*
> >>  		 * Tiled buffers are always shmem paged backed. When they are
> >>  		 * scanned out, they are remapped into DMM/TILER.
> >> @@ -1351,7 +1359,8 @@ struct drm_gem_object *omap_gem_new(struct
> >> drm_device *dev,
> >>  		 */
> >>  		flags &= ~(OMAP_BO_CACHED|OMAP_BO_WC|OMAP_BO_UNCACHED);
> >>  		flags |= tiler_get_cpu_cache_flags();
> >> -	} else if ((flags & OMAP_BO_SCANOUT) && !priv->has_dmm) {
> >> +	} else if ((flags & OMAP_BO_MEM_CONTIG) ||
> >> +		((flags & OMAP_BO_SCANOUT) && !priv->has_dmm)) {
> >>  		/*
> >>  		 * OMAP_BO_SCANOUT hints that the buffer doesn't need to be
> >>  		 * tiled. However, to lower the pressure on memory allocation,
> >> @@ -1411,12 +1420,24 @@ struct drm_gem_object *omap_gem_new(struct
> >> drm_device *dev,
> >>  		goto err_release;
> >>  	}
> >> 
> >> +	if (flags & OMAP_BO_MEM_PIN) {
> >> +		dma_addr_t dummy;
> >> +
> >> +		ret = omap_gem_get_paddr(obj, &dummy, true);
> > 
> > Wouldn't it be better to modify omap_gem_get_paddr() to accept a NULL
> > second parameter ?
> 
> Yes, I think that makes sense.

-- 
Regards,

Laurent Pinchart