[Freedreno] [PATCH 10/12] drm/msm/dpu: initialize dpu encoder and connector for writeback
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu Apr 14 22:21:23 UTC 2022
On 2/4/2022 2:34 PM, Dmitry Baryshkov wrote:
> On 05/02/2022 00:17, Abhinav Kumar wrote:
>> Initialize dpu encoder and connector for writeback if the
>> target supports it in the catalog.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 37 ++++++++++++-----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 62
>> +++++++++++++++++++++++++++++
>> 2 files changed, 88 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index b51a677..3746432 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2066,7 +2066,7 @@ static void dpu_encoder_early_unregister(struct
>> drm_encoder *encoder)
>> }
>> static int dpu_encoder_virt_add_phys_encs(
>> - u32 display_caps,
>> + struct msm_display_info *disp_info,
>> struct dpu_encoder_virt *dpu_enc,
>> struct dpu_enc_phys_init_params *params)
>> {
>> @@ -2085,7 +2085,7 @@ static int dpu_encoder_virt_add_phys_encs(
>> return -EINVAL;
>> }
>> - if (display_caps & MSM_DISPLAY_CAP_VID_MODE) {
>> + if (disp_info->capabilities & MSM_DISPLAY_CAP_VID_MODE) {
>> enc = dpu_encoder_phys_vid_init(params);
>> if (IS_ERR_OR_NULL(enc)) {
>> @@ -2098,7 +2098,7 @@ static int dpu_encoder_virt_add_phys_encs(
>> ++dpu_enc->num_phys_encs;
>> }
>> - if (display_caps & MSM_DISPLAY_CAP_CMD_MODE) {
>> + if (disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) {
>> enc = dpu_encoder_phys_cmd_init(params);
>> if (IS_ERR_OR_NULL(enc)) {
>> @@ -2111,6 +2111,19 @@ static int dpu_encoder_virt_add_phys_encs(
>> ++dpu_enc->num_phys_encs;
>> }
>> + if (disp_info->intf_type == DRM_MODE_ENCODER_VIRTUAL) {
>> + enc = dpu_encoder_phys_wb_init(params);
>> +
>> + if (IS_ERR_OR_NULL(enc)) {
>> + DPU_ERROR_ENC(dpu_enc, "failed to init wb enc: %ld\n",
>> + PTR_ERR(enc));
>> + return enc == NULL ? -EINVAL : PTR_ERR(enc);
>> + }
>> +
>> + dpu_enc->phys_encs[dpu_enc->num_phys_encs] = enc;
>> + ++dpu_enc->num_phys_encs;
>> + }
>> +
>> if (params->split_role == ENC_ROLE_SLAVE)
>> dpu_enc->cur_slave = enc;
>> else
>> @@ -2199,9 +2212,8 @@ static int dpu_encoder_setup_display(struct
>> dpu_encoder_virt *dpu_enc,
>> }
>> if (!ret) {
>> - ret =
>> dpu_encoder_virt_add_phys_encs(disp_info->capabilities,
>> - dpu_enc,
>> - &phys_params);
>> + ret = dpu_encoder_virt_add_phys_encs(disp_info,
>> + dpu_enc, &phys_params);
>> if (ret)
>> DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
>> }
>> @@ -2317,11 +2329,14 @@ struct drm_encoder *dpu_encoder_init(struct
>> drm_device *dev,
>> if (!dpu_enc)
>> return ERR_PTR(-ENOMEM);
>> - rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>> - drm_enc_mode, NULL);
>> - if (rc) {
>> - devm_kfree(dev->dev, dpu_enc);
>> - return ERR_PTR(rc);
>> + /* this is handled by drm_writeback_connector_init for virtual
>> encoder */
>> + if (drm_enc_mode != DRM_MODE_ENCODER_VIRTUAL) {
>> + rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>> + drm_enc_mode, NULL);
>> + if (rc) {
>> + devm_kfree(dev->dev, dpu_enc);
>> + return ERR_PTR(rc);
>> + }
>> }
>> drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 47fe11a..6327ba9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>> reserved.
>> * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark at gmail.com>
>> @@ -15,6 +16,7 @@
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_file.h>
>> #include <drm/drm_vblank.h>
>> +#include <drm/drm_writeback.h>
>> #include "msm_drv.h"
>> #include "msm_mmu.h"
>> @@ -29,6 +31,7 @@
>> #include "dpu_kms.h"
>> #include "dpu_plane.h"
>> #include "dpu_vbif.h"
>> +#include "dpu_writeback.h"
>> #define CREATE_TRACE_POINTS
>> #include "dpu_trace.h"
>> @@ -642,6 +645,56 @@ static int _dpu_kms_initialize_displayport(struct
>> drm_device *dev,
>> return 0;
>> }
>> +static int _dpu_kms_initialize_writeback(struct drm_device *dev,
>> + struct msm_drm_private *priv, struct dpu_kms *dpu_kms)
>> +{
>> + struct drm_encoder *encoder = NULL;
>> + struct msm_display_info info;
>> + int rc, i;
>> + const u32 *wb_formats;
>> + int n_formats;
>> +
>> + encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
>> + if (IS_ERR(encoder)) {
>> + DPU_ERROR("encoder init failed for dsi display\n");
>> + return PTR_ERR(encoder);
>> + }
>> +
>> + memset(&info, 0, sizeof(info));
>> +
>> + for (i = 0; i < dpu_kms->catalog->wb_count; i++) {
>> + if (dpu_kms->catalog->wb[i].id == WB_2) {
>> + wb_formats = dpu_kms->catalog->wb[i].format_list;
>> + n_formats = dpu_kms->catalog->wb[i].num_formats;
>> + }
>> + }
>
> If there is no WB_2 in the catalog, then both wb_formats and n_formats
> would be unused. Also even if there is no WB_2 you'd still try creating
> the writeback below.
>
> I'd suggest to move dpu_encoder_init() + dpu_writeback_init() +
> dpu_encoder_setup() to the separate function. Then you can loop over the
> catslog->wb[] and once WB_2 is found, call this new function.
Makes sense. Will do this.
>
>> +
>> + rc = dpu_writeback_init(dev, encoder, encoder->helper_private,
>> wb_formats,
>> + n_formats);
>> + if (rc) {
>> + DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
>> + drm_encoder_cleanup(encoder);
>> + return rc;
>> + }
>> +
>> + priv->encoders[priv->num_encoders++] = encoder;
>> +
>> + info.num_of_h_tiles = 1;
>> + /* use only WB idx 2 instance for DPU */
>> + info.h_tile_instance[0] = WB_2;
>> + info.capabilities = MSM_DISPLAY_CAP_HOT_PLUG | MSM_DISPLAY_CAP_EDID;
>
> these two capabilities are unused in the code. I'd suggest to drop them
> from msm_drv.h at all.
Let me push that as a separate change but will do it for sure as i dont
see its usage either. Will drop this line from this patch.
>
>> + info.intf_type = encoder->encoder_type;
>> +
>> + rc = dpu_encoder_setup(dev, encoder, &info);
>> + if (rc) {
>> + DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> + encoder->base.id, rc);
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * _dpu_kms_setup_displays - create encoders, bridges and connectors
>> * for underlying displays
>> @@ -668,6 +721,15 @@ static int _dpu_kms_setup_displays(struct
>> drm_device *dev,
>> return rc;
>> }
>> + /* Since WB isn't a driver check the catalog before initializing */
>> + if (dpu_kms->catalog->wb_count) {
>> + rc = _dpu_kms_initialize_writeback(dev, priv, dpu_kms);
>> + if (rc) {
>> + DPU_ERROR("initialize_WB failed, rc = %d\n", rc);
>> + return rc;
>> + }
>> + }
>> +
>> return rc;
>> }
>
>
More information about the dri-devel
mailing list