| Message ID | 1650846730-19226-11-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add writeback block support for DPU | expand |
On 25/04/2022 03:32, Abhinav Kumar wrote: > Make changes to dpu_encoder to support virtual encoder needed > to support writeback for dpu. > > changes in v4: > - squash dpu_encoder pieces from [1] > > [1] https://patchwork.freedesktop.org/patch/483099/?series=102964&rev=2 > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 +++++++++++++++++++----- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 7 ++ > 2 files changed, 83 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 25c7eda..d1e92d89 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1013,9 +1013,18 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) > phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx); > > - if (!phys->hw_intf) { > + if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX) > + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx); > + > + if (!phys->hw_intf && !phys->hw_wb) { > DPU_ERROR_ENC(dpu_enc, > - "no intf block assigned at idx: %d\n", i); > + "no intf or wb block assigned at idx: %d\n", i); > + return; > + } > + > + if (phys->hw_intf && phys->hw_wb) { > + DPU_ERROR_ENC(dpu_enc, > + "invalid phys both intf and wb block at idx: %d\n", i); > return; > } > > @@ -1163,16 +1172,35 @@ static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > { > int i = 0; > > - for (i = 0; i < catalog->intf_count; i++) { > - if (catalog->intf[i].type == type > - && catalog->intf[i].controller_id == controller_id) { > - return catalog->intf[i].id; > + if (type != INTF_WB) { > + for (i = 0; i < catalog->intf_count; i++) { > + if (catalog->intf[i].type == type > + && catalog->intf[i].controller_id == controller_id) { > + return catalog->intf[i].id; > + } > } > } > > return INTF_MAX; > } > > +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog, > + enum dpu_intf_type type, u32 controller_id) > +{ > + int i = 0; > + > + if (type != INTF_WB) > + goto end; > + > + for (i = 0; i < catalog->wb_count; i++) { > + if (catalog->wb[i].id == controller_id) > + return catalog->wb[i].id; > + } > + > +end: > + return WB_MAX; > +} > + > static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, > struct dpu_encoder_phys *phy_enc) > { > @@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) > > dpu_encoder_helper_reset_mixers(phys_enc); > > - for (i = 0; i < dpu_enc->num_phys_encs; i++) { > - if (dpu_enc->phys_encs[i] && phys_enc->hw_intf->ops.bind_pingpong_blk) > - phys_enc->hw_intf->ops.bind_pingpong_blk( > - dpu_enc->phys_encs[i]->hw_intf, false, > - dpu_enc->phys_encs[i]->hw_pp->idx); > - > - /* mark INTF flush as pending */ > - if (phys_enc->hw_ctl->ops.update_pending_flush_intf) > - phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, > - dpu_enc->phys_encs[i]->hw_intf->idx); > + /* > + * TODO: move the once-only operation like CTL flush/trigger > + * into dpu_encoder_virt_disable() and all operations which need > + * to be done per phys encoder into the phys_disable() op. > + */ > + if (phys_enc->hw_wb) { > + /* disable the PP block */ > + if (phys_enc->hw_wb->ops.bind_pingpong_blk) > + phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, false, > + phys_enc->hw_pp->idx); > + > + /* mark WB flush as pending */ > + if (phys_enc->hw_ctl->ops.update_pending_flush_wb) > + phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, phys_enc->hw_wb->idx); > + } else { > + for (i = 0; i < dpu_enc->num_phys_encs; i++) { > + if (dpu_enc->phys_encs[i] && phys_enc->hw_intf->ops.bind_pingpong_blk) > + phys_enc->hw_intf->ops.bind_pingpong_blk( > + dpu_enc->phys_encs[i]->hw_intf, false, > + dpu_enc->phys_encs[i]->hw_pp->idx); > + > + /* mark INTF flush as pending */ > + if (phys_enc->hw_ctl->ops.update_pending_flush_intf) > + phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, > + dpu_enc->phys_encs[i]->hw_intf->idx); > + } > } > > /* reset the merge 3D HW block */ > @@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > case DRM_MODE_ENCODER_TMDS: > intf_type = INTF_DP; > break; > + case DRM_MODE_ENCODER_VIRTUAL: > + intf_type = INTF_WB; > + break; > } > > WARN_ON(disp_info->num_of_h_tiles < 1); > @@ -2149,8 +2196,19 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, > intf_type, > controller_id); > - if (phys_params.intf_idx == INTF_MAX) { > - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", > + > + phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, > + intf_type, controller_id); > + /* > + * For boards which have no physical displays, having no interface > + * is fine because it can still be used with just writeback. > + * If we try without a display on a board which uses a DPU in which > + * writeback is not supported, then this will still fail as it will not > + * find any writeback in the catalog. > + */ > + if ((phys_params.intf_idx == INTF_MAX) && > + (phys_params.wb_idx == WB_MAX)) { > + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", I've commented the previous iteration by the mistake, but the comment still applies to this version too. Let's repeat it, so that we have all the comments in the single version. I think the comment is misleading. It is a phys_params, so it must have etiher intf_idx or wb_idx, but not both of them, despite the board having the interfaces or WB. So somthing like this might be more appropriate: /* * The phys_params might represent either an INTF or a WB unit, but not * both of them at the same time. */ if ((phys_params.intf_idx == INTF_MAX) && (phys_params.wb_idx == WB_MAX)) { .... // error out } if ((phys_params.intf_idx != INTF_MAX) && (phys_params.wb_idx == WB_MAX)) { ........ // error out } Looks good otherwise. > intf_type, controller_id); > ret = -EINVAL; > } > 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 544a9a4..c84b8e8 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > @@ -11,6 +11,7 @@ > > #include "dpu_kms.h" > #include "dpu_hw_intf.h" > +#include "dpu_hw_wb.h" > #include "dpu_hw_pingpong.h" > #include "dpu_hw_ctl.h" > #include "dpu_hw_top.h" > @@ -165,12 +166,14 @@ enum dpu_intr_idx { > * @hw_ctl: Hardware interface to the ctl registers > * @hw_pp: Hardware interface to the ping pong registers > * @hw_intf: Hardware interface to the intf registers > + * @hw_wb: Hardware interface to the wb registers > * @dpu_kms: Pointer to the dpu_kms top level > * @cached_mode: DRM mode cached at mode_set time, acted on in enable > * @enabled: Whether the encoder has enabled and running a mode > * @split_role: Role to play in a split-panel configuration > * @intf_mode: Interface mode > * @intf_idx: Interface index on dpu hardware > + * @wb_idx: Writeback index on dpu hardware > * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes > * @enable_state: Enable state tracking > * @vblank_refcount: Reference count of vblank request > @@ -193,11 +196,13 @@ struct dpu_encoder_phys { > struct dpu_hw_ctl *hw_ctl; > struct dpu_hw_pingpong *hw_pp; > struct dpu_hw_intf *hw_intf; > + struct dpu_hw_wb *hw_wb; > struct dpu_kms *dpu_kms; > struct drm_display_mode cached_mode; > enum dpu_enc_split_role split_role; > enum dpu_intf_mode intf_mode; > enum dpu_intf intf_idx; > + enum dpu_wb wb_idx; > spinlock_t *enc_spinlock; > enum dpu_enc_enable_state enable_state; > atomic_t vblank_refcount; > @@ -243,6 +248,7 @@ struct dpu_encoder_phys_cmd { > * @parent_ops: Callbacks exposed by the parent to the phys_enc > * @split_role: Role to play in a split-panel configuration > * @intf_idx: Interface index this phys_enc will control > + * @wb_idx: Writeback index this phys_enc will control > * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes > */ > struct dpu_enc_phys_init_params { > @@ -251,6 +257,7 @@ struct dpu_enc_phys_init_params { > const struct dpu_encoder_virt_ops *parent_ops; > enum dpu_enc_split_role split_role; > enum dpu_intf intf_idx; > + enum dpu_wb wb_idx; > spinlock_t *enc_spinlock; > }; >
Hi Dmitry On 4/25/2022 5:21 PM, Dmitry Baryshkov wrote: > On 25/04/2022 03:32, Abhinav Kumar wrote: >> Make changes to dpu_encoder to support virtual encoder needed >> to support writeback for dpu. >> >> changes in v4: >> - squash dpu_encoder pieces from [1] >> >> [1] https://patchwork.freedesktop.org/patch/483099/?series=102964&rev=2 >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 >> +++++++++++++++++++----- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 7 ++ >> 2 files changed, 83 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 25c7eda..d1e92d89 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -1013,9 +1013,18 @@ static void >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >> if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) >> phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >> phys->intf_idx); >> - if (!phys->hw_intf) { >> + if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX) >> + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx); >> + >> + if (!phys->hw_intf && !phys->hw_wb) { >> DPU_ERROR_ENC(dpu_enc, >> - "no intf block assigned at idx: %d\n", i); >> + "no intf or wb block assigned at idx: %d\n", i); >> + return; >> + } >> + >> + if (phys->hw_intf && phys->hw_wb) { >> + DPU_ERROR_ENC(dpu_enc, >> + "invalid phys both intf and wb block at idx: >> %d\n", i); >> return; >> } >> @@ -1163,16 +1172,35 @@ static enum dpu_intf >> dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, >> { >> int i = 0; >> - for (i = 0; i < catalog->intf_count; i++) { >> - if (catalog->intf[i].type == type >> - && catalog->intf[i].controller_id == controller_id) { >> - return catalog->intf[i].id; >> + if (type != INTF_WB) { >> + for (i = 0; i < catalog->intf_count; i++) { >> + if (catalog->intf[i].type == type >> + && catalog->intf[i].controller_id == controller_id) { >> + return catalog->intf[i].id; >> + } >> } >> } >> return INTF_MAX; >> } >> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog, >> + enum dpu_intf_type type, u32 controller_id) >> +{ >> + int i = 0; >> + >> + if (type != INTF_WB) >> + goto end; >> + >> + for (i = 0; i < catalog->wb_count; i++) { >> + if (catalog->wb[i].id == controller_id) >> + return catalog->wb[i].id; >> + } >> + >> +end: >> + return WB_MAX; >> +} >> + >> static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >> struct dpu_encoder_phys *phy_enc) >> { >> @@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct >> dpu_encoder_phys *phys_enc) >> dpu_encoder_helper_reset_mixers(phys_enc); >> - for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> - if (dpu_enc->phys_encs[i] && >> phys_enc->hw_intf->ops.bind_pingpong_blk) >> - phys_enc->hw_intf->ops.bind_pingpong_blk( >> - dpu_enc->phys_encs[i]->hw_intf, false, >> - dpu_enc->phys_encs[i]->hw_pp->idx); >> - >> - /* mark INTF flush as pending */ >> - if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >> - >> phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >> - dpu_enc->phys_encs[i]->hw_intf->idx); >> + /* >> + * TODO: move the once-only operation like CTL flush/trigger >> + * into dpu_encoder_virt_disable() and all operations which need >> + * to be done per phys encoder into the phys_disable() op. >> + */ >> + if (phys_enc->hw_wb) { >> + /* disable the PP block */ >> + if (phys_enc->hw_wb->ops.bind_pingpong_blk) >> + phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, >> false, >> + phys_enc->hw_pp->idx); >> + >> + /* mark WB flush as pending */ >> + if (phys_enc->hw_ctl->ops.update_pending_flush_wb) >> + phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, >> phys_enc->hw_wb->idx); >> + } else { >> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> + if (dpu_enc->phys_encs[i] && >> phys_enc->hw_intf->ops.bind_pingpong_blk) >> + phys_enc->hw_intf->ops.bind_pingpong_blk( >> + dpu_enc->phys_encs[i]->hw_intf, false, >> + dpu_enc->phys_encs[i]->hw_pp->idx); >> + >> + /* mark INTF flush as pending */ >> + if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >> + >> phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >> + dpu_enc->phys_encs[i]->hw_intf->idx); >> + } >> } >> /* reset the merge 3D HW block */ >> @@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> case DRM_MODE_ENCODER_TMDS: >> intf_type = INTF_DP; >> break; >> + case DRM_MODE_ENCODER_VIRTUAL: >> + intf_type = INTF_WB; >> + break; >> } >> WARN_ON(disp_info->num_of_h_tiles < 1); >> @@ -2149,8 +2196,19 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >> intf_type, >> controller_id); >> - if (phys_params.intf_idx == INTF_MAX) { >> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id >> %d\n", >> + >> + phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, >> + intf_type, controller_id); >> + /* >> + * For boards which have no physical displays, having no >> interface >> + * is fine because it can still be used with just writeback. >> + * If we try without a display on a board which uses a DPU in >> which >> + * writeback is not supported, then this will still fail as >> it will not >> + * find any writeback in the catalog. >> + */ >> + if ((phys_params.intf_idx == INTF_MAX) && >> + (phys_params.wb_idx == WB_MAX)) { >> + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type >> %d, id %d\n", > > I've commented the previous iteration by the mistake, but the comment > still applies to this version too. Let's repeat it, so that we have all > the comments in the single version. > > I think the comment is misleading. It is a phys_params, so it must have > etiher intf_idx or wb_idx, but not both of them, despite the board > having the interfaces or WB. > > So somthing like this might be more appropriate: > > /* > * The phys_params might represent either an INTF or a WB unit, but not > * both of them at the same time. > */ > > if ((phys_params.intf_idx == INTF_MAX) && (phys_params.wb_idx == WB_MAX)) { > .... // error out > } > > if ((phys_params.intf_idx != INTF_MAX) && (phys_params.wb_idx == WB_MAX)) { > ........ // error out > } I didnt understand this condition. So if there is a valid intf but not valid wb intf, why is that an error. > > > Looks good otherwise. > > >> intf_type, controller_id); >> ret = -EINVAL; >> } >> 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 544a9a4..c84b8e8 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> @@ -11,6 +11,7 @@ >> #include "dpu_kms.h" >> #include "dpu_hw_intf.h" >> +#include "dpu_hw_wb.h" >> #include "dpu_hw_pingpong.h" >> #include "dpu_hw_ctl.h" >> #include "dpu_hw_top.h" >> @@ -165,12 +166,14 @@ enum dpu_intr_idx { >> * @hw_ctl: Hardware interface to the ctl registers >> * @hw_pp: Hardware interface to the ping pong registers >> * @hw_intf: Hardware interface to the intf registers >> + * @hw_wb: Hardware interface to the wb registers >> * @dpu_kms: Pointer to the dpu_kms top level >> * @cached_mode: DRM mode cached at mode_set time, acted on in >> enable >> * @enabled: Whether the encoder has enabled and running a mode >> * @split_role: Role to play in a split-panel configuration >> * @intf_mode: Interface mode >> * @intf_idx: Interface index on dpu hardware >> + * @wb_idx: Writeback index on dpu hardware >> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes >> * @enable_state: Enable state tracking >> * @vblank_refcount: Reference count of vblank request >> @@ -193,11 +196,13 @@ struct dpu_encoder_phys { >> struct dpu_hw_ctl *hw_ctl; >> struct dpu_hw_pingpong *hw_pp; >> struct dpu_hw_intf *hw_intf; >> + struct dpu_hw_wb *hw_wb; >> struct dpu_kms *dpu_kms; >> struct drm_display_mode cached_mode; >> enum dpu_enc_split_role split_role; >> enum dpu_intf_mode intf_mode; >> enum dpu_intf intf_idx; >> + enum dpu_wb wb_idx; >> spinlock_t *enc_spinlock; >> enum dpu_enc_enable_state enable_state; >> atomic_t vblank_refcount; >> @@ -243,6 +248,7 @@ struct dpu_encoder_phys_cmd { >> * @parent_ops: Callbacks exposed by the parent to the phys_enc >> * @split_role: Role to play in a split-panel configuration >> * @intf_idx: Interface index this phys_enc will control >> + * @wb_idx: Writeback index this phys_enc will control >> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes >> */ >> struct dpu_enc_phys_init_params { >> @@ -251,6 +257,7 @@ struct dpu_enc_phys_init_params { >> const struct dpu_encoder_virt_ops *parent_ops; >> enum dpu_enc_split_role split_role; >> enum dpu_intf intf_idx; >> + enum dpu_wb wb_idx; >> spinlock_t *enc_spinlock; >> }; > >
On 26/04/2022 03:44, Abhinav Kumar wrote: > Hi Dmitry > > On 4/25/2022 5:21 PM, Dmitry Baryshkov wrote: >> On 25/04/2022 03:32, Abhinav Kumar wrote: >>> Make changes to dpu_encoder to support virtual encoder needed >>> to support writeback for dpu. >>> >>> changes in v4: >>> - squash dpu_encoder pieces from [1] >>> >>> [1] https://patchwork.freedesktop.org/patch/483099/?series=102964&rev=2 >>> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 >>> +++++++++++++++++++----- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 7 ++ >>> 2 files changed, 83 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 25c7eda..d1e92d89 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -1013,9 +1013,18 @@ static void >>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>> if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) >>> phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>> phys->intf_idx); >>> - if (!phys->hw_intf) { >>> + if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX) >>> + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx); >>> + >>> + if (!phys->hw_intf && !phys->hw_wb) { >>> DPU_ERROR_ENC(dpu_enc, >>> - "no intf block assigned at idx: %d\n", i); >>> + "no intf or wb block assigned at idx: %d\n", i); >>> + return; >>> + } >>> + >>> + if (phys->hw_intf && phys->hw_wb) { >>> + DPU_ERROR_ENC(dpu_enc, >>> + "invalid phys both intf and wb block at idx: >>> %d\n", i); >>> return; >>> } >>> @@ -1163,16 +1172,35 @@ static enum dpu_intf >>> dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, >>> { >>> int i = 0; >>> - for (i = 0; i < catalog->intf_count; i++) { >>> - if (catalog->intf[i].type == type >>> - && catalog->intf[i].controller_id == controller_id) { >>> - return catalog->intf[i].id; >>> + if (type != INTF_WB) { >>> + for (i = 0; i < catalog->intf_count; i++) { >>> + if (catalog->intf[i].type == type >>> + && catalog->intf[i].controller_id == controller_id) { >>> + return catalog->intf[i].id; >>> + } >>> } >>> } >>> return INTF_MAX; >>> } >>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog, >>> + enum dpu_intf_type type, u32 controller_id) >>> +{ >>> + int i = 0; >>> + >>> + if (type != INTF_WB) >>> + goto end; >>> + >>> + for (i = 0; i < catalog->wb_count; i++) { >>> + if (catalog->wb[i].id == controller_id) >>> + return catalog->wb[i].id; >>> + } >>> + >>> +end: >>> + return WB_MAX; >>> +} >>> + >>> static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >>> struct dpu_encoder_phys *phy_enc) >>> { >>> @@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct >>> dpu_encoder_phys *phys_enc) >>> dpu_encoder_helper_reset_mixers(phys_enc); >>> - for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>> - if (dpu_enc->phys_encs[i] && >>> phys_enc->hw_intf->ops.bind_pingpong_blk) >>> - phys_enc->hw_intf->ops.bind_pingpong_blk( >>> - dpu_enc->phys_encs[i]->hw_intf, false, >>> - dpu_enc->phys_encs[i]->hw_pp->idx); >>> - >>> - /* mark INTF flush as pending */ >>> - if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >>> - phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >>> - dpu_enc->phys_encs[i]->hw_intf->idx); >>> + /* >>> + * TODO: move the once-only operation like CTL flush/trigger >>> + * into dpu_encoder_virt_disable() and all operations which need >>> + * to be done per phys encoder into the phys_disable() op. >>> + */ >>> + if (phys_enc->hw_wb) { >>> + /* disable the PP block */ >>> + if (phys_enc->hw_wb->ops.bind_pingpong_blk) >>> + phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, >>> false, >>> + phys_enc->hw_pp->idx); >>> + >>> + /* mark WB flush as pending */ >>> + if (phys_enc->hw_ctl->ops.update_pending_flush_wb) >>> + phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, >>> phys_enc->hw_wb->idx); >>> + } else { >>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>> + if (dpu_enc->phys_encs[i] && >>> phys_enc->hw_intf->ops.bind_pingpong_blk) >>> + phys_enc->hw_intf->ops.bind_pingpong_blk( >>> + dpu_enc->phys_encs[i]->hw_intf, false, >>> + dpu_enc->phys_encs[i]->hw_pp->idx); >>> + >>> + /* mark INTF flush as pending */ >>> + if (phys_enc->hw_ctl->ops.update_pending_flush_intf) >>> + phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, >>> + dpu_enc->phys_encs[i]->hw_intf->idx); >>> + } >>> } >>> /* reset the merge 3D HW block */ >>> @@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> case DRM_MODE_ENCODER_TMDS: >>> intf_type = INTF_DP; >>> break; >>> + case DRM_MODE_ENCODER_VIRTUAL: >>> + intf_type = INTF_WB; >>> + break; >>> } >>> WARN_ON(disp_info->num_of_h_tiles < 1); >>> @@ -2149,8 +2196,19 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >>> intf_type, >>> controller_id); >>> - if (phys_params.intf_idx == INTF_MAX) { >>> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id >>> %d\n", >>> + >>> + phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, >>> + intf_type, controller_id); >>> + /* >>> + * For boards which have no physical displays, having no >>> interface >>> + * is fine because it can still be used with just writeback. >>> + * If we try without a display on a board which uses a DPU >>> in which >>> + * writeback is not supported, then this will still fail as >>> it will not >>> + * find any writeback in the catalog. >>> + */ >>> + if ((phys_params.intf_idx == INTF_MAX) && >>> + (phys_params.wb_idx == WB_MAX)) { >>> + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type >>> %d, id %d\n", >> >> I've commented the previous iteration by the mistake, but the comment >> still applies to this version too. Let's repeat it, so that we have >> all the comments in the single version. >> >> I think the comment is misleading. It is a phys_params, so it must have >> etiher intf_idx or wb_idx, but not both of them, despite the board >> having the interfaces or WB. >> >> So somthing like this might be more appropriate: >> >> /* >> * The phys_params might represent either an INTF or a WB unit, but not >> * both of them at the same time. >> */ >> >> if ((phys_params.intf_idx == INTF_MAX) && (phys_params.wb_idx == >> WB_MAX)) { >> .... // error out >> } >> >> if ((phys_params.intf_idx != INTF_MAX) && (phys_params.wb_idx == >> WB_MAX)) { >> ........ // error out >> } > > I didnt understand this condition. So if there is a valid intf but not > valid wb intf, why is that an error. > c&p error. of course: if ((intf_idx == INTF_MAX) && (wb_idx == INTF_MAX)) {...} if ((intf_idx != INTF_MAX) && (wb_idx != INTF_MAX)) {...} >> >> >> Looks good otherwise. >> >> >>> intf_type, controller_id); >>> ret = -EINVAL; >>> } >>> 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 544a9a4..c84b8e8 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>> @@ -11,6 +11,7 @@ >>> #include "dpu_kms.h" >>> #include "dpu_hw_intf.h" >>> +#include "dpu_hw_wb.h" >>> #include "dpu_hw_pingpong.h" >>> #include "dpu_hw_ctl.h" >>> #include "dpu_hw_top.h" >>> @@ -165,12 +166,14 @@ enum dpu_intr_idx { >>> * @hw_ctl: Hardware interface to the ctl registers >>> * @hw_pp: Hardware interface to the ping pong registers >>> * @hw_intf: Hardware interface to the intf registers >>> + * @hw_wb: Hardware interface to the wb registers >>> * @dpu_kms: Pointer to the dpu_kms top level >>> * @cached_mode: DRM mode cached at mode_set time, acted on in >>> enable >>> * @enabled: Whether the encoder has enabled and running a mode >>> * @split_role: Role to play in a split-panel configuration >>> * @intf_mode: Interface mode >>> * @intf_idx: Interface index on dpu hardware >>> + * @wb_idx: Writeback index on dpu hardware >>> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes >>> * @enable_state: Enable state tracking >>> * @vblank_refcount: Reference count of vblank request >>> @@ -193,11 +196,13 @@ struct dpu_encoder_phys { >>> struct dpu_hw_ctl *hw_ctl; >>> struct dpu_hw_pingpong *hw_pp; >>> struct dpu_hw_intf *hw_intf; >>> + struct dpu_hw_wb *hw_wb; >>> struct dpu_kms *dpu_kms; >>> struct drm_display_mode cached_mode; >>> enum dpu_enc_split_role split_role; >>> enum dpu_intf_mode intf_mode; >>> enum dpu_intf intf_idx; >>> + enum dpu_wb wb_idx; >>> spinlock_t *enc_spinlock; >>> enum dpu_enc_enable_state enable_state; >>> atomic_t vblank_refcount; >>> @@ -243,6 +248,7 @@ struct dpu_encoder_phys_cmd { >>> * @parent_ops: Callbacks exposed by the parent to the phys_enc >>> * @split_role: Role to play in a split-panel configuration >>> * @intf_idx: Interface index this phys_enc will control >>> + * @wb_idx: Writeback index this phys_enc will control >>> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes >>> */ >>> struct dpu_enc_phys_init_params { >>> @@ -251,6 +257,7 @@ struct dpu_enc_phys_init_params { >>> const struct dpu_encoder_virt_ops *parent_ops; >>> enum dpu_enc_split_role split_role; >>> enum dpu_intf intf_idx; >>> + enum dpu_wb wb_idx; >>> spinlock_t *enc_spinlock; >>> }; >> >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 25c7eda..d1e92d89 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1013,9 +1013,18 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx); - if (!phys->hw_intf) { + if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX) + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx); + + if (!phys->hw_intf && !phys->hw_wb) { DPU_ERROR_ENC(dpu_enc, - "no intf block assigned at idx: %d\n", i); + "no intf or wb block assigned at idx: %d\n", i); + return; + } + + if (phys->hw_intf && phys->hw_wb) { + DPU_ERROR_ENC(dpu_enc, + "invalid phys both intf and wb block at idx: %d\n", i); return; } @@ -1163,16 +1172,35 @@ static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, { int i = 0; - for (i = 0; i < catalog->intf_count; i++) { - if (catalog->intf[i].type == type - && catalog->intf[i].controller_id == controller_id) { - return catalog->intf[i].id; + if (type != INTF_WB) { + for (i = 0; i < catalog->intf_count; i++) { + if (catalog->intf[i].type == type + && catalog->intf[i].controller_id == controller_id) { + return catalog->intf[i].id; + } } } return INTF_MAX; } +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog, + enum dpu_intf_type type, u32 controller_id) +{ + int i = 0; + + if (type != INTF_WB) + goto end; + + for (i = 0; i < catalog->wb_count; i++) { + if (catalog->wb[i].id == controller_id) + return catalog->wb[i].id; + } + +end: + return WB_MAX; +} + static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, struct dpu_encoder_phys *phy_enc) { @@ -1887,16 +1915,32 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) dpu_encoder_helper_reset_mixers(phys_enc); - for (i = 0; i < dpu_enc->num_phys_encs; i++) { - if (dpu_enc->phys_encs[i] && phys_enc->hw_intf->ops.bind_pingpong_blk) - phys_enc->hw_intf->ops.bind_pingpong_blk( - dpu_enc->phys_encs[i]->hw_intf, false, - dpu_enc->phys_encs[i]->hw_pp->idx); - - /* mark INTF flush as pending */ - if (phys_enc->hw_ctl->ops.update_pending_flush_intf) - phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, - dpu_enc->phys_encs[i]->hw_intf->idx); + /* + * TODO: move the once-only operation like CTL flush/trigger + * into dpu_encoder_virt_disable() and all operations which need + * to be done per phys encoder into the phys_disable() op. + */ + if (phys_enc->hw_wb) { + /* disable the PP block */ + if (phys_enc->hw_wb->ops.bind_pingpong_blk) + phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, false, + phys_enc->hw_pp->idx); + + /* mark WB flush as pending */ + if (phys_enc->hw_ctl->ops.update_pending_flush_wb) + phys_enc->hw_ctl->ops.update_pending_flush_wb(ctl, phys_enc->hw_wb->idx); + } else { + for (i = 0; i < dpu_enc->num_phys_encs; i++) { + if (dpu_enc->phys_encs[i] && phys_enc->hw_intf->ops.bind_pingpong_blk) + phys_enc->hw_intf->ops.bind_pingpong_blk( + dpu_enc->phys_encs[i]->hw_intf, false, + dpu_enc->phys_encs[i]->hw_pp->idx); + + /* mark INTF flush as pending */ + if (phys_enc->hw_ctl->ops.update_pending_flush_intf) + phys_enc->hw_ctl->ops.update_pending_flush_intf(phys_enc->hw_ctl, + dpu_enc->phys_encs[i]->hw_intf->idx); + } } /* reset the merge 3D HW block */ @@ -2112,6 +2156,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, case DRM_MODE_ENCODER_TMDS: intf_type = INTF_DP; break; + case DRM_MODE_ENCODER_VIRTUAL: + intf_type = INTF_WB; + break; } WARN_ON(disp_info->num_of_h_tiles < 1); @@ -2149,8 +2196,19 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, intf_type, controller_id); - if (phys_params.intf_idx == INTF_MAX) { - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", + + phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, + intf_type, controller_id); + /* + * For boards which have no physical displays, having no interface + * is fine because it can still be used with just writeback. + * If we try without a display on a board which uses a DPU in which + * writeback is not supported, then this will still fail as it will not + * find any writeback in the catalog. + */ + if ((phys_params.intf_idx == INTF_MAX) && + (phys_params.wb_idx == WB_MAX)) { + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", intf_type, controller_id); ret = -EINVAL; } 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 544a9a4..c84b8e8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -11,6 +11,7 @@ #include "dpu_kms.h" #include "dpu_hw_intf.h" +#include "dpu_hw_wb.h" #include "dpu_hw_pingpong.h" #include "dpu_hw_ctl.h" #include "dpu_hw_top.h" @@ -165,12 +166,14 @@ enum dpu_intr_idx { * @hw_ctl: Hardware interface to the ctl registers * @hw_pp: Hardware interface to the ping pong registers * @hw_intf: Hardware interface to the intf registers + * @hw_wb: Hardware interface to the wb registers * @dpu_kms: Pointer to the dpu_kms top level * @cached_mode: DRM mode cached at mode_set time, acted on in enable * @enabled: Whether the encoder has enabled and running a mode * @split_role: Role to play in a split-panel configuration * @intf_mode: Interface mode * @intf_idx: Interface index on dpu hardware + * @wb_idx: Writeback index on dpu hardware * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes * @enable_state: Enable state tracking * @vblank_refcount: Reference count of vblank request @@ -193,11 +196,13 @@ struct dpu_encoder_phys { struct dpu_hw_ctl *hw_ctl; struct dpu_hw_pingpong *hw_pp; struct dpu_hw_intf *hw_intf; + struct dpu_hw_wb *hw_wb; struct dpu_kms *dpu_kms; struct drm_display_mode cached_mode; enum dpu_enc_split_role split_role; enum dpu_intf_mode intf_mode; enum dpu_intf intf_idx; + enum dpu_wb wb_idx; spinlock_t *enc_spinlock; enum dpu_enc_enable_state enable_state; atomic_t vblank_refcount; @@ -243,6 +248,7 @@ struct dpu_encoder_phys_cmd { * @parent_ops: Callbacks exposed by the parent to the phys_enc * @split_role: Role to play in a split-panel configuration * @intf_idx: Interface index this phys_enc will control + * @wb_idx: Writeback index this phys_enc will control * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes */ struct dpu_enc_phys_init_params { @@ -251,6 +257,7 @@ struct dpu_enc_phys_init_params { const struct dpu_encoder_virt_ops *parent_ops; enum dpu_enc_split_role split_role; enum dpu_intf intf_idx; + enum dpu_wb wb_idx; spinlock_t *enc_spinlock; };
Make changes to dpu_encoder to support virtual encoder needed to support writeback for dpu. changes in v4: - squash dpu_encoder pieces from [1] [1] https://patchwork.freedesktop.org/patch/483099/?series=102964&rev=2 Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 94 +++++++++++++++++++----- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 7 ++ 2 files changed, 83 insertions(+), 18 deletions(-)