Re: [PATCH 01/17] drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux