Message-ID: <16986655-0c51-49ac-8088-6548317be7e6@quicinc.com>
Date: Thu, 6 Feb 2025 15:49:53 -0800
From: Jessica Zhang <quic_jesszhan@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Rob Clark <robdclark@...il.com>, <quic_abhinavk@...cinc.com>,
        Sean Paul
	<sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        "David
 Airlie" <airlied@...il.com>,
        Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Simona Vetter <simona@...ll.ch>,
        Simona Vetter <simona.vetter@...ll.ch>, <quic_ebharadw@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Rob Clark
	<robdclark@...omium.org>,
        Ville Syrjälä
	<ville.syrjala@...ux.intel.com>
Subject: Re: [PATCH v5 02/14] drm/msm/dpu: move resource allocation to CRTC



On 1/29/2025 2:04 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 28, 2025 at 07:20:34PM -0800, Jessica Zhang wrote:
>> From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>
>> All resource allocation is centered around the LMs. Then other blocks
>> (except DSCs) are allocated basing on the LMs that was selected, and LM
>> powers up the CRTC rather than the encoder.
>>
>> Moreover if at some point the driver supports encoder cloning,
>> allocating resources from the encoder will be incorrect, as all clones
>> will have different encoder IDs, while LMs are to be shared by these
>> encoders.
>>
>> In addition, move mode_changed() to dpu_crtc as encoder no longer has
>> access to topology information
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>> [quic_abhinavk@...cinc.com: Refactored resource allocation for CDM]
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>> [quic_jesszhan@...cinc.com: Changed to grabbing exising global state]
>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>
>> ---
>> Changes in v5:
>> - Reordered to prevent breaking CI and upon partial applciation
>> - Moved mode_changed() from dpu_encoder to dpu_crtc
>> - Dropped dpu_encoder_needs_dsc_merge() refactor to clean up commit
>> - In dpu_encoder_update_topology(), grab DSC config using dpu_encoder
>>    helper as dpu_encoder->dsc hasn't been set yet
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  79 +++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |   2 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 174 +++++++++-------------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  11 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  19 +--
>>   5 files changed, 144 insertions(+), 141 deletions(-)
>>
> 
>> -/**
>> - * dpu_encoder_virt_check_mode_changed: check if full modeset is required
>> - * @drm_enc:    Pointer to drm encoder structure
>> - * @crtc_state:	Corresponding CRTC state to be checked
>> - * @conn_state: Corresponding Connector's state to be checked
>> - *
>> - * Check if the changes in the object properties demand full mode set.
>> - */
>> -int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
>> -					struct drm_crtc_state *crtc_state,
>> -					struct drm_connector_state *conn_state)
>> +bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_state *state)
>>   {
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_framebuffer *fb;
>>   	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>> -	struct msm_display_topology topology;
>> -
>> -	DPU_DEBUG_ENC(dpu_enc, "\n");
>> -
>> -	/* Using mode instead of adjusted_mode as it wasn't computed yet */
>> -	topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode, crtc_state, conn_state);
>> -
>> -	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>> -		crtc_state->mode_changed = true;
>> -	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>> -		crtc_state->mode_changed = true;
>> -
>> -	return 0;
>> -}
>> -
>> -static int dpu_encoder_virt_atomic_check(
>> -		struct drm_encoder *drm_enc,
>> -		struct drm_crtc_state *crtc_state,
>> -		struct drm_connector_state *conn_state)
>> -{
>> -	struct dpu_encoder_virt *dpu_enc;
>> -	struct msm_drm_private *priv;
>> -	struct dpu_kms *dpu_kms;
>> -	struct drm_display_mode *adj_mode;
>> -	struct msm_display_topology topology;
>> -	struct dpu_global_state *global_state;
>> -	int ret = 0;
>> -
>> -	if (!drm_enc || !crtc_state || !conn_state) {
>> -		DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
>> -				drm_enc != NULL, crtc_state != NULL, conn_state != NULL);
>> -		return -EINVAL;
>> -	}
>> -
>> -	dpu_enc = to_dpu_encoder_virt(drm_enc);
>> -	DPU_DEBUG_ENC(dpu_enc, "\n");
>> -
>> -	priv = drm_enc->dev->dev_private;
>> -	dpu_kms = to_dpu_kms(priv->kms);
>> -	adj_mode = &crtc_state->adjusted_mode;
>> -	global_state = dpu_kms_get_global_state(crtc_state->state);
>> -	if (IS_ERR(global_state))
>> -		return PTR_ERR(global_state);
>>   
>> -	trace_dpu_enc_atomic_check(DRMID(drm_enc));
>> +	if (!drm_enc || !state)
>> +		return false;
>>   
>> -	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
>> +	connector = drm_atomic_get_new_connector_for_encoder(state, drm_enc);
>> +	if (!connector)
>> +		return false;
>>   
>> -	/*
>> -	 * Release and Allocate resources on every modeset
>> -	 */
>> -	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> -		dpu_rm_release(global_state, drm_enc);
>> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
>>   
>> -		if (crtc_state->enable)
>> -			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> -					drm_enc, crtc_state, &topology);
>> +	if (dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) {
>> +		fb = conn_state->writeback_job->fb;
>> +		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb))) {
>> +			if (!dpu_enc->cur_master->hw_cdm)
>> +				return true;
>> +		} else {
>> +			if (dpu_enc->cur_master->hw_cdm)
>> +				return true;
>> +		}
> 
> Nit: this is duplicating a part of the dpu_encoder_update_topology(). It
> would be nice to have a comment here. If there is no need for a new
> versoion, I can probably write something when applying.

Sure, I can add a note that we need to duplicate these checks in 
*_needs_modeset() since topology info is not stored in the encoder or crtc

Thanks,

Jessica Zhang

> 
>>   	}
>>   
>> -	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
>> -
>> -	return ret;
>> +	return false;
>>   }
>>   
>>   static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>> @@ -2612,7 +2545,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
>>   	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
>>   	.atomic_disable = dpu_encoder_virt_atomic_disable,
>>   	.atomic_enable = dpu_encoder_virt_atomic_enable,
>> -	.atomic_check = dpu_encoder_virt_atomic_check,
>>   };
>>   
>>   static const struct drm_encoder_funcs dpu_encoder_funcs = {
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index da133ee4701a329f566f6f9a7255f2f6d050f891..b0ac10ebd02c2b63e6f6f9010a22cdace931cf3b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -80,6 +80,13 @@ int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
>>   
>>   bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc);
>>   
>> +void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
>> +				 struct msm_display_topology *topology,
>> +				 struct drm_atomic_state *state,
>> +				 const struct drm_display_mode *adj_mode);
>> +
>> +bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_state *state);
>> +
>>   void dpu_encoder_prepare_wb_job(struct drm_encoder *drm_enc,
>>   		struct drm_writeback_job *job);
>>   
>> @@ -88,8 +95,4 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
>>   
>>   bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
>>   
>> -int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
>> -					struct drm_crtc_state *crtc_state,
>> -					struct drm_connector_state *conn_state);
>> -
>>   #endif /* __DPU_ENCODER_H__ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 5ce06e25990cb70284d3c3f04ac1e1e1bed6142a..c6b3b2e147b4c61ec93db4a9f01d5a288d2b9eb2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -449,24 +449,11 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
>>   static int dpu_kms_check_mode_changed(struct msm_kms *kms, struct drm_atomic_state *state)
>>   {
>>   	struct drm_crtc_state *new_crtc_state;
>> -	struct drm_connector *connector;
>> -	struct drm_connector_state *new_conn_state;
>> +	struct drm_crtc *crtc;
>>   	int i;
>>   
>> -	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>> -		struct drm_encoder *encoder;
>> -
>> -		WARN_ON(!!new_conn_state->best_encoder != !!new_conn_state->crtc);
>> -
>> -		if (!new_conn_state->crtc || !new_conn_state->best_encoder)
>> -			continue;
>> -
>> -		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
>> -
>> -		encoder = new_conn_state->best_encoder;
>> -
>> -		dpu_encoder_virt_check_mode_changed(encoder, new_crtc_state, new_conn_state);
>> -	}
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
>> +		dpu_crtc_check_mode_changed(new_crtc_state);
>>   
>>   	return 0;
>>   }
>>
>> -- 
>> 2.34.1
>>
> 
> -- 
> With best wishes
> Dmitry

Powered by blists - more mailing lists