On Fri, 1 Dec 2023 at 21:04, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 11/30/2023 11:20 PM, Dmitry Baryshkov wrote:
> > On Fri, 1 Dec 2023 at 02:41, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 8/30/2023 5:11 PM, Dmitry Baryshkov wrote:
> >>> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>> .../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.
> >
> > Doing it earlier is usually better. Can we please do it as a part of
> > this series?
> >
>
> Would be strange as RGB2YUV matrix is not used by anyone other than WB
> till DP lands.
>
> If you are fine with that anomaly, no concerns.
Yes. Because it keeps all instances in a single place.
>
> >>
> >>>> +
> >>>> /**
> >>>> * 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.
> >
> > But can we end up in this function if we have no hw_pp at all?
> >
>
> Just from code flow yes,
>
> dpu_encoder_prepare_for_kickoff ---> phys->ops.prepare_for_kickoff --->
> this function
>
> None of them have !hw_pp.
>
> But, if hw_pp failed allocation, then atomic_check will fail so the
> commit will not happen.
>
> I was thinking of the former, if we are fine with the latter we can drop.
Yes, I'm fine with that, unless there is any special mode (like
ppsplit, cwb, etc.) where we can end up with no PP assigned on
purpose.
>
> >>
> >>>> + 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
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
