Re: [PATCH 01/25] drm/msm/dpu: fix hw ctl retrieval for mixer muxing

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

On 2018-10-09 11:07, Sean Paul wrote:
> On Mon, Oct 08, 2018 at 09:27:18PM -0700, Jeykumar Sankaran wrote:
>> Layer mixer/pingpong block counts and hw ctl block counts
>> will not be same for all the topologies (e.g. layer
>> mixer muxing to single interface)
>> 
>> Use the encoder's split_role info to retrieve the
>> respective control path for programming.
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 96cdf06..d12f896 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1060,6 +1060,7 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>> 
>>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +		int ctl_index;
>> 
>>  		if (phys) {
>>  			if (!dpu_enc->hw_pp[i]) {
>> @@ -1068,14 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  				return;
>>  			}
>> 
>> -			if (!hw_ctl[i]) {
>> +			ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1
> : 0;
>> +
> 
> What if MAX_CHANNELS_PER_ENC isn't 2? Similarly, what if num_phys_encs 
> >
> MAX_CHANNELS_PER_ENC? It seems like there should be a more formal
> relationship
> between all of these verious values (num_of_h_tiles assumed to be <= 2 
> as
> well).
> If one of them changes beyond the assumed bound, the rest of the driver
> falls
> over pretty hard.
> 
MAX_CHANNELS_PER_ENC is set to 2 to represent HW limitation on the 
chipset as
we cannot gang up more than 2 LM chain to an interface. Supporting more 
than 2
might demand much larger changes than validating for boundaries.

num_phys_enc is the max no of phys encoders we create as we are looping 
through
num_of_h_tiles which cannot be more than priv->dsi array size.

So its very unlikely we would expect these loops to go out of bound!

Thanks,
Jeykumar S.
> 
>> +			if (!hw_ctl[ctl_index]) {
>>  				DPU_ERROR_ENC(dpu_enc, "no ctl block
> assigned"
>> -					     "at idx: %d\n", i);
>> +					     "at idx: %d\n", ctl_index);
>>  				return;
> 
> When you return on error here, should you give back the resources that
> you've
> already provisioned?
> 
>>  			}
>> 
>>  			phys->hw_pp = dpu_enc->hw_pp[i];
>> -			phys->hw_ctl = hw_ctl[i];
>> +			phys->hw_ctl = hw_ctl[ctl_index];
>> 
>>  			phys->connector = conn->state->connector;
>>  			if (phys->ops.mode_set)
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno

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