Re: [PATCH v1 2/3] drm/tegra: Fix 2d and 3d clients detaching from IOMMU domain

[Dmitry Osipenko <digetx@gmail.com>]

24.10.2019 19:21, Dmitry Osipenko пишет:
> 24.10.2019 19:09, Dmitry Osipenko пишет:
>> 24.10.2019 18:57, Dmitry Osipenko пишет:
>>> 24.10.2019 18:56, Thierry Reding пишет:
>>>> On Thu, Oct 24, 2019 at 06:47:23PM +0300, Dmitry Osipenko wrote:
>>>>> 24.10.2019 16:50, Thierry Reding пишет:
>>>>>> On Thu, Oct 24, 2019 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>>>>>>> 24.10.2019 14:58, Thierry Reding пишет:
>>>>>>>> On Sun, Jun 23, 2019 at 08:37:42PM +0300, Dmitry Osipenko wrote:
>>>>>>>>> This should should fire up on the DRM's driver module re-loader because
>>>>>>>>> there won't be enough available domains on older Tegra SoCs.
>>>>>>>>>
>>>>>>>>> Cc: stable <stable@vger.kernel.org>
>>>>>>>>> Fixes: 0c407de5ed1a ("drm/tegra: Refactor IOMMU attach/detach")
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/tegra/dc.c   | 4 ++--
>>>>>>>>>  drivers/gpu/drm/tegra/drm.c  | 9 ++++++---
>>>>>>>>>  drivers/gpu/drm/tegra/drm.h  | 3 ++-
>>>>>>>>>  drivers/gpu/drm/tegra/gr2d.c | 4 ++--
>>>>>>>>>  drivers/gpu/drm/tegra/gr3d.c | 4 ++--
>>>>>>>>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>>>>>>>
>>>>>>>> I think I understand what this is trying to do, but the commit message
>>>>>>>> does not help at all. So what's really going on here is that we need to
>>>>>>>> detach the device from the group regardless of whether we're sharing the
>>>>>>>> group or not, just like we attach groups to the shared domain whether
>>>>>>>> they share the same group or not.
>>>>>>>
>>>>>>> Yes, the commit's message could be improved.
>>>>>>>
>>>>>>>> But in that case, I wonder if it's even worth splitting groups the way
>>>>>>>> we are right now. Wouldn't it be better to just put all the devices into
>>>>>>>> the same group and be done with it?
>>>>>>>>
>>>>>>>> The current code gives me headaches every time I read it, so if we can
>>>>>>>> just make it so that all the devices under the DRM device share the same
>>>>>>>> group, this would become a lot easier to deal with. I'm not really
>>>>>>>> convinced that it makes much sense to keep them on separate domains,
>>>>>>>> especially given the constraints on the number of domains available on
>>>>>>>> earlier Tegra devices.
>>>>>>>>
>>>>>>>> Note that sharing a group will also make it much easier for these to use
>>>>>>>> the DMA API if it is backed by an IOMMU.
>>>>>>>
>>>>>>> Probably I'm blanking on everything about IOMMU now.. could you please
>>>>>>> remind me what "IOMMU group" is?
>>>>>>>
>>>>>>> Isn't it that each IOMMU group relates to the HW ID (SWGROUP)? But then
>>>>>>> each display controller has its own SWGROUP.. and thus that sharing just
>>>>>>> doesn't make any sense, hm.
>>>>>>
>>>>>> IOMMU groups are not directly related to SWGROUPs. But by default the
>>>>>> IOMMU framework will share a domain between members of the same IOMMU
>>>>>> group.
>>>>>
>>>>> Ah, I re-figured out that again. The memory controller drivers are
>>>>> defining a single "IOMMU group" for both of the display controllers.
>>>>>
>>>>>> Seems like that's really what we want here, so that when we do
>>>>>> use the DMA API, all the devices part of the DRM device get attached to
>>>>>> the same IOMMU domain, yet if we don't want to use the DMA API we only
>>>>>> need to detach the one group from the backing.
>>>>>
>>>>> Yes, it should be okay to put all DRM devices into the same group, like
>>>>> it is done now for the displays. It also should resolve problem with the
>>>>> domains shortage on T30 since now there are maximum 3 domains in use:
>>>>> host1x, drm and vde.
>>>>>
>>>>> I actually just checked that the original problem still exists
>>>>> and this change solves it as well:
>>>>>
>>>>> ---
>>>>> diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
>>>>> index 5a0f6e0a1643..e71096498436 100644
>>>>> --- a/drivers/memory/tegra/tegra30.c
>>>>> +++ b/drivers/memory/tegra/tegra30.c
>>>>> @@ -1021,6 +1021,9 @@ static const struct tegra_smmu_swgroup
>>>>> tegra30_swgroups[] = {
>>>>>  static const unsigned int tegra30_group_display[] = {
>>>>>  	TEGRA_SWGROUP_DC,
>>>>>  	TEGRA_SWGROUP_DCB,
>>>>> +	TEGRA_SWGROUP_G2,
>>>>> +	TEGRA_SWGROUP_NV,
>>>>> +	TEGRA_SWGROUP_NV2,
>>>>>  };
>>>>>
>>>>>  static const struct tegra_smmu_group_soc tegra30_groups[] = {
>>>>> ---
>>>>>
>>>>> Please let me know whether you're going to make a patch or if I should
>>>>> do it.
>>>>
>>>> I've been testing with a similar change and couldn't find any
>>>> regressions. I've also made the same modifications for Tegra114 and
>>>> Tegra124.
>>>>
>>>> Are you saying that none of these patches are needed anymore? Or do we
>>>> still need a patch to fix detaching? I'm thinking that maybe we can
>>>> drastrically simplify the detachment now by dropping the shared
>>>> parameter altogether.
>>>>
>>>> Let me draft a patch and send out the whole set for testing.
>>>
>>> Seems it's still not ideal because I noticed this in KMSG:
>>>
>>> [    0.703185] Failed to attached device 54200000.dc to IOMMU_mapping
>>> [    0.710404] Failed to attached device 54240000.dc to IOMMU_mapping
>>> [    0.719347] Failed to attached device 54140000.gr2d to IOMMU_mapping
>>> [    0.719569] Failed to attached device 54180000.gr3d to IOMMU_mapping
>>>
>>> which comes from the implicit IOMMU backing.
>>
>> And the error comes from here:
>>
>> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/iommu/iommu.c#L1655
> 
> So the detaching still should be needed, but at the moment the ARM32
> DMA-mapping code is simply not suitable for the case of having multiple
> devices in the same group. I'm wondering whether there are any real
> users for the implicit IOMMU backing on ARM32 at all :/
> 

Apparently the "Failed to attached device 54200000.dc" was always in the
log (I rarely testing the default multi-platform config), it's just the
message is a pr_warn that I wasn't paying attention because it is
colored like pr_info in dmesg :)