Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Mark Zhang <nvmarkzhang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 02/18/2013 03:01 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:40 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>>>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>>>> Add support for the B and C planes which support RGB and YUV pixel
>>>>> formats and can be used as overlays or hardware cursor. Currently
>>>>> only 32-bit RGBA pixel formats are advertised.
>>>>>
>>>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>>> [...]
>>>>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
>>>>> +{
>>>>> +	unsigned int i;
>>>>> +	int err = 0;
>>>>> +
>>>>> +	for (i = 0; i < 2; i++) {
>>>>> +		struct tegra_plane *plane;
>>>>> +
>>>>> +		plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>>>>
>>>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>>>> disable or crtc disable, we should free "struct tegra_plane" which is
>>>> allocated here. But the memory which devm_kzalloc allocates is only
>>>> freed when the driver detach. This makes lots of memory can't be
>>>> recycled when the plane enable/disable frequently.
>>>
>>> Why would we want to do that? I don't think doing so would even work,
>>> since the planes are resources that need to be registered with the DRM
>>> core and therefore can't be allocated on-demand.
>>>
>>
>> I'm wondering if we add power management codes in the future, the
>> crtc/plane will be disabled/enabled frequently, e.g: system going to
>> suspend state then resume.
> 
> In that case it makes even less sense to allocate and deallocate the
> plane every time around. However, any optimizations aside, the allocated
> memory is passed into the core via drm_plane_init(), which registers
> that plane with the given CRTC. So if we deallocate the memory when the
> plane when it is disabled, we'd have to make sure to remove it from the
> core as well. However that would also mean that it disappears from the
> list of planes supported by the CRTC and therefore we wouldn't be able
> to use it again.
> 

Alright, I mixed up the "disable" & "remove" of planes and crtcs. You're
right. Just forget what I said in this thread.

Mark
> Thierry
>