Re: [PATCH] drm/radeon: Move the memset() function call after the return statement

[Liao Yuanhong <liaoyuanhong@vivo.com>]

On 8/14/2025 7:07 PM, Tvrtko Ursulin wrote:

>
> On 14/08/2025 11:11, Christian König wrote:
>> On 14.08.25 11:39, Liao Yuanhong wrote:
>>> Adjust the position of the memset() call to avoid unnecessary 
>>> invocations.
>>
>> Hui? That doesn't seem to make much sense to me.
>>
>> We memset the local variable because we need to make sure that 
>> everything (including padding bytes) is zeroed out.
>>
>> Even if that isn't sometimes necessary because of error handling we 
>> clearly shouldn't try to optimize this.
>
> To interject with a curiousity, it is even worse to move the memset 
> away from the declaration block when the often enabled 
> CONFIG_INIT_STACK_ALL_ZERO is on. At least when they are close 
> compiler can figure out it only needs to memset it once. Move them 
> further apart and sometimes memset happens twice, yay.
>
> Main point when CONFIG_INIT_STACK_ALL_ZERO is on, and it often is, 
> there is no way to avoid the sometimes pointless init. I have some 
> local branches which try to address the worst offenders but it is so 
> much in the noise that I don't think I ever sent them out.
>
> Regards,
>
> Tvrtko
>
If we converts memset() to = { },can it alleviate the problem?


Thanks,

Liao

>>> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
>>> ---
>>>   drivers/gpu/drm/radeon/atombios_crtc.c     |  8 ++++----
>>>   drivers/gpu/drm/radeon/atombios_encoders.c | 20 ++++++++++----------
>>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c 
>>> b/drivers/gpu/drm/radeon/atombios_crtc.c
>>> index 9b3a3a9d60e2..0aae84f5e27c 100644
>>> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
>>> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
>>> @@ -770,13 +770,13 @@ static void 
>>> atombios_crtc_set_disp_eng_pll(struct radeon_device *rdev,
>>>       int index;
>>>       union set_pixel_clock args;
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       index = GetIndexIntoMasterTable(COMMAND, SetPixelClock);
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev,
>>>                      &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       switch (frev) {
>>>       case 1:
>>>           switch (crev) {
>>> @@ -832,12 +832,12 @@ static void atombios_crtc_program_pll(struct 
>>> drm_crtc *crtc,
>>>       int index = GetIndexIntoMasterTable(COMMAND, SetPixelClock);
>>>       union set_pixel_clock args;
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev,
>>>                      &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       switch (frev) {
>>>       case 1:
>>>           switch (crev) {
>>> diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c 
>>> b/drivers/gpu/drm/radeon/atombios_encoders.c
>>> index d1c5e471bdca..f706e21a3509 100644
>>> --- a/drivers/gpu/drm/radeon/atombios_encoders.c
>>> +++ b/drivers/gpu/drm/radeon/atombios_encoders.c
>>> @@ -492,11 +492,11 @@ atombios_dvo_setup(struct drm_encoder 
>>> *encoder, int action)
>>>       int index = GetIndexIntoMasterTable(COMMAND, DVOEncoderControl);
>>>       uint8_t frev, crev;
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev, &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       /* some R4xx chips have the wrong frev */
>>>       if (rdev->family <= CHIP_RV410)
>>>           frev = 1;
>>> @@ -856,8 +856,6 @@ atombios_dig_encoder_setup2(struct drm_encoder 
>>> *encoder, int action, int panel_m
>>>       if (dig->dig_encoder == -1)
>>>           return;
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       if (ASIC_IS_DCE4(rdev))
>>>           index = GetIndexIntoMasterTable(COMMAND, DIGxEncoderControl);
>>>       else {
>>> @@ -870,6 +868,8 @@ atombios_dig_encoder_setup2(struct drm_encoder 
>>> *encoder, int action, int panel_m
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev, &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       switch (frev) {
>>>       case 1:
>>>           switch (crev) {
>>> @@ -1453,11 +1453,11 @@ atombios_external_encoder_setup(struct 
>>> drm_encoder *encoder,
>>>               (radeon_connector->connector_object_id & 
>>> OBJECT_ID_MASK) >> OBJECT_ID_SHIFT;
>>>       }
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev, &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       switch (frev) {
>>>       case 1:
>>>           /* no params on frev 1 */
>>> @@ -1853,11 +1853,11 @@ atombios_set_encoder_crtc_source(struct 
>>> drm_encoder *encoder)
>>>       uint8_t frev, crev;
>>>       struct radeon_encoder_atom_dig *dig;
>>>   -    memset(&args, 0, sizeof(args));
>>> -
>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev, &crev))
>>>           return;
>>>   +    memset(&args, 0, sizeof(args));
>>> +
>>>       switch (frev) {
>>>       case 1:
>>>           switch (crev) {
>>> @@ -2284,11 +2284,11 @@ atombios_dac_load_detect(struct drm_encoder 
>>> *encoder, struct drm_connector *conn
>>>           int index = GetIndexIntoMasterTable(COMMAND, 
>>> DAC_LoadDetection);
>>>           uint8_t frev, crev;
>>>   -        memset(&args, 0, sizeof(args));
>>> -
>>>           if (!atom_parse_cmd_header(rdev->mode_info.atom_context, 
>>> index, &frev, &crev))
>>>               return false;
>>>   +        memset(&args, 0, sizeof(args));
>>> +
>>>           args.sDacload.ucMisc = 0;
>>>             if ((radeon_encoder->encoder_id == 
>>> ENCODER_OBJECT_ID_INTERNAL_DAC1) ||
>>
>