Re: [PATCH v5 5/5] iommu/tegra-smmu: Support managed domains
On 5/12/22 22:00, Thierry Reding wrote:
> -277,7 +278,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned
> type)
> {
> struct tegra_smmu_as *as;
>
> - if (type != IOMMU_DOMAIN_UNMANAGED)
> + if (type != IOMMU_DOMAIN_UNMANAGED &&
> + type != IOMMU_DOMAIN_DMA &&
> + type != IOMMU_DOMAIN_IDENTITY)
> return NULL;
Shouldn't at least pre-210 SoCs be guarded from IOMMU_DOMAIN_DMA? I
don't think that DRM and VDE drivers will work as-is today.
--
Best regards,
Dmitry
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
On 5/4/22 14:52, Robin Murphy wrote: > On 2022-05-04 01:52, Dmitry Osipenko wrote: >> On 4/11/22 16:46, Robin Murphy wrote: >>> @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct >>> host1x_device *dev) >>> struct host1x *host1x = dev_get_drvdata(dev->dev.parent); >>> struct iommu_domain *domain; >>> + /* For starters, this is moot if no IOMMU is available */ >>> + if (!device_iommu_mapped(>dev)) >>> + return false; >> >> Unfortunately this returns false on T30 with enabled IOMMU because we >> don't use IOMMU for Host1x on T30 [1] to optimize performance. We can't >> change it until we will update drivers to support Host1x-dedicated >> buffers. > > Huh, so is dev->dev here not the DRM device? If it is, and > device_iommu_mapped() returns false, then the later iommu_attach_group() > call is going to fail anyway, so there's not much point allocating a > domain. If it's not, then what the heck is host1x_drm_wants_iommu() > actually testing for? The dev->dev is the host1x device and it's the DRM device. The iommu_attach_group() is called for the DRM sub-devices (clients in the Tegra driver), which are the devices sitting on the host1x bus. There is no single GPU device on Tegra, instead it's composed of independent GPU engines and display controllers that are connected to the host1x bus. Host1x also has channel DMA engines that are used by DRM driver. We don't have dedicated devices for the host1x DMA, there is single host1x driver that manages host1x bus and DMA. -- Best regards, Dmitry ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
On 4/11/22 16:46, Robin Murphy wrote: > @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct > host1x_device *dev) > struct host1x *host1x = dev_get_drvdata(dev->dev.parent); > struct iommu_domain *domain; > > + /* For starters, this is moot if no IOMMU is available */ > + if (!device_iommu_mapped(>dev)) > + return false; Unfortunately this returns false on T30 with enabled IOMMU because we don't use IOMMU for Host1x on T30 [1] to optimize performance. We can't change it until we will update drivers to support Host1x-dedicated buffers. [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/host1x/dev.c#L258 -- Best regards, Dmitry ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
On 4/11/22 16:46, Robin Murphy wrote:
> Refactor the confusing logic to make it both clearer and more robust. If
> the host1x parent device does have an IOMMU domain then iommu_present()
> is redundantly true, while otherwise for the 32-bit DMA mask case it
> still doesn't say whether the IOMMU driver actually knows about the DRM
> device or not.
>
> Signed-off-by: Robin Murphy
> ---
>
> v2: Fix logic for older SoCs and clarify.
>
> drivers/gpu/drm/tegra/drm.c | 28
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 9464f522e257..4f2bdab31064 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct
> host1x_device *dev)
> struct host1x *host1x = dev_get_drvdata(dev->dev.parent);
> struct iommu_domain *domain;
>
> + /* For starters, this is moot if no IOMMU is available */
> + if (!device_iommu_mapped(>dev))
> + return false;
> +
> + /*
> + * Tegra20 and Tegra30 don't support addressing memory beyond the
> + * 32-bit boundary, so the regular GATHER opcodes will always be
> + * sufficient and whether or not the host1x is attached to an IOMMU
> + * doesn't matter.
> + */
> + if (host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32))
> + return true;
> +
> /*
>* If the Tegra DRM clients are backed by an IOMMU, push buffers are
>* likely to be allocated beyond the 32-bit boundary if sufficient
> @@ -1122,14 +1135,13 @@ static bool host1x_drm_wants_iommu(struct
> host1x_device *dev)
> domain = iommu_get_domain_for_dev(dev->dev.parent);
>
> /*
> - * Tegra20 and Tegra30 don't support addressing memory beyond the
> - * 32-bit boundary, so the regular GATHER opcodes will always be
> - * sufficient and whether or not the host1x is attached to an IOMMU
> - * doesn't matter.
> + * At the moment, the exact type of domain doesn't actually matter.
> + * Only for 64-bit kernels might this be a managed DMA API domain, and
> + * then only on newer SoCs using arm-smmu, since tegra-smmu doesn't
> + * support default domains at all, and since those SoCs are the same
> + * ones with extended GATHER support, even if it's a passthrough domain
> + * it can still work out OK.
>*/
> - if (!domain && host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32))
> - return true;
> -
> return domain != NULL;
> }
>
> @@ -1149,7 +1161,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
> goto put;
> }
>
> - if (host1x_drm_wants_iommu(dev) && iommu_present(_bus_type)) {
> + if (host1x_drm_wants_iommu(dev)) {
> tegra->domain = iommu_domain_alloc(_bus_type);
> if (!tegra->domain) {
> err = -ENOMEM;
Robin, thank you for the updated version. The patch looks okay to me.
Reviewed-by: Dmitry Osipenko
A bit later I'll also will give it a test, just to be sure because we
had problems with that function in the past.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] drm/tegra: Stop using iommu_present()
On 4/6/22 21:06, Robin Murphy wrote:
> On 2022-04-06 15:32, Dmitry Osipenko wrote:
>> On 4/5/22 17:19, Robin Murphy wrote:
>>> Remove the pointless check. host1x_drm_wants_iommu() cannot return true
>>> unless an IOMMU exists for the host1x platform device, which at the
>>> moment
>>> means the iommu_present() test could never fail.
>>>
>>> Signed-off-by: Robin Murphy
>>> ---
>>> drivers/gpu/drm/tegra/drm.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>>> index 9464f522e257..bc4321561400 100644
>>> --- a/drivers/gpu/drm/tegra/drm.c
>>> +++ b/drivers/gpu/drm/tegra/drm.c
>>> @@ -1149,7 +1149,7 @@ static int host1x_drm_probe(struct
>>> host1x_device *dev)
>>> goto put;
>>> }
>>> - if (host1x_drm_wants_iommu(dev) &&
>>> iommu_present(_bus_type)) {
>>> + if (host1x_drm_wants_iommu(dev)) {
>>> tegra->domain = iommu_domain_alloc(_bus_type);
>>> if (!tegra->domain) {
>>> err = -ENOMEM;
>>
>> host1x_drm_wants_iommu() returns true if there is no IOMMU for the
>> host1x platform device of Tegra20/30 SoCs.
>
> Ah, apparently this is another example of what happens when I write
> patches late on a Friday night...
>
> So on second look, what we want to ascertain here is whether dev has an
> IOMMU, but only if the host1x parent is not addressing-limited, either
> because it can also use the IOMMU, or because all possible addresses are
> small enough anyway, right?
Yes
> Are we specifically looking for the host1x
> having a DMA-API-managed domain, or can that also end up using the
> tegra->domain or another unmanaged domain too?
We have host1x DMA that could have:
1. No IOMMU domain, depending on kernel/DT config
2. Managed domain, on newer SoCs
3. Unmanaged domain, on older SoCs
We have Tegra DRM devices which can:
1. Be attached to a shared unmanaged tegra->domain, on older SoCs
2. Have own managed domains, on newer SoCs
> I can't quite figure out
> from the comments whether it's physical addresses, IOVAs, or both that
> we're concerned with here.
Tegra DRM allocates buffers and submits jobs to h/w using host1x's
channel DMA. DRM framebuffers' addresses are inserted into host1x
command buffers by kernel driver and addresses beyond 32bit space need
to be treated specially, we don't support such addresses in upstream.
IOMMU AS is limited to 32bits on Tegra in upstream kernel for pre-T186
SoCs, it hides 64bit addresses from host1x. Post-T186 SoCs have extra
features that allow kernel driver not to bother about addresses.
For newer ARM64 SoCs there is assumption in the Tegra drivers that IOMMU
always presents, to simplify things.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] drm/tegra: Stop using iommu_present()
On 4/5/22 17:19, Robin Murphy wrote:
> Remove the pointless check. host1x_drm_wants_iommu() cannot return true
> unless an IOMMU exists for the host1x platform device, which at the moment
> means the iommu_present() test could never fail.
>
> Signed-off-by: Robin Murphy
> ---
> drivers/gpu/drm/tegra/drm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 9464f522e257..bc4321561400 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1149,7 +1149,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
> goto put;
> }
>
> - if (host1x_drm_wants_iommu(dev) && iommu_present(_bus_type)) {
> + if (host1x_drm_wants_iommu(dev)) {
> tegra->domain = iommu_domain_alloc(_bus_type);
> if (!tegra->domain) {
> err = -ENOMEM;
host1x_drm_wants_iommu() returns true if there is no IOMMU for the
host1x platform device of Tegra20/30 SoCs.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 3/9] gpu: host1x: Add context device management code
01.03.2022 19:14, cyn...@kapsi.fi пишет:
> +/* host1x context devices */
> +
> +struct host1x_context {
> + struct host1x *host;
> +
> + refcount_t ref;
> + struct pid *owner;
> +
> + struct device dev;
> + u64 dma_mask;
> + u32 stream_id;
> +};
I'm still not very happy about the "context" names. For example here
it's only about the "memory context", then why not to name struct as
host1x_memory_context or host1x_memctx?
It's not good to use generic names for a special things, it hurts
readability of the code. It's important to choose good names.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset
22.02.2022 13:54, Mikko Perttunen пишет:
> On 2/22/22 12:46, Dmitry Osipenko wrote:
>> 22.02.2022 11:27, Mikko Perttunen пишет:
>>> On 2/21/22 22:10, Dmitry Osipenko wrote:
>>>> 21.02.2022 14:44, Mikko Perttunen пишет:
>>>>> On 2/19/22 20:54, Dmitry Osipenko wrote:
>>>>>> 19.02.2022 21:49, Dmitry Osipenko пишет:
>>>>>>> 18.02.2022 14:39, Mikko Perttunen пишет:
>>>>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client
>>>>>>>> *client)
>>>>>>>> +{
>>>>>>>> + struct vic *vic = to_vic(client);
>>>>>>>> + int err;
>>>>>>>> +
>>>>>>>> + err = vic_load_firmware(vic);
>>>>>>>
>>>>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either
>>>>>>> replace this with RPM get/put or do something else.
>>>>>
>>>>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although
>>>>> it looks like it might race with the vic_load_firmware call in
>>>>> vic_runtime_resume which probably needs to be fixed.
>>>>
>>>> It was not clear from the function's name that h/w is untouched, I read
>>>> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename
>>>> vic_load_firmware() to vic_prepare_firmware_image().
>>>>
>>>> And yes, technically lock is needed.
>>>
>>> Yep, I'll consider renaming it.
>>
>> Looking at this all again, I'd suggest to change:
>>
>> int get_streamid_offset(client)
>>
>> to:
>>
>> int get_streamid_offset(client, *offset)
>>
>> and bail out if get_streamid_offset() returns error. It's never okay to
>> ignore errors.
>
> Sure, seems reasonable. We'll still need some error code to indicate
> that context isolation isn't available for the engine and continue on in
> that case but that's better than just ignoring all of them.
Yes, check for -EOPNOTSUPP and skip it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset
22.02.2022 11:27, Mikko Perttunen пишет:
> On 2/21/22 22:10, Dmitry Osipenko wrote:
>> 21.02.2022 14:44, Mikko Perttunen пишет:
>>> On 2/19/22 20:54, Dmitry Osipenko wrote:
>>>> 19.02.2022 21:49, Dmitry Osipenko пишет:
>>>>> 18.02.2022 14:39, Mikko Perttunen пишет:
>>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client)
>>>>>> +{
>>>>>> + struct vic *vic = to_vic(client);
>>>>>> + int err;
>>>>>> +
>>>>>> + err = vic_load_firmware(vic);
>>>>>
>>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either
>>>>> replace this with RPM get/put or do something else.
>>>
>>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although
>>> it looks like it might race with the vic_load_firmware call in
>>> vic_runtime_resume which probably needs to be fixed.
>>
>> It was not clear from the function's name that h/w is untouched, I read
>> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename
>> vic_load_firmware() to vic_prepare_firmware_image().
>>
>> And yes, technically lock is needed.
>
> Yep, I'll consider renaming it.
Looking at this all again, I'd suggest to change:
int get_streamid_offset(client)
to:
int get_streamid_offset(client, *offset)
and bail out if get_streamid_offset() returns error. It's never okay to
ignore errors.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset
21.02.2022 14:44, Mikko Perttunen пишет:
> On 2/19/22 20:54, Dmitry Osipenko wrote:
>> 19.02.2022 21:49, Dmitry Osipenko пишет:
>>> 18.02.2022 14:39, Mikko Perttunen пишет:
>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client)
>>>> +{
>>>> + struct vic *vic = to_vic(client);
>>>> + int err;
>>>> +
>>>> + err = vic_load_firmware(vic);
>>>
>>> You can't invoke vic_load_firmware() while RPM is suspended. Either
>>> replace this with RPM get/put or do something else.
>
> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although
> it looks like it might race with the vic_load_firmware call in
> vic_runtime_resume which probably needs to be fixed.
It was not clear from the function's name that h/w is untouched, I read
"load" as "upload" and then looked at vic_runtime_resume(). I'd rename
vic_load_firmware() to vic_prepare_firmware_image().
And yes, technically lock is needed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 9/9] drm/tegra: Support context isolation
21.02.2022 15:06, Mikko Perttunen пишет:
> On 2/19/22 20:35, Dmitry Osipenko wrote:
>> 18.02.2022 14:39, Mikko Perttunen пишет:
>>> + if (context->memory_context &&
>>> context->client->ops->get_streamid_offset) {
>> ^^^
>>> + int offset =
>>> context->client->ops->get_streamid_offset(context->client);
>>> +
>>> + if (offset >= 0) {
>>> + job->context = context->memory_context;
>>> + job->engine_streamid_offset = offset;
>>> + host1x_context_get(job->context);
>>> + }
>>
>> You should bump refcount unconditionally or you'll get refcnt underflow
>> on put, when offset < 0.
>
> This refcount is intended to be dropped from 'release_job', where it's
> dropped if job->context is set, which it is from this path.
>
>>
>>> + }
>>> +
>>> /*
>>> * job_data is now part of job reference counting, so don't
>>> release
>>> * it from here.
>>> diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c
>>> index 9ab9179d2026..be33da54d12c 100644
>>> --- a/drivers/gpu/drm/tegra/uapi.c
>>> +++ b/drivers/gpu/drm/tegra/uapi.c
>>> @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct
>>> tegra_drm_context *context)
>>> struct tegra_drm_mapping *mapping;
>>> unsigned long id;
>>> + if (context->memory_context)
>>> + host1x_context_put(context->memory_context);
>>
>> The "if (context->memory_context &&
>> context->client->ops->get_streamid_offset)" above doesn't match the "if
>> (context->memory_context)". You'll get refcount underflow.
>
> And this drop is for the refcount implicitly added when allocating the
> memory context through host1x_context_alloc; so these two places should
> be independent.
>
> Please elaborate if I missed something.
You named context as memory_context and then have
context=context->memory_context. Please try to improve the variable
names, like drm_ctx->host1x_ctx for example.
I'm also not a big fan of the "if (ref) put(ref)" pattern. Won't be
better to move all the "if (!NULL)" checks inside of get()/put() and
make the invocations unconditional?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset
19.02.2022 21:49, Dmitry Osipenko пишет:
> 18.02.2022 14:39, Mikko Perttunen пишет:
>> +static int vic_get_streamid_offset(struct tegra_drm_client *client)
>> +{
>> +struct vic *vic = to_vic(client);
>> +int err;
>> +
>> +err = vic_load_firmware(vic);
>
> You can't invoke vic_load_firmware() while RPM is suspended. Either
> replace this with RPM get/put or do something else.
>
>> +if (err < 0)
>> +return err;
>> +
>> +if (vic->can_use_context)
>> +return 0x30;
>> +else
>> +return -ENOTSUPP;
>
> If (!vic->can_use_context)
> return -ENOTSUPP;
>
> return 0x30;
and s/ENOTSUPP/EOPNOTSUPP/
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset
18.02.2022 14:39, Mikko Perttunen пишет:
> +static int vic_get_streamid_offset(struct tegra_drm_client *client)
> +{
> + struct vic *vic = to_vic(client);
> + int err;
> +
> + err = vic_load_firmware(vic);
You can't invoke vic_load_firmware() while RPM is suspended. Either
replace this with RPM get/put or do something else.
> + if (err < 0)
> + return err;
> +
> + if (vic->can_use_context)
> + return 0x30;
> + else
> + return -ENOTSUPP;
If (!vic->can_use_context)
return -ENOTSUPP;
return 0x30;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 9/9] drm/tegra: Support context isolation
18.02.2022 14:39, Mikko Perttunen пишет:
> + if (context->memory_context &&
> context->client->ops->get_streamid_offset) {
^^^
> + int offset =
> context->client->ops->get_streamid_offset(context->client);
> +
> + if (offset >= 0) {
> + job->context = context->memory_context;
> + job->engine_streamid_offset = offset;
> + host1x_context_get(job->context);
> + }
You should bump refcount unconditionally or you'll get refcnt underflow
on put, when offset < 0.
> + }
> +
> /*
>* job_data is now part of job reference counting, so don't release
>* it from here.
> diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c
> index 9ab9179d2026..be33da54d12c 100644
> --- a/drivers/gpu/drm/tegra/uapi.c
> +++ b/drivers/gpu/drm/tegra/uapi.c
> @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct
> tegra_drm_context *context)
> struct tegra_drm_mapping *mapping;
> unsigned long id;
>
> + if (context->memory_context)
> + host1x_context_put(context->memory_context);
The "if (context->memory_context &&
context->client->ops->get_streamid_offset)" above doesn't match the "if
(context->memory_context)". You'll get refcount underflow.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/9] gpu: host1x: Add context bus
19.02.2022 20:54, Dmitry Osipenko пишет: > 18.02.2022 14:39, Mikko Perttunen пишет: >> +config TEGRA_HOST1X_CONTEXT_BUS >> +bool >> + >> config TEGRA_HOST1X >> tristate "NVIDIA Tegra host1x driver" >> depends on ARCH_TEGRA || (ARM && COMPILE_TEST) >> select DMA_SHARED_BUFFER >> +select TEGRA_HOST1X_CONTEXT_BUS > > What is the point of TEGRA_HOST1X_CONTEXT_BUS if it's selected > unconditionally? I see now that it's used by arm-smmu.c, should be okay then. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/9] gpu: host1x: Add context bus
18.02.2022 14:39, Mikko Perttunen пишет: > +config TEGRA_HOST1X_CONTEXT_BUS > + bool > + > config TEGRA_HOST1X > tristate "NVIDIA Tegra host1x driver" > depends on ARCH_TEGRA || (ARM && COMPILE_TEST) > select DMA_SHARED_BUFFER > + select TEGRA_HOST1X_CONTEXT_BUS What is the point of TEGRA_HOST1X_CONTEXT_BUS if it's selected unconditionally? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/9] gpu: host1x: Add context device management code
18.02.2022 14:39, Mikko Perttunen пишет:
> + for (index = 0; index < cdl->len; index++) {
> + struct iommu_fwspec *fwspec;
> +
> + ctx = >devs[index];
> +
> + ctx->host = host1x;
> +
> + device_initialize(>dev);
> +
> + ctx->dev.dma_mask = _device_dma_mask;
> + ctx->dev.coherent_dma_mask = context_device_dma_mask;
> + dev_set_name(>dev, "host1x-ctx.%d", index);
> + ctx->dev.bus = _context_device_bus_type;
host1x_context_device_bus_type will be an undefined symbol if
CONFIG_TEGRA_HOST1X_CONTEXT_BUS=n? Please compile and test all combinations.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/9] gpu: host1x: Add context device management code
18.02.2022 14:39, Mikko Perttunen пишет:
...
> +/*
> + * Due to an issue with T194 NVENC, only 38 bits can be used.
> + * Anyway, 256GiB of IOVA ought to be enough for anyone.
> + */
> +static dma_addr_t context_device_dma_mask = DMA_BIT_MASK(38);
s/dma_addr_t/u64/ ? Apparently you should get compilation warning on ARM32.
https://elixir.bootlin.com/linux/v5.17-rc4/source/include/linux/device.h#L524
> +int host1x_context_list_init(struct host1x *host1x)
> +{
> + struct host1x_context_list *cdl = >context_list;
> + struct host1x_context *ctx;
> + struct device_node *node;
> + int index;
Nitpick: unsigned int
...
> +del_devices:
> + while (--index >= 0)
Nitpick: while (index--)
...
> +void host1x_context_list_free(struct host1x_context_list *cdl)
> +{
> + int i;
Nitpick: unsigned int
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/8] Host1x context isolation support
14.12.2021 17:53, Mikko Perttunen пишет: > On 12/14/21 16:35, Dmitry Osipenko wrote: >> 14.12.2021 11:05, Jon Hunter пишет: >>> Hi all, >>> >>> Still no response on this :-( >> >> I see only two patches on Tegra ML and others on DRI ML. Might be good >> to start with re-sending this whole series and CCing MLs properly. >> > > All patches should have been sent to the same set of addresses. At least > LWN's archive seems to agree.. Indeed, I see that Tegra ML was CCed and I see all patches on Tegra patchwork, but I don't see them all on lore and gmane. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/8] Host1x context isolation support
14.12.2021 11:05, Jon Hunter пишет: > Hi all, > > Still no response on this :-( I see only two patches on Tegra ML and others on DRI ML. Might be good to start with re-sending this whole series and CCing MLs properly. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 23:01, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:58:32PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:54, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 22:24, Nicolin Chen пишет:
>>>>> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned
>>>>>>> int pt_index)
>>>>>>> +{
>>>>>>> + return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>>>>>> +(pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>>>>>> +}
>>>>>>
>>>>>> I'd change the return type to u32 here, for consistency.
>>>>>
>>>>> The whole file defines iova using "unsigned long", which I see
>>>>> as the consistency... If we change it to u32, it'd be probably
>>>>> necessary to change all iova types to u32 too... So I'd rather
>>>>> keep it "unsigned long" here. If you see a big necessity to do
>>>>> that, an additional patch would be nicer IMHO.
>>>>>
>>>>
>>>> In general IOVA is "unsigned long", of course. But in case of Tegra
>>>> SMMU, we know that is always u32.
>>>>
>>>> Alright, I see now that there are other places in the driver code that
>>>> use "unsigned long", so need to change it in this patch.
>>>
>>> I think we should do that in a separate patch that changes the iova
>>> type in the entire tegra-smmu file. I can add one in this series, if
>>> this makes you happy...
>>>
>>
>> Please keep it "unsigned long", no need for extra patches.
>
> Oh, I guess that "so need to change it in this patch" should be
> "so (no) need to change it in this patch" then?
>
Indeed, I missed that typo, sorry.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 22:54, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:24, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned
>>>>> int pt_index)
>>>>> +{
>>>>> + return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>>>> +(pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>>>> +}
>>>>
>>>> I'd change the return type to u32 here, for consistency.
>>>
>>> The whole file defines iova using "unsigned long", which I see
>>> as the consistency... If we change it to u32, it'd be probably
>>> necessary to change all iova types to u32 too... So I'd rather
>>> keep it "unsigned long" here. If you see a big necessity to do
>>> that, an additional patch would be nicer IMHO.
>>>
>>
>> In general IOVA is "unsigned long", of course. But in case of Tegra
>> SMMU, we know that is always u32.
>>
>> Alright, I see now that there are other places in the driver code that
>> use "unsigned long", so need to change it in this patch.
>
> I think we should do that in a separate patch that changes the iova
> type in the entire tegra-smmu file. I can add one in this series, if
> this makes you happy...
>
Please keep it "unsigned long", no need for extra patches.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 22:51, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:40:42PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:32, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu
>>>>> *smmu,
>>>>> if (group->swgrp != swgrp)
>>>>> continue;
>>>>> group->as = NULL;
>>>>> +
>>>>> + if (smmu->debugfs_mappings) {
>>>> Do we really need this check?
>>>>
>>>> Looks like all debugfs_create_dir() usages in this driver are incorrect,
>>>> that function never returns NULL. Please fix this.
>>> debugfs_create_dir returns ERR_PTR on failure. So here should be
>>> to check !IS_ERR. Thanks for pointing it out!
>>>
>>
>> All debugfs functions handle IS_ERR(). GregKH removes all such checks
>> all over the kernel. So the check shouldn't be needed at all, please
>> remove it if it's unneeded or prove that it's needed.
>
> debugfs_create_file can handle a NULL parent, but not ERR_PTR one,
> and then it puts the new node under the root. So either passing an
> ERR_PTR parent or creating orphan nodes here doesn't sound good...
>
What makes you say so? Please show the exact source code that will cause
the problem.
The smmu->debugfs_mappings can't ever be NULL and debugfs_create_file
handles the ERR_PTR [1][2].
[1] https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L397
[2] https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L330
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 22:24, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 10:38, Nicolin Chen пишет:
>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
>>> pt_index)
>>> +{
>>> + return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>> +(pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>> +}
>>
>> I'd change the return type to u32 here, for consistency.
>
> The whole file defines iova using "unsigned long", which I see
> as the consistency... If we change it to u32, it'd be probably
> necessary to change all iova types to u32 too... So I'd rather
> keep it "unsigned long" here. If you see a big necessity to do
> that, an additional patch would be nicer IMHO.
>
In general IOVA is "unsigned long", of course. But in case of Tegra
SMMU, we know that is always u32.
Alright, I see now that there are other places in the driver code that
use "unsigned long", so need to change it in this patch.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 22:32, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 10:38, Nicolin Chen пишет:
>>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu
>>> *smmu,
>>> if (group->swgrp != swgrp)
>>> continue;
>>> group->as = NULL;
>>> +
>>> + if (smmu->debugfs_mappings) {
>> Do we really need this check?
>>
>> Looks like all debugfs_create_dir() usages in this driver are incorrect,
>> that function never returns NULL. Please fix this.
> debugfs_create_dir returns ERR_PTR on failure. So here should be
> to check !IS_ERR. Thanks for pointing it out!
>
All debugfs functions handle IS_ERR(). GregKH removes all such checks
all over the kernel. So the check shouldn't be needed at all, please
remove it if it's unneeded or prove that it's needed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 10:38, Nicolin Chen пишет:
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
> pt_index)
> +{
> + return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +(pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
I'd change the return type to u32 here, for consistency.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
09.12.2021 10:38, Nicolin Chen пишет:
> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
> if (group->swgrp != swgrp)
> continue;
> group->as = NULL;
> +
> + if (smmu->debugfs_mappings) {
Do we really need this check?
Looks like all debugfs_create_dir() usages in this driver are incorrect,
that function never returns NULL. Please fix this.
> + struct dentry *d;
The file name is wrong here.
if (group->soc)
name = group->soc->name;
else
name = group->swgrp->name;
> + d = debugfs_lookup(group->swgrp->name,
> +smmu->debugfs_mappings);
> + debugfs_remove(d);
> + }
This now looks problematic to me. You created debugfs file when the
first member of the shared group was attached to AS, now you remove this
file when any device is detached. The shared debugfs file should be
refcounted or something.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
08.12.2021 11:47, Nicolin Chen пишет: > static void tegra_smmu_attach_as(struct tegra_smmu *smmu, >struct tegra_smmu_as *as, >unsigned int swgroup) > @@ -517,6 +646,12 @@ static void tegra_smmu_attach_as(struct tegra_smmu *smmu, > dev_warn(smmu->dev, >"overwriting group->as for swgroup: %s\n", > swgrp->name); > group->as = as; > + > + if (smmu->debugfs_mappings) > + debugfs_create_file(group->swgrp->name, 0444, > + smmu->debugfs_mappings, group, > + _smmu_debugfs_mappings_fops); I noticed this in KMSG: tegra-mc 7000f000.memory-controller: overwriting group->as for swgroup: g2 debugfs: File 'g2' in directory 'mappings' already present! tegra-mc 7000f000.memory-controller: overwriting group->as for swgroup: g2 debugfs: File 'g2' in directory 'mappings' already present Doesn't look okay, please fix. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
08.12.2021 11:47, Nicolin Chen пишет: > + seq_printf(s, "\t\t%-14s | %-4s | %-10s%s | %-10s%s | %-11s\n", > +"PTE RANGE", "ATTR", > +"PHYS", sizeof(phys_addr_t) > 4 ? "" : "", > +"IOVA", sizeof(dma_addr_t) > 4 ? "" : "", Can we change IOVA to u32? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
Hi,
08.12.2021 11:47, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable to a
> debugfs directory named "mappings".
>
> Attaching an example:
>
> [SWGROUP: xusb_host] [as: (id: 5), (attr: R|W|-), (pd_dma:
> 0x80005000)]
> {
> [index: 1023] 0xf0080040 (count: 52)
> {
> PTE RANGE | ATTR | PHYS | IOVA
> | SIZE
> [#913 , #913 ] | 0x7 | 0x000102674000 |
> 0xfff91000 | 0x1000
> [#914 , #914 ] | 0x7 | 0x000102672000 |
> 0xfff92000 | 0x1000
> [#915 , #915 ] | 0x7 | 0x000102671000 |
> 0xfff93000 | 0x1000
> [#916 , #916 ] | 0x7 | 0x00010267 |
> 0xfff94000 | 0x1000
> [#921 , #921 ] | 0x7 | 0xfcc0 |
> 0xfff99000 | 0x1000
> [#922 , #922 ] | 0x7 | 0x00010266d000 |
> 0xfff9a000 | 0x1000
> [#923 , #923 ] | 0x7 | 0x00010266c000 |
> 0xfff9b000 | 0x1000
> [#948 , #948 ] | 0x7 | 0x000102668000 |
> 0xfffb4000 | 0x1000
> [#949 , #949 ] | 0x7 | 0x000102667000 |
> 0xfffb5000 | 0x1000
> [#950 , #950 ] | 0x7 | 0x000102666000 |
> 0xfffb6000 | 0x1000
> [#951 , #951 ] | 0x7 | 0x000102665000 |
> 0xfffb7000 | 0x1000
> [#952 , #952 ] | 0x7 | 0x00010264b000 |
> 0xfffb8000 | 0x1000
> [#953 , #953 ] | 0x7 | 0x00010264a000 |
> 0xfffb9000 | 0x1000
> [#954 , #954 ] | 0x7 | 0x000102649000 |
> 0xfffba000 | 0x1000
> [#955 , #955 ] | 0x7 | 0x000102648000 |
> 0xfffbb000 | 0x1000
> [#956 , #956 ] | 0x7 | 0x00010260f000 |
> 0xfffbc000 | 0x1000
> [#957 , #957 ] | 0x7 | 0x00010260e000 |
> 0xfffbd000 | 0x1000
> [#958 , #958 ] | 0x7 | 0x00010260d000 |
> 0xfffbe000 | 0x1000
> [#959 , #959 ] | 0x7 | 0x00010260b000 |
> 0xfffbf000 | 0x1000
> [#960 , #992 ] | 0x7 | 0x0001025ea000 |
> 0xfffc | 0x21000
> }
> }
> Total PDEs: 1, total PTEs: 52
The patch is almost good to me, there is one nit.
On older SoCs we put multiple devices into the same shared group and the
debugfs shows it as the first member of the group.
This is what we get on T30 using this v7:
# ls/sys/kernel/debug/smmu/mappings
g2 hc vde
# cat /sys/kernel/debug/smmu/mappings/g2
[SWGROUP: g2] [as: (id: 2), (attr: R|W|-), (pd_dma: 0x834a6000)]
{
[index: 0] 0xf0083494 (count: 1000)
{
PTE RANGE | ATTR | PHYS | IOVA | SIZE
[#0 , #15 ] | 0x7 | 0xbfde | 0x | 0x1
[#16 , #47 ] | 0x7 | 0xbfdc | 0x0001 | 0x2
[#48 , #111 ] | 0x7 | 0xbfd8 | 0x0003 | 0x4
[#112 , #239 ] | 0x7 | 0xbfd0 | 0x0007 | 0x8
[#240 , #495 ] | 0x7 | 0xbfc0 | 0x000f | 0x10
[#496 , #999 ] | 0x7 | 0xbf40 | 0x001f | 0x1f8000
}
}
Total PDEs: 1, total PTEs: 1000
See that name is "g2", meanwhile these mappings are made by display client
driver.
I changed your patch to use the proper group name and to show all members of
the group, see that change in the end of this email.
With my change applied, we get:
# ls/sys/kernel/debug/smmu/mappings
drm hc vde
# cat /sys/kernel/debug/smmu/mappings/drm
[SWGROUP: dc, dcb, g2, nv, nv2] [as: (id: 2), (attr: R|W|-), (pd_dma:
0x8248)]
{
[index: 0] 0xf0083583 (count: 1000)
{
PTE RANGE | ATTR | PHYS | IOVA | SIZE
[#0 , #15 ] | 0x7 | 0xbfde | 0x | 0x1
[#16 , #47 ] | 0x7 | 0xbfdc | 0x0001 | 0x2
[#48 , #111 ] | 0x7 | 0xbfd8 | 0x0003 | 0x4
[#112 , #239 ] | 0x7 | 0xbfd0 | 0x0007 | 0x8
[#240 , #495 ] | 0x7 | 0xbfc0 | 0x000f | 0x10
[#496 , #999 ] | 0x7 | 0xbf40 | 0x001f | 0x1f8000
}
}
Total PDEs: 1, total PTEs: 1000
--- >8 ---
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 575e82076270..fb1326a72038 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -509,6 +509,7 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
static int tegra_smmu_debugfs_mappings_show(struct seq_file *s, void *data)
{
struct tegra_smmu_group *group = s->private;
+ const struct tegra_smmu_group_soc *soc;
const struct tegra_smmu_swgroup *swgrp;
struct tegra_smmu_as *as;
struct
Re: [PATCH v2 5/5] iommu/tegra-smmu: Support managed domains
23.04.2021 19:32, Thierry Reding пишет: > From: Navneet Kumar > > Allow creating identity and DMA API compatible IOMMU domains. When > creating a DMA API compatible domain, make sure to also create the > required cookie. IOMMU_DOMAIN_DMA should be a disaster. It shouldn't work without preparing DRM and VDE drivers at first. We discussed this briefly in the past. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
04.10.2021 22:23, Thierry Reding пишет: > On Sun, Oct 03, 2021 at 04:09:56AM +0300, Dmitry Osipenko wrote: >> 23.04.2021 19:32, Thierry Reding пишет: >>> I've made corresponding changes in the proprietary bootloader, added a >>> compatibility shim in U-Boot (which forwards information created by the >>> proprietary bootloader to the kernel) and the attached patches to test >>> this on Jetson TX1, Jetson TX2 and Jetson AGX Xavier. >> >> Could you please tell what downstream kernel does for coping with the >> identity mappings in conjunction with the original proprietary bootloader? >> >> If there is some other method of passing mappings to kernel, could it be >> supported by upstream? Putting burden on users to upgrade bootloader >> feels a bit inconvenient. > > It depends on the chip generation. As far as I know there have been > several iterations. The earliest was to pass this information via a > command-line option, but more recent versions use device tree to pass > this information in a similar way as described here. However, these > use non-standard DT bindings, so I don't think we can just implement > them as-is. Is it possible to boot upstream kernel with that original bootloader? I remember seeing other platforms, like QCOM, supporting downstream quirks in upstream kernel on a side, i.e. they are undocumented, but the additional support code is there. That is what "normal" people want. You should consider doing that for Tegra too, if possible. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
23.04.2021 19:32, Thierry Reding пишет: > I've made corresponding changes in the proprietary bootloader, added a > compatibility shim in U-Boot (which forwards information created by the > proprietary bootloader to the kernel) and the attached patches to test > this on Jetson TX1, Jetson TX2 and Jetson AGX Xavier. Could you please tell what downstream kernel does for coping with the identity mappings in conjunction with the original proprietary bootloader? If there is some other method of passing mappings to kernel, could it be supported by upstream? Putting burden on users to upgrade bootloader feels a bit inconvenient. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.09.2021 15:09, Dmitry Osipenko пишет:
> 15.09.2021 07:38, Nicolin Chen пишет:
>> On Tue, Sep 14, 2021 at 10:20:30PM +0300, Dmitry Osipenko wrote:
>>> 14.09.2021 21:49, Nicolin Chen пишет:
>>>> On Tue, Sep 14, 2021 at 04:29:15PM +0300, Dmitry Osipenko wrote:
>>>>> 14.09.2021 04:38, Nicolin Chen пишет:
>>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned
>>>>>> int pt_index)
>>>>>> +{
>>>>>> +return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) <<
>>>>>> SMMU_PDE_SHIFT |
>>>>>> + ((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) <<
>>>>>> SMMU_PTE_SHIFT;
>>>>>> +}
>>>>>
>>>>> We know that IOVA is fixed to u32 for this controller. Can we avoid all
>>>>> these dma_addr_t castings? It should make code cleaner a tad, IMO.
>>>>
>>>> Tegra210 actually supports 34-bit IOVA...
>>>>
>>>
>>> It doesn't. 34-bit is PA, 32-bit is VA.
>>>
>>> Quote from T210 TRM:
>>>
>>> "The SMMU is a centralized virtual-to-physical translation for MSS. It
>>> maps a 32-bit virtual address to a 34-bit physical address. If the
>>> client address is 40 bits then bits 39:32 are ignored."
>>
>> If you scroll down by a couple of sections, you can see 34-bit
>> virtual addresses in section 18.6.1.2; and if checking one ASID
>> register, you can see it mention the extra two bits va[33:32].
>
> Thanks for the pointer. It says that only certain memory clients allow
> to combine 4 ASIDs to form 34bit VA space. In this case the PA space is
> split into 4GB areas and there are additional bitfields which configure
> the ASID mapping of each 4GB area. Still each ASID is 32bit.
>
> This is what TRM says:
>
> "For the GPU and other clients with 34-bit address interfaces, the ASID
> registers are extended to point to four ASIDs. The SMMU supports 4GB of
> virtual address space per ASID, so mapping addr[33:32] into ASID[1:0]
> extends the virtual address space of a client to 16GB."
>
>> However, the driver currently sets its geometry.aperture_end to
>> 32-bit, and we can only get 32-bit IOVAs using PDE and PTE only,
>> so I think it should be safe to remove the castings here. I'll
>> wait for a couple of days and see if there'd be other comments
>> for me to address in next version.
>
> You will need to read the special "ASID Assignment Register" which
> supports 4 sub-ASIDs to translate the PA address into the actual VA. By
* VA to PA
> default all clients are limited to a single ASID and upstream kernel
> doesn't support programming of 34bit VAs. So doesn't worth the effort to
> fully translate the VA, IMO.
>
>>> Even if it supported more than 32bit, then the returned ulong is 32bit,
>>> which doesn't make sense.
>>
>> On ARM64 (Tegra210), isn't ulong 64-bit?
>
> Yes, indeed.
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.09.2021 07:38, Nicolin Chen пишет:
> On Tue, Sep 14, 2021 at 10:20:30PM +0300, Dmitry Osipenko wrote:
>> 14.09.2021 21:49, Nicolin Chen пишет:
>>> On Tue, Sep 14, 2021 at 04:29:15PM +0300, Dmitry Osipenko wrote:
>>>> 14.09.2021 04:38, Nicolin Chen пишет:
>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned
>>>>> int pt_index)
>>>>> +{
>>>>> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>>>> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>>>> +}
>>>>
>>>> We know that IOVA is fixed to u32 for this controller. Can we avoid all
>>>> these dma_addr_t castings? It should make code cleaner a tad, IMO.
>>>
>>> Tegra210 actually supports 34-bit IOVA...
>>>
>>
>> It doesn't. 34-bit is PA, 32-bit is VA.
>>
>> Quote from T210 TRM:
>>
>> "The SMMU is a centralized virtual-to-physical translation for MSS. It
>> maps a 32-bit virtual address to a 34-bit physical address. If the
>> client address is 40 bits then bits 39:32 are ignored."
>
> If you scroll down by a couple of sections, you can see 34-bit
> virtual addresses in section 18.6.1.2; and if checking one ASID
> register, you can see it mention the extra two bits va[33:32].
Thanks for the pointer. It says that only certain memory clients allow
to combine 4 ASIDs to form 34bit VA space. In this case the PA space is
split into 4GB areas and there are additional bitfields which configure
the ASID mapping of each 4GB area. Still each ASID is 32bit.
This is what TRM says:
"For the GPU and other clients with 34-bit address interfaces, the ASID
registers are extended to point to four ASIDs. The SMMU supports 4GB of
virtual address space per ASID, so mapping addr[33:32] into ASID[1:0]
extends the virtual address space of a client to 16GB."
> However, the driver currently sets its geometry.aperture_end to
> 32-bit, and we can only get 32-bit IOVAs using PDE and PTE only,
> so I think it should be safe to remove the castings here. I'll
> wait for a couple of days and see if there'd be other comments
> for me to address in next version.
You will need to read the special "ASID Assignment Register" which
supports 4 sub-ASIDs to translate the PA address into the actual VA. By
default all clients are limited to a single ASID and upstream kernel
doesn't support programming of 34bit VAs. So doesn't worth the effort to
fully translate the VA, IMO.
>> Even if it supported more than 32bit, then the returned ulong is 32bit,
>> which doesn't make sense.
>
> On ARM64 (Tegra210), isn't ulong 64-bit?
Yes, indeed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
14.09.2021 21:49, Nicolin Chen пишет:
> On Tue, Sep 14, 2021 at 04:29:15PM +0300, Dmitry Osipenko wrote:
>> 14.09.2021 04:38, Nicolin Chen пишет:
>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
>>> pt_index)
>>> +{
>>> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>> + ((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>> +}
>>
>> We know that IOVA is fixed to u32 for this controller. Can we avoid all
>> these dma_addr_t castings? It should make code cleaner a tad, IMO.
>
> Tegra210 actually supports 34-bit IOVA...
>
It doesn't. 34-bit is PA, 32-bit is VA.
Quote from T210 TRM:
"The SMMU is a centralized virtual-to-physical translation for MSS. It
maps a 32-bit virtual address to a 34-bit physical address. If the
client address is 40 bits then bits 39:32 are ignored."
Even if it supported more than 32bit, then the returned ulong is 32bit,
which doesn't make sense.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs
14.09.2021 04:38, Nicolin Chen пишет:
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
We know that IOVA is fixed to u32 for this controller. Can we avoid all
these dma_addr_t castings? It should make code cleaner a tad, IMO.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/5] iommu: Implement of_iommu_get_resv_regions()
16.07.2021 17:41, Rob Herring пишет:
> On Fri, Jul 2, 2021 at 8:05 AM Dmitry Osipenko wrote:
>>
>> 23.04.2021 19:32, Thierry Reding пишет:
>>> +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
>>> +{
>>> + struct of_phandle_iterator it;
>>> + int err;
>>> +
>>> + of_for_each_phandle(, err, dev->of_node, "memory-region",
>>> "#memory-region-cells", 0) {
>>> + struct iommu_resv_region *region;
>>> + struct of_phandle_args args;
>>> + struct resource res;
>>> +
>>> + args.args_count = of_phandle_iterator_args(, args.args,
>>> MAX_PHANDLE_ARGS);
>>> +
>>> + err = of_address_to_resource(it.node, 0, );
>>> + if (err < 0) {
>>> + dev_err(dev, "failed to parse memory region %pOF:
>>> %d\n",
>>> + it.node, err);
>>> + continue;
>>> + }
>>> +
>>> + if (args.args_count > 0) {
>>> + /*
>>> + * Active memory regions are expected to be accessed
>>> by hardware during
>>> + * boot and must therefore have an identity mapping
>>> created prior to the
>>> + * driver taking control of the hardware. This
>>> ensures that non-quiescent
>>> + * hardware doesn't cause IOMMU faults during boot.
>>> + */
>>> + if (args.args[0] & MEMORY_REGION_IDENTITY_MAPPING) {
>>> + region = iommu_alloc_resv_region(res.start,
>>> resource_size(),
>>> + IOMMU_READ |
>>> IOMMU_WRITE,
>>> +
>>> IOMMU_RESV_DIRECT_RELAXABLE);
>>> + if (!region)
>>> + continue;
>>> +
>>> + list_add_tail(>list, list);
>>> + }
>>> + }
>>> + }
>>> +}
>>> +EXPORT_SYMBOL(of_iommu_get_resv_regions);
>>
>> Any reason why this is not EXPORT_SYMBOL_GPL? I'm curious what is the
>> logic behind the OF symbols in general since it looks like half of them
>> are GPL.
>
> Generally, new ones are _GPL. Old ones probably predate _GPL.
>
> This one is up to the IOMMU maintainers.
Thank you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1] iommu/tegra-smmu: Change debugfs directory name
14.07.2021 13:52, Joerg Roedel пишет: > On Mon, Jul 12, 2021 at 03:13:41AM +0300, Dmitry Osipenko wrote: >> -err = iommu_device_sysfs_add(>iommu, dev, NULL, dev_name(dev)); >> +err = iommu_device_sysfs_add(>iommu, dev, NULL, "smmu"); > > Are you sure no one is relying on the old name so that this change > breaks ABI? IIUC, Thierry and Jon have a testing suite that uses the old name, but it shouldn't be a problem to support the new name in addition to the old name since it's internal/private testing suite. The reason I'm proposing this change is because it's not obvious where the SMMU debug is located when you look at the debugfs directory, it takes some effort to find it. > Also this change means that there is always be only one SMMU > per system. Is that guaranteed too? A single SMMU is guaranteed here. Only latest Tegra SoCs have two SMMUs and those are ARM SMMUs, while this is a legacy Tegra SMMU here. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v1] iommu/tegra-smmu: Change debugfs directory name
Change debugfs directory name to "smmu", which is a much more obvious name than the generic name of the memory controller device-tree node. Signed-off-by: Dmitry Osipenko --- drivers/iommu/tegra-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 0a281833f611..093c270b9245 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -1141,7 +1141,7 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev, tegra_smmu_ahb_enable(); - err = iommu_device_sysfs_add(>iommu, dev, NULL, dev_name(dev)); + err = iommu_device_sysfs_add(>iommu, dev, NULL, "smmu"); if (err) return ERR_PTR(err); -- 2.32.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/5] dt-bindings: reserved-memory: Document memory region specifier
01.07.2021 21:14, Thierry Reding пишет:
> On Tue, Jun 08, 2021 at 06:51:40PM +0200, Thierry Reding wrote:
>> On Fri, May 28, 2021 at 06:54:55PM +0200, Thierry Reding wrote:
>>> On Thu, May 20, 2021 at 05:03:06PM -0500, Rob Herring wrote:
On Fri, Apr 23, 2021 at 06:32:30PM +0200, Thierry Reding wrote:
> From: Thierry Reding
>
> Reserved memory region phandle references can be accompanied by a
> specifier that provides additional information about how that specific
> reference should be treated.
>
> One use-case is to mark a memory region as needing an identity mapping
> in the system's IOMMU for the device that references the region. This is
> needed for example when the bootloader has set up hardware (such as a
> display controller) to actively access a memory region (e.g. a boot
> splash screen framebuffer) during boot. The operating system can use the
> identity mapping flag from the specifier to make sure an IOMMU identity
> mapping is set up for the framebuffer before IOMMU translations are
> enabled for the display controller.
>
> Signed-off-by: Thierry Reding
> ---
> .../reserved-memory/reserved-memory.txt | 21 +++
> include/dt-bindings/reserved-memory.h | 8 +++
> 2 files changed, 29 insertions(+)
> create mode 100644 include/dt-bindings/reserved-memory.h
Sorry for being slow on this. I have 2 concerns.
First, this creates an ABI issue. A DT with cells in 'memory-region'
will not be understood by an existing OS. I'm less concerned about this
if we address that with a stable fix. (Though I'm pretty sure we've
naively added #?-cells in the past ignoring this issue.)
>>>
>>> A while ago I had proposed adding memory-region*s* as an alternative
>>> name for memory-region to make the naming more consistent with other
>>> types of properties (think clocks, resets, gpios, ...). If we added
>>> that, we could easily differentiate between the "legacy" cases where
>>> no #memory-region-cells was allowed and the new cases where it was.
>>>
Second, it could be the bootloader setting up the reserved region. If a
node already has 'memory-region', then adding more regions is more
complicated compared to adding new properties. And defining what each
memory-region entry is or how many in schemas is impossible.
>>>
>>> It's true that updating the property gets a bit complicated, but it's
>>> not exactly rocket science. We really just need to splice the array. I
>>> have a working implemention for this in U-Boot.
>>>
>>> For what it's worth, we could run into the same issue with any new
>>> property that we add. Even if we renamed this to iommu-memory-region,
>>> it's still possible that a bootloader may have to update this property
>>> if it already exists (it could be hard-coded in DT, or it could have
>>> been added by some earlier bootloader or firmware).
>>>
Both could be addressed with a new property. Perhaps something like
'iommu-memory-region = <>;'. I think the 'iommu' prefix is
appropriate given this is entirely because of the IOMMU being in the
mix. I might feel differently if we had other uses for cells, but I
don't really see it in this case.
>>>
>>> I'm afraid that down the road we'll end up with other cases and then we
>>> might proliferate a number of *-memory-region properties with varying
>>> prefixes.
>>>
>>> I am aware of one other case where we might need something like this: on
>>> some Tegra SoCs we have audio processors that will access memory buffers
>>> using a DMA engine. These processors are booted from early firmware
>>> using firmware from system memory. In order to avoid trashing the
>>> firmware, we need to reserve memory. We can do this using reserved
>>> memory nodes. However, the audio DMA engine also uses the SMMU, so we
>>> need to make sure that the firmware memory is marked as reserved within
>>> the SMMU. This is similar to the identity mapping case, but not exactly
>>> the same. Instead of creating a 1:1 mapping, we just want that IOVA
>>> region to be reserved (i.e. IOMMU_RESV_RESERVED instead of
>>> IOMMU_RESV_DIRECT{,_RELAXABLE}).
>>>
>>> That would also fall into the IOMMU domain, but we can't reuse the
>>> iommu-memory-region property for that because then we don't have enough
>>> information to decide which type of reservation we need.
>>>
>>> We could obviously make iommu-memory-region take a specifier, but we
>>> could just as well use memory-regions in that case since we have
>>> something more generic anyway.
>>>
>>> With the #memory-region-cells proposal, we can easily extend the cell in
>>> the specifier with an additional MEMORY_REGION_IOMMU_RESERVE flag to
>>> take that other use case into account. If we than also change to the new
>>> memory-regions property name, we avoid the ABI issue (and we gain a bit
>>> of consistency while at it).
>>
>>
Re: [PATCH v2 2/5] iommu: Implement of_iommu_get_resv_regions()
23.04.2021 19:32, Thierry Reding пишет:
> +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
> +{
> + struct of_phandle_iterator it;
> + int err;
> +
> + of_for_each_phandle(, err, dev->of_node, "memory-region",
> "#memory-region-cells", 0) {
> + struct iommu_resv_region *region;
> + struct of_phandle_args args;
> + struct resource res;
> +
> + args.args_count = of_phandle_iterator_args(, args.args,
> MAX_PHANDLE_ARGS);
> +
> + err = of_address_to_resource(it.node, 0, );
> + if (err < 0) {
> + dev_err(dev, "failed to parse memory region %pOF: %d\n",
> + it.node, err);
> + continue;
> + }
> +
> + if (args.args_count > 0) {
> + /*
> + * Active memory regions are expected to be accessed by
> hardware during
> + * boot and must therefore have an identity mapping
> created prior to the
> + * driver taking control of the hardware. This ensures
> that non-quiescent
> + * hardware doesn't cause IOMMU faults during boot.
> + */
> + if (args.args[0] & MEMORY_REGION_IDENTITY_MAPPING) {
> + region = iommu_alloc_resv_region(res.start,
> resource_size(),
> + IOMMU_READ |
> IOMMU_WRITE,
> +
> IOMMU_RESV_DIRECT_RELAXABLE);
> + if (!region)
> + continue;
> +
> + list_add_tail(>list, list);
> + }
> + }
> + }
> +}
> +EXPORT_SYMBOL(of_iommu_get_resv_regions);
Any reason why this is not EXPORT_SYMBOL_GPL? I'm curious what is the
logic behind the OF symbols in general since it looks like half of them
are GPL.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
29.04.2021 08:51, Krishna Reddy пишет: > Hi Dmitry, > >> Thank you for the answer. Could you please give more information about: >> 1) Are you on software or hardware team, or both? > > I am in the software team and has contributed to initial Tegra SMMU driver in > the downstream along with earlier team member Hiroshi Doyu. > >> 2) Is SMMU a third party IP or developed in-house? > > Tegra SMMU is developed in-house. > >> 3) Do you have a direct access to HDL sources? Are you 100% sure that >> hardware does what you say? > > It was discussed with Hardware team before and again today as well. > Enabling ASID for display engine while it continues to access the buffer > memory is a safe operation. > As per HW team, The only side-effect that can happen is additional latency to > transaction as SMMU caches get warmed up. > >> 4) What happens when CPU writes to ASID register? Does SMMU state machine >> latch ASID status (making it visible) only at a single "safe" point? > > MC makes a decision on routing transaction through either SMMU page tables or > bypassing based on the ASID register value. It > checks the ASID register only once per transaction in the pipeline. Thank you very much for the clarification. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
28.04.2021 08:57, Mikko Perttunen пишет: > On 4/28/21 8:51 AM, Dmitry Osipenko wrote: >> 23.04.2021 19:32, Thierry Reding пишет: >>> Note that there will be no new releases of the bootloader for earlier >>> devices, so adding support for these new DT bindings will not be >>> practical. The bootloaders on those devices do pass information about >>> the active framebuffer via the kernel command-line, so we may want to >>> add code to create reserved regions in the IOMMU based on that. >> >> Since this change requires a bootloader update anyways, why it's not >> possible to fix the bootloader properly, making it to stop all the DMA >> activity before jumping into kernel? >> > > That is not desirable, as then we couldn't have seamless > bootloader-kernel boot splash transition. The seamless transition should be more complicated since it should require to read out the hardware state in order to convert it into DRM state + display panel needs to stay ON. It's a bit questionable whether this is really needed, so far this is not achievable in mainline. Nevertheless, it will be good to have an early simple-framebuffer, which I realized only after sending out the message. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
23.04.2021 19:32, Thierry Reding пишет: > Note that an earlier proposal was to use the existing simple-framebuffer > device tree bindings to transport this information. Unfortunately there > are cases where this is not enough. On Tegra SoCs, for example, the > bootloader will also set up a color space correction lookup table in the > system memory that the display controller will access during boot, > alongside the framebuffer. The simple-framebuffer DT bindings have no > way of describing this (and I guess one could argue that this particular > setup no longer is a "simple" framebuffer), so the above, more flexible > proposal was implemented. Will simple-framebuffer be able to use that reserved region transparently? Or will it require a custom simple-framebuffer driver? Could we make simple-framebuffer support a part of this series? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
23.04.2021 19:32, Thierry Reding пишет: > Note that there will be no new releases of the bootloader for earlier > devices, so adding support for these new DT bindings will not be > practical. The bootloaders on those devices do pass information about > the active framebuffer via the kernel command-line, so we may want to > add code to create reserved regions in the IOMMU based on that. Since this change requires a bootloader update anyways, why it's not possible to fix the bootloader properly, making it to stop all the DMA activity before jumping into kernel? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
27.04.2021 21:30, Krishna Reddy пишет: >> Is it always safe to enable SMMU ASID in a middle of a DMA request made by a >> memory client? > > From MC point of view, It is safe to enable and has been this way from many > years in downstream code for display engine. > It doesn't impact the transactions that have already bypassed SMMU before > enabling SMMU ASID. > Transactions that are yet to pass SMMU stage would go through SMMU once SMMU > ASID is enabled and visible. Hello, Thank you for the answer. Could you please give more information about: 1) Are you on software or hardware team, or both? 2) Is SMMU a third party IP or developed in-house? 3) Do you have a direct access to HDL sources? Are you 100% sure that hardware does what you say? 4) What happens when CPU writes to ASID register? Does SMMU state machine latch ASID status (making it visible) only at a single "safe" point? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
26.04.2021 10:14, Thierry Reding пишет: > On Sat, Apr 24, 2021 at 11:27:10PM +0300, Dmitry Osipenko wrote: >> 23.04.2021 18:23, Dmitry Osipenko пишет: >>> 23.04.2021 18:01, Guillaume Tucker пишет: >>>> On 02/04/2021 15:40, Dmitry Osipenko wrote: >>>>> 01.04.2021 11:55, Nicolin Chen пишет: >>>>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote: >>>>>>> The previous commit fixes problem where display client was attaching too >>>>>>> early to IOMMU during kernel boot in a multi-platform kernel >>>>>>> configuration >>>>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to >>>>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore, >>>>>>> revert it. >>>>>> >>>>>> Sorry for the late reply. I have been busy with downstream tasks. >>>>>> >>>>>> I will give them a try by the end of the week. Yet, probably it'd >>>>>> be better to include Guillaume also as he has the Nyan platform. >>>>>> >>>>> >>>>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which >>>>> I reproduced on Nexus 7. >>>>> >>>>> Guillaume, could you please give a test to these patches on Nyan Big? >>>>> There should be no EMEM errors in the kernel log with this patches. >>>>> >>>>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215 >>>> >>>> So sorry for the very late reply. I have tried the patches but >>>> hit some issues on linux-next, it's not reaching a login prompt >>>> with next-20210422. So I then tried with next-20210419 which >>>> does boot but shows the IOMMU error: >>>> >>>> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1 >>>> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping >>>> >>>> https://lava.collabora.co.uk/scheduler/job/3570052#L1120 >>>> >>>> The branch I'm using with the patches applied can be found here: >>>> >>>> >>>> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/ >>>> >>>> Hope this helps, let me know if you need anything else to be >>>> tested. >>> >>> >>> Hello Guillaume, >>> >>> The current linux-next doesn't boot on all ARM (AFAIK), the older >>> next-20210413 works. The above message should be unrelated to the boot >>> problem. It should be okay to ignore that message as it should be >>> harmless in yours case. >>> >> >> Although, the 20210419 should be good. >> >> Thierry, do you know what those SOR and Nouveau issues are about? > > There's a use-after-free (though it's really a use-before-init) issue in > linux-next at the moment, but a fix has been suggested. The fix for this > along with an additional leak plug is here: > > http://patchwork.ozlabs.org/project/linux-tegra/list/?series=240569 Nice, thank you. Maybe Guillaume could give it a test. > I'm not aware of any Nouveau issues. What version and platform are those > happening on? Are there any logs? I can't seem to find them in this > thread. This all is on Nyan Big using linux-next-20210419. There is a link to the full log above. I see this Nouveau failure: <6>[ 19.323113] nouveau 5700.gpu: Adding to iommu group 2 <6>[ 19.323615] nouveau 5700.gpu: NVIDIA GK20A (0ea000a1) <6>[ 19.323668] nouveau 5700.gpu: imem: using IOMMU <3>[ 19.323795] nouveau 5700.gpu: gr ctor failed: -2 <4>[ 19.323983] nouveau: probe of 5700.gpu failed with error -2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
24.04.2021 23:27, Dmitry Osipenko пишет: > 23.04.2021 18:23, Dmitry Osipenko пишет: >> 23.04.2021 18:01, Guillaume Tucker пишет: >>> On 02/04/2021 15:40, Dmitry Osipenko wrote: >>>> 01.04.2021 11:55, Nicolin Chen пишет: >>>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote: >>>>>> The previous commit fixes problem where display client was attaching too >>>>>> early to IOMMU during kernel boot in a multi-platform kernel >>>>>> configuration >>>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to >>>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore, >>>>>> revert it. >>>>> >>>>> Sorry for the late reply. I have been busy with downstream tasks. >>>>> >>>>> I will give them a try by the end of the week. Yet, probably it'd >>>>> be better to include Guillaume also as he has the Nyan platform. >>>>> >>>> >>>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which >>>> I reproduced on Nexus 7. >>>> >>>> Guillaume, could you please give a test to these patches on Nyan Big? >>>> There should be no EMEM errors in the kernel log with this patches. >>>> >>>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215 >>> >>> So sorry for the very late reply. I have tried the patches but >>> hit some issues on linux-next, it's not reaching a login prompt >>> with next-20210422. So I then tried with next-20210419 which >>> does boot but shows the IOMMU error: >>> >>> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1 >>> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping >>> >>> https://lava.collabora.co.uk/scheduler/job/3570052#L1120 >>> >>> The branch I'm using with the patches applied can be found here: >>> >>> >>> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/ >>> >>> Hope this helps, let me know if you need anything else to be >>> tested. >> >> >> Hello Guillaume, >> >> The current linux-next doesn't boot on all ARM (AFAIK), the older >> next-20210413 works. The above message should be unrelated to the boot >> problem. It should be okay to ignore that message as it should be >> harmless in yours case. >> > > Although, the 20210419 should be good. > > Thierry, do you know what those SOR and Nouveau issues are about? > I don't see DRM driver being loaded at all and no errors related to it in the log. This means that likely some of the DRM sub-drivers is stuck in deferred probe. Guillaume, could you please show contents of /sys/kernel/debug/devices_deferred ? These messages also don't look good: tegra-xusb-padctl 7009f000.padctl: failed to setup XUSB ports: -22 tegra-xusb-padctl: probe of 7009f000.padctl failed with error -22 I don't have T124 and all these problems should be specific to the T124 platform. Somebody with a direct access to hardware should look into it. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
23.04.2021 18:23, Dmitry Osipenko пишет: > 23.04.2021 18:01, Guillaume Tucker пишет: >> On 02/04/2021 15:40, Dmitry Osipenko wrote: >>> 01.04.2021 11:55, Nicolin Chen пишет: >>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote: >>>>> The previous commit fixes problem where display client was attaching too >>>>> early to IOMMU during kernel boot in a multi-platform kernel configuration >>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to >>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore, >>>>> revert it. >>>> >>>> Sorry for the late reply. I have been busy with downstream tasks. >>>> >>>> I will give them a try by the end of the week. Yet, probably it'd >>>> be better to include Guillaume also as he has the Nyan platform. >>>> >>> >>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which >>> I reproduced on Nexus 7. >>> >>> Guillaume, could you please give a test to these patches on Nyan Big? >>> There should be no EMEM errors in the kernel log with this patches. >>> >>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215 >> >> So sorry for the very late reply. I have tried the patches but >> hit some issues on linux-next, it's not reaching a login prompt >> with next-20210422. So I then tried with next-20210419 which >> does boot but shows the IOMMU error: >> >> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1 >> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping >> >> https://lava.collabora.co.uk/scheduler/job/3570052#L1120 >> >> The branch I'm using with the patches applied can be found here: >> >> >> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/ >> >> Hope this helps, let me know if you need anything else to be >> tested. > > > Hello Guillaume, > > The current linux-next doesn't boot on all ARM (AFAIK), the older > next-20210413 works. The above message should be unrelated to the boot > problem. It should be okay to ignore that message as it should be > harmless in yours case. > Although, the 20210419 should be good. Thierry, do you know what those SOR and Nouveau issues are about? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions
23.04.2021 19:32, Thierry Reding пишет: > Hi, > > this is an updated proposal to solve the problem of passing memory > regions that are actively being accessed during boot. The particular > use-case that I need this for is when the bootloader has set up the > display controller to scan out a boot splash screen. During boot the > DMA/IOMMU glue code will attach devices to an IOMMU domain and by > doing so enable IOMMU translations. Typically this will be before a > device driver has had a chance to either disable the display > controller or set up a new framebuffer and map it to the IOMMU. Hello Thierry, Is it always safe to enable SMMU ASID in a middle of a DMA request made by a memory client? The memory controller supports blocking DMA requests, which we are already using for the memory hot-resetting. A block could be needed before ASID is toggled. This needs to be clarified. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
23.04.2021 18:01, Guillaume Tucker пишет: > On 02/04/2021 15:40, Dmitry Osipenko wrote: >> 01.04.2021 11:55, Nicolin Chen пишет: >>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote: >>>> The previous commit fixes problem where display client was attaching too >>>> early to IOMMU during kernel boot in a multi-platform kernel configuration >>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to >>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore, >>>> revert it. >>> >>> Sorry for the late reply. I have been busy with downstream tasks. >>> >>> I will give them a try by the end of the week. Yet, probably it'd >>> be better to include Guillaume also as he has the Nyan platform. >>> >> >> Indeed, thanks. Although, I'm pretty sure that it's the same issue which >> I reproduced on Nexus 7. >> >> Guillaume, could you please give a test to these patches on Nyan Big? >> There should be no EMEM errors in the kernel log with this patches. >> >> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215 > > So sorry for the very late reply. I have tried the patches but > hit some issues on linux-next, it's not reaching a login prompt > with next-20210422. So I then tried with next-20210419 which > does boot but shows the IOMMU error: > > <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1 > <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping > > https://lava.collabora.co.uk/scheduler/job/3570052#L1120 > > The branch I'm using with the patches applied can be found here: > > > https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/ > > Hope this helps, let me know if you need anything else to be > tested. Hello Guillaume, The current linux-next doesn't boot on all ARM (AFAIK), the older next-20210413 works. The above message should be unrelated to the boot problem. It should be okay to ignore that message as it should be harmless in yours case. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
08.04.2021 17:07, Dmitry Osipenko пишет: >> Whatever happened to the idea of creating identity mappings based on the >> obscure tegra_fb_mem (or whatever it was called) command-line option? Is >> that command-line not universally passed to the kernel from bootloaders >> that initialize display? > This is still a good idea! The command-line isn't universally passed > just because it's up to a user to override the cmdline and then we get a > hang (a very slow boot in reality) on T30 since display client takes out > the whole memory bus with the constant SMMU faults. For example I don't > have that cmdline option in my current setups. > Thinking a bit more about the identity.. have you consulted with the memory h/w people about whether it's always safe to enable ASID in a middle of DMA request? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
08.04.2021 16:26, Thierry Reding пишет: > On Thu, Apr 08, 2021 at 02:42:42AM -0700, Nicolin Chen wrote: >> On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote: >>> All consumer-grade Android and Chromebook devices show a splash screen >>> on boot and then display is left enabled when kernel is booted. This >>> behaviour is unacceptable in a case of implicit IOMMU domains to which >>> devices are attached during kernel boot since devices, like display >>> controller, may perform DMA at that time. We can work around this problem >>> by deferring the enable of SMMU translation for a specific devices, >>> like a display controller, until the first IOMMU mapping is created, >>> which works good enough in practice because by that time h/w is already >>> stopped. >>> >>> Signed-off-by: Dmitry Osipenko >> >> For both patches: >> Acked-by: Nicolin Chen >> Tested-by: Nicolin Chen >> >> The WAR looks good to me. Perhaps Thierry would give some input. Nicolin, thank you very much for the help! >> Another topic: >> I think this may help work around the mc-errors, which we have >> been facing on Tegra210 also when we enable IOMMU_DOMAIN_DMA. >> (attached a test patch rebasing on these two) > > Ugh... that's exactly what I was afraid of. Now everybody is going to > think that we can just work around this issue with driver-specific SMMU > hacks... > >> However, GPU would also report errors using DMA domain: >> >> nouveau 5700.gpu: acr: firmware unavailable >> nouveau 5700.gpu: pmu: firmware unavailable >> nouveau 5700.gpu: gr: firmware unavailable >> tegra-mc 70019000.memory-controller: gpusrd: read @0xfffbe200: >> Security violation (TrustZone violation) >> nouveau 5700.gpu: DRM: failed to create kernel channel, -22 >> tegra-mc 70019000.memory-controller: gpusrd: read @0xfffad000: >> Security violation (TrustZone violation) >> nouveau 5700.gpu: fifo: SCHED_ERROR 20 [] >> nouveau 5700.gpu: fifo: SCHED_ERROR 20 [] >> >> Looking at the address, seems that GPU allocated memory in 32-bit >> physical address space behind SMMU, so a violation happened after >> turning on DMA domain I guess... > > The problem with GPU is... extra complicated. You're getting these > faults because you're enabling the IOMMU-backed DMA API, which then > causes the Nouveau driver allocate buffers using the DMA API instead of > explicitly allocating pages and then mapping them using the IOMMU API. > However, there are additional patches needed to teach Nouveau about how > to deal with SMMU and those haven't been merged yet. I've got prototypes > of this, but before the whole framebuffer carveout passing work makes > progress there's little sense in moving individual pieces forward. > > One more not to try and cut corners. We know what the right solution is, > even if it takes a lot of work. I'm willing to ack this patch, or some > version of it, but only as a way of working around things we have no > realistic chance of fixing properly anymore. I still think it would be > best if we could derive identity mappings from command-line arguments on > these platforms because I think most of them will actually set that, and > then the solution becomes at least uniform at the SMMU level. > > For Tegra210 I've already laid out a path to a solution that's going to > be generic and extend to Tegra186 and later as well. We still have issues in the DRM and other drivers that don't allow us to flip ON the IOMMU_DOMAIN_DMA support. My patch addresses the issue with the ARM_DMA_USE_IOMMU option, which allocates the unmanaged domain for DMA purposes on ARM32, causing the trouble in the multiplatform kernel configuration since it's not possible to opt-out from ARM_DMA_USE_IOMMU in this case. Perhaps this needs to be clarified in the commit message. https://elixir.bootlin.com/linux/v5.12-rc6/source/arch/arm/mm/dma-mapping.c#L2078 https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/iommu/iommu.c#L1929 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
08.04.2021 15:40, Thierry Reding пишет:
> On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote:
>> All consumer-grade Android and Chromebook devices show a splash screen
>> on boot and then display is left enabled when kernel is booted. This
>> behaviour is unacceptable in a case of implicit IOMMU domains to which
>> devices are attached during kernel boot since devices, like display
>> controller, may perform DMA at that time. We can work around this problem
>> by deferring the enable of SMMU translation for a specific devices,
>> like a display controller, until the first IOMMU mapping is created,
>> which works good enough in practice because by that time h/w is already
>> stopped.
>>
>> Signed-off-by: Dmitry Osipenko
>> ---
>> drivers/iommu/tegra-smmu.c | 71 ++
>> 1 file changed, 71 insertions(+)
>
> In general I do see why we would want to enable this. However, I think
> this is a bad idea because it's going to proliferate the bad practice of
> not describing things properly in device tree.
>
> Whatever happened to the idea of creating identity mappings based on the
> obscure tegra_fb_mem (or whatever it was called) command-line option? Is
> that command-line not universally passed to the kernel from bootloaders
> that initialize display?
This is still a good idea! The command-line isn't universally passed
just because it's up to a user to override the cmdline and then we get a
hang (a very slow boot in reality) on T30 since display client takes out
the whole memory bus with the constant SMMU faults. For example I don't
have that cmdline option in my current setups.
> That idealistic objection aside, this seems a bit over-engineered for
> the hack that it is. See below.
>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 602aab98c079..af1e4b5adb27 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -60,6 +60,8 @@ struct tegra_smmu_as {
>> dma_addr_t pd_dma;
>> unsigned id;
>> u32 attr;
>> +bool display_attached[2];
>> +bool attached_devices_need_sync;
>> };
>>
>> static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
>> @@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
>> unsigned long offset)
>> return readl(smmu->regs + offset);
>> }
>>
>> +/* all Tegra SoCs use the same group IDs for displays */
>> +#define SMMU_SWGROUP_DC 1
>> +#define SMMU_SWGROUP_DCB2
>> +
>> #define SMMU_CONFIG 0x010
>> #define SMMU_CONFIG_ENABLE (1 << 0)
>>
>> @@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
>> smmu_readl(smmu, SMMU_PTB_ASID);
>> }
>>
>> +static int smmu_swgroup_to_display_id(unsigned int swgroup)
>> +{
>> +switch (swgroup) {
>> +case SMMU_SWGROUP_DC:
>> +return 0;
>> +
>> +case SMMU_SWGROUP_DCB:
>> +return 1;
>> +
>> +default:
>> +return -1;
>> +}
>> +}
>> +
>
> Why do we need to have this two-level mapping? Do we even need to care
> about the specific swgroups IDs?
It's not clear to me what you're meaning here, the swgroup IDs are used
here for determining whether client belongs to a display controller.
> Can we not just simply check at attach
> time if the client that's being attached is a display client and then
> set atteched_devices_need_sync = true?
The reason I made atteched_devices_need_sync opt-out for display clients
instead of
opt-in is to make it clear and easy to override this option once we will
support the identity mappings.
- attached_devices_need_sync = true;
+ attached_devices_need_sync = no_identity_mapping_for_display;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
01.04.2021 11:55, Nicolin Chen пишет: > On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote: >> The previous commit fixes problem where display client was attaching too >> early to IOMMU during kernel boot in a multi-platform kernel configuration >> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to >> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore, >> revert it. > > Sorry for the late reply. I have been busy with downstream tasks. > > I will give them a try by the end of the week. Yet, probably it'd > be better to include Guillaume also as he has the Nyan platform. > Indeed, thanks. Although, I'm pretty sure that it's the same issue which I reproduced on Nexus 7. Guillaume, could you please give a test to these patches on Nyan Big? There should be no EMEM errors in the kernel log with this patches. https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients
All consumer-grade Android and Chromebook devices show a splash screen
on boot and then display is left enabled when kernel is booted. This
behaviour is unacceptable in a case of implicit IOMMU domains to which
devices are attached during kernel boot since devices, like display
controller, may perform DMA at that time. We can work around this problem
by deferring the enable of SMMU translation for a specific devices,
like a display controller, until the first IOMMU mapping is created,
which works good enough in practice because by that time h/w is already
stopped.
Signed-off-by: Dmitry Osipenko
---
drivers/iommu/tegra-smmu.c | 71 ++
1 file changed, 71 insertions(+)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 602aab98c079..af1e4b5adb27 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -60,6 +60,8 @@ struct tegra_smmu_as {
dma_addr_t pd_dma;
unsigned id;
u32 attr;
+ bool display_attached[2];
+ bool attached_devices_need_sync;
};
static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
@@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
unsigned long offset)
return readl(smmu->regs + offset);
}
+/* all Tegra SoCs use the same group IDs for displays */
+#define SMMU_SWGROUP_DC1
+#define SMMU_SWGROUP_DCB 2
+
#define SMMU_CONFIG 0x010
#define SMMU_CONFIG_ENABLE (1 << 0)
@@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
smmu_readl(smmu, SMMU_PTB_ASID);
}
+static int smmu_swgroup_to_display_id(unsigned int swgroup)
+{
+ switch (swgroup) {
+ case SMMU_SWGROUP_DC:
+ return 0;
+
+ case SMMU_SWGROUP_DCB:
+ return 1;
+
+ default:
+ return -1;
+ }
+}
+
static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
{
unsigned long id;
@@ -318,6 +338,9 @@ static struct iommu_domain
*tegra_smmu_domain_alloc(unsigned type)
as->domain.geometry.aperture_end = 0x;
as->domain.geometry.force_aperture = true;
+ /* work around implicit attachment of devices with active DMA */
+ as->attached_devices_need_sync = true;
+
return >domain;
}
@@ -410,6 +433,31 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu,
unsigned int swgroup,
}
}
+static void tegra_smmu_attach_deferred_devices(struct iommu_domain *domain)
+{
+ struct tegra_smmu_as *as = to_smmu_as(domain);
+
+ if (!as->attached_devices_need_sync)
+ return;
+
+ if (as->display_attached[0] || as->display_attached[1]) {
+ struct tegra_smmu *smmu = as->smmu;
+ unsigned int i;
+
+ for (i = 0; i < smmu->soc->num_clients; i++) {
+ const struct tegra_mc_client *client =
>soc->clients[i];
+ const int disp_id =
smmu_swgroup_to_display_id(client->swgroup);
+
+ if (disp_id < 0 || !as->display_attached[disp_id])
+ continue;
+
+ tegra_smmu_enable(smmu, client->swgroup, as->id);
+ }
+ }
+
+ as->attached_devices_need_sync = false;
+}
+
static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
{
@@ -495,10 +543,26 @@ static int tegra_smmu_attach_dev(struct iommu_domain
*domain,
return -ENOENT;
for (index = 0; index < fwspec->num_ids; index++) {
+ const unsigned int swgroup = fwspec->ids[index];
+ const int disp_id = smmu_swgroup_to_display_id(swgroup);
+
err = tegra_smmu_as_prepare(smmu, as);
if (err)
goto disable;
+ if (disp_id >= 0) {
+ as->display_attached[disp_id] = true;
+
+ /*
+* In most cases display is performing DMA before
+* driver is initialized by showing a splash screen
+* and in this case we should defer the h/w attachment
+* until the first mapping is created by display driver.
+*/
+ if (as->attached_devices_need_sync)
+ continue;
+ }
+
tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
@@ -527,6 +591,12 @@ static void tegra_smmu_detach_dev(struct iommu_domain
*domain, struct device *de
return;
for (index = 0; index < fwspec->num_ids; index++) {
+ const unsigned int swgroup = fwspec->ids[index];
+ const int disp_id = smmu_swgroup_to_display_id(swgroup)
[PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook
The previous commit fixes problem where display client was attaching too
early to IOMMU during kernel boot in a multi-platform kernel configuration
which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
revert it.
Signed-off-by: Dmitry Osipenko
---
drivers/iommu/tegra-smmu.c | 71 +-
1 file changed, 1 insertion(+), 70 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index af1e4b5adb27..572a4544ae88 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -869,69 +869,10 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
-
- mc = platform_get_drvdata(pdev);
- if (!mc)
- return NULL;
-
- return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
- struct of_phandle_args *args)
-{
- const struct iommu_ops *ops = smmu->iommu.ops;
- int err;
-
- err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
- if (err < 0) {
- dev_err(dev, "failed to initialize fwspec: %d\n", err);
- return err;
- }
-
- err = ops->of_xlate(dev, args);
- if (err < 0) {
- dev_err(dev, "failed to parse SW group ID: %d\n", err);
- iommu_fwspec_free(dev);
- return err;
- }
-
- return 0;
-}
-
static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
{
- struct device_node *np = dev->of_node;
- struct tegra_smmu *smmu = NULL;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err;
-
- while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- ) == 0) {
- smmu = tegra_smmu_find(args.np);
- if (smmu) {
- err = tegra_smmu_configure(smmu, dev, );
-
- if (err < 0) {
- of_node_put(args.np);
- return ERR_PTR(err);
- }
- }
-
- of_node_put(args.np);
- index++;
- }
+ struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
- smmu = dev_iommu_priv_get(dev);
if (!smmu)
return ERR_PTR(-ENODEV);
@@ -1158,16 +1099,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
if (!smmu)
return ERR_PTR(-ENOMEM);
- /*
-* This is a bit of a hack. Ideally we'd want to simply return this
-* value. However the IOMMU registration process will attempt to add
-* all devices to the IOMMU when bus_set_iommu() is called. In order
-* not to rely on global variables to track the IOMMU instance, we
-* set it here so that it can be looked up from the .probe_device()
-* callback via the IOMMU device's .drvdata field.
-*/
- mc->smmu = smmu;
-
size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
28.03.2021 18:25, Dmitry Osipenko пишет: > 03.03.2021 12:47, Dmitry Osipenko пишет: >> 03.03.2021 02:08, Nicolin Chen пишет: >>> On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote: >>>> 25.02.2021 09:27, Nicolin Chen пишет: >>>> ... >>>>>> The partially revert should be okay, but it's not clear to me what makes >>>>>> difference for T124 since I don't see that problem on T30, which also >>>>>> has active display at a boot time. >>>>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach >>>>> or from of_dma_configure_id/arch_setup_dma_ops? >>>>> >>>> I applied yours debug-patch, please see dmesg.txt attached to the email. >>>> Seems probe-defer of the tegra-dc driver prevents the implicit >>>> tegra_smmu_attach_dev, so it happens to work by accident. >>>> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1 >>>> [0.328641] [] (tegra_smmu_of_xlate) from [] >>>> (of_iommu_xlate+0x51/0x70) >>>> [0.328740] [] (of_iommu_xlate) from [] >>>> (of_iommu_configure+0x127/0x150) >>>> [0.328896] [] (of_iommu_configure) from [] >>>> (of_dma_configure_id+0x1fb/0x2ec) >>>> [0.329060] [] (of_dma_configure_id) from [] >>>> (really_probe+0x7b/0x2a0) >>>> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822 >>>> [0.332234] [] (tegra_smmu_probe_device) from [] >>>> (__iommu_probe_device+0x35/0x1c4) >>>> [0.332391] [] (__iommu_probe_device) from [] >>>> (iommu_probe_device+0x19/0xec) >>>> [0.332545] [] (iommu_probe_device) from [] >>>> (of_iommu_configure+0xfb/0x150) >>>> [0.332701] [] (of_iommu_configure) from [] >>>> (of_dma_configure_id+0x1fb/0x2ec) >>>> [0.332804] [] (of_dma_configure_id) from [] >>>> (really_probe+0x7b/0x2a0) >>>> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572 >>>> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862 >>>> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, >>>> 909: 1: drm >>>> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574 >>>> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858 >>>> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1 >>>> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, >>>> 1543: type 3 >>>> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, >>>> (null), drm >>>> [0.336187] -tegra_smmu_domain_alloc, 284: type 3 >>> [0.336968] [] (tegra_smmu_domain_alloc) from [] >>> (iommu_group_alloc_default_domain+0x4b/0xfa) >>>> [0.337127] [] (iommu_group_alloc_default_domain) from >>>> [] (iommu_probe_device+0x69/0xec) >>>> [0.337285] [] (iommu_probe_device) from [] >>>> (of_iommu_configure+0xfb/0x150) >>>> [0.337441] [] (of_iommu_configure) from [] >>>> (of_dma_configure_id+0x1fb/0x2ec) >>>> [0.337599] [] (of_dma_configure_id) from [] >>>> (really_probe+0x7b/0x2a0) >>>> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272 >>>> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517 >>> Hmm..not sure where this EPROBE_DEFER comes from. >> DC driver on Nexus 7 depends on LVDS bridge and display panel, which >> cause the probe defer. >> >>> But you are right, >>> as of_dma_configure_id() returns because of that so it didn't run to >>> arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain >>> and attaches DC to it on Tegra124. >>> >>> By the way, anyone can accept this change? It doesn't feel right to >>> leave a regression in the newer release... > > Guys, I have a good and bad news. > > The good news is that I figured out why I didn't see this problem on > Nexus 7 and the reason is that I had CONFIG_ARM_DMA_USE_IOMMU=n. > > The other good news is that I have a simple workaround which fixes the > implicit IOMMU problem by deferring the ASID enabling for display clients. > > The bad news is that CONFIG_ARM_DMA_USE_IOMMU=y breaks GPU (DRM, host1x) > drivers because they aren't properly prepared to this case and > CONFIG_ARM_DMA_USE_IOMMU is enabled in multi-platform kernel config. I > will try to fix up the drivers, but not sure how much time this may take. > Oh, actually the old workaround with arm_iommu_detach_device() still works, so we just need to bring it back. I'll prepare the patches. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
03.03.2021 12:47, Dmitry Osipenko пишет: > 03.03.2021 02:08, Nicolin Chen пишет: >> On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote: >>> 25.02.2021 09:27, Nicolin Chen пишет: >>> ... >>>>> The partially revert should be okay, but it's not clear to me what makes >>>>> difference for T124 since I don't see that problem on T30, which also >>>>> has active display at a boot time. >>>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach >>>> or from of_dma_configure_id/arch_setup_dma_ops? >>>> >>> I applied yours debug-patch, please see dmesg.txt attached to the email. >>> Seems probe-defer of the tegra-dc driver prevents the implicit >>> tegra_smmu_attach_dev, so it happens to work by accident. >>> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1 >>> [0.328641] [] (tegra_smmu_of_xlate) from [] >>> (of_iommu_xlate+0x51/0x70) >>> [0.328740] [] (of_iommu_xlate) from [] >>> (of_iommu_configure+0x127/0x150) >>> [0.328896] [] (of_iommu_configure) from [] >>> (of_dma_configure_id+0x1fb/0x2ec) >>> [0.329060] [] (of_dma_configure_id) from [] >>> (really_probe+0x7b/0x2a0) >>> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822 >>> [0.332234] [] (tegra_smmu_probe_device) from [] >>> (__iommu_probe_device+0x35/0x1c4) >>> [0.332391] [] (__iommu_probe_device) from [] >>> (iommu_probe_device+0x19/0xec) >>> [0.332545] [] (iommu_probe_device) from [] >>> (of_iommu_configure+0xfb/0x150) >>> [0.332701] [] (of_iommu_configure) from [] >>> (of_dma_configure_id+0x1fb/0x2ec) >>> [0.332804] [] (of_dma_configure_id) from [] >>> (really_probe+0x7b/0x2a0) >>> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572 >>> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862 >>> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, 909: >>> 1: drm >>> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574 >>> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858 >>> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1 >>> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, >>> 1543: type 3 >>> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, >>> (null), drm >>> [0.336187] -tegra_smmu_domain_alloc, 284: type 3 >> [0.336968] [] (tegra_smmu_domain_alloc) from [] >> (iommu_group_alloc_default_domain+0x4b/0xfa) >>> [0.337127] [] (iommu_group_alloc_default_domain) from >>> [] (iommu_probe_device+0x69/0xec) >>> [0.337285] [] (iommu_probe_device) from [] >>> (of_iommu_configure+0xfb/0x150) >>> [0.337441] [] (of_iommu_configure) from [] >>> (of_dma_configure_id+0x1fb/0x2ec) >>> [0.337599] [] (of_dma_configure_id) from [] >>> (really_probe+0x7b/0x2a0) >>> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272 >>> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517 >> Hmm..not sure where this EPROBE_DEFER comes from. > DC driver on Nexus 7 depends on LVDS bridge and display panel, which > cause the probe defer. > >> But you are right, >> as of_dma_configure_id() returns because of that so it didn't run to >> arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain >> and attaches DC to it on Tegra124. >> >> By the way, anyone can accept this change? It doesn't feel right to >> leave a regression in the newer release... Guys, I have a good and bad news. The good news is that I figured out why I didn't see this problem on Nexus 7 and the reason is that I had CONFIG_ARM_DMA_USE_IOMMU=n. The other good news is that I have a simple workaround which fixes the implicit IOMMU problem by deferring the ASID enabling for display clients. The bad news is that CONFIG_ARM_DMA_USE_IOMMU=y breaks GPU (DRM, host1x) drivers because they aren't properly prepared to this case and CONFIG_ARM_DMA_USE_IOMMU is enabled in multi-platform kernel config. I will try to fix up the drivers, but not sure how much time this may take. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults
26.03.2021 19:55, Dmitry Osipenko пишет: > 26.03.2021 19:35, Thierry Reding пишет: >> On Fri, Mar 26, 2021 at 06:29:28PM +0300, Dmitry Osipenko wrote: >>> 25.03.2021 16:03, Thierry Reding пишет: >>>> From: Thierry Reding >>>> >>>> Hi, >>>> >>>> this is a set of patches that is the result of earlier discussions >>>> regarding early identity mappings that are needed to avoid SMMU faults >>>> during early boot. >>>> >>>> The goal here is to avoid early identity mappings altogether and instead >>>> postpone the need for the identity mappings to when devices are attached >>>> to the SMMU. This works by making the SMMU driver coordinate with the >>>> memory controller driver on when to start enforcing SMMU translations. >>>> This makes Tegra behave in a more standard way and pushes the code to >>>> deal with the Tegra-specific programming into the NVIDIA SMMU >>>> implementation. >>> >>> It is an interesting idea which inspired me to try to apply a somewhat >>> similar thing to Tegra SMMU driver by holding the SMMU ASID enable-bit >>> until display driver allows to toggle it. This means that we will need an >>> extra small tegra-specific SMMU API function, but it should be okay. >>> >>> I typed a patch and seems it's working good, I'll prepare a proper patch if >>> you like it. >> >> That would actually be working around the problem that this patch was >> supposed to prepare for. The reason for this current patch series is to >> make sure SMMU translation isn't enabled until a device has actually >> been attached to the SMMU. Once it has been attached, the assumption is >> that any identity mappings will have been created. >> >> One Tegra SMMU that shouldn't be a problem because translations aren't >> enabled until device attach time. So in other words this patch set is to >> get Tegra186 and later to parity with earlier chips from this point of >> view. >> >> I think the problem that you're trying to work around is better solved >> by establishing these identity mappings. I do have patches to implement >> this for Tegra210 and earlier, though they may require additional work >> if you have bootloaders that don't use standard DT bindings for passing >> information about the framebuffer to the kernel. > > I'm not sure what else reasonable could be done without upgrading to a > very specific version of firmware, which definitely isn't a variant for > older devices which have a wild variety of bootloaders, customized > use-cases and etc. > > We could add a kludge that I'm suggesting as a universal fallback > solution, it should work well for all cases that I care about. > > So we could have the variant with identity mappings, and if mapping > isn't provided, then fall back to the kludge. > I tried a slightly different variant of the kludge by holding the ASID's enable till the first mapping is created for the display clients and IOMMU_DOMAIN_DMA now works properly (no EMEM errors on boot and etc) and without a need to change the DC driver. I also tried to remove the arm_iommu_detach_device() from the VDE driver and we now have 3 implicit domains in use (DRM, HX, VDE[wasted]) + 1 explicit (VDE) on T30, which works okay for today. So technically we could support the IOMMU_DOMAIN_DMA with a couple small changes right now or at least revert the hacks that were needed for Nyan. But in order to enable IOMMU_DOMAIN_DMA properly, we will need to do something about the DMA mappings first in the DRM driver and I also found that implicit IOMMU somehow doesn't work for host1x driver at all, so this needs to be fixed too. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults
26.03.2021 19:35, Thierry Reding пишет: > On Fri, Mar 26, 2021 at 06:29:28PM +0300, Dmitry Osipenko wrote: >> 25.03.2021 16:03, Thierry Reding пишет: >>> From: Thierry Reding >>> >>> Hi, >>> >>> this is a set of patches that is the result of earlier discussions >>> regarding early identity mappings that are needed to avoid SMMU faults >>> during early boot. >>> >>> The goal here is to avoid early identity mappings altogether and instead >>> postpone the need for the identity mappings to when devices are attached >>> to the SMMU. This works by making the SMMU driver coordinate with the >>> memory controller driver on when to start enforcing SMMU translations. >>> This makes Tegra behave in a more standard way and pushes the code to >>> deal with the Tegra-specific programming into the NVIDIA SMMU >>> implementation. >> >> It is an interesting idea which inspired me to try to apply a somewhat >> similar thing to Tegra SMMU driver by holding the SMMU ASID enable-bit until >> display driver allows to toggle it. This means that we will need an extra >> small tegra-specific SMMU API function, but it should be okay. >> >> I typed a patch and seems it's working good, I'll prepare a proper patch if >> you like it. > > That would actually be working around the problem that this patch was > supposed to prepare for. The reason for this current patch series is to > make sure SMMU translation isn't enabled until a device has actually > been attached to the SMMU. Once it has been attached, the assumption is > that any identity mappings will have been created. > > One Tegra SMMU that shouldn't be a problem because translations aren't > enabled until device attach time. So in other words this patch set is to > get Tegra186 and later to parity with earlier chips from this point of > view. > > I think the problem that you're trying to work around is better solved > by establishing these identity mappings. I do have patches to implement > this for Tegra210 and earlier, though they may require additional work > if you have bootloaders that don't use standard DT bindings for passing > information about the framebuffer to the kernel. I'm not sure what else reasonable could be done without upgrading to a very specific version of firmware, which definitely isn't a variant for older devices which have a wild variety of bootloaders, customized use-cases and etc. We could add a kludge that I'm suggesting as a universal fallback solution, it should work well for all cases that I care about. So we could have the variant with identity mappings, and if mapping isn't provided, then fall back to the kludge. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults
25.03.2021 16:03, Thierry Reding пишет:
> From: Thierry Reding
>
> Hi,
>
> this is a set of patches that is the result of earlier discussions
> regarding early identity mappings that are needed to avoid SMMU faults
> during early boot.
>
> The goal here is to avoid early identity mappings altogether and instead
> postpone the need for the identity mappings to when devices are attached
> to the SMMU. This works by making the SMMU driver coordinate with the
> memory controller driver on when to start enforcing SMMU translations.
> This makes Tegra behave in a more standard way and pushes the code to
> deal with the Tegra-specific programming into the NVIDIA SMMU
> implementation.
It is an interesting idea which inspired me to try to apply a somewhat similar
thing to Tegra SMMU driver by holding the SMMU ASID enable-bit until display
driver allows to toggle it. This means that we will need an extra small
tegra-specific SMMU API function, but it should be okay.
I typed a patch and seems it's working good, I'll prepare a proper patch if you
like it.
What do you think about this:
diff --git a/drivers/gpu/drm/grate/dc.c b/drivers/gpu/drm/grate/dc.c
index 45a41586f153..8874cfba40a1 100644
--- a/drivers/gpu/drm/grate/dc.c
+++ b/drivers/gpu/drm/grate/dc.c
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#include
@@ -2640,6 +2641,11 @@ static int tegra_dc_init(struct host1x_client *client)
return err;
}
+ if (dc->soc->sync_smmu) {
+ struct iommu_domain *domain = iommu_get_domain_for_dev(dc->dev);
+ tegra_smmu_sync_domain(domain, dc->dev);
+ }
+
if (dc->soc->wgrps)
primary = tegra_dc_add_shared_planes(drm, dc);
else
@@ -2824,6 +2830,7 @@ static const struct tegra_dc_soc_info tegra20_dc_soc_info
= {
.has_win_b_vfilter_mem_client = true,
.has_win_c_without_vert_filter = true,
.plane_tiled_memory_bandwidth_x2 = false,
+ .sync_smmu = false,
};
static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
@@ -2846,6 +2853,7 @@ static const struct tegra_dc_soc_info tegra30_dc_soc_info
= {
.has_win_b_vfilter_mem_client = true,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = true,
+ .sync_smmu = true,
};
static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
@@ -2868,6 +2876,7 @@ static const struct tegra_dc_soc_info
tegra114_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = true,
+ .sync_smmu = true,
};
static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
@@ -2890,6 +2899,7 @@ static const struct tegra_dc_soc_info
tegra124_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = false,
+ .sync_smmu = true,
};
static const struct tegra_dc_soc_info tegra210_dc_soc_info = {
@@ -2912,6 +2922,7 @@ static const struct tegra_dc_soc_info
tegra210_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = false,
+ .sync_smmu = true,
};
static const struct tegra_windowgroup_soc tegra186_dc_wgrps[] = {
@@ -2961,6 +2972,7 @@ static const struct tegra_dc_soc_info
tegra186_dc_soc_info = {
.wgrps = tegra186_dc_wgrps,
.num_wgrps = ARRAY_SIZE(tegra186_dc_wgrps),
.plane_tiled_memory_bandwidth_x2 = false,
+ .sync_smmu = false,
};
static const struct tegra_windowgroup_soc tegra194_dc_wgrps[] = {
@@ -3010,6 +3022,7 @@ static const struct tegra_dc_soc_info
tegra194_dc_soc_info = {
.wgrps = tegra194_dc_wgrps,
.num_wgrps = ARRAY_SIZE(tegra194_dc_wgrps),
.plane_tiled_memory_bandwidth_x2 = false,
+ .sync_smmu = false,
};
static const struct of_device_id tegra_dc_of_match[] = {
diff --git a/drivers/gpu/drm/grate/dc.h b/drivers/gpu/drm/grate/dc.h
index 316a56131cf1..e0057bf7be99 100644
--- a/drivers/gpu/drm/grate/dc.h
+++ b/drivers/gpu/drm/grate/dc.h
@@ -91,6 +91,7 @@ struct tegra_dc_soc_info {
bool has_win_b_vfilter_mem_client;
bool has_win_c_without_vert_filter;
bool plane_tiled_memory_bandwidth_x2;
+ bool sync_smmu;
};
struct tegra_dc {
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 602aab98c079..e750b1844a88 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -47,6 +47,9 @@ struct tegra_smmu {
struct dentry *debugfs;
struct iommu_device iommu; /* IOMMU Core code handle */
+
+ bool display_synced[2];
+ bool display_enabled[2];
};
struct tegra_smmu_as {
@@ -78,6 +81,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
unsigned long offset)
return readl(smmu->regs + offset);
}
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
25.03.2021 19:11, Dmitry Osipenko пишет:
> 25.03.2021 18:52, Thierry Reding пишет:
>> On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote:
>>> 25.03.2021 16:03, Thierry Reding пишет:
>>>> From: Thierry Reding
>>>>
>>>> From Tegra20 through Tegra210, either the GART or SMMU drivers need
>>>> access to the internals of the memory controller driver because they are
>>>> tightly coupled (in fact, the GART and SMMU are part of the memory
>>>> controller). On later chips, a separate hardware block implements the
>>>> SMMU functionality, so this is no longer needed. However, we still want
>>>> to reuse some of the existing infrastructure on later chips, so split
>>>> the memory controller internals into a separate header file to avoid
>>>> conflicts with the implementation on newer chips.
>>>>
>>>> Signed-off-by: Thierry Reding
>>>> ---
>>>> drivers/iommu/tegra-gart.c | 2 +-
>>>> drivers/iommu/tegra-smmu.c | 2 +-
>>>> drivers/memory/tegra/mc.h | 2 +-
>>>> drivers/memory/tegra/tegra186.c | 12 ---
>>>> include/soc/tegra/mc-internal.h | 62 +
>>>> include/soc/tegra/mc.h | 50 --
>>>> 6 files changed, 72 insertions(+), 58 deletions(-)
>>>> create mode 100644 include/soc/tegra/mc-internal.h
>>>
>>> What about to make T186 to re-use the existing tegra_mc struct? Seems
>>> there is nothing special in that struct which doesn't fit for the newer
>>> SoCs. Please notice that both SMMU and GART are already optional and all
>>> the SoC differences are specified within the tegra_mc_soc. It looks to
>>> me that this could be a much nicer and cleaner variant.
>>
>> The problem is that much of the interesting bits in tegra_mc_soc are
>> basically incompatible between the two. For instance the tegra_mc_client
>> and tegra186_mc_client structures, while they have the same purpose,
>> have completely different content. I didn't see a way to unify that
>> without overly complicating things by making half of the fields
>> basically optional on one or the other SoC generation.
>
> The additional fields aren't problem for T20, which doesn't need most of
> the fields. I'd try to go with the additional fields for now and see how
> it will look like, if it will be bothering too much, then we may
> consider to refactor the drivers more thoroughly (later on, in a
> separate series), with a better/nicer separation and taking into account
> a potential modularization support by the MC drivers.
>
> Using a union for the exclusive fields also could work, although always
> need to be extra careful with the unions.
>
>> Maybe one option would be to split tegra_mc into a tegra_mc_common and
>> then derive tegra_mc and tegra186_mc from that. That way we could share
>> the common bits while still letting the chip-specific differences be
>> handled separately.
>
> But isn't tegra_mc already a superset of tegra186_mc? I think the
> tegra186_mc_client is the main difference here.
>
Another thing we could do is to optimize the size of tegra_mc_client, but not
sure whether it's worthwhile to care about extra ~3KB of data.
This slims down tegra_mc_client by two times:
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index edea9b2b406e..1d652bfc6b44 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -317,11 +317,11 @@ static int tegra_mc_setup_latency_allowance(struct
tegra_mc *mc)
/* write latency allowance defaults */
for (i = 0; i < mc->soc->num_clients; i++) {
const struct tegra_mc_la *la = >soc->clients[i].la;
- u32 value;
+ u32 value, la_mask = la->mask, la_def = la->def;
value = mc_readl(mc, la->reg);
- value &= ~(la->mask << la->shift);
- value |= (la->def & la->mask) << la->shift;
+ value &= ~(la_mask << la->shift);
+ value |= (la_def & la_mask) << la->shift;
mc_writel(mc, value, la->reg);
}
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 46332fa82d10..ecf05484d656 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -1157,7 +1157,7 @@ static void tegra30_mc_tune_client_latency(struct
tegra_mc *mc,
u32 arb_tolerance_compensation_nsec, arb_tolerance_compensation_div;
const struct tegra_mc_la *la = >la;
unsigned int fifo
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
25.03.2021 18:52, Thierry Reding пишет: > On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: >> 25.03.2021 16:03, Thierry Reding пишет: >>> From: Thierry Reding >>> >>> From Tegra20 through Tegra210, either the GART or SMMU drivers need >>> access to the internals of the memory controller driver because they are >>> tightly coupled (in fact, the GART and SMMU are part of the memory >>> controller). On later chips, a separate hardware block implements the >>> SMMU functionality, so this is no longer needed. However, we still want >>> to reuse some of the existing infrastructure on later chips, so split >>> the memory controller internals into a separate header file to avoid >>> conflicts with the implementation on newer chips. >>> >>> Signed-off-by: Thierry Reding >>> --- >>> drivers/iommu/tegra-gart.c | 2 +- >>> drivers/iommu/tegra-smmu.c | 2 +- >>> drivers/memory/tegra/mc.h | 2 +- >>> drivers/memory/tegra/tegra186.c | 12 --- >>> include/soc/tegra/mc-internal.h | 62 + >>> include/soc/tegra/mc.h | 50 -- >>> 6 files changed, 72 insertions(+), 58 deletions(-) >>> create mode 100644 include/soc/tegra/mc-internal.h >> >> What about to make T186 to re-use the existing tegra_mc struct? Seems >> there is nothing special in that struct which doesn't fit for the newer >> SoCs. Please notice that both SMMU and GART are already optional and all >> the SoC differences are specified within the tegra_mc_soc. It looks to >> me that this could be a much nicer and cleaner variant. > > The problem is that much of the interesting bits in tegra_mc_soc are > basically incompatible between the two. For instance the tegra_mc_client > and tegra186_mc_client structures, while they have the same purpose, > have completely different content. I didn't see a way to unify that > without overly complicating things by making half of the fields > basically optional on one or the other SoC generation. The additional fields aren't problem for T20, which doesn't need most of the fields. I'd try to go with the additional fields for now and see how it will look like, if it will be bothering too much, then we may consider to refactor the drivers more thoroughly (later on, in a separate series), with a better/nicer separation and taking into account a potential modularization support by the MC drivers. Using a union for the exclusive fields also could work, although always need to be extra careful with the unions. > Maybe one option would be to split tegra_mc into a tegra_mc_common and > then derive tegra_mc and tegra186_mc from that. That way we could share > the common bits while still letting the chip-specific differences be > handled separately. But isn't tegra_mc already a superset of tegra186_mc? I think the tegra186_mc_client is the main difference here. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header
25.03.2021 16:03, Thierry Reding пишет: > From: Thierry Reding > > From Tegra20 through Tegra210, either the GART or SMMU drivers need > access to the internals of the memory controller driver because they are > tightly coupled (in fact, the GART and SMMU are part of the memory > controller). On later chips, a separate hardware block implements the > SMMU functionality, so this is no longer needed. However, we still want > to reuse some of the existing infrastructure on later chips, so split > the memory controller internals into a separate header file to avoid > conflicts with the implementation on newer chips. > > Signed-off-by: Thierry Reding > --- > drivers/iommu/tegra-gart.c | 2 +- > drivers/iommu/tegra-smmu.c | 2 +- > drivers/memory/tegra/mc.h | 2 +- > drivers/memory/tegra/tegra186.c | 12 --- > include/soc/tegra/mc-internal.h | 62 + > include/soc/tegra/mc.h | 50 -- > 6 files changed, 72 insertions(+), 58 deletions(-) > create mode 100644 include/soc/tegra/mc-internal.h What about to make T186 to re-use the existing tegra_mc struct? Seems there is nothing special in that struct which doesn't fit for the newer SoCs. Please notice that both SMMU and GART are already optional and all the SoC differences are specified within the tegra_mc_soc. It looks to me that this could be a much nicer and cleaner variant. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
16.03.2021 14:16, Thierry Reding пишет: >> +seq_puts(s, "}\n"); >> +seq_printf(s, "Total PDE count: %u\n", pde_count); >> +seq_printf(s, "Total PTE count: %llu\n", pte_count); > Some of the above looks like it wouldn't be very easily consumed by > scripts. Is that something we want to do? Or is this targetted primarily > at human consumption? Output should be parsable using a simple regex. Could you please clarify what exactly isn't easy? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.03.2021 23:36, Nicolin Chen пишет:
> +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> +{
> + struct tegra_smmu_group_debug *group_debug = s->private;
> + const struct tegra_smmu_swgroup *group;
> + struct tegra_smmu_as *as;
> + struct tegra_smmu *smmu;
> + unsigned int pd_index;
> + unsigned int pt_index;
> + unsigned long flags;
> + u64 pte_count = 0;
> + u32 pde_count = 0;
> + u32 val, ptb_reg;
> + u32 *pd;
> +
> + if (!group_debug || !group_debug->priv || !group_debug->group)
> + return 0;
I'm also now curious how difficult would be to read out the actual h/w
state, i.e. check whether ASID is enabled and then dynamically map the
pointed pages instead of using pages allocated by driver. This will show
us the real h/w state. For example this may show mappings left after
bootloader or after reboot/kexec, which could be handy to see.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.03.2021 23:36, Nicolin Chen пишет:
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
Looking at this again, I'm now wondering whether will be better to
replace dma_addr_t with u32 everywhere since SMMU only supports 32bits
for IOVA.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.03.2021 06:35, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
>
> Ataching an example:
Attaching
>
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE | ATTR | PHYS | IOVA
> | SIZE
> [#1023, #1023] | 0x5 | 0x000111a8d000 |
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
>
> Signed-off-by: Nicolin Chen
> ---
Good to me, thanks.
Tested-by: Dmitry Osipenko
Reviewed-by: Dmitry Osipenko
> + for (pd_index = 0; pd_index < SMMU_NUM_PDE; pd_index++) {
> + struct page *pt_page;
> + u32 *addr;
> + unsigned int i;
Unimportant nit: I'd keep lines arranged by length for consistency with
the rest of the code.
...
> + group_debug = devm_kcalloc(dev, soc->num_swgroups,
> sizeof(*group_debug), GFP_KERNEL);
Another nit: this is a long line, I'd split it into two lines to keep
coding style consistent and to improve readability for those who have a
side-by-side code viewing setup.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu/tegra-smmu: Add pagetable mappings to debugfs
14.03.2021 11:06, Nicolin Chen пишет:
> + for (pd_index = 0; pd_index < SMMU_NUM_PDE; pd_index++) {
> + struct page *pt_page;
> + u32 *addr;
> + int i;
unsigned int
and then printf specifiers also should be %u
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu/tegra-smmu: Add pagetable mappings to debugfs
14.03.2021 11:06, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
>
> Ataching an example:
>
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE | ATTR | PHYS | IOVA
> | SIZE
> [#1023, #1023] | 0x5 | 0x000111a8d000 |
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
>
> Signed-off-by: Nicolin Chen
> ---
> Changelog
> v3:
> * Fixed PHYS and IOVA print formats
> * Changed variables to unsigned int type
> * Changed the table outputs to be compact
> v2: https://lkml.org/lkml/2021/3/9/1382
> * Expanded mutex range to the entire function
> * Added as->lock to protect pagetable walkthrough
> * Replaced devm_kzalloc with devm_kcalloc for group_debug
> * Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
> * Dropped as->count check; added WARN_ON when as->count mismatches
> pd[pd_index]
> v1: https://lkml.org/lkml/2020/9/26/70
>
> drivers/iommu/tegra-smmu.c | 175 +++--
> 1 file changed, 170 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 97eb62f667d2..269737d51ad4 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -19,6 +19,11 @@
> #include
> #include
>
> +struct tegra_smmu_group_debug {
> + const struct tegra_smmu_swgroup *group;
> + void *priv;
> +};
> +
> struct tegra_smmu_group {
> struct list_head list;
> struct tegra_smmu *smmu;
> @@ -47,6 +52,8 @@ struct tegra_smmu {
> struct dentry *debugfs;
>
> struct iommu_device iommu; /* IOMMU Core code handle */
> +
> + struct tegra_smmu_group_debug *group_debug;
> };
>
> struct tegra_smmu_as {
> @@ -152,6 +159,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
> unsigned long offset)
>
> #define SMMU_PDE_ATTR(SMMU_PDE_READABLE | SMMU_PDE_WRITABLE
> | \
>SMMU_PDE_NONSECURE)
> +#define SMMU_PTE_ATTR(SMMU_PTE_READABLE | SMMU_PTE_WRITABLE
> | \
> + SMMU_PTE_NONSECURE)
> +#define SMMU_PTE_ATTR_SHIFT (29)
>
> static unsigned int iova_pd_index(unsigned long iova)
> {
> @@ -163,6 +173,12 @@ static unsigned int iova_pt_index(unsigned long iova)
> return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
> }
>
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
> +
> static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
> {
> addr >>= 12;
> @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
> *domain)
> }
>
> static const struct tegra_smmu_swgroup *
> -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
> *index)
> {
> const struct tegra_smmu_swgroup *group = NULL;
> unsigned int i;
> @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
> int swgroup)
> for (i = 0; i < smmu->soc->num_swgroups; i++) {
> if (smmu->soc->swgroups[i].swgroup == swgroup) {
> group = >soc->swgroups[i];
> + if (index)
> + *index = i;
> break;
> }
> }
> @@ -350,19 +368,22 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu,
> unsigned int swgroup)
> }
>
> static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
> - unsigned int asid)
> + struct tegra_smmu_as *as)
> {
> const struct tegra_smmu_swgroup *group;
> + unsigned int asid = as->id;
> unsigned int i;
> u32 value;
>
> - group = tegra_smmu_find_swgroup(smmu, swgroup);
> + group = tegra_smmu_find_swgroup(smmu, swgroup, );
> if (group) {
> value = smmu_readl(smmu, group->reg);
> value &= ~SMMU_ASID_MASK;
> value |= SMMU_ASID_VALUE(asid);
> value |= SMMU_ASID_ENABLE;
> smmu_writel(smmu, value, group->reg);
> + if (smmu->group_debug)
> + smmu->group_debug[i].priv = as;
> } else {
> pr_warn("%s group from swgroup %u not found\n", __func__,
> swgroup);
> @@ -389,13 +410,15 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu,
> unsigned int swgroup,
> unsigned int i;
> u32 value;
>
> - group = tegra_smmu_find_swgroup(smmu, swgroup);
> + group =
[PATCH v1] iommu/tegra-smmu: Make tegra_smmu_probe_device() to handle all IOMMU phandles
The tegra_smmu_probe_device() handles only the first IOMMU device-tree
phandle, skipping the rest. Devices like 3D module on Tegra30 have
multiple IOMMU phandles, one for each h/w block, and thus, only one
IOMMU phandle is added to fwspec for the 3D module, breaking GPU.
Previously this problem was masked by tegra_smmu_attach_dev() which
didn't use the fwspec, but parsed the DT by itself. The previous commit
to tegra-smmu driver partially reverted changes that caused problems for
T124 and now we have tegra_smmu_attach_dev() that uses the fwspec and
the old-buggy variant of tegra_smmu_probe_device() which skips secondary
IOMMUs.
Make tegra_smmu_probe_device() not to skip the secondary IOMMUs. This
fixes a partially attached IOMMU of the 3D module on Tegra30 and now GPU
works properly once again.
Fixes: 765a9d1d02b2 ("iommu/tegra-smmu: Fix mc errors on tegra124-nyan")
Signed-off-by: Dmitry Osipenko
---
drivers/iommu/tegra-smmu.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 97eb62f667d2..602aab98c079 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -849,12 +849,11 @@ static struct iommu_device
*tegra_smmu_probe_device(struct device *dev)
smmu = tegra_smmu_find(args.np);
if (smmu) {
err = tegra_smmu_configure(smmu, dev, );
- of_node_put(args.np);
- if (err < 0)
+ if (err < 0) {
+ of_node_put(args.np);
return ERR_PTR(err);
-
- break;
+ }
}
of_node_put(args.np);
--
2.29.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/tegra-smmu: Add pagetable mappings to debugfs
10.03.2021 06:36, Nicolin Chen пишет:
...
> +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> +{
> + struct tegra_smmu_group_debug *group_debug = s->private;
> + const struct tegra_smmu_swgroup *group;
> + struct tegra_smmu_as *as;
> + struct tegra_smmu *smmu;
> + int pd_index, pt_index;
> +DEFINE_SHOW_ATTRIBUTE(tegra_smmu_mappings);
> +
> static void tegra_smmu_debugfs_init(struct tegra_smmu *smmu)
> {
> + const struct tegra_smmu_soc *soc = smmu->soc;
> + struct tegra_smmu_group_debug *group_debug;
> + struct device *dev = smmu->dev;
> + struct dentry *d;
> + int i;
You should use unsigned types for everything that isn't signed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
11.03.2021 01:17, Nicolin Chen пишет: > On Wed, Mar 10, 2021 at 11:22:57PM +0300, Dmitry Osipenko wrote: >> 10.03.2021 22:13, Dmitry Osipenko пишет: >>> I found that this patch introduced a serious regression on Tegra30 using >>> today's linux-next. Tegra30 has two 3d h/w blocks connected in SLI and >>> only one of the blocks is now attached to IOMMU domain, meaning that GPU >>> is unusable now. All 3d, 2d and display devices share the same "DRM" >>> group on Tegra30. >>> >>> Nicolin, please let me know if have any suggestions. I may take a closer >>> look a day later, for now I'll just revert this patch locally. Thanks in >>> advance. >>> >> >> Actually, this was easy to fix: >> >> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >> index 97eb62f667d2..639d5ceab60b 100644 >> --- a/drivers/iommu/tegra-smmu.c >> +++ b/drivers/iommu/tegra-smmu.c >> @@ -853,8 +853,6 @@ static struct iommu_device >> *tegra_smmu_probe_device(struct device *dev) >> >> if (err < 0) >> return ERR_PTR(err); >> - >> -break; > > Hmm..I don't understand why this "break" causes problems on Tegra30. > The older versions that used _find()+configure() had it also, e.g.: > https://elixir.bootlin.com/linux/v5.9.16/source/drivers/iommu/tegra-smmu.c#L760 > > Dmitry, do you have any idea? > The older variant of tegra_smmu_attach_dev() didn't use fwspec [1], that makes the difference. In other words, the older variant of tegra_smmu_probe_device() was already buggy, but the bug was masked by the tegra_smmu_attach_dev() that didn't use the fwspec. [1] https://elixir.bootlin.com/linux/v5.10.22/source/drivers/iommu/tegra-smmu.c#L476 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/tegra-smmu: Add pagetable mappings to debugfs
10.03.2021 06:36, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
>
> Ataching an example:
>
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf0080013 (1)
> {
> PTE RANGE PHYS IOVASIZEATTR
> #1023 - #1023 0x122e5e0000xf000 0x1000 0x5
> }
> }
> Total PDE count: 1
> Total PTE count: 1
Addresses are 32bit on ARM32, please fix the formatting. The phys_addr_t
and dma_addr_t have special printk specifiers [1].
[1]
https://www.kernel.org/doc/html/latest/core-api/printk-formats.html?highlight=printk#physical-address-types-phys-addr-t
as->pd_dma: 0xc026e81b026c
{
[0] 0xf0082848 (1024)
{
PTE RANGE PHYS IOVASIZE
ATTR
#0- #15 0xc0f9fc40bfde 0x0 0x1
0x7
#16 - #47 0xc0f9fc40bfdc 0x1 0x2
0x7
#48 - #1110xc0f9fc40bfd8 0x3 0x4
0x7
#112 - #2390xc0f9fc40bfd0 0x7 0x8
0x7
#240 - #4950xc0f9fc40bfc0 0xf 0x10
0x7
#496 - #9990xc0f9fc40bf40 0x1f0x1f8000
0x7
#1000 - #1000 0xc0f9fc40acc4 0x3e80000x1000
0x7
#1001 - #1001 0xc0f9fc40bbb95000 0x3e90000x1000
0x7
#1002 - #1002 0xc0f9fc40bbb97000 0x3ea0000x1000
0x7
#1003 - #1003 0xc0f9fc40bbb91000 0x3eb0000x1000
0x7
#1004 - #1005 0xc0f9fc40bb455000 0x3ec0000x2000
0x7
#1006 - #1006 0xc0f9fc40bee9c000 0x3ee0000x1000
0x7
#1007 - #1007 0xc0f9fc40bed2 0x3ef0000x1000
0x7
#1008 - #1008 0xc0f9fc40bbde 0x3f0x1000
0x7
#1009 - #1009 0xc0f9fc40be465000 0x3f10000x1000
0x7
#1010 - #1010 0xc0f9fc40bb505000 0x3f20000x1000
0x7
#1011 - #1011 0xc0f9fc40bdcb9000 0x3f30000x1000
0x7
#1012 - #1012 0xc0f9fc40bcdf9000 0x3f40000x1000
0x7
#1013 - #1013 0xc0f9fc40bb6ce000 0x3f50000x1000
0x7
#1014 - #1014 0xc0f9fc40bcd74000 0x3f60000x1000
0x7
#1015 - #1015 0xc0f9fc40bd0d 0x3f70000x1000
0x7
#1016 - #1016 0xc0f9fc40bb6c5000 0x3f80000x1000
0x7
#1017 - #1017 0xc0f9fc40bd121000 0x3f90000x1000
0x7
#1018 - #1018 0xc0f9fc40bb6df000 0x3fa0000x1000
0x7
#1019 - #1020 0xc0f9fc40bb6e9000 0x3fb0000x2000
0x7
#1021 - #1021 0xc0f9fc40bb713000 0x3fd0000x1000
0x7
#1022 - #1022 0xc0f9fc40bb738000 0x3fe0000x1000
0x7
#1023 - #1023 0xc0f9fc40be444000 0x3ff0000x1000
0x7
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
10.03.2021 22:13, Dmitry Osipenko пишет:
> 19.02.2021 01:07, Nicolin Chen пишет:
>> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
>> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
>> tegra_smmu_configure() that are typically done in the IOMMU core also.
>>
>> This approach works for both existing devices that have DT nodes and other
>> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
>> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>>
>> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>> EMEM address decode error (SMMU translation error [--S])
>> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>> Page fault (SMMU translation error [--S])
>>
>> After debugging, I found that the mentioned commit changed some function
>> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
>> client before display driver gets initialized. I couldn't reproduce exact
>> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>>
>> Actually this Page Fault is a known issue, as on most of Tegra platforms,
>> display gets enabled by the bootloader for the splash screen feature, so
>> it keeps filling the framebuffer memory. A proper fix to this issue is to
>> 1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
>> the same address as the physical address in its page table. Yet, Thierry
>> has been working on the solution above for a year, and it hasn't merged.
>>
>> Therefore, let's partially revert the mentioned commit to fix the errors.
>>
>> The reason why we do a partial revert here is that we can still set priv
>> in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
>> DT, like display, will go through tegra_smmu_configure() at the stage of
>> bus_set_iommu() when SMMU gets probed(), as what it did before we merged
>> the mentioned commit.
>>
>> Once we have the linear map solution for framebuffer memory, this change
>> can be cleaned away.
>>
>> [Big thank to Guillaume who reported and helped debugging/verification]
>>
>> Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>> Reported-by: Guillaume Tucker
>> Signed-off-by: Nicolin Chen
>> ---
>>
>> Guillaume, would you please give a "Tested-by" to this change? Thanks!
>>
>> drivers/iommu/tegra-smmu.c | 72 +-
>> 1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 4a3f095a1c26..97eb62f667d2 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -798,10 +798,70 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
>> iommu_domain *domain,
>> return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
>> }
>>
>> +static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
>> +{
>> +struct platform_device *pdev;
>> +struct tegra_mc *mc;
>> +
>> +pdev = of_find_device_by_node(np);
>> +if (!pdev)
>> +return NULL;
>> +
>> +mc = platform_get_drvdata(pdev);
>> +if (!mc)
>> +return NULL;
>> +
>> +return mc->smmu;
>> +}
>> +
>> +static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
>> +struct of_phandle_args *args)
>> +{
>> +const struct iommu_ops *ops = smmu->iommu.ops;
>> +int err;
>> +
>> +err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
>> +if (err < 0) {
>> +dev_err(dev, "failed to initialize fwspec: %d\n", err);
>> +return err;
>> +}
>> +
>> +err = ops->of_xlate(dev, args);
>> +if (err < 0) {
>> +dev_err(dev, "failed to parse SW group ID: %d\n", err);
>> +iommu_fwspec_free(dev);
>> +return err;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>> {
>> -struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>> +struct device_node *np = dev->of_node;
>> +struct tegra_smmu *smmu = NULL;
>> +struct of_phandle_args args;
>> +
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
19.02.2021 01:07, Nicolin Chen пишет:
> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> tegra_smmu_configure() that are typically done in the IOMMU core also.
>
> This approach works for both existing devices that have DT nodes and other
> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>EMEM address decode error (SMMU translation error [--S])
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>Page fault (SMMU translation error [--S])
>
> After debugging, I found that the mentioned commit changed some function
> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> client before display driver gets initialized. I couldn't reproduce exact
> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>
> Actually this Page Fault is a known issue, as on most of Tegra platforms,
> display gets enabled by the bootloader for the splash screen feature, so
> it keeps filling the framebuffer memory. A proper fix to this issue is to
> 1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
> the same address as the physical address in its page table. Yet, Thierry
> has been working on the solution above for a year, and it hasn't merged.
>
> Therefore, let's partially revert the mentioned commit to fix the errors.
>
> The reason why we do a partial revert here is that we can still set priv
> in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
> DT, like display, will go through tegra_smmu_configure() at the stage of
> bus_set_iommu() when SMMU gets probed(), as what it did before we merged
> the mentioned commit.
>
> Once we have the linear map solution for framebuffer memory, this change
> can be cleaned away.
>
> [Big thank to Guillaume who reported and helped debugging/verification]
>
> Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> Reported-by: Guillaume Tucker
> Signed-off-by: Nicolin Chen
> ---
>
> Guillaume, would you please give a "Tested-by" to this change? Thanks!
>
> drivers/iommu/tegra-smmu.c | 72 +-
> 1 file changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 4a3f095a1c26..97eb62f667d2 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -798,10 +798,70 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
> iommu_domain *domain,
> return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
> }
>
> +static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct tegra_mc *mc;
> +
> + pdev = of_find_device_by_node(np);
> + if (!pdev)
> + return NULL;
> +
> + mc = platform_get_drvdata(pdev);
> + if (!mc)
> + return NULL;
> +
> + return mc->smmu;
> +}
> +
> +static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
> + struct of_phandle_args *args)
> +{
> + const struct iommu_ops *ops = smmu->iommu.ops;
> + int err;
> +
> + err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
> + if (err < 0) {
> + dev_err(dev, "failed to initialize fwspec: %d\n", err);
> + return err;
> + }
> +
> + err = ops->of_xlate(dev, args);
> + if (err < 0) {
> + dev_err(dev, "failed to parse SW group ID: %d\n", err);
> + iommu_fwspec_free(dev);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> {
> - struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> + struct device_node *np = dev->of_node;
> + struct tegra_smmu *smmu = NULL;
> + struct of_phandle_args args;
> + unsigned int index = 0;
> + int err;
> +
> + while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> + ) == 0) {
> + smmu = tegra_smmu_find(args.np);
> + if (smmu) {
> + err = tegra_smmu_configure(smmu, dev, );
> + of_node_put(args.np);
>
> + if (err < 0)
> + return ERR_PTR(err);
> +
> + break;
> + }
> +
> + of_node_put(args.np);
> + index++;
> + }
> +
> + smmu = dev_iommu_priv_get(dev);
> if (!smmu)
> return ERR_PTR(-ENODEV);
>
> @@ -1028,6 +1088,16 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
> if (!smmu)
>
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
03.03.2021 02:08, Nicolin Chen пишет: > On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote: >> 25.02.2021 09:27, Nicolin Chen пишет: >> ... >>>> The partially revert should be okay, but it's not clear to me what makes >>>> difference for T124 since I don't see that problem on T30, which also >>>> has active display at a boot time. >>> >>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach >>> or from of_dma_configure_id/arch_setup_dma_ops? >>> >> >> I applied yours debug-patch, please see dmesg.txt attached to the email. >> Seems probe-defer of the tegra-dc driver prevents the implicit >> tegra_smmu_attach_dev, so it happens to work by accident. > >> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1 >> [0.328641] [] (tegra_smmu_of_xlate) from [] >> (of_iommu_xlate+0x51/0x70) >> [0.328740] [] (of_iommu_xlate) from [] >> (of_iommu_configure+0x127/0x150) >> [0.328896] [] (of_iommu_configure) from [] >> (of_dma_configure_id+0x1fb/0x2ec) >> [0.329060] [] (of_dma_configure_id) from [] >> (really_probe+0x7b/0x2a0) >> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822 >> [0.332234] [] (tegra_smmu_probe_device) from [] >> (__iommu_probe_device+0x35/0x1c4) >> [0.332391] [] (__iommu_probe_device) from [] >> (iommu_probe_device+0x19/0xec) >> [0.332545] [] (iommu_probe_device) from [] >> (of_iommu_configure+0xfb/0x150) >> [0.332701] [] (of_iommu_configure) from [] >> (of_dma_configure_id+0x1fb/0x2ec) >> [0.332804] [] (of_dma_configure_id) from [] >> (really_probe+0x7b/0x2a0) >> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572 >> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862 >> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, 909: >> 1: drm >> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574 >> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858 >> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1 >> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, >> 1543: type 3 >> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, >> (null), drm >> [0.336187] -tegra_smmu_domain_alloc, 284: type 3 > [0.336968] [] (tegra_smmu_domain_alloc) from [] > (iommu_group_alloc_default_domain+0x4b/0xfa) >> [0.337127] [] (iommu_group_alloc_default_domain) from >> [] (iommu_probe_device+0x69/0xec) >> [0.337285] [] (iommu_probe_device) from [] >> (of_iommu_configure+0xfb/0x150) >> [0.337441] [] (of_iommu_configure) from [] >> (of_dma_configure_id+0x1fb/0x2ec) >> [0.337599] [] (of_dma_configure_id) from [] >> (really_probe+0x7b/0x2a0) >> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272 >> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517 > > Hmm..not sure where this EPROBE_DEFER comes from. DC driver on Nexus 7 depends on LVDS bridge and display panel, which cause the probe defer. > But you are right, > as of_dma_configure_id() returns because of that so it didn't run to > arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain > and attaches DC to it on Tegra124. > > By the way, anyone can accept this change? It doesn't feel right to > leave a regression in the newer release... > I think Thierry should give ack. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
25.02.2021 09:27, Nicolin Chen пишет: ... >> The partially revert should be okay, but it's not clear to me what makes >> difference for T124 since I don't see that problem on T30, which also >> has active display at a boot time. > > Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach > or from of_dma_configure_id/arch_setup_dma_ops? > I applied yours debug-patch, please see dmesg.txt attached to the email. Seems probe-defer of the tegra-dc driver prevents the implicit tegra_smmu_attach_dev, so it happens to work by accident. [0.00] Booting Linux on physical CPU 0x0 [0.00] Linux version 5.11.0-next-20210226-3-g89069e0f4a28 (dima@dimapc) (armv7a-hardfloat-linux-gnueabi-gcc (Gentoo 9.3.0-r2 p4) 9.3.0, GNU ld (Gentoo 2.26.1 p1.0) 2.26.1) #7012 SMP PREEMPT Sat Feb 27 11:49:27 MSK 2021 [0.00] CPU: ARMv7 Processor [412fc099] revision 9 (ARMv7), cr=50c5387d [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [0.00] OF: fdt: Machine model: ASUS Google Nexus 7 (Project Bach / ME370TG) E1565 [0.00] Memory policy: Data cache writealloc [0.00] Reserved memory: created CMA memory pool at 0xa000, size 256 MiB [0.00] OF: reserved mem: initialized node linux,cma@8000, compatible id shared-dma-pool [0.00] Zone ranges: [0.00] Normal [mem 0x-0x9f7f] [0.00] HighMem [mem 0x9f80-0xbfdf] [0.00] Movable zone start for each node [0.00] Early memory node ranges [0.00] node 0: [mem 0x8000-0xbfdf] [0.00] Initmem setup node 0 [mem 0x8000-0xbfdf] [0.00] On node 0 totalpages: 261632 [0.00] Normal zone: 1134 pages used for memmap [0.00] Normal zone: 0 pages reserved [0.00] Normal zone: 129024 pages, LIFO batch:31 [0.00] HighMem zone: 132608 pages, LIFO batch:31 [0.00] percpu: Embedded 21 pages/cpu s53324 r8192 d24500 u86016 [0.00] pcpu-alloc: s53324 r8192 d24500 u86016 alloc=21*4096 [0.00] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 260498 [0.00] Kernel command line: tegraid=30.1.3.0.0 mem=1022M@2048M android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 video=tegrafb no_console_suspend=1 console=none debug_uartport=hsport usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 board_info=f41:a00:1:44:2 root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt gpt_sector=61079551 androidboot.bootloader=4.23 androidboot.baseband=1231_0.18.0_0409 init=/usr/lib/systemd/systemd root=/dev/nfs nfsroot=192.168.3.100:/nfs/root,v3,tcp rw ip=192.168.3.101::192.168.3.100:255.255.255.0::usb0 no_console_suspend gpt gpt_sector=61079551 console=tty1 no_console_suspend [0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, linear) [0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, linear) [0.00] mem auto-init: stack:off, heap alloc:off, heap free:off [0.00] Memory: 754680K/1046528K available (10240K kernel code, 1579K rwdata, 5404K rodata, 1024K init, 418K bss, 29704K reserved, 262144K cma-reserved, 268224K highmem) [0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 [0.00] ftrace: allocating 46557 entries in 91 pages [0.00] [ cut here ] [0.00] WARNING: CPU: 0 PID: 0 at arch/arm/kernel/insn.c:15 __arm_gen_branch+0x7f/0x84 [0.00] Modules linked in: [0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.11.0-next-20210226-3-g89069e0f4a28 #7012 [0.00] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [0.00] [] (unwind_backtrace) from [] (show_stack+0x11/0x14) [0.00] [] (show_stack) from [] (dump_stack+0x8d/0x9a) [0.00] [] (dump_stack) from [] (__warn+0xb7/0xc8) [0.00] [] (__warn) from [] (warn_slowpath_fmt+0x45/0x78) [0.00] [] (warn_slowpath_fmt) from [] (__arm_gen_branch+0x7f/0x84) [0.00] [] (__arm_gen_branch) from [] (ftrace_make_nop+0xf/0x20) [0.00] [] (ftrace_make_nop) from [] (ftrace_process_locs+0x211/0x320) [0.00] [] (ftrace_process_locs) from [] (ftrace_init+0x6f/0xbe) [0.00] [] (ftrace_init) from [] (start_kernel+0x42d/0x662) [0.00] [] (start_kernel) from [<>] (0x0) [0.00] random: get_random_bytes called from print_oops_end_marker+0x25/0x64 with crng_init=0 [0.00] ---[ end trace ]--- [0.00] [ ftrace bug ] [0.00] ftrace failed to modify [0.00] [] crash_notes_memory_init+0x4/0x34 [0.00] actual: 23:f0:9c:ec [0.00] Initializing ftrace call sites [0.00] ftrace record flags: 0
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
23.02.2021 05:13, Nicolin Chen пишет:
> Hi Dmitry,
>
> On Sat, Feb 20, 2021 at 08:16:22AM +0300, Dmitry Osipenko wrote:
>> 19.02.2021 01:07, Nicolin Chen пишет:
>>> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>>> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
>>> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
>>> tegra_smmu_configure() that are typically done in the IOMMU core also.
>>>
>>> This approach works for both existing devices that have DT nodes and other
>>> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
>>> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>>>
>>> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>> EMEM address decode error (SMMU translation error [--S])
>>> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>> Page fault (SMMU translation error [--S])
>>>
>>> After debugging, I found that the mentioned commit changed some function
>>> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
>>> client before display driver gets initialized. I couldn't reproduce exact
>>> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>>
>> Hello Nicolin,
>>
>> Could you please explain in a more details what exactly makes the
>> difference for the callback sequence?
>
> Here is a log with 5.11.0-rc6:
> https://lava.collabora.co.uk/scheduler/job/3187849
> [dump_stack was added in some tegra-smmu functions]
>
> And here is a corresponding log with reverting the original commit:
> https://lava.collabora.co.uk/scheduler/job/3187851
>
> Here is a log with 5.11.0-rc7-next-20210210:
> https://lava.collabora.co.uk/scheduler/job/3210245
>
> And here is a corresponding log with reverting the original commit:
> https://lava.collabora.co.uk/scheduler/job/3210596
>
> Both failing logs show that mc errors started right after client DC
> got enabled by ->attach_dev() callback that in the passing logs was
> not called until Host1x driver init. And note that two failing logs
> show that ->attach_dev() could be called from two different sources,
> of_dma_configure_id() or arch_setup_dma_ops().
>
> The reason why ->attach_dev() gets called is probably related to the
> following reasons (sorry, can't be 100% sure as I don't have Tegra124
> or other 32bit Tegra board to test):
> 1) With the commit reverted, all clients are probed in "arch" stage,
>which is even prior to iommu core initialization -- including it
>setting default domain type. This probably messed up the type of
>allocating domains against the default domain type. Also internal
>group is somehow affected. So some condition check in iommu core
>failed and then it bypassed ->attach_dev callback in really_probe
>stage, until Host1x driver does attach_dev again.
>
> 2) 32bit ARM has arch_setup_dma_ops() does an additional set of iommu
>domain allocation + attach_dev(), after of_dma_configure_id() did
>once. This isn't reproducible for me on Tegra210.
>
> As debugging online isn't very efficient, and given that Thierry has
> been working on the linear mapping of framebuffer carveout, I choose
> to partially revert as a quick fix.
The partially revert should be okay, but it's not clear to me what makes
difference for T124 since I don't see that problem on T30, which also
has active display at a boot time.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan
19.02.2021 01:07, Nicolin Chen пишет:
> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> tegra_smmu_configure() that are typically done in the IOMMU core also.
>
> This approach works for both existing devices that have DT nodes and other
> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>EMEM address decode error (SMMU translation error [--S])
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>Page fault (SMMU translation error [--S])
>
> After debugging, I found that the mentioned commit changed some function
> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> client before display driver gets initialized. I couldn't reproduce exact
> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
Hello Nicolin,
Could you please explain in a more details what exactly makes the
difference for the callback sequence?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
05.11.2020 18:49, Thierry Reding пишет: > On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote: >> 24.09.2020 17:01, Thierry Reding пишет: >>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote: >>>> 04.09.2020 15:59, Thierry Reding пишет: >>>>> From: Thierry Reding >>>>> >>>>> Reserved memory regions can be marked as "active" if hardware is >>>>> expected to access the regions during boot and before the operating >>>>> system can take control. One example where this is useful is for the >>>>> operating system to infer whether the region needs to be identity- >>>>> mapped through an IOMMU. >>>>> >>>>> Signed-off-by: Thierry Reding >>>>> --- >>>>> .../bindings/reserved-memory/reserved-memory.txt | 7 +++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >>>>> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >>>>> index 4dd20de6977f..163d2927e4fc 100644 >>>>> --- >>>>> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >>>>> +++ >>>>> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt >>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property >>>>>able to reclaim it back. Typically that means that the operating >>>>>system can use that region to store volatile or cached data that >>>>>can be otherwise regenerated or migrated elsewhere. >>>>> +active (optional) - empty property >>>>> +- If this property is set for a reserved memory region, it indicates >>>>> + that some piece of hardware may be actively accessing this region. >>>>> + Should the operating system want to enable IOMMU protection for a >>>>> + device, all active memory regions must have been identity-mapped >>>>> + in order to ensure that non-quiescent hardware during boot can >>>>> + continue to access the memory. >>>>> >>>>> Linux implementation note: >>>>> - If a "linux,cma-default" property is present, then Linux will use the >>>>> >>>> >>>> Hi, >>>> >>>> Could you please explain what devices need this quirk? I see that you're >>>> targeting Tegra SMMU driver, which means that it should be some pre-T186 >>>> device. >>> >>> Primarily I'm looking at Tegra210 and later, because on earlier devices >>> the bootloader doesn't consistently initialize display. I know that it >>> does on some devices, but not all of them. >> >> AFAIK, all tablet devices starting with Tegra20 that have display panel >> are initializing display at a boot time for showing splash screen. This >> includes all T20/T30/T114 tablets that are already supported by upstream >> kernel. >> >>> This same code should also >>> work on Tegra186 and later (with an ARM SMMU) although the situation is >>> slightly more complicated there because IOMMU translations will fault by >>> default long before these identity mappings can be established. >>> >>>> Is this reservation needed for some device that has display >>>> hardwired to a very specific IOMMU domain at the boot time? >>> >>> No, this is only used to convey information about the active framebuffer >>> to the kernel. In practice the DMA/IOMMU code will use this information >>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for >>> display. >>> >>>> If you're targeting devices that don't have IOMMU enabled by default at >>>> the boot time, then this approach won't work for the existing devices >>>> which won't ever get an updated bootloader. >>> >>> If the devices don't use an IOMMU, then there should be no problem. The >>> extra reserved-memory nodes would still be necessary to ensure that the >>> kernel doesn't reuse the framebuffer memory for the slab allocator, but >>> if no IOMMU is used, then the display controller accessing the memory >>> isn't going to cause problems other than perhaps scanning out data that >>> is no longer a framebuffer. >>> >>> There should also be no problem for dev
Re: [PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes
25.11.2020 00:21, Nicolin Chen пишет: > Hi Joerg, > > These five patches were acked by Thierry and acked-n-tested by > Dmitry a while ago. Would it be possible for you to apply them? > > Thanks! Hi, You probably should try to ping Will Deacon. https://lkml.org/lkml/2020/11/17/243 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
05.10.2020 14:15, Thierry Reding пишет:
> On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
>> 05.10.2020 12:53, Thierry Reding пишет:
>>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>>> static int tegra_smmu_of_xlate(struct device *dev,
>>>>>> struct of_phandle_args *args)
>>>>>> {
>>>>>> +struct platform_device *iommu_pdev =
>>>>>> of_find_device_by_node(args->np);
>>>>>> +struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>>>> u32 id = args->args[0];
>>>>>>
>>>>>> +of_node_put(args->np);
>>>>>> +
>>>>>> +if (!mc || !mc->smmu)
>>>>>> +return -EPROBE_DEFER;
>>>>> platform_get_drvdata(NULL) will crash.
>>>>>
>>>>
>>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>>>
>>> How so? It's technically possible for the iommus property to reference a
>>> device tree node for which no platform device will ever be created, in
>>> which case of_find_device_by_node() will return NULL. That's very
>>> unlikely and perhaps worth just crashing on to make sure it gets fixed
>>> immediately.
>>
>> The tegra_smmu_ops are registered from the SMMU driver itself and MC
>> driver sets platform data before SMMU is initialized, hence device is
>> guaranteed to exist and mc can't be NULL.
>
> Yes, but that assumes that args->np points to the memory controller's
> device tree node. It's obviously a mistake to do this, but I don't think
> anyone will prevent you from doing this:
>
> iommus = <&{/chosen} 0>;
>
> In that case, since no platform device is created for the /chosen node,
> iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
But then Tegra SMMU isn't associated with the device's IOMMU path, and
thus, tegra_smmu_of_xlate() won't be invoked for this device.
> That said, I'm fine with not adding a check for that. If anyone really
> does end up messing this up they deserve the crash.
>
> I'm still a bit undecided about the mc->smmu check because I haven't
> convinced myself yet that it can't happen.
For now I can't see any realistic situation where mc->smmu could be NULL.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
05.10.2020 12:53, Thierry Reding пишет:
> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>> static int tegra_smmu_of_xlate(struct device *dev,
>>>> struct of_phandle_args *args)
>>>> {
>>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>>u32 id = args->args[0];
>>>>
>>>> + of_node_put(args->np);
>>>> +
>>>> + if (!mc || !mc->smmu)
>>>> + return -EPROBE_DEFER;
>>> platform_get_drvdata(NULL) will crash.
>>>
>>
>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>
> How so? It's technically possible for the iommus property to reference a
> device tree node for which no platform device will ever be created, in
> which case of_find_device_by_node() will return NULL. That's very
> unlikely and perhaps worth just crashing on to make sure it gets fixed
> immediately.
The tegra_smmu_ops are registered from the SMMU driver itself and MC
driver sets platform data before SMMU is initialized, hence device is
guaranteed to exist and mc can't be NULL.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
05.10.2020 12:16, Thierry Reding пишет: ... >> I think you meant regmap in regards to protecting IO accesses, but this >> is not what regmap is about if IO accesses are atomic by nature. > > Then why is there regmap-mmio? To protect programming sequences for example, actually that's the only real use-case I saw in kernel drivers once. In our case there are no sequences that require protection, at least I'm not aware about that. >> Secondly, you're missing that tegra30-devfreq driver will also need to >> perform the MC lookup [3] and then driver will no longer work for the >> older DTs if phandle becomes mandatory because these DTs do not have the >> MC phandle in the ACTMON node. >> >> [3] >> https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f >> >> So we will need the fall back for T30/124 as well. > > No, we don't need the fallback because this is new functionality which > can and should be gated on the existence of the new phandle. If there's > no phandle then we have no choice but to use the old code to ensure old > behaviour. You just repeated what I was trying to say:) Perhaps I spent a bit too much time touching that code to the point that lost feeling that there is a need to clarify everything in details. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
05.10.2020 00:57, Nicolin Chen пишет:
> On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
>> 03.10.2020 09:59, Nicolin Chen пишет:
>>> static int tegra_smmu_of_xlate(struct device *dev,
>>>struct of_phandle_args *args)
>>> {
>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>> u32 id = args->args[0];
>>>
>>> + put_device(_pdev->dev);
>>> +
>>> + if (!mc || !mc->smmu)
>>> + return -EPROBE_DEFER;
>>
>> I'm not very excited by seeing code in the patches that can't be
>> explained by the patch authors and will appreciate if you could provide
>> a detailed explanation about why this NULL checking is needed because I
>> think it is unneeded, especially given that other IOMMU drivers don't
>> have such check.
>
> This function could be called from of_iommu_configure(), which is
> a part of other driver's probe(). So I think it's safer to have a
> check. Yet, given mc driver is added to the "arch_initcall" stage,
> you are probably right that there's no really need at this moment
> because all clients should be called after mc/smmu are inited. So
> I'll resend a v6, if that makes you happy.
I wanted to get the explanation :) I'm very curious why it's actually
needed because I'm not 100% sure whether it's not needed at all.
I'd assume that the only possible problem could be if some device is
created in parallel with the MC probing and there is no locking that
could prevent this in the drivers core. It's not apparent to me whether
this situation could happen at all in practice.
The MC is created early and at that time everything is sequential, so
it's indeed should be safe to remove the check.
>> I'm asking this question second time now, please don't ignore review
>> comments next time.
>
> I think I missed your reply or misunderstood it.
>
Okay!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
05.10.2020 10:13, Thierry Reding пишет: ... > Have you also seen that sun50i-iommu does look up the SMMU from a > phandle using of_find_device_by_node()? So I think you've shown yourself > that even "modern" drivers avoid global pointers and look up via > phandle. I have no problem with the lookup by phandle and I'm all for it. It's now apparent to me that you completely missed my point, but that should be my fault that I haven't conveyed it properly from the start. I just wanted to avoid the incompatible DT changes which could break older DTs + I simply wanted to improve the older code without introducing new features, that's it. Anyways, after yours comments I started to look at how the interconnect patches could be improved and found new things, like that OPPs now support ICC and that EMC has a working EMC_STAT, I also discovered syscon and simple-mfd. This means that we won't need the global pointers at all neither for SMMU, nor for interconnect, nor for EMC drivers :) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get
29.09.2020 20:41, Dmitry Osipenko пишет: > 29.09.2020 09:13, Nicolin Chen пишет: >> The tegra_smmu_group_get was added to group devices in different >> SWGROUPs and it'd return a NULL group pointer upon a mismatch at >> tegra_smmu_find_group(), so for most of clients/devices, it very >> likely would mismatch and need a fallback generic_device_group(). >> >> But now tegra_smmu_group_get handles devices in same SWGROUP too, >> which means that it would allocate a group for every new SWGROUP >> or would directly return an existing one upon matching a SWGROUP, >> i.e. any device will go through this function. >> >> So possibility of having a NULL group pointer in device_group() >> is upon failure of either devm_kzalloc() or iommu_group_alloc(). >> In either case, calling generic_device_group() no longer makes a >> sense. Especially for devm_kzalloc() failing case, it'd cause a >> problem if it fails at devm_kzalloc() yet succeeds at a fallback >> generic_device_group(), because it does not create a group->list >> for other devices to match. >> >> This patch simply unwraps the function to clean it up. >> >> Signed-off-by: Nicolin Chen >> --- > > Reviewed-by: Dmitry Osipenko > Tested-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range
29.09.2020 20:42, Dmitry Osipenko пишет: > 29.09.2020 09:13, Nicolin Chen пишет: >> This is used to protect potential race condition at use_count. >> since probes of client drivers, calling attach_dev(), may run >> concurrently. >> >> Signed-off-by: Nicolin Chen >> --- > > It's always better not to mix success and error code paths in order to > keep code readable, but not a big deal in the case of this particular > patch since the changed code is quite simple. Please try to avoid doing > this in the future patches. > > Also, please note that in general it's better to use locked/unlocked > versions for the functions like it's already done for > tegra_smmu_map/unmap, this will remove the need to maintain lockings in > the code. The code touched by this patch doesn't have complicated code > paths, so it's good enough to me. > > Reviewed-by: Dmitry Osipenko > Tested-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
03.10.2020 09:59, Nicolin Chen пишет: > The bus_set_iommu() in tegra_smmu_probe() enumerates all clients > to call in tegra_smmu_probe_device() where each client searches > its DT node for smmu pointer and swgroup ID, so as to configure > an fwspec. But this requires a valid smmu pointer even before mc > and smmu drivers are probed. So in tegra_smmu_probe() we added a > line of code to fill mc->smmu, marking "a bit of a hack". > > This works for most of clients in the DTB, however, doesn't work > for a client that doesn't exist in DTB, a PCI device for example. > > Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when > it's called from bus_set_iommu(), iommu core will let everything > carry on. Then when a client gets probed, of_iommu_configure() in > iommu core will search DTB for swgroup ID and call ->of_xlate() > to prepare an fwspec, similar to tegra_smmu_probe_device() and > tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device() > again, and this time we shall return smmu->iommu pointer properly. > > So we can get rid of tegra_smmu_find() and tegra_smmu_configure() > along with DT polling code by letting the iommu core handle every > thing, except a problem that we search iommus property in DTB not > only for swgroup ID but also for mc node to get mc->smmu pointer > to call dev_iommu_priv_set() and return the smmu->iommu pointer. > So we'll need to find another way to get smmu pointer. > > Referencing the implementation of sun50i-iommu driver, of_xlate() > has client's dev pointer, mc node and swgroup ID. This means that > we can call dev_iommu_priv_set() in of_xlate() instead, so we can > simply get smmu pointer in ->probe_device(). > > This patch reworks tegra_smmu_probe_device() by: > 1) Removing mc->smmu hack in tegra_smmu_probe() so as to return >ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of >tegra_smmu_probe/tegra_mc_probe(). > 2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu >pointer in tegra_smmu_probe_device() to replace DTB polling. > 3) Removing tegra_smmu_configure() accordingly since iommu core >takes care of it. > > This also fixes a problem that previously we added all clients to > iommu groups before iommu core initializes its default domain: > ubuntu@jetson:~$ dmesg | grep iommu > platform smmu_benchmark: Adding to iommu group 0 > platform 1003000.pcie: Adding to iommu group 1 > platform 5000.host1x: Adding to iommu group 2 > platform 5700.gpu: Adding to iommu group 3 > platform 7000c400.i2c: Adding to iommu group 4 > platform 7000c500.i2c: Adding to iommu group 4 > platform 7000c700.i2c: Adding to iommu group 4 > platform 7000d000.i2c: Adding to iommu group 4 > iommu: Default domain type: Translated > > Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have > warnings if switching to IOMMU_DOMAIN_DMA: > iommu: Failed to allocate default IOMMU domain of type 0 for >group (null) - Falling back to IOMMU_DOMAIN_DMA > iommu: Failed to allocate default IOMMU domain of type 0 for >group (null) - Falling back to IOMMU_DOMAIN_DMA > > Now, bypassing the first probe_device() call from bus_set_iommu() > fixes the sequence: > ubuntu@jetson:~$ dmesg | grep iommu > iommu: Default domain type: Translated > tegra-i2c 7000c400.i2c: Adding to iommu group 0 > tegra-i2c 7000c500.i2c: Adding to iommu group 0 > tegra-i2c 7000d000.i2c: Adding to iommu group 0 > tegra-pcie 1003000.pcie: Adding to iommu group 1 > ... > > Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED. > > Signed-off-by: Nicolin Chen > --- Everything looks good to me, apart from the very minor pending question about the NULL-checking. Thanks! Reviewed-by: Dmitry Osipenko Tested-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support
03.10.2020 09:59, Nicolin Chen пишет: > This patch simply adds support for PCI devices. > > Signed-off-by: Nicolin Chen > Reviewed-by: Dmitry Osipenko Small nit: yours s-b tag always should be the last line of the commit message because you're "signing up" words that were written by you. Tested-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
03.10.2020 09:59, Nicolin Chen пишет: > In tegra_smmu_(de)attach_dev() functions, we poll DTB for each > client's iommus property to get swgroup ID in order to prepare > "as" and enable smmu. Actually tegra_smmu_configure() prepared > an fwspec for each client, and added to the fwspec all swgroup > IDs of client DT node in DTB. > > So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as > to replace the redundant DT polling code. > > Signed-off-by: Nicolin Chen > --- I'm still not highly impressed by seeing the !fwspec check in this patch. But I'm not a maintainer of the SMMU driver, hence will leave it up to Thierry and Joerg to decide whether this is good or needs to be improved. Otherwise this patch is good to me, thanks. I tested it on Nexus 7, which is Tegra30. Reviewed-by: Dmitry Osipenko Tested-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
03.10.2020 09:59, Nicolin Chen пишет: > ubuntu@jetson:~$ dmesg | grep iommu > iommu: Default domain type: Translated > tegra-i2c 7000c400.i2c: Adding to iommu group 0 > tegra-i2c 7000c500.i2c: Adding to iommu group 0 > tegra-i2c 7000d000.i2c: Adding to iommu group 0 > tegra-pcie 1003000.pcie: Adding to iommu group 1 Could you please explain how you got I2C into IOMMU? Are you testing vanilla upstream kerne? Upstream DT doesn't assign AHB group to I2C controllers, nor to APB DMA controller. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
03.10.2020 09:59, Nicolin Chen пишет:
> static int tegra_smmu_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> u32 id = args->args[0];
>
> + put_device(_pdev->dev);
> +
> + if (!mc || !mc->smmu)
> + return -EPROBE_DEFER;
I'm not very excited by seeing code in the patches that can't be
explained by the patch authors and will appreciate if you could provide
a detailed explanation about why this NULL checking is needed because I
think it is unneeded, especially given that other IOMMU drivers don't
have such check.
I'm asking this question second time now, please don't ignore review
comments next time.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
03.10.2020 02:53, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 22:45, Nicolin Chen пишет:
>>> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>>>>struct device *dev)
>>>>> {
>>>>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>>> struct tegra_smmu_as *as = to_smmu_as(domain);
>>>>> - struct device_node *np = dev->of_node;
>>>>> - struct of_phandle_args args;
>>>>> unsigned int index = 0;
>>>>> int err = 0;
>>>>>
>>>>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>>>> -)) {
>>>>> - unsigned int swgroup = args.args[0];
>>>>> -
>>>>> - if (args.np != smmu->dev->of_node) {
>>>>> - of_node_put(args.np);
>>>>> - continue;
>>>>> - }
>>>>> -
>>>>> - of_node_put(args.np);
>>>>> + if (!fwspec)
>>>>> + return -ENOENT;
>>>>
>>>> Could the !fwspec ever be true here as well?
>>>
>>> There are multiple callers of this function. It's really not that
>>> straightforward to track every one of them. So I'd rather have it
>>> here as other iommu drivers do. We are human beings, so we could
>>> have missed something somewhere, especially callers are not from
>>> tegra-* drivers.
>>>
>>
>> I'm looking at the IOMMU core and it requires device to be in IOMMU
>> group before attach_dev() could be called.
>>
>> The group can't be assigned to device without the fwspec, see
>> tegra_smmu_device_group().
>>
>> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
>> NULL in attach_dev(), some not checking anything, some check both and
>> only arm-smmu checks the fwspec.
>
> As I said a couple of days ago, I don't like to assume that the
> callers won't change. And this time, it's from open code. So I
> don't want to assume that there won't be a change.
>
> If you are confident that there is no need to add such a check,
> please send patches to remove those checks in those drivers to
> see if others would agree. I would be willing to remove it after
> that. Otherwise, I'd like to keep this.
>
> Thanks for the review.
>
I haven't tried to check every code path very thoroughly, expecting you
to do it since you're making this patch. Maybe there is a real reason
why majority of drivers do the checks and it would be good to know why.
Although, it's not critical in this particular case and indeed the
checks could be improved later on.
It looks to me that at least will be a bit better/cleaner to check the
dev_iommu_priv_get() for NULL instead of fwspec because the private
variable depends on the fwspec presence and there is a similar check in
probe_device, hence checks will be more consistent.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
02.10.2020 22:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>> struct device *dev)
>>> {
>>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>> struct tegra_smmu_as *as = to_smmu_as(domain);
>>> - struct device_node *np = dev->of_node;
>>> - struct of_phandle_args args;
>>> unsigned int index = 0;
>>> int err = 0;
>>>
>>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - )) {
>>> - unsigned int swgroup = args.args[0];
>>> -
>>> - if (args.np != smmu->dev->of_node) {
>>> - of_node_put(args.np);
>>> - continue;
>>> - }
>>> -
>>> - of_node_put(args.np);
>>> + if (!fwspec)
>>> + return -ENOENT;
>>
>> Could the !fwspec ever be true here as well?
>
> There are multiple callers of this function. It's really not that
> straightforward to track every one of them. So I'd rather have it
> here as other iommu drivers do. We are human beings, so we could
> have missed something somewhere, especially callers are not from
> tegra-* drivers.
>
I'm looking at the IOMMU core and it requires device to be in IOMMU
group before attach_dev() could be called.
The group can't be assigned to device without the fwspec, see
tegra_smmu_device_group().
Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
NULL in attach_dev(), some not checking anything, some check both and
only arm-smmu checks the fwspec.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 21:01, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>>> {
>>> - struct device_node *np = dev->of_node;
>>> - struct tegra_smmu *smmu = NULL;
>>> - struct of_phandle_args args;
>>> - unsigned int index = 0;
>>> - int err;
>>> -
>>> - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - ) == 0) {
>>> - smmu = tegra_smmu_find(args.np);
>>> - if (smmu) {
>>> - err = tegra_smmu_configure(smmu, dev, );
>>> - of_node_put(args.np);
>>> -
>>> - if (err < 0)
>>> - return ERR_PTR(err);
>>> -
>>> - /*
>>> -* Only a single IOMMU master interface is currently
>>> -* supported by the Linux kernel, so abort after the
>>> -* first match.
>>> -*/
>>> - dev_iommu_priv_set(dev, smmu);
>>> -
>>> - break;
>>> - }
>>> -
>>> - of_node_put(args.np);
>>> - index++;
>>> - }
>>> + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>
>>> if (!smmu)
>>> return ERR_PTR(-ENODEV);
>>
>> The !smmu can't ever be true now, isn't it? Then please remove it.
>
> How can you be so sure? Have you read my commit message? The whole
> point of removing the hack in tegra_smmu_probe() is to return the
> ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
> when mc->smmu is not assigned it, as it's assigned after we return
> tegra_smmu_probe() while bus_set_iommu() is still in the middle of
> the tegra_smmu_probe().
>
My bad, I probably missed that was looking at the probe_device(), looks
good then.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support
02.10.2020 09:08, Nicolin Chen пишет: > This patch simply adds support for PCI devices. > > Signed-off-by: Nicolin Chen > --- Reviewed-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
