RE: [PATCH 11/22] drm/amd/display: Disable phantom OTG after enable for plane disable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[AMD Official Use Only - General]

Hi Nathan,
Thanks for reporting this issue.

Hi Alvin,
Please see inline.

-----Original Message-----
From: Nathan Chancellor <nathan@xxxxxxxxxx> 
Sent: Thursday, November 10, 2022 11:39 PM
To: Liu, HaoPing (Alan) <HaoPing.Liu@xxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@xxxxxxx>; Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; Li, Roman <Roman.Li@xxxxxxx>; Chiu, Solomon <Solomon.Chiu@xxxxxxx>; Pillai, Aurabindo <Aurabindo.Pillai@xxxxxxx>; Lee, Alvin <Alvin.Lee2@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>; Lei, Jun <Jun.Lei@xxxxxxx>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@xxxxxxx>; Gutierrez, Agustin <Agustin.Gutierrez@xxxxxxx>; Kotarac, Pavle <Pavle.Kotarac@xxxxxxx>
Subject: Re: [PATCH 11/22] drm/amd/display: Disable phantom OTG after enable for plane disable

Hi Alan,

On Thu, Nov 03, 2022 at 12:01:06AM +0800, Alan Liu wrote:
> From: Alvin Lee <Alvin.Lee2@xxxxxxx>
> 
> [Description]
> - Need to disable phantom OTG after it's enabled
>   in order to restore it to it's original state.
> - If it's enabled and then an MCLK switch comes in
>   we may not prefetch the correct data since the phantom
>   OTG could already be in the middle of the frame.
> 
> Reviewed-by: Jun Lei <Jun.Lei@xxxxxxx>
> Acked-by: Alan Liu <HaoPing.Liu@xxxxxxx>
> Signed-off-by: Alvin Lee <Alvin.Lee2@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc.c           | 14 +++++++++++++-
>  drivers/gpu/drm/amd/display/dc/dcn32/dcn32_optc.c  |  8 ++++++++
>  .../drm/amd/display/dc/inc/hw/timing_generator.h   |  1 +
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index da808996e21d..9c3704c4d7e4 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -1055,6 +1055,7 @@ static void disable_dangling_plane(struct dc *dc, struct dc_state *context)
>  	struct dc_state *dangling_context = dc_create_state(dc);
>  	struct dc_state *current_ctx;
>  	struct pipe_ctx *pipe;
> +	struct timing_generator *tg;
>  
>  	if (dangling_context == NULL)
>  		return;
> @@ -1098,6 +1099,7 @@ static void disable_dangling_plane(struct dc 
> *dc, struct dc_state *context)
>  
>  		if (should_disable && old_stream) {
>  			pipe = &dc->current_state->res_ctx.pipe_ctx[i];
> +			tg = pipe->stream_res.tg;
>  			/* When disabling plane for a phantom pipe, we must turn on the
>  			 * phantom OTG so the disable programming gets the double buffer
>  			 * update. Otherwise the pipe will be left in a partially disabled 
> @@ -1105,7 +1107,8 @@ static void disable_dangling_plane(struct dc *dc, struct dc_state *context)
>  			 * again for different use.
>  			 */
>  			if (old_stream->mall_stream_config.type == SUBVP_PHANTOM) {
> -				pipe->stream_res.tg->funcs->enable_crtc(pipe->stream_res.tg);
> +				if (tg->funcs->enable_crtc)
> +					tg->funcs->enable_crtc(tg);
>  			}
>  			dc_rem_all_planes_for_stream(dc, old_stream, dangling_context);
>  			disable_all_writeback_pipes_for_stream(dc, old_stream, 
> dangling_context); @@ -1122,6 +1125,15 @@ static void disable_dangling_plane(struct dc *dc, struct dc_state *context)
>  				dc->hwss.interdependent_update_lock(dc, dc->current_state, false);
>  				dc->hwss.post_unlock_program_front_end(dc, dangling_context);
>  			}
> +			/* We need to put the phantom OTG back into it's default (disabled) state or we
> +			 * can get corruption when transition from one SubVP config to a different one.
> +			 * The OTG is set to disable on falling edge of VUPDATE so the plane disable
> +			 * will still get it's double buffer update.
> +			 */
> +			if (old_stream->mall_stream_config.type == SUBVP_PHANTOM) {
> +				if (tg->funcs->disable_phantom_crtc)
> +					tg->funcs->disable_phantom_crtc(tg);
> +			}
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_optc.c 
> b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_optc.c
> index 2b33eeb213e2..2ee798965bc2 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_optc.c
> @@ -167,6 +167,13 @@ static void optc32_phantom_crtc_post_enable(struct timing_generator *optc)
>  	REG_WAIT(OTG_CLOCK_CONTROL, OTG_BUSY, 0, 1, 100000);  }
>  
> +static void optc32_disable_phantom_otg(struct timing_generator *optc) 
> +{
> +	struct optc *optc1 = DCN10TG_FROM_TG(optc);
> +
> +	REG_UPDATE(OTG_CONTROL, OTG_MASTER_EN, 0); }
> +
>  static void optc32_set_odm_bypass(struct timing_generator *optc,
>  		const struct dc_crtc_timing *dc_crtc_timing)  { @@ -260,6 +267,7 @@ 
> static struct timing_generator_funcs dcn32_tg_funcs = {
>  		.enable_crtc = optc32_enable_crtc,
>  		.disable_crtc = optc32_disable_crtc,
>  		.phantom_crtc_post_enable = optc32_phantom_crtc_post_enable,
> +		.disable_phantom_crtc = optc32_disable_phantom_otg,
>  		/* used by enable_timing_synchronization. Not need for FPGA */
>  		.is_counter_moving = optc1_is_counter_moving,
>  		.get_position = optc1_get_position, diff --git 
> a/drivers/gpu/drm/amd/display/dc/inc/hw/timing_generator.h 
> b/drivers/gpu/drm/amd/display/dc/inc/hw/timing_generator.h
> index 65f18f9dad34..43eb61961e0f 100644
> --- a/drivers/gpu/drm/amd/display/dc/inc/hw/timing_generator.h
> +++ b/drivers/gpu/drm/amd/display/dc/inc/hw/timing_generator.h
> @@ -184,6 +184,7 @@ struct timing_generator_funcs {
>  	bool (*disable_crtc)(struct timing_generator *tg);  #ifdef 
> CONFIG_DRM_AMD_DC_DCN
>  	void (*phantom_crtc_post_enable)(struct timing_generator *tg);
> +	void (*disable_phantom_crtc)(struct timing_generator *tg);

Hi @Lee, Alvin
I think we should move the above line out of #define CONFIG_DRM_AMD_DC_DCN, right?
Thanks.

>  #endif
>  	bool (*immediate_disable_crtc)(struct timing_generator *tg);
>  	bool (*is_counter_moving)(struct timing_generator *tg);
> --
> 2.25.1
> 
> 

This breaks the build without CONFIG_DRM_AMD_DC_DCN:

  drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:1134:20: error: no member named 'disable_phantom_crtc' in 'struct timing_generator_funcs'
                                  if (tg->funcs->disable_phantom_crtc)
                                      ~~~~~~~~~  ^
  drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:1135:17: error: no member named 'disable_phantom_crtc' in 'struct timing_generator_funcs'
                                          tg->funcs->disable_phantom_crtc(tg);
                                          ~~~~~~~~~  ^
  2 errors generated.

Cheers,
Nathan




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux