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 <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
