Re: [PATCH 6/7] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Friday 10 Mar 2017 14:13:08 Tomi Valkeinen wrote:
> On 10/03/17 11:39, Laurent Pinchart wrote:
> > The is_cache_coherent() function currently returns true when the mapping
> > is not cache-coherent. This isn't a bug as such as the callers interpret
> > cache-coherent as meaning that the driver has to handle the coherency
> > manually, but it is nonetheless very confusing. Fix it and add a bit
> > more documentation to explain how cached buffers are handled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_gem.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> > b/drivers/gpu/drm/omapdrm/omap_gem.c index 945bda8e330d..59594ec0d159
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> > @@ -732,16 +732,21 @@ int omap_gem_roll(struct drm_gem_object *obj,
> > uint32_t roll)> 
> >   * Memory Management & DMA Sync
> >   */
> > 
> > -/**
> > - * shmem buffers that are mapped cached can simulate coherency via using
> > - * page faulting to keep track of dirty pages
> > +/*
> > + * shmem buffers that are mapped cached are not coherent.
> > + *
> > + * We keep track of dirty pages using page faulting to perform cache
> > management. + * When a page is mapped to the CPU in read/write mode the
> > device can't access + * it and omap_obj->dma_addrs[i] is NULL. When a
> > page is mapped to the device + * the omap_obj->dma_addrs[i] is set to the
> > DMA address, and the page is + * unmapped from the CPU.
> > 
> >   */
> >  
> >  static inline bool is_cached_coherent(struct drm_gem_object *obj)
> >  {
> >  
> >  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> > 
> > -	return (omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> > -		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED);
> > +	return !((omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> > +		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED));
> > 
> >  }
> >  
> >  /* Sync the buffer for CPU access.. note pages should already be
> > 
> > @@ -752,7 +757,10 @@ void omap_gem_cpu_sync(struct drm_gem_object *obj,
> > int pgoff)> 
> >  	struct drm_device *dev = obj->dev;
> >  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> > 
> > -	if (is_cached_coherent(obj) && omap_obj->dma_addrs[pgoff]) {
> > +	if (!is_cached_coherent(obj))
> > +		return;
> 
> I think the ! is extra there. If the obj _is_ coherent, we can just return.

You're right, my bad. I'll fix that in v2.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel