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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.


-- 
With best wishes
Dmitry