On Thu, Sep 03, 2020 at 10:23:35PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 15, 2020 at 03:42:21PM -0700, Manasi Navare wrote:
> > From: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > 
> > Enabling is done in a special sequence and so should plane updates
> > be. Ideally the end user never notices the second pipe is used,
> > so use the vblank evasion to cover both pipes.
> > 
> > This way ideally everything will be tear free, and updates are
> > really atomic as userspace expects it.
> > 
> > ****This needs to be checked if it still works since lot of refactoring
> > in skl_commit_modeset_enables
> > 
> > v2:
> > * Manual Rebase (Manasi)
> > * Refactoring on intel_update_crtc and enable_crtc and removing
> > special trans_port_sync_update (Manasi)
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.nav...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 120 +++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_sprite.c  |  25 +++-
> >  drivers/gpu/drm/i915/display/intel_sprite.h  |   3 +-
> >  3 files changed, 129 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a1011414da6d..00b26863ffc6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15656,7 +15656,7 @@ static void intel_update_crtc(struct 
> > intel_atomic_state *state,
> >     else
> >             i9xx_update_planes_on_crtc(state, crtc);
> >  
> > -   intel_pipe_update_end(new_crtc_state);
> > +   intel_pipe_update_end(new_crtc_state, NULL);
> >  
> >     /*
> >      * We usually enable FIFO underrun interrupts as part of the
> > @@ -15754,6 +15754,52 @@ static void intel_commit_modeset_disables(struct 
> > intel_atomic_state *state)
> >     }
> >  }
> >  
> > +static void intel_update_bigjoiner(struct intel_crtc *crtc,
> > +                              struct intel_atomic_state *state,
> > +                              struct intel_crtc_state *old_crtc_state,
> > +                              struct intel_crtc_state *new_crtc_state)
> > +{
> > +   struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +   bool modeset = needs_modeset(new_crtc_state);
> > +   struct intel_crtc *slave = new_crtc_state->bigjoiner_linked_crtc;
> > +   struct intel_crtc_state *new_slave_crtc_state =
> > +           intel_atomic_get_new_crtc_state(state, slave);
> > +
> > +   if (modeset) {
> > +           /* Enable slave first */
> > +           intel_crtc_update_active_timings(new_slave_crtc_state);
> > +           dev_priv->display.crtc_enable(state, slave);
> > +
> > +           /* Then master */
> > +           intel_crtc_update_active_timings(new_crtc_state);
> > +           dev_priv->display.crtc_enable(state, crtc);
> > +
> > +           /* vblanks work again, re-enable pipe CRC. */
> > +           intel_crtc_enable_pipe_crc(crtc);
> > +
> > +   } else {
> > +           intel_pre_plane_update(state, crtc);
> > +           intel_pre_plane_update(state, slave);
> > +
> > +           if (new_crtc_state->update_pipe)
> > +                   intel_encoders_update_pipe(state, crtc);
> > +   }
> > +
> > +   /*
> > +    * Perform vblank evasion around commit operation, and make sure to
> > +    * commit both planes simultaneously for best results.
> > +    */
> > +   intel_pipe_update_start(new_crtc_state);
> > +
> > +   commit_pipe_config(state, crtc);
> > +   commit_pipe_config(state, slave);
> > +
> > +   skl_update_planes_on_crtc(state, crtc);
> > +   skl_update_planes_on_crtc(state, slave);
> > +
> > +   intel_pipe_update_end(new_crtc_state, new_slave_crtc_state);
> > +}
> 
> I think this should ideally all go away and just the normal logic
> in commit_modeset_enables() should deal with it. Just like it does
> for port sync/mst pipe dependencies.
>

Yes I think so too. Except for the intel_pipe_update_end where
now we send the new_slave_crtc_state() so thats still something I need to figure
how it will work in normal code without special bigjoiner handling.

I think the 2p2p transcoder ports sync code initially had a special trans port 
sync handling
function and thats why this was written this way but with the new code we just 
use
the regular modeset_enables function with no special handling

Manasi
 
> > +
> >  static void intel_commit_modeset_enables(struct intel_atomic_state *state)
> >  {
> >     struct intel_crtc_state *new_crtc_state;
> > @@ -15772,15 +15818,22 @@ static void intel_commit_modeset_enables(struct 
> > intel_atomic_state *state)
> >  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> >  {
> >     struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > -   struct intel_crtc *crtc;
> > +   struct intel_crtc *crtc, *slave;
> >     struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> >     struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> > +   struct skl_ddb_entry new_entries[I915_MAX_PIPES] = {};
> >     u8 update_pipes = 0, modeset_pipes = 0;
> > +   const struct intel_crtc_state *slave_crtc_state;
> >     int i;
> >  
> >     for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, 
> > new_crtc_state, i) {
> >             enum pipe pipe = crtc->pipe;
> >  
> > +           if (new_crtc_state->bigjoiner_slave) {
> > +                   /* We're updated from master */
> > +                   continue;
> > +           }
> > +
> >             if (!new_crtc_state->hw.active)
> >                     continue;
> >  
> > @@ -15791,6 +15844,34 @@ static void skl_commit_modeset_enables(struct 
> > intel_atomic_state *state)
> >             } else {
> >                     modeset_pipes |= BIT(pipe);
> >             }
> > +
> > +           if (new_crtc_state->bigjoiner) {
> > +                   slave = new_crtc_state->bigjoiner_linked_crtc;
> > +                   slave_crtc_state =
> > +                           intel_atomic_get_new_crtc_state(state,
> > +                                                           slave);
> > +
> > +                   /* put both entries in */
> > +                   new_entries[i].start = new_crtc_state->wm.skl.ddb.start;
> > +                   new_entries[i].end = slave_crtc_state->wm.skl.ddb.end;
> > +           } else {
> > +                   new_entries[i] = new_crtc_state->wm.skl.ddb;
> > +           }
> > +
> > +           /* ignore allocations for crtc's that have been turned off 
> > during modeset. */
> > +           if (needs_modeset(new_crtc_state))
> > +                   continue;
> > +
> > +           if (old_crtc_state->bigjoiner) {
> > +                   slave = old_crtc_state->bigjoiner_linked_crtc;
> > +                   slave_crtc_state =
> > +                           intel_atomic_get_old_crtc_state(state, slave);
> > +
> > +                   entries[i].start = old_crtc_state->wm.skl.ddb.start;
> > +                   entries[i].end = slave_crtc_state->wm.skl.ddb.end;
> > +           } else {
> > +                   entries[i] = old_crtc_state->wm.skl.ddb;
> > +           }
> 
> Why is this here? Can't see why the current code wouldn't work just fine
> for bigjoiner too.
> 
> >     }
> >  
> >     /*
> > @@ -15806,28 +15887,34 @@ static void skl_commit_modeset_enables(struct 
> > intel_atomic_state *state)
> >             for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >                                                 new_crtc_state, i) {
> >                     enum pipe pipe = crtc->pipe;
> > +                   bool ddb_changed;
> >  
> >                     if ((update_pipes & BIT(pipe)) == 0)
> >                             continue;
> >  
> > -                   if 
> > (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > +                   if (skl_ddb_allocation_overlaps(&new_entries[pipe],
> >                                                     entries, 
> > I915_MAX_PIPES, pipe))
> >                             continue;
> >  
> > -                   entries[pipe] = new_crtc_state->wm.skl.ddb;
> > +                   ddb_changed = !skl_ddb_entry_equal(&new_entries[pipe], 
> > &entries[pipe]);
> > +                   entries[pipe] = new_entries[pipe];
> >                     update_pipes &= ~BIT(pipe);
> >  
> > -                   intel_update_crtc(state, crtc);
> > -
> >                     /*
> >                      * If this is an already active pipe, it's DDB changed,
> >                      * and this isn't the last pipe that needs updating
> >                      * then we need to wait for a vblank to pass for the
> >                      * new ddb allocation to take effect.
> >                      */
> > -                   if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
> > -                                            &old_crtc_state->wm.skl.ddb) &&
> > -                       (update_pipes | modeset_pipes))
> > +                   if (new_crtc_state->bigjoiner) {
> > +                           intel_update_bigjoiner(crtc, state,
> > +                                                  old_crtc_state,
> > +                                                  new_crtc_state);
> > +                   } else {
> > +                           intel_update_crtc(state, crtc);
> > +                   }
> > +
> > +                   if (ddb_changed && (update_pipes | modeset_pipes))
> >                             intel_wait_for_vblank(dev_priv, pipe);
> >             }
> >     }
> > @@ -15863,9 +15950,18 @@ static void skl_commit_modeset_enables(struct 
> > intel_atomic_state *state)
> >             if ((modeset_pipes & BIT(pipe)) == 0)
> >                     continue;
> >  
> > +           WARN_ON(skl_ddb_allocation_overlaps(&new_entries[pipe],
> > +                                               entries, I915_MAX_PIPES, 
> > pipe));
> > +
> > +           entries[pipe] = new_entries[pipe];
> >             modeset_pipes &= ~BIT(pipe);
> >  
> > -           intel_enable_crtc(state, crtc);
> > +           if (new_crtc_state->bigjoiner)
> > +                   intel_update_bigjoiner(crtc, state,
> > +                                          old_crtc_state,
> > +                                          new_crtc_state);
> > +           else
> > +                   intel_enable_crtc(state, crtc);
> >     }
> >  
> >     /*
> > @@ -15877,10 +15973,10 @@ static void skl_commit_modeset_enables(struct 
> > intel_atomic_state *state)
> >             if ((update_pipes & BIT(pipe)) == 0)
> >                     continue;
> >  
> > -           drm_WARN_ON(&dev_priv->drm, 
> > skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> > +           drm_WARN_ON(&dev_priv->drm, 
> > skl_ddb_allocation_overlaps(&new_entries[pipe],
> >                                                                     
> > entries, I915_MAX_PIPES, pipe));
> >  
> > -           entries[pipe] = new_crtc_state->wm.skl.ddb;
> > +           entries[pipe] = new_entries[pipe];
> >             update_pipes &= ~BIT(pipe);
> >  
> >             intel_update_crtc(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 60eeed06a780..eaae5df546fe 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -99,6 +99,8 @@ void intel_pipe_update_start(const struct 
> > intel_crtc_state *new_crtc_state)
> >  
> >     /* FIXME needs to be calibrated sensibly */
> >     min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
> > +                                                 new_crtc_state->bigjoiner 
> > ?
> > +                                                 2 * 
> > VBLANK_EVASION_TIME_US :
> >                                                   VBLANK_EVASION_TIME_US);
> >     max = vblank_start - 1;
> >  
> > @@ -191,7 +193,8 @@ void intel_pipe_update_start(const struct 
> > intel_crtc_state *new_crtc_state)
> >   * re-enables interrupts and verifies the update was actually completed
> >   * before a vblank.
> >   */
> > -void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> > +void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state,
> > +                      struct intel_crtc_state *slave_crtc_state)
> >  {
> >     struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> >     enum pipe pipe = crtc->pipe;
> > @@ -206,16 +209,26 @@ void intel_pipe_update_end(struct intel_crtc_state 
> > *new_crtc_state)
> >      * Would be slightly nice to just grab the vblank count and arm the
> >      * event outside of the critical section - the spinlock might spin for a
> >      * while ... */
> > -   if (new_crtc_state->uapi.event) {
> > -           drm_WARN_ON(&dev_priv->drm,
> > -                       drm_crtc_vblank_get(&crtc->base) != 0);
> > +   if (new_crtc_state->uapi.event || (slave_crtc_state && 
> > slave_crtc_state->uapi.event)) {
> > +           if (new_crtc_state->uapi.event)
> > +                   drm_WARN_ON(&dev_priv->drm,
> > +                               drm_crtc_vblank_get(&crtc->base) != 0);
> > +           if (slave_crtc_state && slave_crtc_state->uapi.event)
> > +                   drm_WARN_ON(&dev_priv->drm,
> > +                               drm_crtc_vblank_get(&crtc->base) != 0);
> >  
> >             spin_lock(&crtc->base.dev->event_lock);
> > -           drm_crtc_arm_vblank_event(&crtc->base,
> > -                                     new_crtc_state->uapi.event);
> > +           if (new_crtc_state->uapi.event)
> > +                   drm_crtc_arm_vblank_event(&crtc->base,
> > +                                             new_crtc_state->uapi.event);
> > +           if (slave_crtc_state && slave_crtc_state->uapi.event)
> > +                   drm_crtc_arm_vblank_event(&crtc->base,
> > +                                             slave_crtc_state->uapi.event);
> >             spin_unlock(&crtc->base.dev->event_lock);
> >  
> >             new_crtc_state->uapi.event = NULL;
> > +           if (slave_crtc_state)
> > +                   slave_crtc_state->uapi.event = NULL;
> >     }
> >  
> >     local_irq_enable();
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.h 
> > b/drivers/gpu/drm/i915/display/intel_sprite.h
> > index cd2104ba1ca1..15e7c112ec77 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.h
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.h
> > @@ -24,7 +24,8 @@ struct intel_plane *intel_sprite_plane_create(struct 
> > drm_i915_private *dev_priv,
> >  int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, void *data,
> >                                 struct drm_file *file_priv);
> >  void intel_pipe_update_start(const struct intel_crtc_state 
> > *new_crtc_state);
> > -void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state);
> > +void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state,
> > +                      struct intel_crtc_state *slave_crtc_state);
> >  int intel_plane_check_stride(const struct intel_plane_state *plane_state);
> >  int intel_plane_check_src_coordinates(struct intel_plane_state 
> > *plane_state);
> >  int chv_plane_check_rotation(const struct intel_plane_state *plane_state);
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to