Re: [PATCH v2 07/10] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Tomi Valkeinen]

On 24/04/17 17:25, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Monday 24 Apr 2017 13:44:23 Tomi Valkeinen wrote:
>> On 21/04/17 00:33, 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 
>>> ---
>>> Changes since v1:
>>>
>>> - Fixed one mistakenly inverted cache coherency check
>>> ---
>>>
>>>  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 d6d38e569fff..43b60a146edf
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>>> @@ -719,16 +719,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 use shmem only with TILER. Not all SoCs have TILER (and you can
>> disable TILER on the SoCs that have it).
>>
>> As this patch is more or less a cleanup, I'm not sure if the above
>> should be part of this patch. But it makes me wonder: if we don't use
>> shmem, we use dma_alloc_writecombine.
> 
> Only for OMAP_BO_SCANOUT buffers. Other buffers will still be allocated 
> through shmem.

Right... I hope nobody allocates those. omapdrm should be used for dss
buffers, which means scanout...

>> Do we still end up mapping it to the userspace as cached?
> 
> Well, in that case, we actually end up not mapping it at all if the user 
> requests cached mapping :-) omap_gem_mmap_obj() will return with a WARN_ON() 
> and omap_gem_pin() (which used to be omap_gem_get_paddr()) will return -
> EINVAL.
> 
> I believe we should disallow non-contiguous buffers without a DMM at 
> allocation time. As for cached mapping of contiguous buffers, the dirty page 
> tracking implementation requires shmem at the moment. This could be fixed, 
> but 
> isn't trivial. Do you see value in doing so, or should cached mappings of 
> contiguous buffers be disallowed for the time being ?

I think the answer depends on whether cached buffers are usable
(performance-wise). If not, we should drop cached buffer support
totally. If yes, then we should support them for contiguous buffers too.

>>>  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));
>>
>> Regardless of whether we support non-shmem cached buffers, isn't the
>> above overly complex? Why can't we just check for OMAP_BO_CACHED? Isn't
>> that the only case where the buffer is not coherent? At the moment this
>> function sounds more like "is_shmem_cached_coherent".
> 
> You're right. I propose fixing that in an additional patch to avoid potential 
> changes to the behaviour in this one.

Sounds good.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/10] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Laurent Pinchart]

Hi Tomi,

On Monday 24 Apr 2017 13:44:23 Tomi Valkeinen wrote:
> On 21/04/17 00:33, 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 
> > ---
> > Changes since v1:
> > 
> > - Fixed one mistakenly inverted cache coherency check
> > ---
> > 
> >  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 d6d38e569fff..43b60a146edf
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> > @@ -719,16 +719,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 use shmem only with TILER. Not all SoCs have TILER (and you can
> disable TILER on the SoCs that have it).
> 
> As this patch is more or less a cleanup, I'm not sure if the above
> should be part of this patch. But it makes me wonder: if we don't use
> shmem, we use dma_alloc_writecombine.

Only for OMAP_BO_SCANOUT buffers. Other buffers will still be allocated 
through shmem.

> Do we still end up mapping it to the userspace as cached?

Well, in that case, we actually end up not mapping it at all if the user 
requests cached mapping :-) omap_gem_mmap_obj() will return with a WARN_ON() 
and omap_gem_pin() (which used to be omap_gem_get_paddr()) will return -
EINVAL.

I believe we should disallow non-contiguous buffers without a DMM at 
allocation time. As for cached mapping of contiguous buffers, the dirty page 
tracking implementation requires shmem at the moment. This could be fixed, but 
isn't trivial. Do you see value in doing so, or should cached mappings of 
contiguous buffers be disallowed for the time being ?

> I think having two different caching types for the same memory is illegal.

It used to be documented as leading to unspecified behaviour in the ARM ARM, 
but I think that has been changed. If I'm not mistaken it doesn't cause any 
problem in practice, at least on the TI platforms I tested back when dealing 
with cache on OMAP3 :-)

> > + *
> > + * 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));
> 
> Regardless of whether we support non-shmem cached buffers, isn't the
> above overly complex? Why can't we just check for OMAP_BO_CACHED? Isn't
> that the only case where the buffer is not coherent? At the moment this
> function sounds more like "is_shmem_cached_coherent".

You're right. I propose fixing that in an additional patch to avoid potential 
changes to the behaviour in this one.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 07/10] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Tomi Valkeinen]

On 21/04/17 00:33, 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 
> ---
> Changes since v1:
> 
> - Fixed one mistakenly inverted cache coherency check
> ---
>  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 d6d38e569fff..43b60a146edf 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -719,16 +719,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 use shmem only with TILER. Not all SoCs have TILER (and you can
disable TILER on the SoCs that have it).

As this patch is more or less a cleanup, I'm not sure if the above
should be part of this patch. But it makes me wonder: if we don't use
shmem, we use dma_alloc_writecombine. Do we still end up mapping it to
the userspace as cached? I think having two different caching types for
the same memory is illegal.

> + *
> + * 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));

Regardless of whether we support non-shmem cached buffers, isn't the
above overly complex? Why can't we just check for OMAP_BO_CACHED? Isn't
that the only case where the buffer is not coherent? At the moment this
function sounds more like "is_shmem_cached_coherent".

 Tomi



signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel