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

[Tvrtko Ursulin <tursulin@ursulin.net>]

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

>> 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) ||
>