[Freedreno] [PATCH v2 10/17] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed Apr 20 22:38:52 UTC 2022
On 21/04/2022 01:06, Abhinav Kumar wrote:
> Hi Dmitry
>
> On 4/20/2022 11:46 AM, Abhinav Kumar wrote:
>>
>>
>> On 4/20/2022 11:37 AM, Dmitry Baryshkov wrote:
>>> On Wed, 20 Apr 2022 at 20:41, Abhinav Kumar
>>> <quic_abhinavk at quicinc.com> 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 at quicinc.com>
>>>>>> ---
>>>>>> 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.
>
> I dug into the history of dpu_encoder_helper_phys_cleanup() API more in
> downstream.
>
> So this API seems to be a bit poorly designed in downstream too.
>
> This gets called from phys->ops.disable() today which is anyway called
> from within a loop.
>
> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>
> if (phys->ops.disable)
> phys->ops.disable(phys);
> }
>
> So technically we dont need a loop even for the intf cases because of that.
>
> But, while calling for interfaces, downstream makes sure to call this
> just once for the master and skip for the same.
>
> Here the loop is needed just to set the flush bits even for the slave
> and let the master do the actual flush.
>
> Since today, we are not calling this for interface yet, I think i can
> get rid of the loop even for the interface case for uniformity.
>
> When I extend this API to be called for interface cases, we can decide
> how to handle master/slave cases.
>
> Let me know what you think.
This correspons more or less the the idea that I have been toying for
quite some time but never had time to finish: a higher level that would
receive the topology (a list of all LMs, CTLs, INTFs, etc) and wrap
around all CTL calls, taking care of ACTIVE vs non-active CTL, single
flush vs multiple flushes, etc. I postponed this ideas, as I could not
foresee what would be the requirements of WB support.
In the disable case I think you'd want to call a disable_pipe function
in a loop and then to flush all the pending changes. I'd prefer to have
a directly called function rather than a callback.
Does that answer your question?
>
>>>>>
>>>>>> + /* 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.
>>
>> Thank you for your consideration.
>>
>> I will post a follow-up change to fix this no later than next week.
>> I will even try doing it this week itself.
>>
>> I will fix everything together so that it looks like a separate series
>> of "separating intf_idx and wb_idx" and there is no intermediate
>> mismatch.
>>
>> Will leave a FIXME here as well.
>>
>>>
>>>>>
>>>>>> 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
More information about the dri-devel
mailing list