Re: [PATCH 19/20] drm/xlnx: Initialize DRM driver instance with CMA helper macro

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3004 bytes --]

Hi

Am 13.08.20 um 15:36 schrieb Laurent Pinchart:
> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Thu, Aug 13, 2020 at 10:36:43AM +0200, Thomas Zimmermann wrote:
>> The xlnx driver uses CMA helpers with default callback functions.
>> Initialize the driver structure with the rsp CMA helper macro. The
>> driver is being converted to use GEM object functions as part of
>> this change.
>>
>> Two callbacks, .dumb_destroy and .gem_prime_import, were initialized
>> to their default implementations, so they are just kept empty now.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 14 +-------------
>>  1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> index 26328c76305b..058044dcc062 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> @@ -80,19 +80,7 @@ static struct drm_driver zynqmp_dpsub_drm_driver = {
>>  	.driver_features		= DRIVER_MODESET | DRIVER_GEM |
>>  					  DRIVER_ATOMIC,
>>  
>> -	.prime_handle_to_fd		= drm_gem_prime_handle_to_fd,
>> -	.prime_fd_to_handle		= drm_gem_prime_fd_to_handle,
>> -	.gem_prime_export		= drm_gem_prime_export,
>> -	.gem_prime_import		= drm_gem_prime_import,
>> -	.gem_prime_get_sg_table		= drm_gem_cma_prime_get_sg_table,
>> -	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
>> -	.gem_prime_vmap			= drm_gem_cma_prime_vmap,
>> -	.gem_prime_vunmap		= drm_gem_cma_prime_vunmap,
>> -	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
>> -	.gem_free_object_unlocked	= drm_gem_cma_free_object,
>> -	.gem_vm_ops			= &drm_gem_cma_vm_ops,
>> -	.dumb_create			= zynqmp_dpsub_dumb_create,
>> -	.dumb_destroy			= drm_gem_dumb_destroy,
>> +	DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),
> 
> The only effective change here is
> 
> -	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table,
> -	.gem_prime_mmap			= drm_gem_cma_prime_mmap,
> +	.gem_prime_import_sg_table	= drm_gem_cma_prime_import_sg_table_vmap,
> +	.gem_prime_mmap			= drm_gem_prime_mmap,
> 
> The change is significant, and I have a hard time following the code to
> verify that it's correct, or if it's an undesired side effect. If it's
> correct, could the change be mentioned in the commit message, with at
> least a brief explanation of why this is correct, and what the
> consequences here ?

I think this is a mistake that I didn't notice. Thanks for spotting it.
Initializing the driver structure
DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE should fix it. I'll change this
for the patch's next revision.

Best regards
Thomas

> 
>>  
>>  	.fops				= &zynqmp_dpsub_drm_fops,
>>  
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]