2019-07-01 18:20:15

by Jeffrey Hugo

[permalink] [raw]
Subject: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

Creating the msm gem address space requires a reference to the dev where
the iommu is located. The driver currently assumes this is the same as
the platform device, which breaks when the iommu is outside of the
platform device. Use the drm_device instead, which happens to always have
a reference to the proper device.

Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 4a60f5fca6b0..1347a5223918 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
mdelay(16);

if (config->platform.iommu) {
- aspace = msm_gem_address_space_create(&pdev->dev,
+ aspace = msm_gem_address_space_create(dev->dev,
config->platform.iommu, "mdp5");
if (IS_ERR(aspace)) {
ret = PTR_ERR(aspace);
--
2.17.1


2019-07-01 19:47:26

by Rob Clark

[permalink] [raw]
Subject: Re: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

On Mon, Jul 1, 2019 at 10:39 AM Jeffrey Hugo <[email protected]> wrote:
>
> Creating the msm gem address space requires a reference to the dev where
> the iommu is located. The driver currently assumes this is the same as
> the platform device, which breaks when the iommu is outside of the
> platform device. Use the drm_device instead, which happens to always have
> a reference to the proper device.
>
> Signed-off-by: Jeffrey Hugo <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 4a60f5fca6b0..1347a5223918 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->platform.iommu) {
> - aspace = msm_gem_address_space_create(&pdev->dev,
> + aspace = msm_gem_address_space_create(dev->dev,
> config->platform.iommu, "mdp5");

hmm, do you have a tree somewhere with your dt files? This makes me
think the display iommu is hooked up somewhere differently compared
to, say, msm8916.dtsi

BR,
-R

> if (IS_ERR(aspace)) {
> ret = PTR_ERR(aspace);
> --
> 2.17.1
>

2019-07-01 20:23:21

by Jeffrey Hugo

[permalink] [raw]
Subject: Re: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

On Mon, Jul 1, 2019 at 1:45 PM Rob Clark <[email protected]> wrote:
>
> On Mon, Jul 1, 2019 at 10:39 AM Jeffrey Hugo <[email protected]> wrote:
> >
> > Creating the msm gem address space requires a reference to the dev where
> > the iommu is located. The driver currently assumes this is the same as
> > the platform device, which breaks when the iommu is outside of the
> > platform device. Use the drm_device instead, which happens to always have
> > a reference to the proper device.
> >
> > Signed-off-by: Jeffrey Hugo <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 4a60f5fca6b0..1347a5223918 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> > mdelay(16);
> >
> > if (config->platform.iommu) {
> > - aspace = msm_gem_address_space_create(&pdev->dev,
> > + aspace = msm_gem_address_space_create(dev->dev,
> > config->platform.iommu, "mdp5");
>
> hmm, do you have a tree somewhere with your dt files? This makes me
> think the display iommu is hooked up somewhere differently compared
> to, say, msm8916.dtsi

I'll post something somewhere and forward it to you.

2019-07-03 04:09:22

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

On Mon 01 Jul 10:39 PDT 2019, Jeffrey Hugo wrote:

> Creating the msm gem address space requires a reference to the dev where
> the iommu is located. The driver currently assumes this is the same as
> the platform device, which breaks when the iommu is outside of the
> platform device. Use the drm_device instead, which happens to always have
> a reference to the proper device.
>
> Signed-off-by: Jeffrey Hugo <[email protected]>

Sorry, but on db820c this patch results in:

[ 64.803263] msm_mdp 901000.mdp: [drm:mdp5_kms_init [msm]] *ERROR* failed to attach iommu: -19

Followed by 3 oopses as we're trying to fail the initialization.

Regards,
Bjorn

> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 4a60f5fca6b0..1347a5223918 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->platform.iommu) {
> - aspace = msm_gem_address_space_create(&pdev->dev,
> + aspace = msm_gem_address_space_create(dev->dev,
> config->platform.iommu, "mdp5");
> if (IS_ERR(aspace)) {
> ret = PTR_ERR(aspace);
> --
> 2.17.1
>

2019-07-03 12:27:04

by Rob Clark

[permalink] [raw]
Subject: Re: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

On Tue, Jul 2, 2019 at 9:08 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Mon 01 Jul 10:39 PDT 2019, Jeffrey Hugo wrote:
>
> > Creating the msm gem address space requires a reference to the dev where
> > the iommu is located. The driver currently assumes this is the same as
> > the platform device, which breaks when the iommu is outside of the
> > platform device. Use the drm_device instead, which happens to always have
> > a reference to the proper device.
> >
> > Signed-off-by: Jeffrey Hugo <[email protected]>
>
> Sorry, but on db820c this patch results in:
>
> [ 64.803263] msm_mdp 901000.mdp: [drm:mdp5_kms_init [msm]] *ERROR* failed to attach iommu: -19
>
> Followed by 3 oopses as we're trying to fail the initialization.

yeah, that is kinda what I suspected would happen. I guess to deal
with how things are hooked up on 8998, perhaps the best thing is to
first try &pdev->dev, and then if that fails try dev->dev

BR,
-R

> Regards,
> Bjorn
>
> > ---
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 4a60f5fca6b0..1347a5223918 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -702,7 +702,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> > mdelay(16);
> >
> > if (config->platform.iommu) {
> > - aspace = msm_gem_address_space_create(&pdev->dev,
> > + aspace = msm_gem_address_space_create(dev->dev,
> > config->platform.iommu, "mdp5");
> > if (IS_ERR(aspace)) {
> > ret = PTR_ERR(aspace);
> > --
> > 2.17.1
> >

2019-07-03 15:12:17

by Jeffrey Hugo

[permalink] [raw]
Subject: Re: [PATCH] drm/msm/mdp5: Use drm_device for creating gem address space

On Wed, Jul 3, 2019 at 6:25 AM Rob Clark <[email protected]> wrote:
>
> On Tue, Jul 2, 2019 at 9:08 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Mon 01 Jul 10:39 PDT 2019, Jeffrey Hugo wrote:
> >
> > > Creating the msm gem address space requires a reference to the dev where
> > > the iommu is located. The driver currently assumes this is the same as
> > > the platform device, which breaks when the iommu is outside of the
> > > platform device. Use the drm_device instead, which happens to always have
> > > a reference to the proper device.
> > >
> > > Signed-off-by: Jeffrey Hugo <[email protected]>
> >
> > Sorry, but on db820c this patch results in:
> >
> > [ 64.803263] msm_mdp 901000.mdp: [drm:mdp5_kms_init [msm]] *ERROR* failed to attach iommu: -19
> >
> > Followed by 3 oopses as we're trying to fail the initialization.
>
> yeah, that is kinda what I suspected would happen. I guess to deal
> with how things are hooked up on 8998, perhaps the best thing is to
> first try &pdev->dev, and then if that fails try dev->dev

Thanks for the test feedback Bjorn. Its unfortunate this solution
didn't work as I expected. I'll give Rob's suggestion a shot and spin
another version.