On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>> ---
> >>> .../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.
--
With best wishes
Dmitry
