Re: [PATCH] drm/tegra: Remove of_dma_configure() from host1x_device_add()

[Jon Hunter <jonathanh@nvidia.com>]

On 31/01/2024 15:33, Jason Gunthorpe wrote:
> On Tue, Jan 30, 2024 at 09:55:18PM +0000, Jon Hunter wrote:
>>
>> On 30/01/2024 16:15, Jason Gunthorpe wrote:
>>> This was added in commit c95469aa5a18 ("gpu: host1x: Set DMA ops on device
>>> creation") with the note:
>>>
>>>       Currently host1x-instanciated devices have their dma_ops left to NULL,
>>>       which makes any DMA operation (like buffer import) on ARM64 fallback
>>>       to the dummy_dma_ops and fail with an error.
>>>
>>> Since commit 14891af3799e ("iommu: Move the iommu driver sysfs setup into
>>> iommu_init/deinit_device()") this call now fails because the struct device
>>> is not fully configured enough to setup the sysfs and we now catch that
>>> error.
>>>
>>> This failure means the DMA ops are no longer set during this failing call.
> 
> Looking at it more it seems the arch dma ops are setup still, we
> ignore the failure on multiple levels :(
> 
>>> It seems this is no longer a problem because
>>> commit 07397df29e57 ("dma-mapping: move dma configuration to bus
>>> infrastructure") added another call to of_dma_configure() inside the
>>> bus_type->dma_configure() callback.
>>>
>>> So long as a driver is probed the to the device it will have DMA properly
>>> setup in the ordinary way.
>>>
>>> Remove the unnecessary call which also removes the new long print:
>>>
>>> [    1.200004] host1x drm: iommu configuration for device failed with -ENOENT
>>>
>>> Reported-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>> Closes: https://lore.kernel.org/all/bbmhcoghrprmbdibnjum6lefix2eoquxrde7wyqeulm4xabmlm@b6jy32saugqh/
>>> Reported-by: Jon Hunter <jonathanh@nvidia.com>
>>> Closes: https://lore.kernel.org/all/b0334c5e-3a6c-4b58-b525-e72bed8899b3@nvidia.com/
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    drivers/gpu/host1x/bus.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>> index 84d042796d2e66..61214d35cadc34 100644
>>> --- a/drivers/gpu/host1x/bus.c
>>> +++ b/drivers/gpu/host1x/bus.c
>>> @@ -458,8 +458,6 @@ static int host1x_device_add(struct host1x *host1x,
>>>    	device->dev.bus = &host1x_bus_type;
>>>    	device->dev.parent = host1x->dev;
>>> -	of_dma_configure(&device->dev, host1x->dev->of_node, true);
>>> -
>>>    	device->dev.dma_parms = &device->dma_parms;
>>>    	dma_set_max_seg_size(&device->dev, UINT_MAX);
>>
>>
>> In my case the warning is coming from the of_dma_configure_id() in
>> drivers/gpu/host1x/context.c. So with the above change I am still seeing the
>> warning.
> 
> You mean this sequence?
> 
> 		err = device_add(&ctx->dev);
> 		if (err) {
> 			dev_err(host1x->dev, "could not add context device %d: %d\n", i, err);
> 			put_device(&ctx->dev);
> 			goto unreg_devices;
> 		}
> 
> 		err = of_dma_configure_id(&ctx->dev, node, true, &i);
> 		if (err) {
> 			dev_err(host1x->dev, "IOMMU configuration failed for context device %d: %d\n",
> 				i, err);
> 			device_unregister(&ctx->dev);
> 			goto unreg_devices;
> 		}

Yes this sequence.

> I didn't seem an obvious place that this would get fixed up later?
> 
> device_add() was done before so the iommu_device_link() shouldn't be
> failing? Are you hitting a duplicate link (ie remove the nowarn from
> iommu_device_link())


Removing the '_nowarn' does appear to fix it, although it is not clear 
to me why?

Thanks
Jon

-- 
nvpublic