[PATCH] drm/ast: Inline drm_simple_encoder_init()

[Thomas Zimmermann]

Hi

Am 26.06.24 um 11:19 schrieb Dmitry Baryshkov:
> On Wed, 26 Jun 2024 at 12:19, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>> Hi
>>
>> Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
>>> On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
>>>>> On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
>>>>>> The function drm_simple_encoder_init() is a trivial helper and
>>>>>> deprecated. Replace it with the regular call to drm_encoder_init().
>>>>>> Resolves the dependency on drm_simple_kms_helper.h. No functional
>>>>>> changes.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>>>> ---
>>>>>>     drivers/gpu/drm/ast/ast_mode.c | 45 ++++++++++++++++++++++++++++++----
>>>>>>     1 file changed, 40 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>>>>>> index 6695af70768f..2fd9c78eab73 100644
>>>>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>>>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>>>>> @@ -45,7 +45,6 @@
>>>>>>     #include <drm/drm_managed.h>
>>>>>>     #include <drm/drm_panic.h>
>>>>>>     #include <drm/drm_probe_helper.h>
>>>>>> -#include <drm/drm_simple_kms_helper.h>
>>>>>>     #include "ast_ddc.h"
>>>>>>     #include "ast_drv.h"
>>>>>> @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
>>>>>>             return 0;
>>>>>>     }
>>>>>> +/*
>>>>>> + * VGA Encoder
>>>>>> + */
>>>>>> +
>>>>>> +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
>>>>>> +  .destroy = drm_encoder_cleanup,
>>>>>> +};
>>>>>> +
>>>>>>     /*
>>>>>>      * VGA Connector
>>>>>>      */
>>>>>> @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
>>>>>>             struct drm_connector *connector = &ast->output.vga.connector;
>>>>>>             int ret;
>>>>>> -  ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
>>>>>> +  ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
>>>>>> +                         DRM_MODE_ENCODER_DAC, NULL);
>>>>> What about using drmm_encoder_init() instead? It will call
>>>>> drm_encoder_cleanup automatically.
>>>> IIRC the original use case for the drmm_encoder_*() funcs was to solve
>>>> problems with the clean-up order if the encoder was added dynamically. The
>>>> hardware for ast is entirely static and ast uses drmm_mode_config_init() for
>>>> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
>>>> like a bit of wasted resources for no gain.
>>> I'd say it's qui pro quo. We are wasting resources on drmm handling, but
>>> then keep it by dropping all drm_encoder_funcs instances.
>> With drm_encoder_init() there's a static-const declared struct in RO
>> memory. With drmm_encoder_init(), there's a kalloc for the managed
>> callback data. It's RW memory and the alloc can fail. Therefore I prefer
>> drm_encoder_init() in this case.
> Ack.

One more though on this: we could discuss if we want some default 
cleanup. Such as if no cleanup pointer has been set, we always call 
drm_encoder_cleanup(). But IMHO that needs to be consistent among all 
elements of the pipeline (planes, CRTCs, etc), and we need to document 
clearly which and why the default has been chosen.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)