Re: [PATCH] drm/exynos: Fix cleanup of IOMMU related objects

[Inki Dae <inki.dae@samsung.com>]

20. 3. 5. 오후 5:13에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 05.03.2020 09:01, Inki Dae wrote:
>> Hi Marek,
>>
>> 20. 3. 2. 오후 11:27에 Marek Szyprowski 이(가) 쓴 글:
>>> Store the IOMMU mapping created by device core of each Exynos DRM
>>> sub-device and restore it when Exynos DRM driver is unbound. This fixes
>>> IOMMU initialization failure for the second time when deferred probe is
>>> triggered from the bind() callback of master's compound DRM driver.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_dma.c       | 22 +++++++++++--------
>>>   drivers/gpu/drm/exynos/exynos_drm_drv.h       |  6 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_fimc.c      |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_gsc.c       |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_rotator.c   |  5 +++--
>>>   drivers/gpu/drm/exynos/exynos_drm_scaler.c    |  6 +++--
>>>   drivers/gpu/drm/exynos/exynos_mixer.c         |  7 ++++--
>>>   11 files changed, 47 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> index 8428ae12dfa5..1f79bc2a881e 100644
>>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
>>> @@ -55,6 +55,7 @@ static const char * const decon_clks_name[] = {
>>>   struct decon_context {
>>>   	struct device			*dev;
>>>   	struct drm_device		*drm_dev;
>>> +	void				*dma_priv;
>>>   	struct exynos_drm_crtc		*crtc;
>>>   	struct exynos_drm_plane		planes[WINDOWS_NR];
>>>   	struct exynos_drm_plane_config	configs[WINDOWS_NR];
>>> @@ -644,7 +645,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>>>   
>>>   	decon_clear_channels(ctx->crtc);
>>>   
>>> -	return exynos_drm_register_dma(drm_dev, dev);
>>> +	return exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
>>>   }
>>>   
>>>   static void decon_unbind(struct device *dev, struct device *master, void *data)
>>> @@ -654,7 +655,7 @@ static void decon_unbind(struct device *dev, struct device *master, void *data)
>>>   	decon_atomic_disable(ctx->crtc);
>>>   
>>>   	/* detach this sub driver from iommu mapping if supported. */
>>> -	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
>>> +	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
>>>   }
>>>   
>>>   static const struct component_ops decon_component_ops = {
>>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> index ff59c641fa80..1eed3327999f 100644
>>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> @@ -40,6 +40,7 @@
>>>   struct decon_context {
>>>   	struct device			*dev;
>>>   	struct drm_device		*drm_dev;
>>> +	void				*dma_priv;
>>>   	struct exynos_drm_crtc		*crtc;
>>>   	struct exynos_drm_plane		planes[WINDOWS_NR];
>>>   	struct exynos_drm_plane_config	configs[WINDOWS_NR];
>>> @@ -127,13 +128,13 @@ static int decon_ctx_initialize(struct decon_context *ctx,
>>>   
>>>   	decon_clear_channels(ctx->crtc);
>>>   
>>> -	return exynos_drm_register_dma(drm_dev, ctx->dev);
>>> +	return exynos_drm_register_dma(drm_dev, ctx->dev, &ctx->dma_priv);
>>>   }
>>>   
>>>   static void decon_ctx_remove(struct decon_context *ctx)
>>>   {
>>>   	/* detach this sub driver from iommu mapping if supported. */
>>> -	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
>>> +	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
>>>   }
>>>   
>>>   static u32 decon_calc_clkdiv(struct decon_context *ctx,
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dma.c b/drivers/gpu/drm/exynos/exynos_drm_dma.c
>>> index 9ebc02768847..482bec7756fa 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dma.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dma.c
>>> @@ -58,7 +58,7 @@ static inline void clear_dma_max_seg_size(struct device *dev)
>>>    * mapping.
>>>    */
>>>   static int drm_iommu_attach_device(struct drm_device *drm_dev,
>>> -				struct device *subdrv_dev)
>>> +				struct device *subdrv_dev, void **dma_priv)
>>>   {
>>>   	struct exynos_drm_private *priv = drm_dev->dev_private;
>>>   	int ret;
>>> @@ -74,7 +74,8 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
>>>   		return ret;
>>>   
>>>   	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
>>> -		if (to_dma_iommu_mapping(subdrv_dev))
>>> +		*dma_priv = to_dma_iommu_mapping(subdrv_dev);
>>> +		if (*dma_priv)
>>>   			arm_iommu_detach_device(subdrv_dev);
>>>   
>>>   		ret = arm_iommu_attach_device(subdrv_dev, priv->mapping);
>>> @@ -98,19 +99,21 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
>>>    * mapping
>>>    */
>>>   static void drm_iommu_detach_device(struct drm_device *drm_dev,
>>> -				struct device *subdrv_dev)
>>> +				    struct device *subdrv_dev, void **dma_priv)
>>>   {
>>>   	struct exynos_drm_private *priv = drm_dev->dev_private;
>>>   
>>> -	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))
>>> +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
>>>   		arm_iommu_detach_device(subdrv_dev);
>>> -	else if (IS_ENABLED(CONFIG_IOMMU_DMA))
>>> +		arm_iommu_attach_device(subdrv_dev, *dma_priv);
>> I don't see why arm_iommu_attach_device function should be called again at drm_iommu_detach_device function.
>> I think it should be no problem without arm_iommu_attach_device call.
> 
> If device is not attached again to the mapping created by the DMA 
> framework, it will be later considered as a device without IOMMU.
> 
>> If there is any problem without arm_iommu_attach_device function call then maybe getting wrong somewhere but not here.
> 
> The problem will be during the second initialization of Exynos DRM, 
> mainly if the first initialization is interrupted by deferred probe. 
> This issue would be also visible when Exynos DRM is compiled as module 
> and loaded, removed and loaded again. Sadly, due to some circular 
> dependencies, this is not yet possible without the hacks.

Could you add above your comments on the code so that other developers can understand why the hacks should be used here?

Thanks,
Inki Dae

> 
> Best regards
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel