On Sun, Jul 6, 2025 at 7:02 AM Dmitry Baryshkov
<dmitry.barysh...@oss.qualcomm.com> wrote:
>
> On Sun, 6 Jul 2025 at 16:11, Rob Clark <rob.cl...@oss.qualcomm.com> wrote:
> >
> > On Sun, Jul 6, 2025 at 3:50 AM Dmitry Baryshkov
> > <dmitry.barysh...@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.barysh...@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
