[PATCH v5 10/19] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Apr 26 00:46:08 UTC 2022
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 at 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;
>>> };
>>
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list