Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com>
Date:   Tue, 11 Apr 2023 16:32:07 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH] drm/msm/dpu: always program dsc active bits
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>, <robdclark@gmail.com>,
        <sean@poorly.run>, <swboyd@chromium.org>, <dianders@chromium.org>,
        <vkoul@kernel.org>, <daniel@ffwll.ch>, <airlied@gmail.com>,
        <agross@kernel.org>, <andersson@kernel.org>
CC:     <quic_sbillaka@quicinc.com>, <freedreno@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <1681247095-1201-1-git-send-email-quic_khsieh@quicinc.com>
 <30585d08-88ca-e3a9-6f83-128d49d1d1fc@linaro.org>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <30585d08-88ca-e3a9-6f83-128d49d1d1fc@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 4/11/2023 3:17 PM, Dmitry Baryshkov wrote:
> On 12/04/2023 00:04, Kuogee Hsieh wrote:
>> In current code, the dsc active bits are set only if the cfg->dsc is set.
>> However, for displays which are hot-pluggable, there can be a use-case
>> of disconnecting a DSC supported sink and connecting a non-DSC sink.
>>
>> For those cases we need to clear DSC active bits during teardown.
> 
> Please correct me if I'm wrong here, shouldn't we start using 
> reset_intf_cfg() during teardown / unplug?
> 

This is actually a good point. Since PSR landed this cycle, we are doing 
dpu_encoder_helper_phys_cleanup() even for video mode path,

22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine 
disable")

I was doing it only for writeback path as I had not validated video mode 
enough with the dpu_encoder_helper_phys_cleanup() API.

But looking closely, I think there is an issue with the flush logic in 
that API for video mode.

The reset API, calls a ctl->ops.trigger_flush(ctl); but its getting 
called after timing engine turns off today so this wont take any effect.

We need to improve that API and add the missing pieces for it to work 
correctly with video mode and re-validate the issue for which PSR made 
that change. So needs more work there.

This change works because the timing engine is enabled right after this 
call and will trigger the flush with it.

The only drawback of this change is DSC_ACTIVE will always get written 
to either with 0 or the right value but only once during enable.

I think this change is fine till we finish the rest of the pieces. We 
can add the if (cfg->dsc) back to this when we fix the reset_intf_cfg() 
to handle DSC and dpu_encoder_helper_phys_cleanup() to handle flush 
correctly.

I will take up that work.

>>
>> Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> index bbdc95c..88e4efe 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>       if (cfg->merge_3d)
>>           DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
>>                     BIT(cfg->merge_3d - MERGE_3D_0));
>> -    if (cfg->dsc) {
>> -        DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>> -        DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>> -    }
>> +
>> +    DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
>> +    DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>>   }
>>   static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>