[Intel-gfx] susetting the remaining swioltb couplin in DRM

[Intel-gfx] susetting the remaining swioltb couplin in DRM

[Christoph Hellwig]

Hi i915 and nouveau maintainers,

any chance I could get some help to remove the remaining direct
driver calls into swiotlb, namely swiotlb_max_segment and
is_swiotlb_active.  Either should not matter to a driver as they
should be written to the DMA API.

In the i915 case it seems like the driver should use
dma_alloc_noncontiguous and/or dma_alloc_noncoherent to allocate
DMAable memory instead of using alloc_page and the streaming
dma mapping helpers.

For the latter it seems like it should just stop passing
use_dma_alloc == true to ttm_device_init and/or that function
should switch to use dma_alloc_noncoherent.

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Rodrigo Vivi]

On Mon, Jul 11, 2022 at 10:26:14AM +0200, Christoph Hellwig wrote:
> Hi i915 and nouveau maintainers,
> 
> any chance I could get some help to remove the remaining direct
> driver calls into swiotlb, namely swiotlb_max_segment and
> is_swiotlb_active.  Either should not matter to a driver as they
> should be written to the DMA API.

Hi Christoph,

while we take a look here, could you please share the reasons
behind sunsetting this calls?

> 
> In the i915 case it seems like the driver should use
> dma_alloc_noncontiguous and/or dma_alloc_noncoherent to allocate
> DMAable memory instead of using alloc_page and the streaming
> dma mapping helpers.
> 
> For the latter it seems like it should just stop passing
> use_dma_alloc == true to ttm_device_init and/or that function
> should switch to use dma_alloc_noncoherent.

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Christoph Hellwig]

On Mon, Jul 11, 2022 at 04:31:49PM -0400, Rodrigo Vivi wrote:
> On Mon, Jul 11, 2022 at 10:26:14AM +0200, Christoph Hellwig wrote:
> > Hi i915 and nouveau maintainers,
> > 
> > any chance I could get some help to remove the remaining direct
> > driver calls into swiotlb, namely swiotlb_max_segment and
> > is_swiotlb_active.  Either should not matter to a driver as they
> > should be written to the DMA API.
> 
> Hi Christoph,
> 
> while we take a look here, could you please share the reasons
> behind sunsetting this calls?

Because they are a completely broken layering violation.  A driver has
absolutely no business knowing the dma-mapping violation.  The DMA
API reports what we think is all useful constraints (e.g.
dma_max_mapping_size()), and provides useful APIs to (e.g.
dma_alloc_noncoherent or dma_alloc_noncontiguous) to allocate pages
that can be mapped without bounce buffering and drivers should use
the proper API instead of poking into one particular implementation
and restrict it from changing.

swiotlb_max_segment in particular returns a value that isn't actually
correct (a driver can't just use all of swiotlb) AND actually doesn't
work as is in various scenarious that are becoming more common,
most notably host with memory encryption schemes that always require
bounce buffering.

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Tvrtko Ursulin]

Hi,

On 12/07/2022 06:00, Christoph Hellwig wrote:
> On Mon, Jul 11, 2022 at 04:31:49PM -0400, Rodrigo Vivi wrote:
>> On Mon, Jul 11, 2022 at 10:26:14AM +0200, Christoph Hellwig wrote:
>>> Hi i915 and nouveau maintainers,
>>>
>>> any chance I could get some help to remove the remaining direct
>>> driver calls into swiotlb, namely swiotlb_max_segment and
>>> is_swiotlb_active.  Either should not matter to a driver as they
>>> should be written to the DMA API.
>>
>> Hi Christoph,
>>
>> while we take a look here, could you please share the reasons
>> behind sunsetting this calls?
> 
> Because they are a completely broken layering violation.  A driver has
> absolutely no business knowing the dma-mapping violation.  The DMA
> API reports what we think is all useful constraints (e.g.
> dma_max_mapping_size()), and provides useful APIs to (e.g.
> dma_alloc_noncoherent or dma_alloc_noncontiguous) to allocate pages
> that can be mapped without bounce buffering and drivers should use
> the proper API instead of poking into one particular implementation
> and restrict it from changing.
> 
> swiotlb_max_segment in particular returns a value that isn't actually
> correct (a driver can't just use all of swiotlb) AND actually doesn't
> work as is in various scenarious that are becoming more common,
> most notably host with memory encryption schemes that always require
> bounce buffering.

All these are either in the internal backend or in the old shmem 
backend. I understand both are soon to be retired or deprecated. I think.

+ Matt & Thomas, and Bob actually as well, as I think authorities in the 
shmem, TTM and internal backend at the moment. Could you guys please 
have look if and how the TTM backend needs to handle this and what is 
the timeline of retirement if relevant?

Regards,

Tvrtko

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Robert Beckett]

On 18/07/2022 12:36, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 12/07/2022 06:00, Christoph Hellwig wrote:
>> On Mon, Jul 11, 2022 at 04:31:49PM -0400, Rodrigo Vivi wrote:
>>> On Mon, Jul 11, 2022 at 10:26:14AM +0200, Christoph Hellwig wrote:
>>>> Hi i915 and nouveau maintainers,
>>>>
>>>> any chance I could get some help to remove the remaining direct
>>>> driver calls into swiotlb, namely swiotlb_max_segment and
>>>> is_swiotlb_active.  Either should not matter to a driver as they
>>>> should be written to the DMA API.
>>>
>>> Hi Christoph,
>>>
>>> while we take a look here, could you please share the reasons
>>> behind sunsetting this calls?
>>
>> Because they are a completely broken layering violation.  A driver has
>> absolutely no business knowing the dma-mapping violation.  The DMA
>> API reports what we think is all useful constraints (e.g.
>> dma_max_mapping_size()), and provides useful APIs to (e.g.
>> dma_alloc_noncoherent or dma_alloc_noncontiguous) to allocate pages
>> that can be mapped without bounce buffering and drivers should use
>> the proper API instead of poking into one particular implementation
>> and restrict it from changing.
>>
>> swiotlb_max_segment in particular returns a value that isn't actually
>> correct (a driver can't just use all of swiotlb) AND actually doesn't
>> work as is in various scenarious that are becoming more common,
>> most notably host with memory encryption schemes that always require
>> bounce buffering.
> 
> All these are either in the internal backend or in the old shmem 
> backend. I understand both are soon to be retired or deprecated. I think.
> 
> + Matt & Thomas, and Bob actually as well, as I think authorities in the 
> shmem, TTM and internal backend at the moment. Could you guys please 
> have look if and how the TTM backend needs to handle this and what is 
> the timeline of retirement if relevant?
> 
> Regards,
> 
> Tvrtko

So currently these are used directly in the internal backend and 
indirectly via i915_sg_segment_size() in shmem, ttm and userptr backends.

internal and userptr are being refactored currently (internal is ready 
but lacking review), but the refactoring would just make them use the 
ttm backend which still uses these.

It seems to me like a simple solution would be to just replace 
swiotlb_max_segment() calls with dma_max_mapping_size() as a drop in 
replacement. This follows the same logic as drm_prime_pages_to_sg().

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Lyude Paul]

Hi! Sorry about the slow reply to this, been busy with a bunch of other
pressing nouveau work lately.

Anyway, the steps look pretty simple here so I can see if I can write up a
patch shortly :)

On Mon, 2022-07-11 at 10:26 +0200, Christoph Hellwig wrote:
> Hi i915 and nouveau maintainers,
> 
> any chance I could get some help to remove the remaining direct
> driver calls into swiotlb, namely swiotlb_max_segment and
> is_swiotlb_active.  Either should not matter to a driver as they
> should be written to the DMA API.
> 
> In the i915 case it seems like the driver should use
> dma_alloc_noncontiguous and/or dma_alloc_noncoherent to allocate
> DMAable memory instead of using alloc_page and the streaming
> dma mapping helpers.
> 
> For the latter it seems like it should just stop passing
> use_dma_alloc == true to ttm_device_init and/or that function
> should switch to use dma_alloc_noncoherent.
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [Intel-gfx] susetting the remaining swioltb couplin in DRM

[Lyude Paul]

Actually, seems like I do have a question. I see that you mention that we
could stop passing use_dma_alloc=true to ttm_device_init() or use
dma_alloc_noncoherent(). I'm not an expert on this nouveau's mm, but after
skimming it seems like if we can simply check whether or not we need all
noncoherent allocations without getting is_swiotlb_active() involved - which
should be easy. Does this seem like a workable solution, or is there something
I'm missing here?

On Thu, 2022-07-28 at 17:17 -0400, Lyude Paul wrote:
> Hi! Sorry about the slow reply to this, been busy with a bunch of other
> pressing nouveau work lately.
> 
> Anyway, the steps look pretty simple here so I can see if I can write up a
> patch shortly :)
> 
> On Mon, 2022-07-11 at 10:26 +0200, Christoph Hellwig wrote:
> > Hi i915 and nouveau maintainers,
> > 
> > any chance I could get some help to remove the remaining direct
> > driver calls into swiotlb, namely swiotlb_max_segment and
> > is_swiotlb_active.  Either should not matter to a driver as they
> > should be written to the DMA API.
> > 
> > In the i915 case it seems like the driver should use
> > dma_alloc_noncontiguous and/or dma_alloc_noncoherent to allocate
> > DMAable memory instead of using alloc_page and the streaming
> > dma mapping helpers.
> > 
> > For the latter it seems like it should just stop passing
> > use_dma_alloc == true to ttm_device_init and/or that function
> > should switch to use dma_alloc_noncoherent.
> > 
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat