[PATCH v5 10/19] drm/msm/dpu: make changes to dpu_encoder to support virtual encoder
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Apr 26 00:44:49 UTC 2022
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.
>
>
> 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;
>> };
>
>
More information about the dri-devel
mailing list