diff mbox series

[11/11] drm/i915/ddi: simplify intel_ddi_get_encoder_pipes() slightly

Message ID 0aa1274597fa84a0dc3c9ccf7bb20997d1d154bf.1731941270.git.jani.nikula@intel.com (mailing list archive)
State New
Headers show
Series drm/i915: MST and DDI cleanups and refactoring | expand

Commit Message

Jani Nikula Nov. 18, 2024, 2:49 p.m. UTC 
Use a temporary variable for DDI mode to simplify the conditions. This
is in line with the other places that read DDI mode.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Imre Deak Nov. 19, 2024, 11:40 a.m. UTC | #1 
On Mon, Nov 18, 2024 at 04:49:06PM +0200, Jani Nikula wrote:
> Use a temporary variable for DDI mode to simplify the conditions. This
> is in line with the other places that read DDI mode.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 33628cbc0f72..e25b712bf03b 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -818,7 +818,7 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>  	mst_pipe_mask = 0;
>  	for_each_pipe(dev_priv, p) {
>  		enum transcoder cpu_transcoder = (enum transcoder)p;
> -		unsigned int port_mask, ddi_select;
> +		u32 port_mask, ddi_select, ddi_mode;
>  		intel_wakeref_t trans_wakeref;
>  
>  		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
> @@ -842,9 +842,10 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>  		if ((tmp & port_mask) != ddi_select)
>  			continue;
>  
> -		if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST ||
> -		    (HAS_DP20(display) &&
> -		     (tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B))
> +		ddi_mode = tmp & TRANS_DDI_MODE_SELECT_MASK;
> +
> +		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST ||
> +		    (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)))

nit: the above condition and the fdi counterpart is used elsewhere too,
so could use a helper. The patchset looks ok regardless:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  			mst_pipe_mask |= BIT(p);
>  
>  		*pipe_mask |= BIT(p);
> -- 
> 2.39.5
>
Jani Nikula Nov. 19, 2024, 1:11 p.m. UTC | #2 
On Tue, 19 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Nov 18, 2024 at 04:49:06PM +0200, Jani Nikula wrote:
>> Use a temporary variable for DDI mode to simplify the conditions. This
>> is in line with the other places that read DDI mode.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 33628cbc0f72..e25b712bf03b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -818,7 +818,7 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  	mst_pipe_mask = 0;
>>  	for_each_pipe(dev_priv, p) {
>>  		enum transcoder cpu_transcoder = (enum transcoder)p;
>> -		unsigned int port_mask, ddi_select;
>> +		u32 port_mask, ddi_select, ddi_mode;
>>  		intel_wakeref_t trans_wakeref;
>>  
>>  		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
>> @@ -842,9 +842,10 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  		if ((tmp & port_mask) != ddi_select)
>>  			continue;
>>  
>> -		if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST ||
>> -		    (HAS_DP20(display) &&
>> -		     (tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B))
>> +		ddi_mode = tmp & TRANS_DDI_MODE_SELECT_MASK;
>> +
>> +		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST ||
>> +		    (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)))
>
> nit: the above condition and the fdi counterpart is used elsewhere too,
> so could use a helper. The patchset looks ok regardless:

It'll need to change anyway going forward, as the latter needs better
SST vs. MST detection.

> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks,
Jani.


>
>>  			mst_pipe_mask |= BIT(p);
>>  
>>  		*pipe_mask |= BIT(p);
>> -- 
>> 2.39.5
>>
Jani Nikula Nov. 20, 2024, 11:35 a.m. UTC | #3 
On Tue, 19 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Nov 18, 2024 at 04:49:06PM +0200, Jani Nikula wrote:
>> Use a temporary variable for DDI mode to simplify the conditions. This
>> is in line with the other places that read DDI mode.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 33628cbc0f72..e25b712bf03b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -818,7 +818,7 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  	mst_pipe_mask = 0;
>>  	for_each_pipe(dev_priv, p) {
>>  		enum transcoder cpu_transcoder = (enum transcoder)p;
>> -		unsigned int port_mask, ddi_select;
>> +		u32 port_mask, ddi_select, ddi_mode;
>>  		intel_wakeref_t trans_wakeref;
>>  
>>  		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
>> @@ -842,9 +842,10 @@ static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
>>  		if ((tmp & port_mask) != ddi_select)
>>  			continue;
>>  
>> -		if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST ||
>> -		    (HAS_DP20(display) &&
>> -		     (tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B))
>> +		ddi_mode = tmp & TRANS_DDI_MODE_SELECT_MASK;
>> +
>> +		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST ||
>> +		    (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)))
>
> nit: the above condition and the fdi counterpart is used elsewhere too,
> so could use a helper. The patchset looks ok regardless:
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Hum, so was this for the entire series? Also how about v2 of patch 6?

Please consider either replying to the cover letter or for each
individual patch, so that b4 gather the trailers automagically.

BR,
Jani.


>
>>  			mst_pipe_mask |= BIT(p);
>>  
>>  		*pipe_mask |= BIT(p);
>> -- 
>> 2.39.5
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 33628cbc0f72..e25b712bf03b 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -818,7 +818,7 @@  static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
 	mst_pipe_mask = 0;
 	for_each_pipe(dev_priv, p) {
 		enum transcoder cpu_transcoder = (enum transcoder)p;
-		unsigned int port_mask, ddi_select;
+		u32 port_mask, ddi_select, ddi_mode;
 		intel_wakeref_t trans_wakeref;
 
 		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
@@ -842,9 +842,10 @@  static void intel_ddi_get_encoder_pipes(struct intel_encoder *encoder,
 		if ((tmp & port_mask) != ddi_select)
 			continue;
 
-		if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST ||
-		    (HAS_DP20(display) &&
-		     (tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B))
+		ddi_mode = tmp & TRANS_DDI_MODE_SELECT_MASK;
+
+		if (ddi_mode == TRANS_DDI_MODE_SELECT_DP_MST ||
+		    (ddi_mode == TRANS_DDI_MODE_SELECT_FDI_OR_128B132B && HAS_DP20(display)))
 			mst_pipe_mask |= BIT(p);
 
 		*pipe_mask |= BIT(p);