[PATCH] drm/amd/display: Old sequence for HUBP blank
Aurabindo Pillai
aurabindo.pillai at amd.com
Wed Feb 17 15:36:29 UTC 2021
On 2021-02-17 10:29 a.m., Rodrigo Siqueira wrote:
> On 02/17, Aurabindo Pillai wrote:
>>
>>
>> On 2021-02-17 8:40 a.m., Rodrigo Siqueira wrote:
>>> Hi,
>>>
>>> Is this commit a direct revert from
>>> be7af780ef3cbb8fe1004db48dc66caf2da595cd ?
>>>
>>> If so, I recommend you to use the standard way to identify "revert"
>>> commits by using 'Revert "Commit header"' and the message "This reverts
>>> commit HASH" followed by the original commit description.
>>
>> This is not an exact revert. It includes two more hunks which were not part
>> of the original commit. But logically this should be one unit, and hence
>> having them separated might introduce regressions.
>
> Ok,
> In this case, remove the "revert" in the commit description to avoid
> misleading information and add the Fixes tag at the end. With these
> changes:
>
> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
Thanks for the suggestions and review!
>
>>>
>>> Thanks
>>> Siqueira
>>>
>>> On 02/16, Aurabindo Pillai wrote:
>>>> This reverts commit be7af780ef3cbb8fe1004db48dc66caf2da595cd because the
>>>> new proposed sequence for HUBP blanking causes regressions. This change
>>>> brings back the old sequence.
>>>>
>>>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
>>>> Signed-off-by: Bhawanpreet Lakha <bhawanpreet.lakha at amd.com>
>>>> ---
>>>> .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 36 +++--------
>>>> .../amd/display/dc/dcn10/dcn10_hw_sequencer.h | 3 -
>>>> .../gpu/drm/amd/display/dc/dcn10/dcn10_init.c | 1 -
>>>> .../drm/amd/display/dc/dcn20/dcn20_hwseq.c | 13 +++-
>>>> .../gpu/drm/amd/display/dc/dcn20/dcn20_init.c | 1 -
>>>> .../gpu/drm/amd/display/dc/dcn21/dcn21_init.c | 1 -
>>>> .../drm/amd/display/dc/dcn30/dcn30_hwseq.c | 62 ++++---------------
>>>> .../drm/amd/display/dc/dcn30/dcn30_hwseq.h | 4 --
>>>> .../gpu/drm/amd/display/dc/dcn30/dcn30_init.c | 1 -
>>>> .../drm/amd/display/dc/dcn301/dcn301_init.c | 1 -
>>>> .../gpu/drm/amd/display/dc/inc/hw_sequencer.h | 4 --
>>>> 11 files changed, 30 insertions(+), 97 deletions(-)
>>>>
>>>> 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 89912bb5014f..361a97edc8ee 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
>>>> @@ -2635,7 +2635,7 @@ static void dcn10_update_dchubp_dpp(
>>>> hws->funcs.update_plane_addr(dc, pipe_ctx);
>>>> if (is_pipe_tree_visible(pipe_ctx))
>>>> - dc->hwss.set_hubp_blank(dc, pipe_ctx, false);
>>>> + hubp->funcs->set_blank(hubp, false);
>>>> }
>>>> void dcn10_blank_pixel_data(
>>>> @@ -3146,16 +3146,13 @@ void dcn10_setup_stereo(struct pipe_ctx *pipe_ctx, struct dc *dc)
>>>> return;
>>>> }
>>>> -static struct pipe_ctx *get_pipe_ctx_by_hubp_inst(struct dc_state *context, int mpcc_inst)
>>>> +static struct hubp *get_hubp_by_inst(struct resource_pool *res_pool, int mpcc_inst)
>>>> {
>>>> int i;
>>>> - for (i = 0; i < MAX_PIPES; i++) {
>>>> - if (context->res_ctx.pipe_ctx[i].plane_res.hubp
>>>> - && context->res_ctx.pipe_ctx[i].plane_res.hubp->inst == mpcc_inst) {
>>>> - return &context->res_ctx.pipe_ctx[i];
>>>> - }
>>>> -
>>>> + for (i = 0; i < res_pool->pipe_count; i++) {
>>>> + if (res_pool->hubps[i]->inst == mpcc_inst)
>>>> + return res_pool->hubps[i];
>>>> }
>>>> ASSERT(false);
>>>> return NULL;
>>>> @@ -3178,23 +3175,11 @@ void dcn10_wait_for_mpcc_disconnect(
>>>> for (mpcc_inst = 0; mpcc_inst < MAX_PIPES; mpcc_inst++) {
>>>> if (pipe_ctx->stream_res.opp->mpcc_disconnect_pending[mpcc_inst]) {
>>>> - struct pipe_ctx *restore_bottom_pipe;
>>>> - struct pipe_ctx *restore_top_pipe;
>>>> - struct pipe_ctx *inst_pipe_ctx = get_pipe_ctx_by_hubp_inst(dc->current_state, mpcc_inst);
>>>> + struct hubp *hubp = get_hubp_by_inst(res_pool, mpcc_inst);
>>>> - ASSERT(inst_pipe_ctx);
>>>> res_pool->mpc->funcs->wait_for_idle(res_pool->mpc, mpcc_inst);
>>>> pipe_ctx->stream_res.opp->mpcc_disconnect_pending[mpcc_inst] = false;
>>>> - /*
>>>> - * Set top and bottom pipes NULL, as we don't want
>>>> - * to blank those pipes when disconnecting from MPCC
>>>> - */
>>>> - restore_bottom_pipe = inst_pipe_ctx->bottom_pipe;
>>>> - restore_top_pipe = inst_pipe_ctx->top_pipe;
>>>> - inst_pipe_ctx->top_pipe = inst_pipe_ctx->bottom_pipe = NULL;
>>>> - dc->hwss.set_hubp_blank(dc, inst_pipe_ctx, true);
>>>> - inst_pipe_ctx->top_pipe = restore_top_pipe;
>>>> - inst_pipe_ctx->bottom_pipe = restore_bottom_pipe;
>>>> + hubp->funcs->set_blank(hubp, true);
>>>> }
>>>> }
>>>> @@ -3747,10 +3732,3 @@ void dcn10_get_clock(struct dc *dc,
>>>> dc->clk_mgr->funcs->get_clock(dc->clk_mgr, context, clock_type, clock_cfg);
>>>> }
>>>> -
>>>> -void dcn10_set_hubp_blank(const struct dc *dc,
>>>> - struct pipe_ctx *pipe_ctx,
>>>> - bool blank_enable)
>>>> -{
>>>> - pipe_ctx->plane_res.hubp->funcs->set_blank(pipe_ctx->plane_res.hubp, blank_enable);
>>>> -}
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.h b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.h
>>>> index 89e6dfb63da0..dee8ad1ebaa4 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.h
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.h
>>>> @@ -204,8 +204,5 @@ void dcn10_wait_for_pending_cleared(struct dc *dc,
>>>> struct dc_state *context);
>>>> void dcn10_set_hdr_multiplier(struct pipe_ctx *pipe_ctx);
>>>> void dcn10_verify_allow_pstate_change_high(struct dc *dc);
>>>> -void dcn10_set_hubp_blank(const struct dc *dc,
>>>> - struct pipe_ctx *pipe_ctx,
>>>> - bool blank_enable);
>>>> #endif /* __DC_HWSS_DCN10_H__ */
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_init.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_init.c
>>>> index 2f1b802e66a1..254300b06b43 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_init.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_init.c
>>>> @@ -79,7 +79,6 @@ static const struct hw_sequencer_funcs dcn10_funcs = {
>>>> .set_backlight_level = dce110_set_backlight_level,
>>>> .set_abm_immediate_disable = dce110_set_abm_immediate_disable,
>>>> .set_pipe = dce110_set_pipe,
>>>> - .set_hubp_blank = dcn10_set_hubp_blank,
>>>> };
>>>> static const struct hwseq_private_funcs dcn10_private_funcs = {
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>>>> index b79a17f6a9cc..48d1e0e2cf75 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>>
>> Additional hunk 1
>>>> @@ -1576,7 +1576,7 @@ static void dcn20_update_dchubp_dpp(
>>>> if (pipe_ctx->update_flags.bits.enable)
>>>> - dc->hwss.set_hubp_blank(dc, pipe_ctx, false);
>>>> + hubp->funcs->set_blank(hubp, false);
>>>> }
>>>> @@ -1772,10 +1772,19 @@ void dcn20_post_unlock_program_front_end(
>>>> for (i = 0; i < dc->res_pool->pipe_count; i++) {
>>>> struct pipe_ctx *pipe = &context->res_ctx.pipe_ctx[i];
>>>> + struct pipe_ctx *mpcc_pipe;
>>>> if (pipe->vtp_locked) {
>>>> - dc->hwss.set_hubp_blank(dc, pipe, true);
>>>> + dc->hwseq->funcs.wait_for_blank_complete(pipe->stream_res.opp);
>>>> + pipe->plane_res.hubp->funcs->set_blank(pipe->plane_res.hubp, true);
>>>> pipe->vtp_locked = false;
>>>> +
>>>> + for (mpcc_pipe = pipe->bottom_pipe; mpcc_pipe; mpcc_pipe = mpcc_pipe->bottom_pipe)
>>>> + mpcc_pipe->plane_res.hubp->funcs->set_blank(mpcc_pipe->plane_res.hubp, true);
>>>> +
>>>> + for (i = 0; i < dc->res_pool->pipe_count; i++)
>>>> + if (context->res_ctx.pipe_ctx[i].update_flags.bits.disable)
>>>> + dc->hwss.disable_plane(dc, &dc->current_state->res_ctx.pipe_ctx[i]);
>>>> }
>>>> }
>>>> /* WA to apply WM setting*/
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_init.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_init.c
>>>> index 51a4166e9750..de9dcbeea150 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_init.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_init.c
>>>> @@ -94,7 +94,6 @@ static const struct hw_sequencer_funcs dcn20_funcs = {
>>>> .optimize_timing_for_fsft = dcn20_optimize_timing_for_fsft,
>>>> #endif
>>>> .set_disp_pattern_generator = dcn20_set_disp_pattern_generator,
>>>> - .set_hubp_blank = dcn10_set_hubp_blank,
>>>> };
>>>> static const struct hwseq_private_funcs dcn20_private_funcs = {
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_init.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_init.c
>>>> index 0597391b2171..074e2713257f 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_init.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_init.c
>>>> @@ -99,7 +99,6 @@ static const struct hw_sequencer_funcs dcn21_funcs = {
>>>> #endif
>>>> .is_abm_supported = dcn21_is_abm_supported,
>>>> .set_disp_pattern_generator = dcn20_set_disp_pattern_generator,
>>>> - .set_hubp_blank = dcn10_set_hubp_blank,
>>>> };
>>>> static const struct hwseq_private_funcs dcn21_private_funcs = {
>>>> 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 ab93da667d51..d84164f0000c 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
>>>> @@ -938,53 +938,6 @@ void dcn30_hardware_release(struct dc *dc)
>>>> dc->res_pool->hubbub, true, true);
>>>> }
>>>> -void dcn30_set_hubp_blank(const struct dc *dc,
>>>> - struct pipe_ctx *pipe_ctx,
>>>> - bool blank_enable)
>>>> -{
>>>> - struct pipe_ctx *mpcc_pipe;
>>>> - struct pipe_ctx *odm_pipe;
>>>> -
>>>> - if (blank_enable) {
>>>> - struct plane_resource *plane_res = &pipe_ctx->plane_res;
>>>> - struct stream_resource *stream_res = &pipe_ctx->stream_res;
>>>> -
>>>> - /* Wait for enter vblank */
>>>> - stream_res->tg->funcs->wait_for_state(stream_res->tg, CRTC_STATE_VBLANK);
>>>> -
>>>> - /* Blank HUBP to allow p-state during blank on all timings */
>>>> - pipe_ctx->plane_res.hubp->funcs->set_blank(pipe_ctx->plane_res.hubp, true);
>>>> - /* Confirm hubp in blank */
>>>> - ASSERT(plane_res->hubp->funcs->hubp_in_blank(plane_res->hubp));
>>>> - /* Toggle HUBP_DISABLE */
>>>> - plane_res->hubp->funcs->hubp_soft_reset(plane_res->hubp, true);
>>>> - plane_res->hubp->funcs->hubp_soft_reset(plane_res->hubp, false);
>>>> - for (mpcc_pipe = pipe_ctx->bottom_pipe; mpcc_pipe; mpcc_pipe = mpcc_pipe->bottom_pipe) {
>>>> - mpcc_pipe->plane_res.hubp->funcs->set_blank(mpcc_pipe->plane_res.hubp, true);
>>>> - /* Confirm hubp in blank */
>>>> - ASSERT(mpcc_pipe->plane_res.hubp->funcs->hubp_in_blank(mpcc_pipe->plane_res.hubp));
>>>> - /* Toggle HUBP_DISABLE */
>>>> - mpcc_pipe->plane_res.hubp->funcs->hubp_soft_reset(mpcc_pipe->plane_res.hubp, true);
>>>> - mpcc_pipe->plane_res.hubp->funcs->hubp_soft_reset(mpcc_pipe->plane_res.hubp, false);
>>>> -
>>>> - }
>>>> - for (odm_pipe = pipe_ctx->next_odm_pipe; odm_pipe; odm_pipe = odm_pipe->next_odm_pipe) {
>>>> - odm_pipe->plane_res.hubp->funcs->set_blank(odm_pipe->plane_res.hubp, true);
>>>> - /* Confirm hubp in blank */
>>>> - ASSERT(odm_pipe->plane_res.hubp->funcs->hubp_in_blank(odm_pipe->plane_res.hubp));
>>>> - /* Toggle HUBP_DISABLE */
>>>> - odm_pipe->plane_res.hubp->funcs->hubp_soft_reset(odm_pipe->plane_res.hubp, true);
>>>> - odm_pipe->plane_res.hubp->funcs->hubp_soft_reset(odm_pipe->plane_res.hubp, false);
>>>> - }
>>>> - } else {
>>>> - pipe_ctx->plane_res.hubp->funcs->set_blank(pipe_ctx->plane_res.hubp, false);
>>>> - for (mpcc_pipe = pipe_ctx->bottom_pipe; mpcc_pipe; mpcc_pipe = mpcc_pipe->bottom_pipe)
>>>> - mpcc_pipe->plane_res.hubp->funcs->set_blank(mpcc_pipe->plane_res.hubp, false);
>>>> - for (odm_pipe = pipe_ctx->next_odm_pipe; odm_pipe; odm_pipe = odm_pipe->next_odm_pipe)
>>>> - odm_pipe->plane_res.hubp->funcs->set_blank(odm_pipe->plane_res.hubp, false);
>>>> - }
>>>> -}
>>>> -
>>>> void dcn30_set_disp_pattern_generator(const struct dc *dc,
>>>> struct pipe_ctx *pipe_ctx,
>>>> enum controller_dp_test_pattern test_pattern,
>>>> @@ -994,6 +947,7 @@ void dcn30_set_disp_pattern_generator(const struct dc *dc,
>>>> int width, int height, int offset)
>>>> {
>>>> struct stream_resource *stream_res = &pipe_ctx->stream_res;
>>>> + struct pipe_ctx *mpcc_pipe;
>>>> if (test_pattern != CONTROLLER_DP_TEST_PATTERN_VIDEOMODE) {
>>>> pipe_ctx->vtp_locked = false;
>>>> @@ -1005,12 +959,20 @@ void dcn30_set_disp_pattern_generator(const struct dc *dc,
>>
>> Additional hunk 2
>>>> if (stream_res->tg->funcs->is_tg_enabled(stream_res->tg)) {
>>>> if (stream_res->tg->funcs->is_locked(stream_res->tg))
>>>> pipe_ctx->vtp_locked = true;
>>>> - else
>>>> - dc->hwss.set_hubp_blank(dc, pipe_ctx, true);
>>>> + else {
>>>> + /* Blank HUBP to allow p-state during blank on all timings */
>>>> + pipe_ctx->plane_res.hubp->funcs->set_blank(pipe_ctx->plane_res.hubp, true);
>>>> +
>>>> + for (mpcc_pipe = pipe_ctx->bottom_pipe; mpcc_pipe; mpcc_pipe = mpcc_pipe->bottom_pipe)
>>>> + mpcc_pipe->plane_res.hubp->funcs->set_blank(mpcc_pipe->plane_res.hubp, true);
>>>> + }
>>>> }
>>>> } else {
>>>> - dc->hwss.set_hubp_blank(dc, pipe_ctx, false);
>>>> /* turning off DPG */
>>>> + pipe_ctx->plane_res.hubp->funcs->set_blank(pipe_ctx->plane_res.hubp, false);
>>>> + for (mpcc_pipe = pipe_ctx->bottom_pipe; mpcc_pipe; mpcc_pipe = mpcc_pipe->bottom_pipe)
>>>> + mpcc_pipe->plane_res.hubp->funcs->set_blank(mpcc_pipe->plane_res.hubp, false);
>>>> +
>>>> stream_res->opp->funcs->opp_set_disp_pattern_generator(stream_res->opp, test_pattern, color_space,
>>>> color_depth, solid_color, width, height, offset);
>>>> }
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
>>>> index 3b7d4812e311..e9a0005288d3 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
>>>> @@ -80,8 +80,4 @@ void dcn30_set_disp_pattern_generator(const struct dc *dc,
>>>> const struct tg_color *solid_color,
>>>> int width, int height, int offset);
>>>> -void dcn30_set_hubp_blank(const struct dc *dc,
>>>> - struct pipe_ctx *pipe_ctx,
>>>> - bool blank_enable);
>>>> -
>>>> #endif /* __DC_HWSS_DCN30_H__ */
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_init.c b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_init.c
>>>> index 204444fead97..c4c14e9c1309 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_init.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_init.c
>>>> @@ -98,7 +98,6 @@ static const struct hw_sequencer_funcs dcn30_funcs = {
>>>> .hardware_release = dcn30_hardware_release,
>>>> .set_pipe = dcn21_set_pipe,
>>>> .set_disp_pattern_generator = dcn30_set_disp_pattern_generator,
>>>> - .set_hubp_blank = dcn30_set_hubp_blank,
>>>> };
>>>> static const struct hwseq_private_funcs dcn30_private_funcs = {
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
>>>> index b8bf6d61005b..bdad72140cbc 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
>>>> @@ -98,7 +98,6 @@ static const struct hw_sequencer_funcs dcn301_funcs = {
>>>> .set_abm_immediate_disable = dcn21_set_abm_immediate_disable,
>>>> .set_pipe = dcn21_set_pipe,
>>>> .set_disp_pattern_generator = dcn30_set_disp_pattern_generator,
>>>> - .set_hubp_blank = dcn30_set_hubp_blank,
>>>> };
>>>> static const struct hwseq_private_funcs dcn301_private_funcs = {
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h b/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
>>>> index 0586ab2ffd6a..613b3e3cb46a 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
>>>> +++ b/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
>>>> @@ -231,10 +231,6 @@ struct hw_sequencer_funcs {
>>>> enum dc_color_depth color_depth,
>>>> const struct tg_color *solid_color,
>>>> int width, int height, int offset);
>>>> -
>>>> - void (*set_hubp_blank)(const struct dc *dc,
>>>> - struct pipe_ctx *pipe_ctx,
>>>> - bool blank_enable);
>>>> };
>>>> void color_space_to_black_color(
>>>> --
>>>> 2.30.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CRodrigo.Siqueira%40amd.com%7C7700f89c242d4d5b837308d8d2d6cec7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637491167330054893%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uxidmH%2FvP7CIAaeYbJHtqQ1GM%2BZt5zfbNyfCWnSkhR4%3D&reserved=0
>>>
>>
>> --
>> Regards,
>> Aurabindo Pillai
>
More information about the amd-gfx
mailing list