linux-kernel - Re: [Nouveau] [PATCH v4 4/6] drm/nouveau: synchronize BOs when required

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 11 Jul 2014 18:35:16 +0900
From:	Alexandre Courbot <acourbot@...dia.com>
To:	Daniel Vetter <daniel@...ll.ch>
CC:	Ben Skeggs <bskeggs@...hat.com>, David Airlie <airlied@...ux.ie>,
	David Herrmann <dh.herrmann@...il.com>,
	Lucas Stach <dev@...xeye.de>,
	Thierry Reding <thierry.reding@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...onical.com>,
	<nouveau@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
	<dri-devel@...ts.freedesktop.org>, <linux-tegra@...r.kernel.org>,
	Alexandre Courbot <gnurou@...il.com>
Subject: Re: [Nouveau] [PATCH v4 4/6] drm/nouveau: synchronize BOs when required

On 07/11/2014 04:41 PM, Daniel Vetter wrote:
> On Fri, Jul 11, 2014 at 11:40:27AM +0900, Alexandre Courbot wrote:
>> On 07/10/2014 10:04 PM, Daniel Vetter wrote:
>>> On Tue, Jul 08, 2014 at 05:25:59PM +0900, Alexandre Courbot wrote:
>>>> On architectures for which access to GPU memory is non-coherent,
>>>> caches need to be flushed and invalidated explicitly when BO control
>>>> changes between CPU and GPU.
>>>>
>>>> This patch adds buffer synchronization functions which invokes the
>>>> correct API (PCI or DMA) to ensure synchronization is effective.
>>>>
>>>> Based on the TTM DMA cache helper patches by Lucas Stach.
>>>>
>>>> Signed-off-by: Lucas Stach <dev@...xeye.de>
>>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>>>> ---
>>>>   drivers/gpu/drm/nouveau/nouveau_bo.c  | 56 +++++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/nouveau/nouveau_bo.h  |  2 ++
>>>>   drivers/gpu/drm/nouveau/nouveau_gem.c | 12 ++++++++
>>>>   3 files changed, 70 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> index 67e9e8e2e2ec..47e4e8886769 100644
>>>> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> @@ -402,6 +402,60 @@ nouveau_bo_unmap(struct nouveau_bo *nvbo)
>>>>   		ttm_bo_kunmap(&nvbo->kmap);
>>>>   }
>>>>
>>>> +void
>>>> +nouveau_bo_sync_for_device(struct nouveau_bo *nvbo)
>>>> +{
>>>> +	struct nouveau_drm *drm = nouveau_bdev(nvbo->bo.bdev);
>>>> +	struct nouveau_device *device = nouveau_dev(drm->dev);
>>>> +	struct ttm_dma_tt *ttm_dma = (struct ttm_dma_tt *)nvbo->bo.ttm;
>>>> +	int i;
>>>> +
>>>> +	if (!ttm_dma)
>>>> +		return;
>>>> +
>>>> +	if (nv_device_is_cpu_coherent(device) || nvbo->force_coherent)
>>>> +		return;
>>>
>>> Is the is_cpu_coherent check really required? On coherent platforms the
>>> sync_for_foo should be a noop. It's the dma api's job to encapsulate this
>>> knowledge so that drivers can be blissfully ignorant. The explicit
>>> is_coherent check makes this a bit leaky. And same comment that underlying
>>> the bus-specifics dma-mapping functions are identical.
>>
>> I think you are right, the is_cpu_coherent check should not be needed here.
>> I still think we should have separate paths for the PCI/DMA cases though,
>> unless you can point me to a source that clearly states that the PCI API is
>> deprecated and that DMA should be used instead.
>
> Ah, on 2nd look I've found it again. Quoting
> Documentation/DMA-API-HOWTO.txt:
>
> "Note that the DMA API works with any bus independent of the underlying
> microprocessor architecture. You should use the DMA API rather than the
> bus-specific DMA API, i.e., use the dma_map_*() interfaces rather than the
> pci_map_*() interfaces."
>
> The advice is fairly strong here I think ;-) And imo the idea makes sense,
> since it allows drivers like nouveau here to care much less about the
> actual bus used to get data to/from the ip block. And if you look at intel
> gfx it makes even more sense since the pci layer we have is really just a
> thin fake shim whacked on top of the hw (on SoCs at least).

Indeed, I stand corrected. :) That's good news actually, as it will 
simplify the code. Thanks for pointing that out!

I will send a new revision that makes use of the DMA API exclusively and 
will remove the nv_device_map/unmap() functions which are pretty useless 
now.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/