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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxx>
>> ---
>>  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: signature.asc
Description: OpenPGP digital signature