[PATCH 01/17] drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon Jan 29 04:00:15 UTC 2024
On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
>>>
>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>>>
>>>>> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
>>>>> ---
>>>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 +--
>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 ++++++++++---------
>>>>> 2 files changed, 18 insertions(+), 17 deletions(-)
>>>>>
>>>>> 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 993f263433314..37ac385727c3b 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>>> * @hw_intf: Hardware interface to the intf registers
>>>>> * @hw_wb: Hardware interface to the wb registers
>>>>> * @hw_cdm: Hardware interface to the CDM registers
>>>>> + * @cdm_cfg: CDM block config needed to store WB/DP block's CDM
>>>>> configuration
>>>>
>>>> Please realign the description.
>>> Ack
>>>>
>>>>> * @dpu_kms: Pointer to the dpu_kms top level
>>>>> * @cached_mode: DRM mode cached at mode_set time, acted on in
>>>>> enable
>>>>> * @vblank_ctl_lock: Vblank ctl mutex lock to protect
>>>>> vblank_refcount
>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>>> struct dpu_hw_intf *hw_intf;
>>>>> struct dpu_hw_wb *hw_wb;
>>>>> struct dpu_hw_cdm *hw_cdm;
>>>>> + struct dpu_hw_cdm_cfg cdm_cfg;
>>>>
>>>> It might be slightly better to move it after all the pointers, so
>>>> after the dpu_kms.
>>> Ack
>>>>
>>>>> struct dpu_kms *dpu_kms;
>>>>> struct drm_display_mode cached_mode;
>>>>> struct mutex vblank_ctl_lock;
>>>>> @@ -213,7 +215,6 @@ 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
>>>>> @@ -223,7 +224,6 @@ 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 4cd2d9e3131a4..072fc6950e496 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
>>>>> @@ -269,28 +269,21 @@ static void
>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>>> * This API does not handle
>>>>> DPU_CHROMA_H1V2.
>>>>> * @phys_enc:Pointer to physical encoder
>>>>> */
>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>> dpu_encoder_phys *phys_enc)
>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>> dpu_encoder_phys *phys_enc,
>>>>> + const struct dpu_format *dpu_fmt,
>>>>> + u32 output_type)
>>>>> {
>>>>> 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;
>>>>> + cdm_cfg = &phys_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;
>>>>> @@ -306,10 +299,10 @@ static void
>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>>> 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_width = phys_enc->cached_mode.hdisplay;
>>>>> + cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>>>
>>>> This is a semantic change. Instead of passing the FB size, this passes
>>>> the mode dimensions. They are not guaranteed to be the same,
>>>> especially for the WB case.
>>>>
>>
>> The WB job is storing the output FB of WB. I cannot think of a use-case
>> where this cannot match the current mode programmed to the WB encoder.
>>
>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
>> plane can scale the contents but here thats not the case. Here its the
>> output FB of WB.
>
> Is it a part of WB uAPI, to have the FB dimensions equal to mode
> dimensions? Or is it just our current limitation? I can easily imagine
> WB outputting data to a part of the FB (just like we can clip FB using
> plane's clip rectangle).
>
> This boils down to a question, whether CDM should be setup in terms of
> actual output date or the physical memory buffer parameters. I suspect
> the former is the case (which makes this change correct). But it
> either should be described in the commit message or (even better)
> split to a separate commit.
>
I would say its a combination of both today.
The way I would look at it is even if WB crops a certain section of FB,
that will not change the FB size. FB size of WB should match the rest of
the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
decides to write to only a small section of FB (cropping), then we need
another WB property like CROP_ROI so that we can program the WB to only
write to a small section of the programmed FB. So in some sense, there
is no such support in DRM UAPI today. Hence the FB of WB is the full
mode of the WB.
CDM is before WB so follows the rest of the pipeline that is whatever
the data feeding it was programmed to.
>
More information about the dri-devel
mailing list