Re: [PATCH 2/3] drm/msm/mdp5: move iommu_domain_alloc() call close to its usage

[Robin Murphy <robin.murphy@xxxxxxx>]

On 2022-05-01 11:10, Dmitry Baryshkov wrote:
> Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
> This allows us to drop final bits of struct mdp5_cfg_platform which
> remained from the pre-DT days.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 16 ----------------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h |  6 ------
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++--
>   3 files changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 1bf9ff5dbabc..714effb967ff 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -1248,8 +1248,6 @@ static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
>   	{ .revision = 3, .config = { .hw = &sdm630_config } },
>   };
>   
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev);
> -
>   const struct mdp5_cfg_hw *mdp5_cfg_get_hw_config(struct mdp5_cfg_handler *cfg_handler)
>   {
>   	return cfg_handler->config.hw;
> @@ -1274,10 +1272,8 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   		uint32_t major, uint32_t minor)
>   {
>   	struct drm_device *dev = mdp5_kms->dev;
> -	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct mdp5_cfg_handler *cfg_handler;
>   	const struct mdp5_cfg_handler *cfg_handlers;
> -	struct mdp5_cfg_platform *pconfig;
>   	int i, ret = 0, num_handlers;
>   
>   	cfg_handler = kzalloc(sizeof(*cfg_handler), GFP_KERNEL);
> @@ -1320,9 +1316,6 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   	cfg_handler->revision = minor;
>   	cfg_handler->config.hw = mdp5_cfg;
>   
> -	pconfig = mdp5_get_config(pdev);
> -	memcpy(&cfg_handler->config.platform, pconfig, sizeof(*pconfig));
> -
>   	DBG("MDP5: %s hw config selected", mdp5_cfg->name);
>   
>   	return cfg_handler;
> @@ -1333,12 +1326,3 @@ struct mdp5_cfg_handler *mdp5_cfg_init(struct mdp5_kms *mdp5_kms,
>   
>   	return ERR_PTR(ret);
>   }
> -
> -static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
> -{
> -	static struct mdp5_cfg_platform config = {};
> -
> -	config.iommu = iommu_domain_alloc(&platform_bus_type);
> -
> -	return &config;
> -}
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> index 6b03d7899309..c2502cc33864 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h
> @@ -104,14 +104,8 @@ struct mdp5_cfg_hw {
>   	uint32_t max_clk;
>   };
>   
> -/* platform config data (ie. from DT, or pdata) */
> -struct mdp5_cfg_platform {
> -	struct iommu_domain *iommu;
> -};
> -
>   struct mdp5_cfg {
>   	const struct mdp5_cfg_hw *hw;
> -	struct mdp5_cfg_platform platform;
>   };
>   
>   struct mdp5_kms;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 9b7bbc3adb97..1c67c2c828cd 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -558,6 +558,7 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	struct msm_gem_address_space *aspace;
>   	int irq, i, ret;
>   	struct device *iommu_dev;
> +	struct iommu_domain *iommu;
>   
>   	ret = mdp5_init(to_platform_device(dev->dev), dev);
>   
> @@ -601,14 +602,15 @@ static int mdp5_kms_init(struct drm_device *dev)
>   	}
>   	mdelay(16);
>   
> -	if (config->platform.iommu) {
> +	iommu = iommu_domain_alloc(&platform_bus_type);

To preempt the next change down the line as well, could this be 
rearranged to work as iommu_domain_alloc(iommu_dev->bus)?

> +	if (iommu) {
>   		struct msm_mmu *mmu;
>   
>   		iommu_dev = &pdev->dev;
>   		if (!dev_iommu_fwspec_get(iommu_dev))

The fwspec helpers are more of an internal thing between the IOMMU 
drivers and the respective firmware code - I'd rather that external API 
users stuck consistently to using device_iommu_mapped() (it should give 
the same result).

Otherwise, thanks for sorting this out!

Robin.

>   			iommu_dev = iommu_dev->parent;
>   
> -		mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
> +		mmu = msm_iommu_new(iommu_dev, iommu);
>   
>   		aspace = msm_gem_address_space_create(mmu, "mdp5",
>   			0x1000, 0x100000000 - 0x1000);