| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3085d544-b6d9-5064-2789-2bbccf4b6818@quicinc.com>
Date: Thu, 30 Nov 2023 16:41:55 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <freedreno@...ts.freedesktop.org>, Rob Clark <robdclark@...il.com>,
"Sean Paul" <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
"David Airlie" <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
<dri-devel@...ts.freedesktop.org>, <quic_jesszhan@...cinc.com>,
<quic_parellan@...cinc.com>, <quic_khsieh@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/16] drm/msm/dpu: add an API to setup the CDM block for
writeback
On 8/30/2023 5:11 PM, Dmitry Baryshkov wrote:
> On Thu, 31 Aug 2023 at 01:50, Abhinav Kumar <quic_abhinavk@...cinc.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@...cinc.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
>>
>
>
Powered by blists - more mailing lists