Re: [PATCH 11/16] drm/msm/dpu: add an API to setup the CDM block for writeback

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 8/30/2023 5:11 PM, Dmitry Baryshkov wrote:
> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Add an API dpu_encoder_helper_phys_setup_cdm() which can be used by
>> the writeback encoder to setup the CDM block.
>>
>> Currently, this is defined and used within the writeback's physical
>> encoder layer however, the function can be modified to be used to setup
>> the CDM block even for non-writeback interfaces.
>>
>> Until those modifications are planned and made, keep it local to
>> writeback.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |   3 +
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 123 +++++++++++++++++-
>>   2 files changed, 125 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index 510c1c41ddbc..93a8ae67beff 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -16,6 +16,7 @@
>>   #include "dpu_hw_pingpong.h"
>>   #include "dpu_hw_ctl.h"
>>   #include "dpu_hw_top.h"
>> +#include "dpu_hw_cdm.h"
>>   #include "dpu_encoder.h"
>>   #include "dpu_crtc.h"
>>
>> @@ -209,6 +210,7 @@ static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>    * @wbirq_refcount:     Reference count of writeback interrupt
>>    * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>    * @wb_cfg:  writeback block config to store fb related details
>> + * @cdm_cfg: cdm block config needed to store writeback block's CDM configuration
>>    * @wb_conn: backpointer to writeback connector
>>    * @wb_job: backpointer to current writeback job
>>    * @dest:   dpu buffer layout for current writeback output buffer
>> @@ -218,6 +220,7 @@ struct dpu_encoder_phys_wb {
>>          atomic_t wbirq_refcount;
>>          int wb_done_timeout_cnt;
>>          struct dpu_hw_wb_cfg wb_cfg;
>> +       struct dpu_hw_cdm_cfg cdm_cfg;
>>          struct drm_writeback_connector *wb_conn;
>>          struct drm_writeback_job *wb_job;
>>          struct dpu_hw_fmt_layout dest;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> index 4c2736c3ee6d..11935aac9fd5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> @@ -24,6 +24,20 @@
>>   #define to_dpu_encoder_phys_wb(x) \
>>          container_of(x, struct dpu_encoder_phys_wb, base)
>>
>> +#define TO_S15D16(_x_)((_x_) << 7)
>> +
>> +static struct dpu_csc_cfg dpu_encoder_phys_wb_rgb2yuv_601l = {
>> +       {
>> +               TO_S15D16(0x0083), TO_S15D16(0x0102), TO_S15D16(0x0032),
>> +               TO_S15D16(0x1fb5), TO_S15D16(0x1f6c), TO_S15D16(0x00e1),
>> +               TO_S15D16(0x00e1), TO_S15D16(0x1f45), TO_S15D16(0x1fdc)
>> +       },
>> +       { 0x00, 0x00, 0x00 },
>> +       { 0x0040, 0x0200, 0x0200 },
>> +       { 0x000, 0x3ff, 0x000, 0x3ff, 0x000, 0x3ff },
>> +       { 0x040, 0x3ac, 0x040, 0x3c0, 0x040, 0x3c0 },
>> +};
> 
> Nit: we probably need to have a single place with all dpu_csc_cfg entries.
> 

hmmm ... so we have YUV2RGB matrices for dpu plane and RGB2YUV matrices 
for WB and DP.

We can move all this to dpu_hw_util.c but lets do that in the DP series 
as that completes the consumer list of these matrices.

>> +
>>   /**
>>    * dpu_encoder_phys_wb_is_master - report wb always as master encoder
>>    * @phys_enc:  Pointer to physical encoder
>> @@ -225,6 +239,112 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>          }
>>   }
>>
>> +/**
>> + * dpu_encoder_phys_wb_setup_cdp - setup chroma down sampling block
>> + * @phys_enc:Pointer to physical encoder
>> + */
>> +static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>> +{
>> +       struct dpu_hw_cdm *hw_cdm;
>> +       struct dpu_hw_cdm_cfg *cdm_cfg;
>> +       struct dpu_hw_pingpong *hw_pp;
>> +       struct dpu_encoder_phys_wb *wb_enc;
>> +       const struct msm_format *format;
>> +       const struct dpu_format *dpu_fmt;
>> +       struct drm_writeback_job *wb_job;
>> +       int ret;
>> +
>> +       if (!phys_enc)
>> +               return;
>> +
>> +       wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>> +       cdm_cfg = &wb_enc->cdm_cfg;
>> +       hw_pp = phys_enc->hw_pp;
>> +       hw_cdm = phys_enc->hw_cdm;
>> +       wb_job = wb_enc->wb_job;
>> +
>> +       format = msm_framebuffer_format(wb_enc->wb_job->fb);
>> +       dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
>> +
>> +       if (!hw_cdm)
>> +               return;
>> +
>> +       if (!DPU_FORMAT_IS_YUV(dpu_fmt)) {
>> +               DPU_DEBUG("[enc:%d] cdm_disable fmt:%x\n", DRMID(phys_enc->parent),
>> +                         dpu_fmt->base.pixel_format);
>> +               if (hw_cdm->ops.disable)
>> +                       hw_cdm->ops.disable(hw_cdm);
>> +
>> +               return;
>> +       }
>> +
>> +       memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>> +
>> +       cdm_cfg->output_width = wb_job->fb->width;
>> +       cdm_cfg->output_height = wb_job->fb->height;
>> +       cdm_cfg->output_fmt = dpu_fmt;
>> +       cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
>> +       cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
>> +                       CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
>> +
>> +       /* enable 10 bit logic */
>> +       switch (cdm_cfg->output_fmt->chroma_sample) {
>> +       case DPU_CHROMA_RGB:
>> +               cdm_cfg->h_cdwn_type = CDM_CDWN_DISABLE;
>> +               cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
>> +               break;
>> +       case DPU_CHROMA_H2V1:
>> +               cdm_cfg->h_cdwn_type = CDM_CDWN_COSITE;
>> +               cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
>> +               break;
>> +       case DPU_CHROMA_420:
>> +               cdm_cfg->h_cdwn_type = CDM_CDWN_COSITE;
>> +               cdm_cfg->v_cdwn_type = CDM_CDWN_OFFSITE;
>> +               break;
>> +       case DPU_CHROMA_H1V2:
>> +       default:
>> +               DPU_ERROR("[enc:%d] unsupported chroma sampling type\n",
>> +                         DRMID(phys_enc->parent));
>> +               cdm_cfg->h_cdwn_type = CDM_CDWN_DISABLE;
>> +               cdm_cfg->v_cdwn_type = CDM_CDWN_DISABLE;
>> +               break;
>> +       }
>> +
>> +       DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%X,%d,%d,%d,%d]\n",
>> +                 DRMID(phys_enc->parent), cdm_cfg->output_width,
>> +                 cdm_cfg->output_height, cdm_cfg->output_fmt->base.pixel_format,
>> +                 cdm_cfg->output_type, cdm_cfg->output_bit_depth,
>> +                 cdm_cfg->h_cdwn_type, cdm_cfg->v_cdwn_type);
>> +
>> +       if (hw_cdm && hw_cdm->ops.setup_csc_data) {
>> +               ret = hw_cdm->ops.setup_csc_data(hw_cdm, &dpu_encoder_phys_wb_rgb2yuv_601l);
>> +               if (ret < 0) {
>> +                       DPU_ERROR("[enc:%d] failed to setup CSC; ret:%d\n",
>> +                                 DRMID(phys_enc->parent), ret);
>> +                       return;
>> +               }
>> +       }
>> +
>> +       if (hw_cdm && hw_cdm->ops.setup_cdwn) {
> 
> You have checked for (!hw_cdm) several lines above. We can drop this
> condition here.
> 

Ack.

>> +               ret = hw_cdm->ops.setup_cdwn(hw_cdm, cdm_cfg);
>> +               if (ret < 0) {
>> +                       DPU_ERROR("[enc:%d] failed to setup CDWN; ret:%d\n",
>> +                                 DRMID(phys_enc->parent), ret);
>> +                       return;
>> +               }
>> +       }
>> +
>> +       if (hw_cdm && hw_pp && hw_cdm->ops.enable) {
> 
> And what if !hw_pp ? Can it happen here? No, if I understand correctly.
> 

I dont see any other protection for !hw_pp in this flow so would prefer 
to keep it.

>> +               cdm_cfg->pp_id = hw_pp->idx;
>> +               ret = hw_cdm->ops.enable(hw_cdm, cdm_cfg);
> 
> As we are calling these three ops in a row, can we merge them together
> into a single callback to be called from dpu_encoder.c?
> 

Good idea. I can add a csc_cfg entry to cdm_cfg and merge all three into 
the enable() op itself and drop the other two.

>> +               if (ret < 0) {
>> +                       DPU_ERROR("[enc:%d] failed to enable CDM; ret:%d\n",
>> +                                 DRMID(phys_enc->parent), ret);
>> +                       return;
>> +               }
>> +       }
>> +}
>> +
>>   /**
>>    * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
>>    * @phys_enc:  Pointer to physical encoder
>> @@ -348,8 +468,9 @@ static void dpu_encoder_phys_wb_setup(
>>
>>          dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>
>> -       dpu_encoder_phys_wb_setup_ctl(phys_enc);
>> +       dpu_encoder_helper_phys_setup_cdm(phys_enc);
>>
>> +       dpu_encoder_phys_wb_setup_ctl(phys_enc);
>>   }
>>
>>   static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
>> --
>> 2.40.1
>>
> 
>