On Wed, 20 Apr 2022 at 20:41, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 4/20/2022 12:44 AM, Dmitry Baryshkov wrote:
> > On 20/04/2022 04:46, Abhinav Kumar wrote:
> >> Make changes to dpu_encoder to support virtual encoder needed
> >> to support writeback for dpu.
> >>
> >> changes in v2:
> >> - add the writeback parts to dpu_encoder_helper_phys_cleanup
> >> - rebase on tip of msm-next and fix related dependencies
> >> - get the writeback blocks directly from RM
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 71
> >> +++++++++++++++++-------
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 3 +
> >> 2 files changed, 54 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 0e31ad3..06b8631 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -928,6 +928,7 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >> int num_lm, num_ctl, num_pp, num_dsc;
> >> unsigned int dsc_mask = 0;
> >> + enum dpu_hw_blk_type blk_type;
> >> int i;
> >> if (!drm_enc) {
> >> @@ -1009,12 +1010,21 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >> phys->hw_pp = dpu_enc->hw_pp[i];
> >> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >> - 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 (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
> >> INTF_MODE_WB_LINE)
> >> + blk_type = DPU_HW_BLK_WB;
> >> + else
> >> + blk_type = DPU_HW_BLK_INTF;
> >> +
> >> + if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >> + if (blk_type == DPU_HW_BLK_INTF)
> >> + phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >> phys->intf_idx);
> >> + else if (blk_type == DPU_HW_BLK_WB)
> >> + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
> >> phys->intf_idx);
> >> + }
> >> - if (!phys->hw_intf) {
> >> + if (!phys->hw_intf && !phys->hw_wb) {
> >> DPU_ERROR_ENC(dpu_enc,
> >> - "no intf block assigned at idx: %d\n", i);
> >> + "no intf ow wb block assigned at idx: %d\n", i);
> >
> > or wb
> ack
> >
> >> return;
> >> }
> >> @@ -1157,15 +1167,22 @@ static void dpu_encoder_virt_disable(struct
> >> drm_encoder *drm_enc)
> >> mutex_unlock(&dpu_enc->enc_lock);
> >> }
> >> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >> *catalog,
> >> enum dpu_intf_type type, u32 controller_id)
> >> {
> >> 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;
> >> + }
> >> + }
> >> + } else {
> >> + for (i = 0; i < catalog->wb_count; i++) {
> >> + if (catalog->wb[i].id == controller_id)
> >> + return catalog->wb[i].id;
> >> }
> >> }
> >> @@ -1886,16 +1903,27 @@ 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);
> >> + if (phys_enc->hw_wb) {
> >
> > I think this adds a hidden knowledge here. That there is always just a
> > single phys_enc for the WB encoder. I'd still do this cleanup in a loop
> > together with the INTF cleanup.
> alright, I can make this change.
> >
> >> + /* 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 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);
> >> + /* 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 */
> >> @@ -2111,6 +2139,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);
> >> @@ -2145,11 +2176,11 @@ static int dpu_encoder_setup_display(struct
> >> dpu_encoder_virt *dpu_enc,
> >> DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >> i, controller_id, phys_params.split_role);
> >> - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> >> + phys_params.intf_idx =
> >> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> >> intf_type,
> >> controller_id);
> >
> > I'd prefer to be more explicit here. intf_idx is of type enum dpu_intf,
> > while WBs are enumerated with enum dpu_wb.
> >
> > I's suggest adding a separate phys_params.wb_idx and a
> > dpu_encoder_get_wb() and calling one here depending on intf_type.
> >
> > Then at the dpu_encoder_virt_atomic_mode_set() you don't need to check
> > intf_mode, but get both intf and wb and report an error if none was
> > provided.
>
> Your suggestion is valid and I also thought about it.
>
> Let me explain what prevented me from making the change here and put it
> in a to-do bucket.
>
> 1) This needs a slighly bigger cleanup including the traces, debug
> prints and some helpers as none of them are aware of the wb_idx
>
> 2) Some of the checks need to be adjusted like this one
>
> if (phys_params.intf_idx == INTF_MAX) {
> DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d,
> id %d\n",
> intf_type, controller_id);
> ret = -EINVAL;
> }
>
> So, when we have a separate intf_idx and a wb_idx, having either one of
> them is enough . Consider a case where a board has no physical display.
> That would have no intf_idx but wb_idx can be valid.
>
> So i think this needs a little careful cleanup.
>
> Considering that I need to test that out more, I decided that for a
> short-term we can live with the fact that wb_idx is of type enum
> dpu_intf because dpu_encoder layer understands only that.
>
> Let me know if you agree on this separation to go as a follow-up.
Actually both your points vote for the immediate separation rather
than doing it in a follow-up.
For example, if you see an error reported against INTF_2 on a board
where INTF_2 is actually used, you can not determine whether it is an
issue with INTF_2 or with WB_2.
In fact even reporting an error (or a trace) against INTF_NONE (or
INTF_MAX) might be a better option. It would clearly denote that the
issue is related to the non-INTF.
I think we can merge the patch as is, but I'd strongly prefer either
to see an update or a (nearly) immediate followup.
What do you think about an interim solution? We split the
intf_idx/wb_idx in these series, but all the debugging can be updated
later. This way once we see the report against INTF_NONE, we can
deduce that it's WB.
> >
> >> if (phys_params.intf_idx == INTF_MAX) {
> >> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id
> >> %d\n",
> >> + 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..0b80af4 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,6 +166,7 @@ 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
> >> @@ -193,6 +195,7 @@ 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;
> >
> >
--
With best wishes
Dmitry
