Re: [PATCH v5 15/19] drm/msm/dpu: initialize dpu encoder and connector for writeback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >> ---
> >>   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@xxxxxxxxx>
> >>    */
> >>   
> >> @@ -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 ?

> >> +	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;
> >>   }
> >>   

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux