Re: [PATCH 3/3] drm/msm/mdp4: use msm_kms_init_vm() instead of duplicating it

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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