[PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu
Hi, On 2016å¹´06æ10æ¥ 16:03, Tomasz Figa wrote: > Hi, > > On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng > wrote: >> Rockchip DRM used the arm special API, arm_iommu_*(), to attach >> iommu for ARM32 SoCs. This patch convert to common iommu API >> so it would support ARM64 like RK3399. >> >> The general idea is domain_alloc(), attach_device() and >> arch_setup_dma_ops() to set dma_ops manually for DRM at the last. >> >> Signed-off-by: Shunqian Zheng >> --- >> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 >> +++- >> drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + >> 2 files changed, 89 insertions(+), 42 deletions(-) >> > Please see my comments inline. > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> index f5a68fc..7965a66 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> @@ -14,8 +14,6 @@ >>* GNU General Public License for more details. >>*/ >> >> -#include >> - >> #include >> #include >> #include >> @@ -24,6 +22,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "rockchip_drm_drv.h" >> #include "rockchip_drm_fb.h" >> @@ -46,7 +46,8 @@ static bool is_support_iommu = true; >> int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, >> struct device *dev) >> { >> - struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping; >> + struct rockchip_drm_private *private = drm_dev->dev_private; >> + struct iommu_domain *domain = private->domain; >> int ret; >> >> if (!is_support_iommu) >> @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device >> *drm_dev, >> >> dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >> >> - return arm_iommu_attach_device(dev, mapping); >> + ret = iommu_attach_device(domain, dev); >> + > nit: Unnecessary blank line. Will fix it in v3. > >> + if (ret) { >> + dev_err(dev, "Failed to attach iommu device\n"); >> + return ret; >> + } > nit: On the other hand, a blank line here would improve readability. Will fix it in v3. > >> + arch_setup_dma_ops(dev, 0x, SZ_2G, >> + (struct iommu_ops *)dev->bus->iommu_ops, false); > This is casting a const pointer to a non-const pointer. which isn't > really a good idea. I can see that arch_setup_dma_ops() requires a > writable pointer, though. Looking at the implementations of > arch_setup_dma_ops() around the platforms (namely arm and arm64...), > it makes me wonder if the prototype shouldn't be changed to const > instead. Actually, kernel-next changed iommu_ops to const by: 53c92d7 iommu: of: enforce const-ness of struct iommu_ops Will remove casting in the v3. > >> + return 0; >> } >> >> void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, >> struct device *dev) >> { >> - if (!is_support_iommu) >> - return; >> + struct rockchip_drm_private *private = drm_dev->dev_private; >> + struct iommu_domain *domain = private->domain; >> >> - arm_iommu_detach_device(dev); >> + if (is_support_iommu) >> + iommu_detach_device(domain, dev); >> } >> >> int rockchip_register_crtc_funcs(struct drm_crtc *crtc, >> @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct >> drm_device *dev, >> priv->crtc_funcs[pipe]->disable_vblank(crtc); >> } >> >> +static int rockchip_drm_init_iommu(struct drm_device *drm_dev) >> +{ >> + struct rockchip_drm_private *private = drm_dev->dev_private; >> + struct device *dev = drm_dev->dev; >> + int ret; >> + >> + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), >> + GFP_KERNEL); >> + if (!dev->dma_parms) { >> + ret = -ENOMEM; >> + return ret; > nit: return -ENOMEM; Will fix it in v3. > >> + } >> + >> + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); >> + if (ret) { >> + dev_err(dev, "Failed to set coherent mask\n"); >> + return ret; >> + } >> + >> + dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >> + >> + private->domain = iommu_domain_alloc(_bus_type); >> + if (!private->domain) >> + return -ENOMEM; >> + >> + ret = iommu_get_dma_cookie(private->domain); >> + if (ret) { >> + dev_err(dev, "Failed to get dma cookie\n"); >> + goto err_free_domain; >> + } >> + >> + ret = iommu_dma_init_domain(private->domain, 0x, SZ_2G); > I guess djkurtz's TODO comment could be preserved here. Agree. Thank you very much. Shunqian > > Best regards, > Tomasz > > >
[PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu
Hi, On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng wrote: > Rockchip DRM used the arm special API, arm_iommu_*(), to attach > iommu for ARM32 SoCs. This patch convert to common iommu API > so it would support ARM64 like RK3399. > > The general idea is domain_alloc(), attach_device() and > arch_setup_dma_ops() to set dma_ops manually for DRM at the last. > > Signed-off-by: Shunqian Zheng > --- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 > +++- > drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + > 2 files changed, 89 insertions(+), 42 deletions(-) > Please see my comments inline. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index f5a68fc..7965a66 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -14,8 +14,6 @@ > * GNU General Public License for more details. > */ > > -#include > - > #include > #include > #include > @@ -24,6 +22,8 @@ > #include > #include > #include > +#include > +#include > > #include "rockchip_drm_drv.h" > #include "rockchip_drm_fb.h" > @@ -46,7 +46,8 @@ static bool is_support_iommu = true; > int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, >struct device *dev) > { > - struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping; > + struct rockchip_drm_private *private = drm_dev->dev_private; > + struct iommu_domain *domain = private->domain; > int ret; > > if (!is_support_iommu) > @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device > *drm_dev, > > dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > > - return arm_iommu_attach_device(dev, mapping); > + ret = iommu_attach_device(domain, dev); > + nit: Unnecessary blank line. > + if (ret) { > + dev_err(dev, "Failed to attach iommu device\n"); > + return ret; > + } nit: On the other hand, a blank line here would improve readability. > + arch_setup_dma_ops(dev, 0x, SZ_2G, > + (struct iommu_ops *)dev->bus->iommu_ops, false); This is casting a const pointer to a non-const pointer. which isn't really a good idea. I can see that arch_setup_dma_ops() requires a writable pointer, though. Looking at the implementations of arch_setup_dma_ops() around the platforms (namely arm and arm64...), it makes me wonder if the prototype shouldn't be changed to const instead. > + return 0; > } > > void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, > struct device *dev) > { > - if (!is_support_iommu) > - return; > + struct rockchip_drm_private *private = drm_dev->dev_private; > + struct iommu_domain *domain = private->domain; > > - arm_iommu_detach_device(dev); > + if (is_support_iommu) > + iommu_detach_device(domain, dev); > } > > int rockchip_register_crtc_funcs(struct drm_crtc *crtc, > @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct > drm_device *dev, > priv->crtc_funcs[pipe]->disable_vblank(crtc); > } > > +static int rockchip_drm_init_iommu(struct drm_device *drm_dev) > +{ > + struct rockchip_drm_private *private = drm_dev->dev_private; > + struct device *dev = drm_dev->dev; > + int ret; > + > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > + GFP_KERNEL); > + if (!dev->dma_parms) { > + ret = -ENOMEM; > + return ret; nit: return -ENOMEM; > + } > + > + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); > + if (ret) { > + dev_err(dev, "Failed to set coherent mask\n"); > + return ret; > + } > + > + dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + > + private->domain = iommu_domain_alloc(_bus_type); > + if (!private->domain) > + return -ENOMEM; > + > + ret = iommu_get_dma_cookie(private->domain); > + if (ret) { > + dev_err(dev, "Failed to get dma cookie\n"); > + goto err_free_domain; > + } > + > + ret = iommu_dma_init_domain(private->domain, 0x, SZ_2G); I guess djkurtz's TODO comment could be preserved here. Best regards, Tomasz
[PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu
Rockchip DRM used the arm special API, arm_iommu_*(), to attach iommu for ARM32 SoCs. This patch convert to common iommu API so it would support ARM64 like RK3399. The general idea is domain_alloc(), attach_device() and arch_setup_dma_ops() to set dma_ops manually for DRM at the last. Signed-off-by: Shunqian Zheng --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + 2 files changed, 89 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index f5a68fc..7965a66 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -14,8 +14,6 @@ * GNU General Public License for more details. */ -#include - #include #include #include @@ -24,6 +22,8 @@ #include #include #include +#include +#include #include "rockchip_drm_drv.h" #include "rockchip_drm_fb.h" @@ -46,7 +46,8 @@ static bool is_support_iommu = true; int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, struct device *dev) { - struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping; + struct rockchip_drm_private *private = drm_dev->dev_private; + struct iommu_domain *domain = private->domain; int ret; if (!is_support_iommu) @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); - return arm_iommu_attach_device(dev, mapping); + ret = iommu_attach_device(domain, dev); + + if (ret) { + dev_err(dev, "Failed to attach iommu device\n"); + return ret; + } + arch_setup_dma_ops(dev, 0x, SZ_2G, + (struct iommu_ops *)dev->bus->iommu_ops, false); + return 0; } void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, struct device *dev) { - if (!is_support_iommu) - return; + struct rockchip_drm_private *private = drm_dev->dev_private; + struct iommu_domain *domain = private->domain; - arm_iommu_detach_device(dev); + if (is_support_iommu) + iommu_detach_device(domain, dev); } int rockchip_register_crtc_funcs(struct drm_crtc *crtc, @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev, priv->crtc_funcs[pipe]->disable_vblank(crtc); } +static int rockchip_drm_init_iommu(struct drm_device *drm_dev) +{ + struct rockchip_drm_private *private = drm_dev->dev_private; + struct device *dev = drm_dev->dev; + int ret; + + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), + GFP_KERNEL); + if (!dev->dma_parms) { + ret = -ENOMEM; + return ret; + } + + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); + if (ret) { + dev_err(dev, "Failed to set coherent mask\n"); + return ret; + } + + dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); + + private->domain = iommu_domain_alloc(_bus_type); + if (!private->domain) + return -ENOMEM; + + ret = iommu_get_dma_cookie(private->domain); + if (ret) { + dev_err(dev, "Failed to get dma cookie\n"); + goto err_free_domain; + } + + ret = iommu_dma_init_domain(private->domain, 0x, SZ_2G); + if (ret) { + dev_err(dev, "Failed to init domain\n"); + goto err_put_cookie; + } + + ret = rockchip_drm_dma_attach_device(drm_dev, dev); + if (ret) { + dev_err(dev, "Failed to attach device\n"); + goto err_put_cookie; + } + + return 0; + +err_put_cookie: + iommu_put_dma_cookie(private->domain); +err_free_domain: + iommu_domain_free(private->domain); + + return ret; +} + +static void rockchip_iommu_cleanup(struct drm_device *drm_dev) +{ + struct rockchip_drm_private *private = drm_dev->dev_private; + + iommu_put_dma_cookie(private->domain); + iommu_domain_free(private->domain); +} + static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) { struct rockchip_drm_private *private; - struct dma_iommu_mapping *mapping = NULL; struct device *dev = drm_dev->dev; struct drm_connector *connector; int ret; @@ -153,38 +223,18 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) rockchip_drm_mode_config_init(drm_dev); - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), - GFP_KERNEL); - if (!dev->dma_parms) { - ret = -ENOMEM; - goto err_config_cleanup; - } -