[RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Apr 22 18:18:30 UTC 2022
Hi Dmitry
On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
>>> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk at quicinc.com> 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 at quicinc.com>
>>>>>
>>>>> 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.
>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> 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];
>>>>>> }
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>
More information about the dri-devel
mailing list