Use the msm_kms_init_vm() function to allocate memory manager instead of
hand-coding a copy of it. Although MDP4 platforms don't have MDSS
device, it's still safe to use the function as all MDP4 devices have
IOMMU and the parent of the MDP4 is the root SoC device.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 27 +++++----------------------
1 file changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 88296c41d1a5eb0e16cb6ec4d0475000b6318c4e..41d236d30e71ebb6ac8a59052529f36fadf15cd7 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -391,11 +391,9 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
static int mdp4_kms_init(struct drm_device *dev)
{
- struct platform_device *pdev = to_platform_device(dev->dev);
struct msm_drm_private *priv = dev->dev_private;
struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
struct msm_kms *kms = NULL;
- struct msm_mmu *mmu;
struct drm_gpuvm *vm;
int ret;
u32 major, minor;
@@ -458,29 +456,14 @@ static int mdp4_kms_init(struct drm_device *dev)
mdp4_disable(mdp4_kms);
mdelay(16);
- mmu = msm_iommu_new(&pdev->dev, 0);
- if (IS_ERR(mmu)) {
- ret = PTR_ERR(mmu);
+ vm = msm_kms_init_vm(mdp4_kms->dev);
+ if (IS_ERR(vm)) {
+ ret = PTR_ERR(vm);
goto fail;
- } else if (!mmu) {
- DRM_DEV_INFO(dev->dev, "no IOMMU configuration is no longer supported\n");
- ret = -ENODEV;
- goto fail;
- } else {
- vm = msm_gem_vm_create(dev, mmu, "mdp4",
- 0x1000, 0x100000000 - 0x1000,
- true);
-
- if (IS_ERR(vm)) {
- if (!IS_ERR(mmu))
- mmu->funcs->destroy(mmu);
- ret = PTR_ERR(vm);
- goto fail;
- }
-
- kms->vm = vm;
}
+ kms->vm = vm;
+
ret = modeset_init(mdp4_kms);
if (ret) {
DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
--
2.39.5On Sun, Jul 6, 2025 at 3:50 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> Use the msm_kms_init_vm() function to allocate memory manager instead of
> hand-coding a copy of it. Although MDP4 platforms don't have MDSS
> device, it's still safe to use the function as all MDP4 devices have
> IOMMU and the parent of the MDP4 is the root SoC device.
So, originally the distinction was that mdp4 didn't have the mdss
wrapper. Maybe it works out because device_iommu_mapped(mdp_dev)
returns true?
BR,
-R
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 27 +++++----------------------
> 1 file changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 88296c41d1a5eb0e16cb6ec4d0475000b6318c4e..41d236d30e71ebb6ac8a59052529f36fadf15cd7 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -391,11 +391,9 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
>
> static int mdp4_kms_init(struct drm_device *dev)
> {
> - struct platform_device *pdev = to_platform_device(dev->dev);
> struct msm_drm_private *priv = dev->dev_private;
> struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
> struct msm_kms *kms = NULL;
> - struct msm_mmu *mmu;
> struct drm_gpuvm *vm;
> int ret;
> u32 major, minor;
> @@ -458,29 +456,14 @@ static int mdp4_kms_init(struct drm_device *dev)
> mdp4_disable(mdp4_kms);
> mdelay(16);
>
> - mmu = msm_iommu_new(&pdev->dev, 0);
> - if (IS_ERR(mmu)) {
> - ret = PTR_ERR(mmu);
> + vm = msm_kms_init_vm(mdp4_kms->dev);
> + if (IS_ERR(vm)) {
> + ret = PTR_ERR(vm);
> goto fail;
> - } else if (!mmu) {
> - DRM_DEV_INFO(dev->dev, "no IOMMU configuration is no longer supported\n");
> - ret = -ENODEV;
> - goto fail;
> - } else {
> - vm = msm_gem_vm_create(dev, mmu, "mdp4",
> - 0x1000, 0x100000000 - 0x1000,
> - true);
> -
> - if (IS_ERR(vm)) {
> - if (!IS_ERR(mmu))
> - mmu->funcs->destroy(mmu);
> - ret = PTR_ERR(vm);
> - goto fail;
> - }
> -
> - kms->vm = vm;
> }
>
> + kms->vm = vm;
> +
> ret = modeset_init(mdp4_kms);
> if (ret) {
> DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
>
> --
> 2.39.5
>
On Sun, 6 Jul 2025 at 16:11, Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>
> On Sun, Jul 6, 2025 at 3:50 AM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > Use the msm_kms_init_vm() function to allocate memory manager instead of
> > hand-coding a copy of it. Although MDP4 platforms don't have MDSS
> > device, it's still safe to use the function as all MDP4 devices have
> > IOMMU and the parent of the MDP4 is the root SoC device.
>
> So, originally the distinction was that mdp4 didn't have the mdss
> wrapper. Maybe it works out because device_iommu_mapped(mdp_dev)
> returns true?
Yes, as expressed in the cover letter.
>
> BR,
> -R
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 27 +++++----------------------
> > 1 file changed, 5 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > index 88296c41d1a5eb0e16cb6ec4d0475000b6318c4e..41d236d30e71ebb6ac8a59052529f36fadf15cd7 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > @@ -391,11 +391,9 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
> >
> > static int mdp4_kms_init(struct drm_device *dev)
> > {
> > - struct platform_device *pdev = to_platform_device(dev->dev);
> > struct msm_drm_private *priv = dev->dev_private;
> > struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
> > struct msm_kms *kms = NULL;
> > - struct msm_mmu *mmu;
> > struct drm_gpuvm *vm;
> > int ret;
> > u32 major, minor;
> > @@ -458,29 +456,14 @@ static int mdp4_kms_init(struct drm_device *dev)
> > mdp4_disable(mdp4_kms);
> > mdelay(16);
> >
> > - mmu = msm_iommu_new(&pdev->dev, 0);
> > - if (IS_ERR(mmu)) {
> > - ret = PTR_ERR(mmu);
> > + vm = msm_kms_init_vm(mdp4_kms->dev);
> > + if (IS_ERR(vm)) {
> > + ret = PTR_ERR(vm);
> > goto fail;
> > - } else if (!mmu) {
> > - DRM_DEV_INFO(dev->dev, "no IOMMU configuration is no longer supported\n");
> > - ret = -ENODEV;
> > - goto fail;
> > - } else {
> > - vm = msm_gem_vm_create(dev, mmu, "mdp4",
> > - 0x1000, 0x100000000 - 0x1000,
> > - true);
> > -
> > - if (IS_ERR(vm)) {
> > - if (!IS_ERR(mmu))
> > - mmu->funcs->destroy(mmu);
> > - ret = PTR_ERR(vm);
> > - goto fail;
> > - }
> > -
> > - kms->vm = vm;
> > }
> >
> > + kms->vm = vm;
> > +
> > ret = modeset_init(mdp4_kms);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
> >
> > --
> > 2.39.5
> >
--
With best wishes
Dmitry
On Sun, Jul 6, 2025 at 7:02 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Sun, 6 Jul 2025 at 16:11, Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >
> > On Sun, Jul 6, 2025 at 3:50 AM Dmitry Baryshkov
> > <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > >
> > > Use the msm_kms_init_vm() function to allocate memory manager instead of
> > > hand-coding a copy of it. Although MDP4 platforms don't have MDSS
> > > device, it's still safe to use the function as all MDP4 devices have
> > > IOMMU and the parent of the MDP4 is the root SoC device.
> >
> > So, originally the distinction was that mdp4 didn't have the mdss
> > wrapper. Maybe it works out because device_iommu_mapped(mdp_dev)
> > returns true?
>
> Yes, as expressed in the cover letter.
Right, but with this patch, I think nothing is enforcing that, so we
could end up dereferencing mdp_dev->parent if the device did not have
an iommu.
I guess you could solve that with an extra device_iommu_mapped() in
mdp4_kms_init()
BR,
-R
> >
> > BR,
> > -R
> >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > ---
> > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 27 +++++----------------------
> > > 1 file changed, 5 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > > index 88296c41d1a5eb0e16cb6ec4d0475000b6318c4e..41d236d30e71ebb6ac8a59052529f36fadf15cd7 100644
> > > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > > @@ -391,11 +391,9 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
> > >
> > > static int mdp4_kms_init(struct drm_device *dev)
> > > {
> > > - struct platform_device *pdev = to_platform_device(dev->dev);
> > > struct msm_drm_private *priv = dev->dev_private;
> > > struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
> > > struct msm_kms *kms = NULL;
> > > - struct msm_mmu *mmu;
> > > struct drm_gpuvm *vm;
> > > int ret;
> > > u32 major, minor;
> > > @@ -458,29 +456,14 @@ static int mdp4_kms_init(struct drm_device *dev)
> > > mdp4_disable(mdp4_kms);
> > > mdelay(16);
> > >
> > > - mmu = msm_iommu_new(&pdev->dev, 0);
> > > - if (IS_ERR(mmu)) {
> > > - ret = PTR_ERR(mmu);
> > > + vm = msm_kms_init_vm(mdp4_kms->dev);
> > > + if (IS_ERR(vm)) {
> > > + ret = PTR_ERR(vm);
> > > goto fail;
> > > - } else if (!mmu) {
> > > - DRM_DEV_INFO(dev->dev, "no IOMMU configuration is no longer supported\n");
> > > - ret = -ENODEV;
> > > - goto fail;
> > > - } else {
> > > - vm = msm_gem_vm_create(dev, mmu, "mdp4",
> > > - 0x1000, 0x100000000 - 0x1000,
> > > - true);
> > > -
> > > - if (IS_ERR(vm)) {
> > > - if (!IS_ERR(mmu))
> > > - mmu->funcs->destroy(mmu);
> > > - ret = PTR_ERR(vm);
> > > - goto fail;
> > > - }
> > > -
> > > - kms->vm = vm;
> > > }
> > >
> > > + kms->vm = vm;
> > > +
> > > ret = modeset_init(mdp4_kms);
> > > if (ret) {
> > > DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
> > >
> > > --
> > > 2.39.5
> > >
>
>
>
> --
> With best wishes
> Dmitry
On 06/07/2025 18:11, Rob Clark wrote:
> On Sun, Jul 6, 2025 at 7:02 AM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
>>
>> On Sun, 6 Jul 2025 at 16:11, Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>>>
>>> On Sun, Jul 6, 2025 at 3:50 AM Dmitry Baryshkov
>>> <dmitry.baryshkov@oss.qualcomm.com> wrote:
>>>>
>>>> Use the msm_kms_init_vm() function to allocate memory manager instead of
>>>> hand-coding a copy of it. Although MDP4 platforms don't have MDSS
>>>> device, it's still safe to use the function as all MDP4 devices have
>>>> IOMMU and the parent of the MDP4 is the root SoC device.
>>>
>>> So, originally the distinction was that mdp4 didn't have the mdss
>>> wrapper. Maybe it works out because device_iommu_mapped(mdp_dev)
>>> returns true?
>>
>> Yes, as expressed in the cover letter.
>
> Right, but with this patch, I think nothing is enforcing that, so we
> could end up dereferencing mdp_dev->parent if the device did not have
> an iommu.
>
> I guess you could solve that with an extra device_iommu_mapped() in
> mdp4_kms_init()
... or adding have_mdss arg to msm_kms_init_vm().
Anyway, let's probably get first two patches in, I'll repost the third
patch later on.
>
> BR,
> -R
>
>>>
>>> BR,
>>> -R
>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 27 +++++----------------------
>>>> 1 file changed, 5 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>>> index 88296c41d1a5eb0e16cb6ec4d0475000b6318c4e..41d236d30e71ebb6ac8a59052529f36fadf15cd7 100644
>>>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>>> @@ -391,11 +391,9 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
>>>>
>>>> static int mdp4_kms_init(struct drm_device *dev)
>>>> {
>>>> - struct platform_device *pdev = to_platform_device(dev->dev);
>>>> struct msm_drm_private *priv = dev->dev_private;
>>>> struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
>>>> struct msm_kms *kms = NULL;
>>>> - struct msm_mmu *mmu;
>>>> struct drm_gpuvm *vm;
>>>> int ret;
>>>> u32 major, minor;
>>>> @@ -458,29 +456,14 @@ static int mdp4_kms_init(struct drm_device *dev)
>>>> mdp4_disable(mdp4_kms);
>>>> mdelay(16);
>>>>
>>>> - mmu = msm_iommu_new(&pdev->dev, 0);
>>>> - if (IS_ERR(mmu)) {
>>>> - ret = PTR_ERR(mmu);
>>>> + vm = msm_kms_init_vm(mdp4_kms->dev);
>>>> + if (IS_ERR(vm)) {
>>>> + ret = PTR_ERR(vm);
>>>> goto fail;
>>>> - } else if (!mmu) {
>>>> - DRM_DEV_INFO(dev->dev, "no IOMMU configuration is no longer supported\n");
>>>> - ret = -ENODEV;
>>>> - goto fail;
>>>> - } else {
>>>> - vm = msm_gem_vm_create(dev, mmu, "mdp4",
>>>> - 0x1000, 0x100000000 - 0x1000,
>>>> - true);
>>>> -
>>>> - if (IS_ERR(vm)) {
>>>> - if (!IS_ERR(mmu))
>>>> - mmu->funcs->destroy(mmu);
>>>> - ret = PTR_ERR(vm);
>>>> - goto fail;
>>>> - }
>>>> -
>>>> - kms->vm = vm;
>>>> }
>>>>
>>>> + kms->vm = vm;
>>>> +
>>>> ret = modeset_init(mdp4_kms);
>>>> if (ret) {
>>>> DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
>>>>
>>>> --
>>>> 2.39.5
>>>>
>>
>>
>>
>> --
>> With best wishes
>> Dmitry
--
With best wishes
Dmitry
© 2016 - 2025 Red Hat, Inc.