Re: [PATCH] DRM: add drm gem CMA helper

[Lars-Peter Clausen <lars@metafoo.de>]

On 05/29/2012 04:46 PM, Lars-Peter Clausen wrote:
> On 05/29/2012 04:10 PM, Sascha Hauer wrote:
>> Many embedded drm devices do not have a IOMMU and no dedicated
>> memory for graphics. These devices use CMA (Contiguous Memory
>> Allocator) backed graphics memory. This patch provides helper
>> functions to be able to share the code.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>
>> Lars-Peter, please let me know if this fits your needs or if you are missing
>> something.
>>
> 
> Awesome :) The overall structure looks basically like what I had, so I can
> just use these functions as drop-in replacement. I had some minor
> differences in the implementation though. Comments inline.

Did some initial testing and it seems to work fine :)

> 
>>  drivers/gpu/drm/Kconfig              |    6 +
>>  drivers/gpu/drm/Makefile             |    1 +
>>  drivers/gpu/drm/drm_gem_cma_helper.c |  321 ++++++++++++++++++++++++++++++++++
>>  include/drm/drm_gem_cma_helper.h     |   47 +++++
>>  4 files changed, 375 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>>  create mode 100644 include/drm/drm_gem_cma_helper.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index e354bc0..f62717e 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -53,6 +53,12 @@ config DRM_TTM
>>  	  GPU memory types. Will be enabled automatically if a device driver
>>  	  uses it.
>>  
>> +config DRM_GEM_CMA_HELPER
>> +	tristate
>> +	depends on DRM
>> +	help
>> +	  Choose this if you need the GEM cma helper functions
> 
> This shouldn't have a help text as it should be selected by the driver and
> not by the user. Also the 'depends on DRM' can go away, since it becomes
> meaningless if the symbol is not user-selectable.
> 

Sorry, ignore that. I though it would appear in menuconfig if it had an help
text. But it needs to have a title. The "depends on" can still go away though.

>> +
>> +#define to_dma_alloc_gem_obj(x) \
>> +	container_of(x,	struct drm_gem_cma_obj, base)

This looks like it has been missed during some renaming. It should probably
be "to_drm_gem_cma_obj". And maybe make it an inline function.

Thanks,
- Lars