[PATCH v5 15/19] drm/msm/dpu: initialize dpu encoder and connector for writeback
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Apr 26 14:59:46 UTC 2022
On 4/26/2022 7:56 AM, Laurent Pinchart wrote:
> On Tue, Apr 26, 2022 at 05:11:41AM +0300, Dmitry Baryshkov wrote:
>> On 26/04/2022 03:32, Laurent Pinchart wrote:
>>> On Sun, Apr 24, 2022 at 05:32:06PM -0700, Abhinav Kumar wrote:
>>>> Initialize dpu encoder and connector for writeback if the
>>>> target supports it in the catalog.
>>>>
>>>> changes in v2:
>>>> - start initialing the encoder for writeback since we
>>>> have migrated to using drm_writeback_connector_init_with_encoder()
>>>> - instead of checking for WB_2 inside _dpu_kms_initialize_writeback
>>>> call it only when its WB_2
>>>> - rebase on tip of msm-next and remove usage of priv->encoders
>>>>
>>>> changes in v3:
>>>> - none
>>>>
>>>> changes in v4:
>>>> - fix copyright years order
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++----
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 ++++++++++++++++++++++++++++-
>>>> 2 files changed, 80 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 24870eb..2d79002 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2102,7 +2102,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)
>>>> {
>>>> @@ -2121,7 +2121,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)) {
>>>> @@ -2134,7 +2134,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)) {
>>>> @@ -2147,6 +2147,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
>>>> @@ -2248,9 +2261,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");
>>>> }
>>>> @@ -2367,8 +2379,9 @@ 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);
>>>> + drm_enc_mode, NULL);
>>>> if (rc) {
>>>> devm_kfree(dev->dev, dpu_enc);
>>>> return ERR_PTR(rc);
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index c683cab..9a406e1 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -1,7 +1,9 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> /*
>>>> - * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
>>>> * Copyright (C) 2013 Red Hat
>>>> + * Copyright (c) 2014-2018, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + *
>>>> * Author: Rob Clark <robdclark at gmail.com>
>>>> */
>>>>
>>>> @@ -15,6 +17,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 +32,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"
>>>> @@ -648,6 +652,45 @@ 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,
>>>> + const u32 *wb_formats, int n_formats)
>>>> +{
>>>> + struct drm_encoder *encoder = NULL;
>>>> + struct msm_display_info info;
>>>> + int rc;
>>>> +
>>>> + encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
>>>
>>> I'm puzzled. I thought the whole purpose of the
>>> drm_writeback_connector_init_with_encoder() function was to share an
>>> encoder between writeback and a real display output, but the encoder you
>>> create here seems to be specific to writeback. What am I missing ?
>>
>> The encoder instance is specific to the writeback, however the encoder's
>> code is shared between the virtual and 'real' encoders. All the code for
>> encoder callbacks, resource management, irq handling is shared between
>> them. It wouldn't be practical to duplicate or rewrite the whole
>> dpu_encoder.c. During the design stage we have discussed several other
>> possibile solutions. All of them look pretty ugly.
>
> Does it mean that the output paths and the writeback paths have separate
> resources ?
They share pretty much all of the hardware resources such as clocks ,
interrupts and fetch bandwidth management.
>
>>>> + if (IS_ERR(encoder)) {
>>>> + DPU_ERROR("encoder init failed for dsi display\n");
>>>> + return PTR_ERR(encoder);
>>>> + }
>>>> +
>>>> + memset(&info, 0, sizeof(info));
>>>> +
>>>> + rc = dpu_writeback_init(dev, encoder, wb_formats,
>>>> + n_formats);
>>>> + if (rc) {
>>>> + DPU_ERROR("dpu_writeback_init, rc = %d\n", rc);
>>>> + drm_encoder_cleanup(encoder);
>>>> + return rc;
>>>> + }
>>>> +
>>>> + info.num_of_h_tiles = 1;
>>>> + /* use only WB idx 2 instance for DPU */
>>>> + info.h_tile_instance[0] = WB_2;
>>>> + 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
>>>> @@ -661,6 +704,7 @@ static int _dpu_kms_setup_displays(struct drm_device *dev,
>>>> struct dpu_kms *dpu_kms)
>>>> {
>>>> int rc = 0;
>>>> + int i;
>>>>
>>>> rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>>>> if (rc) {
>>>> @@ -674,6 +718,21 @@ 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) {
>>>> + for (i = 0; i < dpu_kms->catalog->wb_count; i++) {
>>>> + if (dpu_kms->catalog->wb[i].id == WB_2) {
>>>> + rc = _dpu_kms_initialize_writeback(dev, priv, dpu_kms,
>>>> + dpu_kms->catalog->wb[i].format_list,
>>>> + dpu_kms->catalog->wb[i].num_formats);
>>>> + if (rc) {
>>>> + DPU_ERROR("initialize_WB failed, rc = %d\n", rc);
>>>> + return rc;
>>>> + }
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> return rc;
>>>> }
>>>>
>
More information about the dri-devel
mailing list