Re: [RFC PATCH v2 05/13] drm/msm/dpu: get rid of struct dpu_rm_requirements

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
> The struct dpu_rm_requirements was used to wrap display topology and
> hw resources, which meant INTF indices. As of commit ef58e0ad3436
> ("drm/msm/dpu: get INTF blocks directly rather than through RM") the hw
> resources struct was removed, leaving struct dpu_rm_requirements
> containing a single field (topology). Remove the useless wrapper.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---

Irrespective of where we plan to have the topology, this change doesn't 
seem incorrect as such.

The only thing I can think of is when we need more information to be 
passed to the RM to allocate the blocks in addition to the topology this 
struct could have been expanded.

So one example I can think of is lets say I want to add CDM block 
support. Then that information is outside of topology today because I 
will use CDM if my output format is yuv. It has nothing to do with 
topology but that block still needs to come from RM.

I know that usually I have lost on these type of discussions saying that 
if the code is not there yet, it should be dropped but I do have a plan 
to add that support soon probably by the next cycle. That time we will 
need some sort of wrapper to hold the topology and "extra" information 
to allocate the blocks.

One alternative ofcourse is to expand dpu_rm_reserve() to accept 
something like "needs_cdm" but this is not scalable.

Thoughts?

>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69 +++++++--------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 +-
>   3 files changed, 23 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4ee708264f3b..a2cb23dea0b8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -638,7 +638,7 @@ static int dpu_encoder_virt_atomic_check(
>   
>   		if (!crtc_state->active_changed || crtc_state->enable)
>   			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -					drm_enc, crtc_state, topology);
> +					drm_enc, crtc_state, &topology);
>   	}
>   
>   	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index f4dda88a73f7..952e139c0234 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -24,15 +24,6 @@ static inline bool reserved_by_other(uint32_t *res_map, int idx,
>   	return res_map[idx] && res_map[idx] != enc_id;
>   }
>   
> -/**
> - * struct dpu_rm_requirements - Reservation requirements parameter bundle
> - * @topology:  selected topology for the display
> - * @hw_res:	   Hardware resources required as reported by the encoders
> - */
> -struct dpu_rm_requirements {
> -	struct msm_display_topology topology;
> -};
> -
>   int dpu_rm_destroy(struct dpu_rm *rm)
>   {
>   	int i;
> @@ -329,14 +320,13 @@ static bool _dpu_rm_check_lm_peer(struct dpu_rm *rm, int primary_idx,
>    *      mixer in rm->pingpong_blks[].
>    * @dspp_idx: output parameter, index of dspp block attached to the layer
>    *      mixer in rm->dspp_blks[].
> - * @reqs: input parameter, rm requirements for HW blocks needed in the
> - *      datapath.
> + * @topology:  selected topology for the display
>    * Return: true if lm matches all requirements, false otherwise
>    */
>   static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>   		struct dpu_global_state *global_state,
>   		uint32_t enc_id, int lm_idx, int *pp_idx, int *dspp_idx,
> -		struct dpu_rm_requirements *reqs)
> +		struct msm_display_topology *topology)
>   {
>   	const struct dpu_lm_cfg *lm_cfg;
>   	int idx;
> @@ -361,7 +351,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>   	}
>   	*pp_idx = idx;
>   
> -	if (!reqs->topology.num_dspp)
> +	if (!topology->num_dspp)
>   		return true;
>   
>   	idx = lm_cfg->dspp - DSPP_0;
> @@ -383,7 +373,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>   static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   			       struct dpu_global_state *global_state,
>   			       uint32_t enc_id,
> -			       struct dpu_rm_requirements *reqs)
> +			       struct msm_display_topology *topology)
>   
>   {
>   	int lm_idx[MAX_BLOCKS];
> @@ -391,14 +381,14 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   	int dspp_idx[MAX_BLOCKS] = {0};
>   	int i, j, lm_count = 0;
>   
> -	if (!reqs->topology.num_lm) {
> -		DPU_ERROR("invalid number of lm: %d\n", reqs->topology.num_lm);
> +	if (!topology->num_lm) {
> +		DPU_ERROR("invalid number of lm: %d\n", topology->num_lm);
>   		return -EINVAL;
>   	}
>   
>   	/* Find a primary mixer */
>   	for (i = 0; i < ARRAY_SIZE(rm->mixer_blks) &&
> -			lm_count < reqs->topology.num_lm; i++) {
> +			lm_count < topology->num_lm; i++) {
>   		if (!rm->mixer_blks[i])
>   			continue;
>   
> @@ -407,7 +397,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   
>   		if (!_dpu_rm_check_lm_and_get_connected_blks(rm, global_state,
>   				enc_id, i, &pp_idx[lm_count],
> -				&dspp_idx[lm_count], reqs)) {
> +				&dspp_idx[lm_count], topology)) {
>   			continue;
>   		}
>   
> @@ -415,7 +405,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   
>   		/* Valid primary mixer found, find matching peers */
>   		for (j = i + 1; j < ARRAY_SIZE(rm->mixer_blks) &&
> -				lm_count < reqs->topology.num_lm; j++) {
> +				lm_count < topology->num_lm; j++) {
>   			if (!rm->mixer_blks[j])
>   				continue;
>   
> @@ -428,7 +418,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   			if (!_dpu_rm_check_lm_and_get_connected_blks(rm,
>   					global_state, enc_id, j,
>   					&pp_idx[lm_count], &dspp_idx[lm_count],
> -					reqs)) {
> +					topology)) {
>   				continue;
>   			}
>   
> @@ -437,7 +427,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   		}
>   	}
>   
> -	if (lm_count != reqs->topology.num_lm) {
> +	if (lm_count != topology->num_lm) {
>   		DPU_DEBUG("unable to find appropriate mixers\n");
>   		return -ENAVAIL;
>   	}
> @@ -446,7 +436,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>   		global_state->mixer_to_enc_id[lm_idx[i]] = enc_id;
>   		global_state->pingpong_to_enc_id[pp_idx[i]] = enc_id;
>   		global_state->dspp_to_enc_id[dspp_idx[i]] =
> -			reqs->topology.num_dspp ? enc_id : 0;
> +			topology->num_dspp ? enc_id : 0;
>   
>   		trace_dpu_rm_reserve_lms(lm_idx[i] + LM_0, enc_id,
>   					 pp_idx[i] + PINGPONG_0);
> @@ -539,44 +529,30 @@ static int _dpu_rm_make_reservation(
>   		struct dpu_rm *rm,
>   		struct dpu_global_state *global_state,
>   		struct drm_encoder *enc,
> -		struct dpu_rm_requirements *reqs)
> +		struct msm_display_topology *topology)
>   {
>   	int ret;
>   
> -	ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id, reqs);
> +	ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id, topology);
>   	if (ret) {
>   		DPU_ERROR("unable to find appropriate mixers\n");
>   		return ret;
>   	}
>   
>   	ret = _dpu_rm_reserve_ctls(rm, global_state, enc->base.id,
> -				&reqs->topology);
> +				   topology);
>   	if (ret) {
>   		DPU_ERROR("unable to find appropriate CTL\n");
>   		return ret;
>   	}
>   
> -	ret  = _dpu_rm_reserve_dsc(rm, global_state, enc, &reqs->topology);
> +	ret  = _dpu_rm_reserve_dsc(rm, global_state, enc, topology);
>   	if (ret)
>   		return ret;
>   
>   	return ret;
>   }
>   
> -static int _dpu_rm_populate_requirements(
> -		struct drm_encoder *enc,
> -		struct dpu_rm_requirements *reqs,
> -		struct msm_display_topology req_topology)
> -{
> -	reqs->topology = req_topology;
> -
> -	DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
> -		      reqs->topology.num_lm, reqs->topology.num_dsc,
> -		      reqs->topology.num_intf);
> -
> -	return 0;
> -}
> -
>   static void _dpu_rm_clear_mapping(uint32_t *res_mapping, int cnt,
>   				  uint32_t enc_id)
>   {
> @@ -608,9 +584,8 @@ int dpu_rm_reserve(
>   		struct dpu_global_state *global_state,
>   		struct drm_encoder *enc,
>   		struct drm_crtc_state *crtc_state,
> -		struct msm_display_topology topology)
> +		struct msm_display_topology *topology)
>   {
> -	struct dpu_rm_requirements reqs;
>   	int ret;
>   
>   	/* Check if this is just a page-flip */
> @@ -625,13 +600,11 @@ int dpu_rm_reserve(
>   	DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
>   		      enc->base.id, crtc_state->crtc->base.id);
>   
> -	ret = _dpu_rm_populate_requirements(enc, &reqs, topology);
> -	if (ret) {
> -		DPU_ERROR("failed to populate hw requirements\n");
> -		return ret;
> -	}
> +	DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
> +		      topology->num_lm, topology->num_dsc,
> +		      topology->num_intf);
>   
> -	ret = _dpu_rm_make_reservation(rm, global_state, enc, &reqs);
> +	ret = _dpu_rm_make_reservation(rm, global_state, enc, topology);
>   	if (ret)
>   		DPU_ERROR("failed to reserve hw resources: %d\n", ret);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index d62c2edb2460..f05697462856 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -71,7 +71,7 @@ int dpu_rm_reserve(struct dpu_rm *rm,
>   		struct dpu_global_state *global_state,
>   		struct drm_encoder *drm_enc,
>   		struct drm_crtc_state *crtc_state,
> -		struct msm_display_topology topology);
> +		struct msm_display_topology *topology);
>   
>   /**
>    * dpu_rm_reserve - Given the encoder for the display chain, release any