ACK/Cmnt: [J] [PATCH 1/1] drm/amd/display: Add helper for blanking all dp displays
Stefan Bader
stefan.bader at canonical.com
Fri Nov 18 09:13:32 UTC 2022
On 18.11.22 09:25, Kai-Heng Feng wrote:
> From: "Leo (Hanghong) Ma" <hanghong.ma at amd.com>
>
> BugLink: https://bugs.launchpad.net/bugs/1996740
>
> [Why & How]
> 1. The code to blank all dp display have been called many times,
> so add helpers in dc_link to make it more concise.
> 2. Add some check to fix the dmesg errors at boot and resume from S3
> on dcn3.1 during DQE's promotion test.
>
> Reviewed-by: Alvin Lee <alvin.lee2 at amd.com>
> Reviewed-by: Wesley Chalmers <Wesley.Chalmers at amd.com>
> Reviewed-by: Aric Cyr <Aric.Cyr at amd.com>
> Acked-by: Anson Jacob <Anson.Jacob at amd.com>
> Tested-by: Daniel Wheeler <daniel.wheeler at amd.com>
> Signed-off-by: Leo (Hanghong) Ma <hanghong.ma at amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> (backported from commit ebd1e719695824ca2b9225094a669fef35620676)
> [khfeng: resolve conflicts of non-upstream version of previous backports]
> Signed-off-by: Kai-Heng Feng <kai.heng.feng at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
Rather based on the test results. The delta is rather hard to grasp. But also
this is a rather good example on how the current enablement process is flawed.
It is rather common that, especially with gpu drivers, adding new support tends
to break existing hardware. More often that not there is a ping-pong of changes
until things are settled.
This is why I always try to convince people that we should hold back porting
enablement changes into stable releases until support has stabelized. And that
will mean more testing across more platforms (new and older) by humans. Because
some issues are not hard lockups and crashes. That is more expensive in people
and time. But on the other hand, how expensive is regressing users out there
overall?
-Stefan
> drivers/gpu/drm/amd/display/dc/core/dc_link.c | 51 +++++++++++++++++++
> drivers/gpu/drm/amd/display/dc/dc_link.h | 4 ++
> .../display/dc/dce110/dce110_hw_sequencer.c | 13 +----
> .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 41 ++-------------
> .../drm/amd/display/dc/dcn30/dcn30_hwseq.c | 39 ++------------
> .../drm/amd/display/dc/dcn31/dcn31_hwseq.c | 37 ++------------
> 6 files changed, 65 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index 4b523f0dd9abd..58cd8e702bb05 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -1927,6 +1927,57 @@ static enum dc_status enable_link_dp_mst(
> return enable_link_dp(state, pipe_ctx);
> }
>
> +void dc_link_blank_all_dp_displays(struct dc *dc)
> +{
> + unsigned int i;
> + uint8_t dpcd_power_state = '\0';
> + enum dc_status status = DC_ERROR_UNEXPECTED;
> +
> + for (i = 0; i < dc->link_count; i++) {
> + if ((dc->links[i]->connector_signal != SIGNAL_TYPE_DISPLAY_PORT) ||
> + (dc->links[i]->priv == NULL) || (dc->links[i]->local_sink == NULL))
> + continue;
> +
> + /* DP 2.0 spec requires that we read LTTPR caps first */
> + dp_retrieve_lttpr_cap(dc->links[i]);
> + /* if any of the displays are lit up turn them off */
> + status = core_link_read_dpcd(dc->links[i], DP_SET_POWER,
> + &dpcd_power_state, sizeof(dpcd_power_state));
> +
> + if (status == DC_OK && dpcd_power_state == DP_POWER_STATE_D0)
> + dc_link_blank_dp_stream(dc->links[i], true);
> + }
> +
> +}
> +
> +void dc_link_blank_dp_stream(struct dc_link *link, bool hw_init)
> +{
> + unsigned int j;
> + struct dc *dc = link->ctx->dc;
> + enum signal_type signal = link->connector_signal;
> +
> + if ((signal == SIGNAL_TYPE_EDP) ||
> + (signal == SIGNAL_TYPE_DISPLAY_PORT)) {
> + if (link->ep_type == DISPLAY_ENDPOINT_PHY &&
> + link->link_enc->funcs->get_dig_frontend &&
> + link->link_enc->funcs->is_dig_enabled(link->link_enc)) {
> + unsigned int fe = link->link_enc->funcs->get_dig_frontend(link->link_enc);
> +
> + if (fe != ENGINE_ID_UNKNOWN)
> + for (j = 0; j < dc->res_pool->stream_enc_count; j++) {
> + if (fe == dc->res_pool->stream_enc[j]->id) {
> + dc->res_pool->stream_enc[j]->funcs->dp_blank(link,
> + dc->res_pool->stream_enc[j]);
> + break;
> + }
> + }
> + }
> +
> + if ((!link->wa_flags.dp_keep_receiver_powered) || hw_init)
> + dp_receiver_power_ctrl(link, false);
> + }
> +}
> +
> static bool get_ext_hdmi_settings(struct pipe_ctx *pipe_ctx,
> enum engine_id eng_id,
> struct ext_hdmi_settings *settings)
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h b/drivers/gpu/drm/amd/display/dc/dc_link.h
> index 2fa9b46267346..da6d21e34662a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_link.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
> @@ -273,6 +273,10 @@ bool dc_link_setup_psr(struct dc_link *dc_link,
>
> void dc_link_get_psr_residency(const struct dc_link *link, uint32_t *residency);
>
> +void dc_link_blank_all_dp_displays(struct dc *dc);
> +
> +void dc_link_blank_dp_stream(struct dc_link *link, bool hw_init);
> +
> /* Request DC to detect if there is a Panel connected.
> * boot - If this call is during initial boot.
> * Return false for any type of detection failure or MST detection
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> index a869fa19c3d73..8fef01945c413 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> @@ -1562,21 +1562,10 @@ static void power_down_encoders(struct dc *dc)
> {
> int i;
>
> - /* do not know BIOS back-front mapping, simply blank all. It will not
> - * hurt for non-DP
> - */
> - for (i = 0; i < dc->res_pool->stream_enc_count; i++) {
> - dc->res_pool->stream_enc[i]->funcs->dp_blank(dc->links[i],
> - dc->res_pool->stream_enc[i]);
> - }
> -
> for (i = 0; i < dc->link_count; i++) {
> enum signal_type signal = dc->links[i]->connector_signal;
>
> - if ((signal == SIGNAL_TYPE_EDP) ||
> - (signal == SIGNAL_TYPE_DISPLAY_PORT))
> - if (!dc->links[i]->wa_flags.dp_keep_receiver_powered)
> - dp_receiver_power_ctrl(dc->links[i], false);
> + dc_link_blank_dp_stream(dc->links[i], false);
>
> if (signal != SIGNAL_TYPE_EDP)
> signal = SIGNAL_TYPE_NONE;
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> index bdc4234123451..2ba7a8bce9c56 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> @@ -1305,7 +1305,7 @@ void dcn10_init_pipes(struct dc *dc, struct dc_state *context)
>
> void dcn10_init_hw(struct dc *dc)
> {
> - int i, j;
> + int i;
> struct abm *abm = dc->res_pool->abm;
> struct dmcu *dmcu = dc->res_pool->dmcu;
> struct dce_hwseq *hws = dc->hwseq;
> @@ -1397,43 +1397,8 @@ void dcn10_init_hw(struct dc *dc)
> }
>
> /* we want to turn off all dp displays before doing detection */
> - if (dc->config.power_down_display_on_boot) {
> - uint8_t dpcd_power_state = '\0';
> - enum dc_status status = DC_ERROR_UNEXPECTED;
> -
> - for (i = 0; i < dc->link_count; i++) {
> - if (dc->links[i]->connector_signal != SIGNAL_TYPE_DISPLAY_PORT)
> - continue;
> -
> - /* DP 2.0 requires that LTTPR Caps be read first */
> - dp_retrieve_lttpr_cap(dc->links[i]);
> -
> - /*
> - * If any of the displays are lit up turn them off.
> - * The reason is that some MST hubs cannot be turned off
> - * completely until we tell them to do so.
> - * If not turned off, then displays connected to MST hub
> - * won't light up.
> - */
> - status = core_link_read_dpcd(dc->links[i], DP_SET_POWER,
> - &dpcd_power_state, sizeof(dpcd_power_state));
> - if (status == DC_OK && dpcd_power_state == DP_POWER_STATE_D0) {
> - /* blank dp stream before power off receiver*/
> - if (dc->links[i]->link_enc->funcs->get_dig_frontend) {
> - unsigned int fe = dc->links[i]->link_enc->funcs->get_dig_frontend(dc->links[i]->link_enc);
> -
> - for (j = 0; j < dc->res_pool->stream_enc_count; j++) {
> - if (fe == dc->res_pool->stream_enc[j]->id) {
> - dc->res_pool->stream_enc[j]->funcs->dp_blank(dc->links[i],
> - dc->res_pool->stream_enc[j]);
> - break;
> - }
> - }
> - }
> - dp_receiver_power_ctrl(dc->links[i], false);
> - }
> - }
> - }
> + if (dc->config.power_down_display_on_boot)
> + dc_link_blank_all_dp_displays(dc);
>
> if (hws->funcs.enable_power_gating_plane)
> hws->funcs.enable_power_gating_plane(dc->hwseq, true);
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> index 625386c04acd2..2a453e808b956 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> @@ -437,7 +437,7 @@ void dcn30_init_hw(struct dc *dc)
> struct dce_hwseq *hws = dc->hwseq;
> struct dc_bios *dcb = dc->ctx->dc_bios;
> struct resource_pool *res_pool = dc->res_pool;
> - int i, j;
> + int i;
> int edp_num;
> uint32_t backlight = MAX_BACKLIGHT_LEVEL;
>
> @@ -534,41 +534,8 @@ void dcn30_init_hw(struct dc *dc)
> hws->funcs.dsc_pg_control(hws, res_pool->dscs[i]->inst, false);
>
> /* we want to turn off all dp displays before doing detection */
> - if (dc->config.power_down_display_on_boot) {
> - uint8_t dpcd_power_state = '\0';
> - enum dc_status status = DC_ERROR_UNEXPECTED;
> -
> - for (i = 0; i < dc->link_count; i++) {
> - if (dc->links[i]->connector_signal != SIGNAL_TYPE_DISPLAY_PORT)
> - continue;
> - /* DP 2.0 states that LTTPR regs must be read first */
> - dp_retrieve_lttpr_cap(dc->links[i]);
> -
> - /* if any of the displays are lit up turn them off */
> - status = core_link_read_dpcd(dc->links[i], DP_SET_POWER,
> - &dpcd_power_state, sizeof(dpcd_power_state));
> - if (status == DC_OK && dpcd_power_state == DP_POWER_STATE_D0) {
> - /* blank dp stream before power off receiver*/
> - if (dc->links[i]->link_enc->funcs->get_dig_frontend) {
> - unsigned int fe;
> -
> - fe = dc->links[i]->link_enc->funcs->get_dig_frontend(
> - dc->links[i]->link_enc);
> - if (fe == ENGINE_ID_UNKNOWN)
> - continue;
> -
> - for (j = 0; j < dc->res_pool->stream_enc_count; j++) {
> - if (fe == dc->res_pool->stream_enc[j]->id) {
> - dc->res_pool->stream_enc[j]->funcs->dp_blank(dc->links[i],
> - dc->res_pool->stream_enc[j]);
> - break;
> - }
> - }
> - }
> - dp_receiver_power_ctrl(dc->links[i], false);
> - }
> - }
> - }
> + if (dc->config.power_down_display_on_boot)
> + dc_link_blank_all_dp_displays(dc);
>
> if (hws->funcs.enable_power_gating_plane)
> hws->funcs.enable_power_gating_plane(dc->hwseq, true);
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c
> index e5b3c6ab04fca..72d95de92d60b 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c
> @@ -71,7 +71,7 @@ void dcn31_init_hw(struct dc *dc)
> struct dc_bios *dcb = dc->ctx->dc_bios;
> struct resource_pool *res_pool = dc->res_pool;
> uint32_t backlight = MAX_BACKLIGHT_LEVEL;
> - int i, j;
> + int i;
> int edp_num;
>
> if (dc->clk_mgr && dc->clk_mgr->funcs->init_clocks)
> @@ -178,39 +178,8 @@ void dcn31_init_hw(struct dc *dc)
> dmub_enable_outbox_notification(dc->ctx->dmub_srv);
>
> /* we want to turn off all dp displays before doing detection */
> - if (dc->config.power_down_display_on_boot) {
> - uint8_t dpcd_power_state = '\0';
> - enum dc_status status = DC_ERROR_UNEXPECTED;
> -
> - for (i = 0; i < dc->link_count; i++) {
> - if (dc->links[i]->connector_signal != SIGNAL_TYPE_DISPLAY_PORT)
> - continue;
> -
> - /* if any of the displays are lit up turn them off */
> - status = core_link_read_dpcd(dc->links[i], DP_SET_POWER,
> - &dpcd_power_state, sizeof(dpcd_power_state));
> - if (status == DC_OK && dpcd_power_state == DP_POWER_STATE_D0) {
> - /* blank dp stream before power off receiver*/
> - if (dc->links[i]->link_enc->funcs->get_dig_frontend) {
> - unsigned int fe;
> -
> - fe = dc->links[i]->link_enc->funcs->get_dig_frontend(
> - dc->links[i]->link_enc);
> - if (fe == ENGINE_ID_UNKNOWN)
> - continue;
> -
> - for (j = 0; j < dc->res_pool->stream_enc_count; j++) {
> - if (fe == dc->res_pool->stream_enc[j]->id) {
> - dc->res_pool->stream_enc[j]->funcs->dp_blank(dc->links[i],
> - dc->res_pool->stream_enc[j]);
> - break;
> - }
> - }
> - }
> - dp_receiver_power_ctrl(dc->links[i], false);
> - }
> - }
> - }
> + if (dc->config.power_down_display_on_boot)
> + dc_link_blank_all_dp_displays(dc);
>
> if (hws->funcs.enable_power_gating_plane)
> hws->funcs.enable_power_gating_plane(dc->hwseq, true);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20221118/23e80e38/attachment.sig>
More information about the kernel-team
mailing list