| Message ID | 20220419130422.1033699-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm: Revert "drm/msm: Stop using iommu_present()" | expand |
On 2022-04-19 14:04, Dmitry Baryshkov wrote: > This reverts commit e2a88eabb02410267519b838fb9b79f5206769be. The commit > in question makes msm_use_mmu() check whether the DRM 'component master' > device is translated by the IOMMU. At this moment it is the 'mdss' > device. > However on platforms using the MDP5 driver (e.g. MSM8916/APQ8016, > MSM8996/APQ8096) it's the mdp5 device, which has the iommus property > (and thus is "translated by the IOMMU"). This results in these devices > being broken with the following lines in the dmesg. > > [drm] Initialized msm 1.9.0 20130625 for 1a00000.mdss on minor 0 > msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pm4.fw from new location > msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pfp.fw from new location > msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 > msm 1a00000.mdss: could not allocate stolen bo > msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 > msm 1a00000.mdss: [drm:msm_alloc_stolen_fb] *ERROR* failed to allocate buffer object > msm 1a00000.mdss: [drm:msm_fbdev_create] *ERROR* failed to allocate fb > > Getting the mdp5 device pointer from this function is not that easy at > this moment. Thus this patch is reverted till the MDSS rework [1] lands. > It will make the mdp5/dpu1 device component master and the check will be > legit. > > [1] https://patchwork.freedesktop.org/series/98525/ Oh, DRM... If that series is going to land got 5.19, could you please implement the correct equivalent of this patch within it? I'm fine with the revert for now if this patch doesn't work properly in all cases, but I have very little sympathy left for DRM drivers riding roughshod over all the standard driver model abstractions because they're "special". iommu_present() *needs* to go away, so if it's left to me to have a second go at fixing this driver next cycle, you're liable to get some abomination based on of_find_compatible_node() or similar, and I'll probably be demanding an ack to take it through the IOMMU tree ;) Thanks, Robin. > Fixes: e2a88eabb024 ("drm/msm: Stop using iommu_present()") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/msm_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index b6702b0fafcb..e2b5307b2360 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -263,7 +263,7 @@ bool msm_use_mmu(struct drm_device *dev) > struct msm_drm_private *priv = dev->dev_private; > > /* a2xx comes with its own MMU */ > - return priv->is_a2xx || device_iommu_mapped(dev->dev); > + return priv->is_a2xx || iommu_present(&platform_bus_type); > } > > static int msm_init_vram(struct drm_device *dev)
On 20/04/2022 00:04, Robin Murphy wrote: > On 2022-04-19 14:04, Dmitry Baryshkov wrote: >> This reverts commit e2a88eabb02410267519b838fb9b79f5206769be. The commit >> in question makes msm_use_mmu() check whether the DRM 'component master' >> device is translated by the IOMMU. At this moment it is the 'mdss' >> device. >> However on platforms using the MDP5 driver (e.g. MSM8916/APQ8016, >> MSM8996/APQ8096) it's the mdp5 device, which has the iommus property >> (and thus is "translated by the IOMMU"). This results in these devices >> being broken with the following lines in the dmesg. >> >> [drm] Initialized msm 1.9.0 20130625 for 1a00000.mdss on minor 0 >> msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pm4.fw from >> new location >> msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pfp.fw from >> new location >> msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 >> msm 1a00000.mdss: could not allocate stolen bo >> msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 >> msm 1a00000.mdss: [drm:msm_alloc_stolen_fb] *ERROR* failed to allocate >> buffer object >> msm 1a00000.mdss: [drm:msm_fbdev_create] *ERROR* failed to allocate fb >> >> Getting the mdp5 device pointer from this function is not that easy at >> this moment. Thus this patch is reverted till the MDSS rework [1] lands. >> It will make the mdp5/dpu1 device component master and the check will be >> legit. >> >> [1] https://patchwork.freedesktop.org/series/98525/ > > Oh, DRM... > > If that series is going to land got 5.19, could you please implement the > correct equivalent of this patch within it? Yes, that's the plan. I'm sending a reworked version of your patch shortly (but it still depends on [1]). > > I'm fine with the revert for now if this patch doesn't work properly in > all cases, but I have very little sympathy left for DRM drivers riding > roughshod over all the standard driver model abstractions because > they're "special". iommu_present() *needs* to go away, so if it's left > to me to have a second go at fixing this driver next cycle, you're > liable to get some abomination based on of_find_compatible_node() or > similar, and I'll probably be demanding an ack to take it through the > IOMMU tree ;) No need for such measures :-) > > Thanks, > Robin. > >> Fixes: e2a88eabb024 ("drm/msm: Stop using iommu_present()") >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/msm_drv.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_drv.c >> b/drivers/gpu/drm/msm/msm_drv.c >> index b6702b0fafcb..e2b5307b2360 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.c >> +++ b/drivers/gpu/drm/msm/msm_drv.c >> @@ -263,7 +263,7 @@ bool msm_use_mmu(struct drm_device *dev) >> struct msm_drm_private *priv = dev->dev_private; >> /* a2xx comes with its own MMU */ >> - return priv->is_a2xx || device_iommu_mapped(dev->dev); >> + return priv->is_a2xx || iommu_present(&platform_bus_type); >> } >> static int msm_init_vram(struct drm_device *dev)
On 2022-04-19 22:08, Dmitry Baryshkov wrote: > On 20/04/2022 00:04, Robin Murphy wrote: >> On 2022-04-19 14:04, Dmitry Baryshkov wrote: >>> This reverts commit e2a88eabb02410267519b838fb9b79f5206769be. The commit >>> in question makes msm_use_mmu() check whether the DRM 'component master' >>> device is translated by the IOMMU. At this moment it is the 'mdss' >>> device. >>> However on platforms using the MDP5 driver (e.g. MSM8916/APQ8016, >>> MSM8996/APQ8096) it's the mdp5 device, which has the iommus property >>> (and thus is "translated by the IOMMU"). This results in these devices >>> being broken with the following lines in the dmesg. >>> >>> [drm] Initialized msm 1.9.0 20130625 for 1a00000.mdss on minor 0 >>> msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pm4.fw >>> from new location >>> msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pfp.fw >>> from new location >>> msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 >>> msm 1a00000.mdss: could not allocate stolen bo >>> msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 >>> msm 1a00000.mdss: [drm:msm_alloc_stolen_fb] *ERROR* failed to >>> allocate buffer object >>> msm 1a00000.mdss: [drm:msm_fbdev_create] *ERROR* failed to allocate fb >>> >>> Getting the mdp5 device pointer from this function is not that easy at >>> this moment. Thus this patch is reverted till the MDSS rework [1] lands. >>> It will make the mdp5/dpu1 device component master and the check will be >>> legit. >>> >>> [1] https://patchwork.freedesktop.org/series/98525/ >> >> Oh, DRM... >> >> If that series is going to land got 5.19, could you please implement >> the correct equivalent of this patch within it? > > Yes, that's the plan. I'm sending a reworked version of your patch > shortly (but it still depends on [1]). > >> >> I'm fine with the revert for now if this patch doesn't work properly >> in all cases, but I have very little sympathy left for DRM drivers >> riding roughshod over all the standard driver model abstractions >> because they're "special". iommu_present() *needs* to go away, so if >> it's left to me to have a second go at fixing this driver next cycle, >> you're liable to get some abomination based on >> of_find_compatible_node() or similar, and I'll probably be demanding >> an ack to take it through the IOMMU tree ;) > > No need for such measures :-) Awesome, thanks! Robin.
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index b6702b0fafcb..e2b5307b2360 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -263,7 +263,7 @@ bool msm_use_mmu(struct drm_device *dev) struct msm_drm_private *priv = dev->dev_private; /* a2xx comes with its own MMU */ - return priv->is_a2xx || device_iommu_mapped(dev->dev); + return priv->is_a2xx || iommu_present(&platform_bus_type); } static int msm_init_vram(struct drm_device *dev)
This reverts commit e2a88eabb02410267519b838fb9b79f5206769be. The commit in question makes msm_use_mmu() check whether the DRM 'component master' device is translated by the IOMMU. At this moment it is the 'mdss' device. However on platforms using the MDP5 driver (e.g. MSM8916/APQ8016, MSM8996/APQ8096) it's the mdp5 device, which has the iommus property (and thus is "translated by the IOMMU"). This results in these devices being broken with the following lines in the dmesg. [drm] Initialized msm 1.9.0 20130625 for 1a00000.mdss on minor 0 msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pm4.fw from new location msm 1a00000.mdss: [drm:adreno_request_fw] loaded qcom/a300_pfp.fw from new location msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 msm 1a00000.mdss: could not allocate stolen bo msm 1a00000.mdss: [drm:get_pages] *ERROR* could not get pages: -28 msm 1a00000.mdss: [drm:msm_alloc_stolen_fb] *ERROR* failed to allocate buffer object msm 1a00000.mdss: [drm:msm_fbdev_create] *ERROR* failed to allocate fb Getting the mdp5 device pointer from this function is not that easy at this moment. Thus this patch is reverted till the MDSS rework [1] lands. It will make the mdp5/dpu1 device component master and the check will be legit. [1] https://patchwork.freedesktop.org/series/98525/ Fixes: e2a88eabb024 ("drm/msm: Stop using iommu_present()") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/msm_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)