On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
> > 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.
> >
>
> 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.
--
With best wishes
Dmitry
