| Message ID | 20240125193834.7065-2-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for CDM over DP | expand |
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@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. > * @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. > 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. > cdm_cfg->output_fmt = dpu_fmt; > - cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB; > + cdm_cfg->output_type = output_type; > cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ? > CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT; > cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l; > @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup( > struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; > struct drm_display_mode mode = phys_enc->cached_mode; > struct drm_framebuffer *fb = NULL; > + struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); > + struct drm_writeback_job *wb_job; > + const struct msm_format *format; > + const struct dpu_format *dpu_fmt; > + > + 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); > > DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", > hw_wb->idx - WB_0, mode.name, > @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup( > > dpu_encoder_phys_wb_setup_fb(phys_enc, fb); > > - dpu_encoder_helper_phys_setup_cdm(phys_enc); > + dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); > > dpu_encoder_phys_wb_setup_ctl(phys_enc); > }
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@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. > >> cdm_cfg->output_fmt = dpu_fmt; >> - cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB; >> + cdm_cfg->output_type = output_type; >> cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ? >> CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT; >> cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l; >> @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup( >> struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; >> struct drm_display_mode mode = phys_enc->cached_mode; >> struct drm_framebuffer *fb = NULL; >> + struct dpu_encoder_phys_wb *wb_enc = >> to_dpu_encoder_phys_wb(phys_enc); >> + struct drm_writeback_job *wb_job; >> + const struct msm_format *format; >> + const struct dpu_format *dpu_fmt; >> + >> + 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); >> DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", >> hw_wb->idx - WB_0, mode.name, >> @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup( >> dpu_encoder_phys_wb_setup_fb(phys_enc, fb); >> - dpu_encoder_helper_phys_setup_cdm(phys_enc); >> + dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, >> CDM_CDWN_OUTPUT_WB); >> dpu_encoder_phys_wb_setup_ctl(phys_enc); >> } >
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@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. >>> cdm_cfg->output_fmt = dpu_fmt; >>> - cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB; >>> + cdm_cfg->output_type = output_type; >>> cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ? >>> CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT; >>> cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l; >>> @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup( >>> struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; >>> struct drm_display_mode mode = phys_enc->cached_mode; >>> struct drm_framebuffer *fb = NULL; >>> + struct dpu_encoder_phys_wb *wb_enc = >>> to_dpu_encoder_phys_wb(phys_enc); >>> + struct drm_writeback_job *wb_job; >>> + const struct msm_format *format; >>> + const struct dpu_format *dpu_fmt; >>> + >>> + 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); >>> DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", >>> hw_wb->idx - WB_0, mode.name, >>> @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup( >>> dpu_encoder_phys_wb_setup_fb(phys_enc, fb); >>> - dpu_encoder_helper_phys_setup_cdm(phys_enc); >>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, >>> CDM_CDWN_OUTPUT_WB); >>> dpu_encoder_phys_wb_setup_ctl(phys_enc); >>> } >>
On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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.
On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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. >
On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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. I'd say, CROP_ROI can refer to cropping of the image source (esp. in the cloned output case). For writing to the part of the FB there can be DST_X/_Y/_W/_H properties. But this becomes off-topic. > CDM is before WB so follows the rest of the pipeline that is whatever > the data feeding it was programmed to. Yes. So the change is correct, but it should be split or documented properly. I prefer the first option.
On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote: >>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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. > > I'd say, CROP_ROI can refer to cropping of the image source (esp. in > the cloned output case). For writing to the part of the FB there can > be DST_X/_Y/_W/_H properties. But this becomes off-topic. > >> CDM is before WB so follows the rest of the pipeline that is whatever >> the data feeding it was programmed to. > > Yes. So the change is correct, but it should be split or documented > properly. I prefer the first option. > Ok just to clarify you prefer below part of the change to be moved to its own commit right? - 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; If so, ack. >
On Mon, 29 Jan 2024 at 06:33, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote: > >>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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. > > > > I'd say, CROP_ROI can refer to cropping of the image source (esp. in > > the cloned output case). For writing to the part of the FB there can > > be DST_X/_Y/_W/_H properties. But this becomes off-topic. > > > >> CDM is before WB so follows the rest of the pipeline that is whatever > >> the data feeding it was programmed to. > > > > Yes. So the change is correct, but it should be split or documented > > properly. I prefer the first option. > > > > Ok just to clarify you prefer below part of the change to be moved to > its own commit right? > > - 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; > > If so, ack. Yes.
On 1/28/2024 9:12 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 06:33, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote: >>> On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote: >>>>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@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@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. >>> I'd say, CROP_ROI can refer to cropping of the image source (esp. in >>> the cloned output case). For writing to the part of the FB there can >>> be DST_X/_Y/_W/_H properties. But this becomes off-topic. >>> >>>> CDM is before WB so follows the rest of the pipeline that is whatever >>>> the data feeding it was programmed to. >>> Yes. So the change is correct, but it should be split or documented >>> properly. I prefer the first option. >>> >> Ok just to clarify you prefer below part of the change to be moved to >> its own commit right? >> >> - 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; >> >> If so, ack. > Yes. Ack will change in next version. > >
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 * @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; 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; cdm_cfg->output_fmt = dpu_fmt; - cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB; + cdm_cfg->output_type = output_type; cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ? CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT; cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l; @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup( struct dpu_hw_wb *hw_wb = phys_enc->hw_wb; struct drm_display_mode mode = phys_enc->cached_mode; struct drm_framebuffer *fb = NULL; + struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc); + struct drm_writeback_job *wb_job; + const struct msm_format *format; + const struct dpu_format *dpu_fmt; + + 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); DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n", hw_wb->idx - WB_0, mode.name, @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup( dpu_encoder_phys_wb_setup_fb(phys_enc, fb); - dpu_encoder_helper_phys_setup_cdm(phys_enc); + dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB); dpu_encoder_phys_wb_setup_ctl(phys_enc); }
Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP. Signed-off-by: Paloma Arellano <quic_parellan@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(-)