Re: [PATCH 2/4] drm/amd/display: Use optc31_disable_crtc for DCN 31 and 401

[Mario Limonciello <mario.limonciello@amd.com>]

On 5/5/2025 11:27 AM, Alex Hung wrote:
> On 5/4/25 16:11, Rodrigo Siqueira wrote:
>> DCN401 and DCN31 share the same implementation for disabling CRTC. This
>> commit makes DCN401 use the DCN31 implementation and removes the code
>> duplication in the DCN401.
> 
> Hi Rodrigo
> 
> optc31_disable_crtc is not the same as optc401_disable_crtc. Please see 
> the dfiff below:
> 
> < /* disable_crtc - call ASIC Control Object to disable Timing 
> generator. */
> < static bool optc31_disable_crtc(struct timing_generator *optc)
> ---
>  > /* disable_crtc */
>  > bool optc401_disable_crtc(struct timing_generator *optc)
> 147,148c232
> <                       1, 100000);
> <       optc1_clear_optc_underflow(optc);
> ---
>  >                       1, 150000);
> 152,155c236,237
> 
> 
> However, optc31_disable_crtc is the same as optc35_disable_crtc (patch 
> 3?) and optc32_disable_crtc is the same as optc401_disable_crtc.

Is that last argument a timeout?  How about just extending the timeout 
to be the same for all of them?  That should be relatively harmless, no?

> 
>>
>> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
>> ---
>>   .../amd/display/dc/optc/dcn31/dcn31_optc.c    |  2 +-
>>   .../amd/display/dc/optc/dcn31/dcn31_optc.h    |  2 ++
>>   .../amd/display/dc/optc/dcn401/dcn401_optc.c  | 34 +------------------
>>   .../amd/display/dc/optc/dcn401/dcn401_optc.h  |  1 -
>>   4 files changed, 4 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.c b/ 
>> drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.c
>> index 13c1f95b5ced..e6246e5ba86f 100644
>> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.c
>> @@ -125,7 +125,7 @@ bool optc31_enable_crtc(struct timing_generator 
>> *optc)
>>   }
>>   /* disable_crtc - call ASIC Control Object to disable Timing 
>> generator. */
>> -static bool optc31_disable_crtc(struct timing_generator *optc)
>> +bool optc31_disable_crtc(struct timing_generator *optc)
>>   {
>>       struct optc *optc1 = DCN10TG_FROM_TG(optc);
>> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.h b/ 
>> drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.h
>> index af67eeaf8505..db7e51fc787e 100644
>> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.h
>> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn31/dcn31_optc.h
>> @@ -267,6 +267,8 @@ void dcn31_timing_generator_init(struct optc *optc1);
>>   bool optc31_immediate_disable_crtc(struct timing_generator *optc);
>> +bool optc31_disable_crtc(struct timing_generator *optc);
>> +
>>   bool optc31_enable_crtc(struct timing_generator *optc);
>>   void optc31_set_drr(struct timing_generator *optc, const struct 
>> drr_params *params);
>> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.c 
>> b/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.c
>> index 6eba48de58ff..f472d2efe026 100644
>> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.c
>> @@ -170,38 +170,6 @@ void optc401_set_h_timing_div_manual_mode(struct 
>> timing_generator *optc, bool ma
>>               OTG_H_TIMING_DIV_MODE_MANUAL, manual_mode ? 1 : 0);
>>   }
>> -/* disable_crtc */
>> -bool optc401_disable_crtc(struct timing_generator *optc)
>> -{
>> -    struct optc *optc1 = DCN10TG_FROM_TG(optc);
>> -
>> -    REG_UPDATE_5(OPTC_DATA_SOURCE_SELECT,
>> -            OPTC_SEG0_SRC_SEL, 0xf,
>> -            OPTC_SEG1_SRC_SEL, 0xf,
>> -            OPTC_SEG2_SRC_SEL, 0xf,
>> -            OPTC_SEG3_SRC_SEL, 0xf,
>> -            OPTC_NUM_OF_INPUT_SEGMENT, 0);
>> -
>> -    REG_UPDATE(OPTC_MEMORY_CONFIG,
>> -            OPTC_MEM_SEL, 0);
>> -
>> -    /* disable otg request until end of the first line
>> -     * in the vertical blank region
>> -     */
>> -    REG_UPDATE(OTG_CONTROL,
>> -            OTG_MASTER_EN, 0);
>> -
>> -    REG_UPDATE(CONTROL,
>> -            VTG0_ENABLE, 0);
>> -
>> -    /* CRTC disabled, so disable  clock. */
>> -    REG_WAIT(OTG_CLOCK_CONTROL,
>> -            OTG_BUSY, 0,
>> -            1, 150000);
>> -
>> -    return true;
>> -}
>> -
>>   void optc401_phantom_crtc_post_enable(struct timing_generator *optc)
>>   {
>>       struct optc *optc1 = DCN10TG_FROM_TG(optc);
>> @@ -435,7 +403,7 @@ static struct timing_generator_funcs 
>> dcn401_tg_funcs = {
>>           .setup_vertical_interrupt2 = optc1_setup_vertical_interrupt2,
>>           .program_global_sync = optc401_program_global_sync,
>>           .enable_crtc = optc31_enable_crtc,
>> -        .disable_crtc = optc401_disable_crtc,
>> +        .disable_crtc = optc31_disable_crtc,
>>           .phantom_crtc_post_enable = optc401_phantom_crtc_post_enable,
>>           .disable_phantom_crtc = optc401_disable_phantom_otg,
>>           /* used by enable_timing_synchronization. Not need for FPGA */
>> diff --git a/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.h 
>> b/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.h
>> index 8e795e06e615..be74fd709d43 100644
>> --- a/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.h
>> +++ b/drivers/gpu/drm/amd/display/dc/optc/dcn401/dcn401_optc.h
>> @@ -180,7 +180,6 @@ void optc401_program_global_sync(
>>           int vupdate_offset,
>>           int vupdate_width,
>>           int pstate_keepout);
>> -bool optc401_disable_crtc(struct timing_generator *optc);
>>   void optc401_phantom_crtc_post_enable(struct timing_generator *optc);
>>   void optc401_disable_phantom_otg(struct timing_generator *optc);
>>   void optc401_set_odm_bypass(struct timing_generator *optc,
>