Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

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

 



On Fri, 22 Apr 2022 at 21:18, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
> Hi Dmitry
>
> On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
> > On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>>>
> >>>> Hi Dmitry
> >>>>
> >>>> Thanks for the review.
> >>>>
> >>>> One question below.
> >>>>
> >>>> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> >>>>> On 21/04/2022 23:48, Abhinav Kumar wrote:
> >>>>>> Using intf_idx even for writeback interfaces is confusing
> >>>>>> because intf_idx is of type enum dpu_intf but the index used
> >>>>>> for writeback is of type enum dpu_wb.
> >>>>>>
> >>>>>> In addition, this makes it easier to separately check the
> >>>>>> availability of the two as its possible that there are boards
> >>>>>> which don't have any physical display connected but can still
> >>>>>> work in writeback mode.
> >>>>>>
> >>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >>>>>
> >>>>> Looks good, two minor issues bellow.
> >>>>>
> >>>>> With them fixed, I'd even squash this patch into the corresponding patch
> >>>>> of the previous patchset.
> >>>>>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
> >>>>>> +++++++++++++-----------
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
> >>>>>>     3 files changed, 40 insertions(+), 28 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> index 9c12841..054d7e4 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> @@ -962,7 +962,6 @@ 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) {
> >>>>>> @@ -1044,17 +1043,11 @@ 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 (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)
> >>>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >>>>>> phys->intf_idx);
> >>>>>> -        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->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >>>>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
> >>>>>
> >>>>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
> >>>>
> >>>> So there is an error if
> >>>>
> >>>> 1) Neither wb NOR intf are present
> >>>> 2) Both wb AND intf are present for the physical encoder?
> >>>>
> >>>> The second check is okay for now to add but considering concurrent
> >>>> writeback then that wouldn't assumption be wrong since both physical and
> >>>> wb interfaces can go with the same encoder?
> >>>
> >>> To the same encoder, but not to the same physical encoder. Here we
> >>> check the phys_enc parameters.
> >>
> >> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
> >> Get the acks on them.
> >>
> >> Then will absorb into WB series and re-post it.
> >
> > Sounds like a good plan!
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>             if (!phys->hw_intf && !phys->hw_wb) {
> >>>>>>                 DPU_ERROR_ENC(dpu_enc,
> >>>>>> @@ -1201,7 +1194,7 @@ 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_or_wb(struct dpu_mdss_cfg
> >>>>>> *catalog,
> >>>>>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >>>>>>             enum dpu_intf_type type, u32 controller_id)
> >>>>>>     {
> >>>>>>         int i = 0;
> >>>>>> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >>>>>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >>>>>>                     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;
> >>>>>> -        }
> >>>>>>         }
> >>>>>>         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;
> >>>>>
> >>>>> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
> >>>> ack, i guess in that case even the places checking the return value of
> >>>> this function need to be changed.
> >>>
> >>> Yes, of course.
>
> INTF_NONE/WB_NONE is not of enum dpu_intf or enum dpu_wb, its of type
> enum dpu_intf_mode
>
> Do we want to add them to dpu_intf/dpu_wb with a -1 value OR leave it
> as-it-is.

Let's leave it as is then.

>
> >>>
> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>>     static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >>>>>>             struct dpu_encoder_phys *phy_enc)
> >>>>>>     {
> >>>>>> @@ -2249,18 +2254,21 @@ 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);
> >>>>>> -        /*
> >>>>>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
> >>>>>> -         * enum dpu_intf type for wb_idx and also to be able to
> >>>>>> -         * not bail out when there is no intf for boards which dont
> >>>>>> -         * have a display connected to them.
> >>>>>> -         * Having a valid wb_idx but not a intf_idx can be a valid
> >>>>>> -         * combination moving forward.
> >>>>>> -         */
> >>>>>> -        phys_params.intf_idx =
> >>>>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> >>>>>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> >>>>>>                                                         intf_type,
> >>>>>>                                                         controller_id);
> >>>>>> -        if (phys_params.intf_idx == INTF_MAX) {
> >>>>>> +
> >>>>>> +        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 04d037e..f2af07d 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
> >>>>>>      * @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
> >>>>>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
> >>>>>>         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;
> >>>>>> @@ -275,6 +277,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 {
> >>>>>> @@ -283,6 +286,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_rm.h
> >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> index ba82e54..2f34a31 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
> >>>>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
> >>>>>>      * @rm: DPU Resource Manager handle
> >>>>>>      * @wb_idx: WB index
> >>>>>>      */
> >>>>>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>>>> dpu_intf wb_idx)
> >>>>>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>>>> dpu_wb wb_idx)
> >>>>>>     {
> >>>>>>         return rm->hw_wb[wb_idx - WB_0];
> >>>>>>     }
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux