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@xxxxxxxxx>
>> ---
>> 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;
