Re: [PATCH v5 17/19] drm/msm/dpu: remove RM dependency on connector state

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-09-06 09:14, Jordan Crouse wrote:
> On Wed, Sep 05, 2018 at 07:08:26PM -0700, Jeykumar Sankaran wrote:
>> Connector states were passed around RM to update the custom
>> topology connector property with chosen topology data. Now that
>> we got rid of both custom properties and topology names, this
>> change cleans up the mechanism to pass connector states across
>> RM helpers and encoder functions.
>> 
>> changes in v5:
>> 	- Introduced in the series
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 15 +++---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  4 +-
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  3 +-
>>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  3 +-
>>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  7 ++-
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 60
> +++++-----------------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             |  2 -
>>  7 files changed, 27 insertions(+), 67 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 0d43525..18f5d1d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -436,15 +436,14 @@ int dpu_encoder_helper_unregister_irq(struct
> dpu_encoder_phys *phys_enc,
>>  }
>> 
>>  void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>> -		struct dpu_encoder_hw_resources *hw_res,
>> -		struct drm_connector_state *conn_state)
>> +				  struct dpu_encoder_hw_resources *hw_res)
>>  {
>>  	struct dpu_encoder_virt *dpu_enc = NULL;
>>  	int i = 0;
>> 
>> -	if (!hw_res || !drm_enc || !conn_state) {
>> -		DPU_ERROR("invalid argument(s), drm_enc %d, res %d, state
> %d\n",
>> -				drm_enc != 0, hw_res != 0, conn_state !=
> 0);
>> +	if (!hw_res || !drm_enc) {
>> +		DPU_ERROR("invalid argument(s), drm_enc %d, res %d\n",
>> +			  drm_enc != 0, hw_res != 0);
> 
> As far as I can tell both hw_res and drm_enc will be set for all the
> callers so
> this check shouldn't be needed. If you are paranoid you can switch to a
> WARN_ON() and get rid of the custom error message but I don't think you
> need to.
> 
>>  		return;
>>  	}
>> 
>> @@ -458,7 +457,7 @@ void dpu_encoder_get_hw_resources(struct 
>> drm_encoder
> *drm_enc,
>>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> 
>>  		if (phys && phys->ops.get_hw_resources)
>> -			phys->ops.get_hw_resources(phys, hw_res,
> conn_state);
>> +			phys->ops.get_hw_resources(phys, hw_res);
>>  	}
>>  }
>> 
>> @@ -652,7 +651,7 @@ static int dpu_encoder_virt_atomic_check(
>>  		if (drm_atomic_crtc_needs_modeset(crtc_state)
>>  				&& dpu_enc->mode_set_complete) {
>>  			ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc,
> crtc_state,
>> -				conn_state, topology, true);
>> +					     topology, true);
>>  			dpu_enc->mode_set_complete = false;
>>  		}
>>  	}
>> @@ -1044,7 +1043,7 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>> 
>>  	/* Reserve dynamic resources now. Indicating non-AtomicTest phase
> */
>>  	ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, drm_enc->crtc->state,
>> -			conn->state, topology, false);
>> +			     topology, false);
>>  	if (ret) {
>>  		DPU_ERROR_ENC(dpu_enc,
>>  				"failed to reserve hw resources, %d\n",
> ret);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index f109b4d..34ac5b6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -51,11 +51,9 @@ struct dpu_encoder_kickoff_params {
>>   * dpu_encoder_get_hw_resources - Populate table of required hardware
> resources
>>   * @encoder:	encoder pointer
>>   * @hw_res:	resource table to populate with encoder required 
>> resources
>> - * @conn_state:	report hw reqs based on this proposed connector
> state
>>   */
>>  void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>> -		struct dpu_encoder_hw_resources *hw_res,
>> -		struct drm_connector_state *conn_state);
>> +				  struct dpu_encoder_hw_resources
> *hw_res);
>> 
>>  /**
>>   * dpu_encoder_register_vblank_callback - provide callback to encoder
> that
>> 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 a00b222..3fe4ed9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -140,8 +140,7 @@ struct dpu_encoder_phys_ops {
>>  			    struct drm_connector_state *conn_state);
>>  	void (*destroy)(struct dpu_encoder_phys *encoder);
>>  	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>> -			struct dpu_encoder_hw_resources *hw_res,
>> -			struct drm_connector_state *conn_state);
>> +				 struct dpu_encoder_hw_resources *hw_res);
>>  	int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool
> enable);
>>  	int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>>  	int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 5c89868..f277a69 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -599,8 +599,7 @@ static void dpu_encoder_phys_cmd_destroy(struct
> dpu_encoder_phys *phys_enc)
>> 
>>  static void dpu_encoder_phys_cmd_get_hw_resources(
>>  		struct dpu_encoder_phys *phys_enc,
>> -		struct dpu_encoder_hw_resources *hw_res,
>> -		struct drm_connector_state *conn_state)
>> +		struct dpu_encoder_hw_resources *hw_res)
>>  {
>>  	struct dpu_encoder_phys_cmd *cmd_enc =
>>  		to_dpu_encoder_phys_cmd(phys_enc);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index 6de13f4..fd51fe6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -537,12 +537,11 @@ static void dpu_encoder_phys_vid_destroy(struct
> dpu_encoder_phys *phys_enc)
>> 
>>  static void dpu_encoder_phys_vid_get_hw_resources(
>>  		struct dpu_encoder_phys *phys_enc,
>> -		struct dpu_encoder_hw_resources *hw_res,
>> -		struct drm_connector_state *conn_state)
>> +		struct dpu_encoder_hw_resources *hw_res)
>>  {
>>  	if (!phys_enc || !hw_res) {
>> -		DPU_ERROR("invalid arg(s), enc %d hw_res %d conn_state
> %d\n",
>> -				phys_enc != 0, hw_res != 0, conn_state !=
> 0);
>> +		DPU_ERROR("invalid arg(s), enc %d hw_res %d\n",
>> +			  phys_enc != 0, hw_res != 0);
> 
> I don't think this is possible either.
The next patch in the series take care of these checks in encoder.
https://patchwork.freedesktop.org/patch/247509/

> 
>>  		return;
>>  	}
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 86466f0..1164561 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -682,7 +682,6 @@ static int _dpu_rm_make_next_rsvp(
>>  		struct dpu_rm *rm,
>>  		struct drm_encoder *enc,
>>  		struct drm_crtc_state *crtc_state,
>> -		struct drm_connector_state *conn_state,
>>  		struct dpu_rm_rsvp *rsvp,
>>  		struct dpu_rm_requirements *reqs)
>>  {
>> @@ -728,7 +727,6 @@ static int _dpu_rm_populate_requirements(
>>  		struct dpu_rm *rm,
>>  		struct drm_encoder *enc,
>>  		struct drm_crtc_state *crtc_state,
>> -		struct drm_connector_state *conn_state,
>>  		struct dpu_rm_requirements *reqs,
>>  		struct msm_display_topology req_topology)
>>  {
>> @@ -736,7 +734,7 @@ static int _dpu_rm_populate_requirements(
>> 
>>  	memset(reqs, 0, sizeof(*reqs));
>> 
>> -	dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
>> +	dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
>> 
>>  	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
>>  		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
>> @@ -780,29 +778,12 @@ static struct dpu_rm_rsvp *_dpu_rm_get_rsvp(
>>  	return NULL;
>>  }
>> 
>> -static struct drm_connector *_dpu_rm_get_connector(
>> -		struct drm_encoder *enc)
>> -{
>> -	struct drm_connector *conn = NULL;
>> -	struct list_head *connector_list =
>> -			&enc->dev->mode_config.connector_list;
>> -
>> -	list_for_each_entry(conn, connector_list, head)
>> -		if (conn->encoder == enc)
>> -			return conn;
>> -
>> -	return NULL;
>> -}
>> -
>>  /**
>>   * _dpu_rm_release_rsvp - release resources and release a reservation
>>   * @rm:	KMS handle
>>   * @rsvp:	RSVP pointer to release and release resources for
>>   */
>> -static void _dpu_rm_release_rsvp(
>> -		struct dpu_rm *rm,
>> -		struct dpu_rm_rsvp *rsvp,
>> -		struct drm_connector *conn)
>> +static void _dpu_rm_release_rsvp(struct dpu_rm *rm, struct 
>> dpu_rm_rsvp
> *rsvp)
>>  {
>>  	struct dpu_rm_rsvp *rsvp_c, *rsvp_n;
>>  	struct dpu_rm_hw_blk *blk;
>> @@ -843,7 +824,6 @@ static void _dpu_rm_release_rsvp(
>>  void dpu_rm_release(struct dpu_rm *rm, struct drm_encoder *enc)
>>  {
>>  	struct dpu_rm_rsvp *rsvp;
>> -	struct drm_connector *conn;
>> 
>>  	if (!rm || !enc) {
>>  		DPU_ERROR("invalid params\n");
>> @@ -858,21 +838,12 @@ void dpu_rm_release(struct dpu_rm *rm, struct
> drm_encoder *enc)
>>  		goto end;
>>  	}
>> 
>> -	conn = _dpu_rm_get_connector(enc);
>> -	if (!conn) {
>> -		DPU_ERROR("failed to get connector for enc %d\n",
> enc->base.id);
>> -		goto end;
>> -	}
>> -
>> -	_dpu_rm_release_rsvp(rm, rsvp, conn);
>> +	_dpu_rm_release_rsvp(rm, rsvp);
>>  end:
>>  	mutex_unlock(&rm->rm_lock);
>>  }
>> 
>> -static int _dpu_rm_commit_rsvp(
>> -		struct dpu_rm *rm,
>> -		struct dpu_rm_rsvp *rsvp,
>> -		struct drm_connector_state *conn_state)
>> +static int _dpu_rm_commit_rsvp(struct dpu_rm *rm, struct dpu_rm_rsvp
> *rsvp)
>>  {
>>  	struct dpu_rm_hw_blk *blk;
>>  	enum dpu_hw_blk_type type;
>> @@ -899,7 +870,6 @@ int dpu_rm_reserve(
>>  		struct dpu_rm *rm,
>>  		struct drm_encoder *enc,
>>  		struct drm_crtc_state *crtc_state,
>> -		struct drm_connector_state *conn_state,
>>  		struct msm_display_topology topology,
>>  		bool test_only)
>>  {
>> @@ -907,7 +877,7 @@ int dpu_rm_reserve(
>>  	struct dpu_rm_requirements reqs;
>>  	int ret;
>> 
>> -	if (!rm || !enc || !crtc_state || !conn_state) {
>> +	if (!rm || !enc || !crtc_state) {
> 
> I also don't think this is possible.
Will remove the checks!
> 
>>  		DPU_ERROR("invalid arguments\n");
>>  		return -EINVAL;
>>  	}
> 
> <snip>
> 
> Jordan

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno