[1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER
diff mbox series

Message ID 20200123002415.31478-1-manasi.d.navare@intel.com
State New
Headers show
Series
  • [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER
Related show

Commit Message

Manasi Navare Jan. 23, 2020, 12:24 a.m. UTC
In the port sync mode, for the master crtc, the master_transcoder is INVALID.
In that case since its value is -1, do not set the bit in the bitmask.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ville Syrjala Jan. 23, 2020, 2:01 p.m. UTC | #1
On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> In that case since its value is -1, do not set the bit in the bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> -				   BIT(new_crtc_state->master_transcoder);
> +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state->master_transcoder);
>  
>  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
>  				new_crtc_state->uapi.mode_changed = true;
> -- 
> 2.19.1
José Roberto de Souza Jan. 23, 2020, 6:25 p.m. UTC | #2
On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is
> INVALID.
> In that case since its value is -1, do not set the bit in the
> bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state-
> >sync_mode_slaves_mask |
> -				   BIT(new_crtc_state-
> >master_transcoder);
> +			u8 trans = new_crtc_state-
> >sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder !=
> INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state-
> >master_transcoder);

Why not set master_transcoder in port sync master too? Would avoid have
this check here and in future other places.

>  
>  			if (intel_cpu_transcoders_need_modeset(state,
> trans)) {
>  				new_crtc_state->uapi.mode_changed =
> true;
Ville Syrjala Jan. 23, 2020, 6:31 p.m. UTC | #3
On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > In the port sync mode, for the master crtc, the master_transcoder is
> > INVALID.
> > In that case since its value is -1, do not set the bit in the
> > bitmask.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > port sync")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 878d331b9e8c..79f9054078ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  		}
> >  
> >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > -			u8 trans = new_crtc_state-
> > >sync_mode_slaves_mask |
> > -				   BIT(new_crtc_state-
> > >master_transcoder);
> > +			u8 trans = new_crtc_state-
> > >sync_mode_slaves_mask;
> > +
> > +			if (new_crtc_state->master_transcoder !=
> > INVALID_TRANSCODER)
> > +				trans |= BIT(new_crtc_state-
> > >master_transcoder);
> 
> Why not set master_transcoder in port sync master too? Would avoid have
> this check here and in future other places.

Not how the hardware works. So would complicate hw readout and
programming code needlessly.

> 
> >  
> >  			if (intel_cpu_transcoders_need_modeset(state,
> > trans)) {
> >  				new_crtc_state->uapi.mode_changed =
> > true;
Ville Syrjala Jan. 23, 2020, 6:39 p.m. UTC | #4
On Thu, Jan 23, 2020 at 08:31:55PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > > In the port sync mode, for the master crtc, the master_transcoder is
> > > INVALID.
> > > In that case since its value is -1, do not set the bit in the
> > > bitmask.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > > port sync")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 878d331b9e8c..79f9054078ea 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		}
> > >  
> > >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > -			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask |
> > > -				   BIT(new_crtc_state-
> > > >master_transcoder);
> > > +			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask;
> > > +
> > > +			if (new_crtc_state->master_transcoder !=
> > > INVALID_TRANSCODER)
> > > +				trans |= BIT(new_crtc_state-
> > > >master_transcoder);
> > 
> > Why not set master_transcoder in port sync master too? Would avoid have
> > this check here and in future other places.
> 
> Not how the hardware works. So would complicate hw readout and
> programming code needlessly.

Hmm. Actually not that much maybe. Readout already has to trawl
everything to populate the slave bitmask. So the change to programming
part would just be something like

+ if (cpu_transcoder != master_transcoder)
	enable port sync mode;

So could maybe be done. Might make state dumps a bit more confusing
though since we definitely do not enable port sync mode on the
master transcoder.

> 
> > 
> > >  
> > >  			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> 
> -- 
> Ville Syrjälä
> Intel
Manasi Navare Jan. 23, 2020, 10 p.m. UTC | #5
On Thu, Jan 23, 2020 at 08:31:55PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > > In the port sync mode, for the master crtc, the master_transcoder is
> > > INVALID.
> > > In that case since its value is -1, do not set the bit in the
> > > bitmask.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > > port sync")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 878d331b9e8c..79f9054078ea 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		}
> > >  
> > >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > -			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask |
> > > -				   BIT(new_crtc_state-
> > > >master_transcoder);
> > > +			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask;
> > > +
> > > +			if (new_crtc_state->master_transcoder !=
> > > INVALID_TRANSCODER)
> > > +				trans |= BIT(new_crtc_state-
> > > >master_transcoder);
> > 
> > Why not set master_transcoder in port sync master too? Would avoid have
> > this check here and in future other places.
> 
> Not how the hardware works. So would complicate hw readout and
> programming code needlessly.

Yes and thats how we identify that it is the master since its master trans is INVALID

Manasi

> 
> > 
> > >  
> > >  			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> 
> -- 
> Ville Syrjälä
> Intel
Manasi Navare Jan. 23, 2020, 10:04 p.m. UTC | #6
On Thu, Jan 23, 2020 at 04:01:28PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> > In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> > In that case since its value is -1, do not set the bit in the bitmask.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review, I think populating master transcoder in master would also need us
to change all the helpers for is_trans_port_sync_mode and the readout and the commit_modeset_enables logic.
Its doable but too risky to change at this point incase it causes any regressions.

Still assume your r-b on this?

Manasi
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 878d331b9e8c..79f9054078ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		}
> >  
> >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> > -				   BIT(new_crtc_state->master_transcoder);
> > +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> > +
> > +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> > +				trans |= BIT(new_crtc_state->master_transcoder);
> >  
> >  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
> >  				new_crtc_state->uapi.mode_changed = true;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
Manasi Navare Jan. 24, 2020, 1:13 a.m. UTC | #7
Thanks for the review, pushed to dinq

Manasi

On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> In that case since its value is -1, do not set the bit in the bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> -				   BIT(new_crtc_state->master_transcoder);
> +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state->master_transcoder);
>  
>  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
>  				new_crtc_state->uapi.mode_changed = true;
> -- 
> 2.19.1
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 878d331b9e8c..79f9054078ea 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14649,8 +14649,10 @@  static int intel_atomic_check(struct drm_device *dev,
 		}
 
 		if (is_trans_port_sync_mode(new_crtc_state)) {
-			u8 trans = new_crtc_state->sync_mode_slaves_mask |
-				   BIT(new_crtc_state->master_transcoder);
+			u8 trans = new_crtc_state->sync_mode_slaves_mask;
+
+			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
+				trans |= BIT(new_crtc_state->master_transcoder);
 
 			if (intel_cpu_transcoders_need_modeset(state, trans)) {
 				new_crtc_state->uapi.mode_changed = true;