| Message ID | 20220501101022.3931295-3-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: fixes for KMS iommu handling | expand |
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@linaro.org> > --- > 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);
On Tue, 3 May 2022 at 13:57, Robin Murphy <robin.murphy@arm.com> wrote: > > 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@linaro.org> > > --- > > 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)? I'd prefer to split this into the separate change, if you don't mind. > > > + 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). Let me check that it works correctly and spin a v2 afterwards. > > 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);
On 2022-05-03 14:30, Dmitry Baryshkov wrote: > On Tue, 3 May 2022 at 13:57, Robin Murphy <robin.murphy@arm.com> wrote: >> >> 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@linaro.org> >>> --- >>> 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)? > > I'd prefer to split this into the separate change, if you don't mind. Oh, for sure, divide the patches however you see fit - I'm just hoping to save your time overall by getting all the IOMMU-related refactoring done now as a single series rather than risk me coming back and breaking things again in a few months :) Cheers, Robin. > >> >>> + 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). > > Let me check that it works correctly and spin a v2 afterwards. > >> >> 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); > > >
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); + if (iommu) { struct msm_mmu *mmu; iommu_dev = &pdev->dev; if (!dev_iommu_fwspec_get(iommu_dev)) 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);
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@linaro.org> --- 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(-)