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: <5319b87a-9a4c-1786-9ea9-b9015ee56357@quicinc.com>
Date:   Fri, 12 May 2023 11:47:23 -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 v8 6/8] drm/msm/dpu: separate DSC flush update out of
 interface
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>,
        <dri-devel@lists.freedesktop.org>, <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_jesszhan@quicinc.com>, <quic_sbillaka@quicinc.com>,
        <marijn.suijten@somainline.org>, <freedreno@lists.freedesktop.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1683914423-17612-1-git-send-email-quic_khsieh@quicinc.com>
 <1683914423-17612-7-git-send-email-quic_khsieh@quicinc.com>
 <91f63678-aade-2f42-1311-1bc706ebdc91@linaro.org>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <91f63678-aade-2f42-1311-1bc706ebdc91@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 5/12/2023 11:21 AM, Dmitry Baryshkov wrote:
> On 12/05/2023 21:00, Kuogee Hsieh wrote:
>> Current DSC flush update is piggyback inside dpu_hw_ctl_intf_cfg_v1().
>> This patch separates DSC flush away from dpu_hw_ctl_intf_cfg_v1() by
>> adding dpu_hw_ctl_update_pending_flush_dsc_v1() to handle both per
>> DSC engine and DSC flush bits at same time to make it consistent with
>> the location of flush programming of other dpu sub blocks.
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c  | 22 ++++++++++++++++------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h  | 10 ++++++++++
>>   3 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index ffa6f04..5cae70e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1834,12 +1834,18 @@ dpu_encoder_dsc_initial_line_calc(struct 
>> drm_dsc_config *dsc,
>>       return DIV_ROUND_UP(total_pixels, dsc->slice_width);
>>   }
>> -static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>> +static void dpu_encoder_dsc_pipe_cfg(struct dpu_encoder_virt *dpu_enc,
>> +                     struct dpu_hw_dsc *hw_dsc,
>>                        struct dpu_hw_pingpong *hw_pp,
>>                        struct drm_dsc_config *dsc,
>>                        u32 common_mode,
>>                        u32 initial_lines)
>>   {
>> +    struct dpu_encoder_phys *cur_master = dpu_enc->cur_master;
>> +    struct dpu_hw_ctl *ctl;
>> +
>> +    ctl = cur_master->hw_ctl;
> 
> Just for my understanding: if we have a bonded DSI @ sdm845, should both 
> flashes go to the master CTL or each flush should go to the 
> corresponding CTL?
> 

Is this question for DSC or just general question about flush?

I dont see an explicit DSC flush needed in sdm845 at the ctl level.

If the question is about general flush involving two control paths, we 
need to combine the flushes and they goto the master only. Please refer 
to below part in sde_encoder.c

4243 	/* for split flush, combine pending flush masks and send to master */
4244 	if (pending_flush.pending_flush_mask && sde_enc->cur_master) {
4245 		ctl = sde_enc->cur_master->hw_ctl;
4246 		if (config_changed && ctl->ops.reg_dma_flush)
4247 			ctl->ops.reg_dma_flush(ctl, is_regdma_blocking);
4248 		_sde_encoder_trigger_flush(&sde_enc->base, sde_enc->cur_master,
4249 						&pending_flush,
4250 						config_changed);
4251 	}


> I'm going to send patches that utilize single CTL for sm8150+ after the 
> DSC lands, so I'd like to understand this part.
> 
>> +
>>       if (hw_dsc->ops.dsc_config)
>>           hw_dsc->ops.dsc_config(hw_dsc, dsc, common_mode, 
>> initial_lines);
>> @@ -1854,6 +1860,9 @@ static void dpu_encoder_dsc_pipe_cfg(struct 
>> dpu_hw_dsc *hw_dsc,
>>       if (hw_pp->ops.enable_dsc)
>>           hw_pp->ops.enable_dsc(hw_pp);
>> +
>> +    if (ctl->ops.update_pending_flush_dsc)
>> +        ctl->ops.update_pending_flush_dsc(ctl, hw_dsc->idx);
>>   }
>>   static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc,
>> @@ -1898,7 +1907,8 @@ static void dpu_encoder_prep_dsc(struct 
>> dpu_encoder_virt *dpu_enc,
>>       initial_lines = dpu_encoder_dsc_initial_line_calc(dsc, enc_ip_w);
>>       for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
>> -        dpu_encoder_dsc_pipe_cfg(hw_dsc[i], hw_pp[i], dsc, 
>> dsc_common_mode, initial_lines);
>> +        dpu_encoder_dsc_pipe_cfg(dpu_enc, hw_dsc[i], hw_pp[i], dsc,
>> +                     dsc_common_mode, initial_lines);
>>   }
>>   void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
>> 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 4f7cfa9..f3a50cc 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> @@ -139,6 +139,11 @@ static inline void 
>> dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
>>                   CTL_DSPP_n_FLUSH(dspp - DSPP_0),
>>                   ctx->pending_dspp_flush_mask[dspp - DSPP_0]);
>>           }
>> +
>> +    if (ctx->pending_flush_mask & BIT(DSC_IDX))
>> +        DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH,
>> +                  ctx->pending_dsc_flush_mask);
>> +
>>       DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>>   }
>> @@ -285,6 +290,13 @@ static void 
>> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
>>       ctx->pending_flush_mask |= BIT(MERGE_3D_IDX);
>>   }
>> +static void dpu_hw_ctl_update_pending_flush_dsc_v1(struct dpu_hw_ctl 
>> *ctx,
>> +                           enum dpu_dsc dsc_num)
>> +{
>> +    ctx->pending_dsc_flush_mask |= BIT(dsc_num - DSC_0);
>> +    ctx->pending_flush_mask |= BIT(DSC_IDX);
>> +}
>> +
>>   static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl 
>> *ctx,
>>       enum dpu_dspp dspp, u32 dspp_sub_blk)
>>   {
>> @@ -502,9 +514,6 @@ static void dpu_hw_ctl_intf_cfg_v1(struct 
>> dpu_hw_ctl *ctx,
>>       if ((test_bit(DPU_CTL_VM_CFG, &ctx->caps->features)))
>>           mode_sel = CTL_DEFAULT_GROUP_ID  << 28;
>> -    if (cfg->dsc)
>> -        DPU_REG_WRITE(&ctx->hw, CTL_DSC_FLUSH, cfg->dsc);
>> -
>>       if (cfg->intf_mode_sel == DPU_CTL_MODE_SEL_CMD)
>>           mode_sel |= BIT(17);
>> @@ -524,10 +533,8 @@ 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);
>> +    if (cfg->dsc)
>>           DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
>> -    }
>>   }
>>   static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
>> @@ -630,6 +637,9 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops 
>> *ops,
>>           ops->update_pending_flush_merge_3d =
>>               dpu_hw_ctl_update_pending_flush_merge_3d_v1;
>>           ops->update_pending_flush_wb = 
>> dpu_hw_ctl_update_pending_flush_wb_v1;
>> +
>> +        ops->update_pending_flush_dsc =
>> +            dpu_hw_ctl_update_pending_flush_dsc_v1;
>>       } else {
>>           ops->trigger_flush = dpu_hw_ctl_trigger_flush;
>>           ops->setup_intf_cfg = dpu_hw_ctl_intf_cfg;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> index 6292002..d4869a0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
>> @@ -158,6 +158,15 @@ struct dpu_hw_ctl_ops {
>>           enum dpu_dspp blk, u32 dspp_sub_blk);
>>       /**
>> +     * OR in the given flushbits to the cached pending_(dsc_)flush_mask
>> +     * No effect on hardware
>> +     * @ctx       : ctl path ctx pointer
>> +     * @blk       : interface block index
>> +     */
>> +    void (*update_pending_flush_dsc)(struct dpu_hw_ctl *ctx,
>> +                     enum dpu_dsc blk);
>> +
>> +    /**
>>        * Write the value of the pending_flush_mask to hardware
>>        * @ctx       : ctl path ctx pointer
>>        */
>> @@ -245,6 +254,7 @@ struct dpu_hw_ctl {
>>       u32 pending_wb_flush_mask;
>>       u32 pending_merge_3d_flush_mask;
>>       u32 pending_dspp_flush_mask[DSPP_MAX - DSPP_0];
>> +    u32 pending_dsc_flush_mask;
>>       /* ops */
>>       struct dpu_hw_ctl_ops ops;
>