Re: [PATCH v2 1/2] drm: Add DRM_GEM_FOPS

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 07.06.22 um 16:58 schrieb Rob Clark:
> On Mon, Jun 6, 2022 at 11:56 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 06.06.22 um 21:54 schrieb Rob Clark:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> The DEFINE_DRM_GEM_FOPS() helper is a bit limiting if a driver wants to
>>> provide additional file ops, like show_fdinfo().
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    include/drm/drm_gem.h | 26 ++++++++++++++++++--------
>>>    1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index 9d7c61a122dc..dc88d4a2cdf6 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -314,6 +314,23 @@ struct drm_gem_object {
>>>        const struct drm_gem_object_funcs *funcs;
>>>    };
>>>
>>> +/**
>>> + * DRM_GEM_FOPS - Default drm GEM file operations
>>> + *
>>> + * This macro provides a shorthand for setting the GEM file ops in the
>>> + * &file_operations structure.
>>
>> I would appreciate a reference to DEFINE_DRM_GEM_FOPS. Something along
>> the lines of 'if all you need are the default ops, use DEFINE_DRM_GEM_FOPS'.
>>
>>> + */
>>> +#define DRM_GEM_FOPS \
>>> +     .open           = drm_open,\
>>> +     .release        = drm_release,\
>>> +     .unlocked_ioctl = drm_ioctl,\
>>> +     .compat_ioctl   = drm_compat_ioctl,\
>>> +     .poll           = drm_poll,\
>>> +     .read           = drm_read,\
>>> +     .llseek         = noop_llseek,\
>>> +     .mmap           = drm_gem_mmap
>>> +
>>> +
>>
>> Only one empty line please.
>>
>>>    /**
>>>     * DEFINE_DRM_GEM_FOPS() - macro to generate file operations for GEM drivers
>>>     * @name: name for the generated structure
>>> @@ -330,14 +347,7 @@ struct drm_gem_object {
>>>    #define DEFINE_DRM_GEM_FOPS(name) \
>>>        static const struct file_operations name = {\
>>>                .owner          = THIS_MODULE,\
>>
>> Is there a specific reason why .owner is still set here? I suspect that
>> DRM_GEM_FOPS is strictly for callback functions?
> 
> I was on the fence about that one, but it seemed better to not mix
> "magic" and the callbacks.. but I could be convinced in either
> direction

I think you made the right choice. It's cleaner and most drivers will 
want to use DEFINE_DRM_GEM_FOPS, which includes the magic.

Best regards
Thomas

> 
>> In any case
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> thx, I'll fixup the other nits in v3.
> 
>>
>> Best regards
>> Thomas
>>
>>> -             .open           = drm_open,\
>>> -             .release        = drm_release,\
>>> -             .unlocked_ioctl = drm_ioctl,\
>>> -             .compat_ioctl   = drm_compat_ioctl,\
>>> -             .poll           = drm_poll,\
>>> -             .read           = drm_read,\
>>> -             .llseek         = noop_llseek,\
>>> -             .mmap           = drm_gem_mmap,\
>>> +             DRM_GEM_FOPS,\
>>>        }
>>>
>>>    void drm_gem_object_release(struct drm_gem_object *obj);
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev

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

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