[Freedreno] [PATCH 11/16] drm/msm/dpu: add an API to setup the CDM block for writeback
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Dec 1 19:04:32 UTC 2023
On 11/30/2023 11:20 PM, Dmitry Baryshkov wrote:
> On Fri, 1 Dec 2023 at 02:41, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 8/30/2023 5:11 PM, Dmitry Baryshkov wrote:
>>> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk at 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 at 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.
>
> 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.
>>
>>>> +
>>>> /**
>>>> * 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.
>>
>>>> + 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
>>>>
>>>
>>>
>
>
>
More information about the Freedreno
mailing list