| Message ID | 20200123002415.31478-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER | expand |
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
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;
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;
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
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
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
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 >
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;
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(-)