[PATCH 2/2] drm/cma-helper: Implement mmap as GEM CMA object functions

[Kieran Bingham]

Hi Thomas,

On 14/01/2021 15:15, Thomas Zimmermann wrote:
>>>> On 23/11/2020 11:56, Thomas Zimmermann wrote:
>>>>> The new GEM object function drm_gem_cma_mmap() sets the VMA flags
>>>>> and offset as in the old implementation and immediately maps in the
>>>>> buffer's memory pages.
>>>>>
>>>>> Changing CMA helpers to use the GEM object function allows for the
>>>>> removal of the special implementations for mmap and gem_prime_mmap
>>>>> callbacks. The regular functions drm_gem_mmap() and
>>>>> drm_gem_prime_mmap()
>>>>> are now used.
>>>>
>>>> I've encountered a memory leak regression in our Renesas R-Car DU
>>>> tests,
>>>> and git bisection has led me to this patch (as commit f5ca8eb6f9).
>>>>
>>>> Running the tests sequentially, while grepping /proc/meminfo for
>>>> Cma, it
>>>> is evident that CMA memory is not released, until exhausted and the
>>>> allocations fail (seen in [0]) shown by the error report:
>>>>
>>>>>       self.fbs.append(pykms.DumbFramebuffer(self.card, mode.hdisplay,
>>>>> mode.vdisplay, "XR24"))
>>>>> ValueError: DRM_IOCTL_MODE_CREATE_DUMB failed: Cannot allocate memory
>>>>
>>>>
>>>> Failing tests at f5ca8eb6f9 can be seen at [0], while the tests pass
>>>> successfully [1] on the commit previous to that (bc2532ab7c2):
>>>>
>>>> Reverting f5ca8eb6f9 also produces a successful pass [2]
>>>>
>>>>    [0] https://paste.ubuntu.com/p/VjPGPgswxR/ # Failed at f5ca8eb6f9
>>>>    [1] https://paste.ubuntu.com/p/78RRp2WpNR/ # Success at bc2532ab7c2
>>>>    [2] https://paste.ubuntu.com/p/qJKjZZN2pt/ # Success with revert
>>>>
>>>>
>>>> I don't believe we handle mmap specially in the RCar-DU driver, so I
>>>> wonder if this issue has hit anyone else as well?
>>>>
>>>> Any ideas of a repair without a revert ? Or do we just need to submit a
>>>> revert?
>>>
>>> I think we might not be setting the VMA ops and therefore not finalize
>>> the BO correctly. Could you please apply the attched (quick-and-dirty)
>>> patch and try again?
>>
>> Thanks for the quick response.
>>
>> I can confirm the quick-and-dirty patch resolves the issue:
>>    https://paste.ubuntu.com/p/sKDp3dNvwV/
>>
>> You can add a
>> Tested-by: Kieran Bingham <kieran.bingham+renesas at ideasonboard.com>
> 
> Great! If you don't mind, I'd also add you in the Reported-by tag.

Certainly!

>>
>> if it stays like that, but I suspect there might be a better place to
>> initialise the ops rather than in the mmap call itself.
> 
> I think that's the fix, basically. We could put such a line as a
> fall-back somewhere into the DRM core code. But I don't know if this
> really works with all drivers. Maybe there's one that requires vm_ops to
> be NULL.

Ok, that's reaching beyond code I've explored, so I'll leave it to you.


> Thanks for reporting this issue and testing quickly.

Thanks for fixing so quickly :-)

Regards

Kieran