Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display
 clients
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     Joerg Roedel <joro@8bytes.org>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Krishna Reddy <vdumpa@nvidia.com>,
        Nicolin Chen <nicoleotsuka@gmail.com>,
        Will Deacon <will@kernel.org>,
        iommu@lists.linux-foundation.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210328233256.20494-1-digetx@gmail.com>
 <YG75urcXAb90Jj12@orome.fritz.box>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <c4b42a3d-d260-9a69-4ee7-8ad586741f8c@gmail.com>
Date:   Thu, 8 Apr 2021 17:07:21 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <YG75urcXAb90Jj12@orome.fritz.box>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 <digetx@gmail.com>
>> ---
>>  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_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;
>> +	}
>> +}
>> +
> 
> 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;