On Fri, Apr 28, 2023 at 08:47:23PM +0300, Imre Deak wrote:
> On Fri, Apr 28, 2023 at 08:22:54PM +0300, Imre Deak wrote:
> > On Fri, Apr 28, 2023 at 05:02:35PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 26, 2023 at 07:52:59PM +0300, Imre Deak wrote:
> > > > [...]
> > > > +static u32 get_transcoder_pipes(struct drm_i915_private *i915, u32
> > > > transcoder_mask)
> > > > +{
> > > > + struct intel_crtc *slave_crtc;
> > > > + u32 pipes = 0;
> > > > +
> > > > + for_each_intel_crtc(&i915->drm, slave_crtc) {
> > > > + struct intel_crtc_state *slave_crtc_state =
> > > > + to_intel_crtc_state(slave_crtc->base.state);
> > > > +
> > > > + if (slave_crtc_state->cpu_transcoder ==
> > > > INVALID_TRANSCODER)
> > > > + continue;
> > > > +
> > > > + if (transcoder_mask &
> > > > BIT(slave_crtc_state->cpu_transcoder))
> > > > + pipes |= BIT(slave_crtc->pipe);
> > > > + }
> > > > +
> > > > + return pipes;
> > > > +}
> > > > +
> > > > +static u32 get_bigjoiner_slave_pipes(struct drm_i915_private *i915,
> > > > u32 master_pipes)
> > > > +{
> > > > + struct intel_crtc *master_crtc;
> > > > + u32 pipes = 0;
> > > > +
> > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, master_crtc,
> > > > master_pipes) {
> > > > + struct intel_crtc_state *master_crtc_state =
> > > > + to_intel_crtc_state(master_crtc->base.state);
> > > > +
> > > > + pipes |=
> > > > intel_crtc_bigjoiner_slave_pipes(master_crtc_state);
> > > > + }
> > > > +
> > > > + return pipes;
> > > > +}
> > > > +
> > > > +static void kill_bigjoiner_slave_noatomic(struct intel_crtc
> > > > *master_crtc)
> > > > +{
> > > > + struct drm_i915_private *i915 = to_i915(master_crtc->base.dev);
> > > > + struct intel_crtc_state *master_crtc_state =
> > > > + to_intel_crtc_state(master_crtc->base.state);
> > > > + struct intel_crtc *slave_crtc;
> > > > +
> > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc,
> > > > +
> > > > intel_crtc_bigjoiner_slave_pipes(master_crtc_state)) {
> > > > + struct intel_crtc_state *slave_crtc_state =
> > > > + to_intel_crtc_state(slave_crtc->base.state);
> > > > +
> > > > + slave_crtc_state->bigjoiner_pipes = 0;
> > > > + }
> > > > +
> > > > + master_crtc_state->bigjoiner_pipes = 0;
> > > > +}
> > > > +
> > > > +static void disable_crtc_with_slaves(struct intel_crtc *crtc,
> > > > + struct drm_modeset_acquire_ctx
> > > > *ctx)
> > > > +{
> > > > + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > > > + struct intel_crtc_state *crtc_state =
> > > > to_intel_crtc_state(crtc->base.state);
> > > > + u32 bigjoiner_masters = BIT(crtc->pipe) |
> > > > + get_transcoder_pipes(i915,
> > > > crtc_state->sync_mode_slaves_mask);
> > >
> > > The resulting bitmask would seem to also include the bigjoiner slaves.
> >
> > Hrm, doesn't sync_mode_slaves_mask contain only the transcoder bits for
> > the port-synced master CRTCs? I assumed that those port-synced master
> > CRTCs would point to their slave pipes via their
> > crtc_state->bigjoiner_pipes mask.
>
> Ah, understood the issue, the port_synced slave CRTCs need to get
> disabled before the port_sync master.
>
> So I think the following gives the correct order:
>
> u32 portsync_slaves = get_transcoder_pipes(i915,
>
> crtc_state->sync_mode_slaves_mask);
> u32 bigjoiner_slaves = get_bigjoiner_slave_pipes(i915,
> BIT(crtc->pipe) |
> portsync_slaves);
>
> for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc,
> bigjoiner_slaves)
> intel_crtc_disable_noatomic(temp_crtc, ctx);
>
> for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc,
> portsync_slaves) {
> intel_crtc_disable_noatomic(temp_crtc, ctx);
> kill_bigjoiner_slave_noatomic(temp_crtc);
> }
>
> intel_crtc_disable_noatomic(crtc);
> kill_bigjoiner_slave_noatomic(crtc);
>
> ?
And that a bigjoiner slave pipe's transcoder points to the master
transcoder :/ so those slave pipes need to be filtered out in
get_transcoder_pipes().
> > > > + u32 bigjoiner_slaves = get_bigjoiner_slave_pipes(i915,
> > > > bigjoiner_masters);
> > > > + struct intel_crtc *temp_crtc;
> > > > +
> > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc,
> > > > bigjoiner_slaves)
> > > > + intel_crtc_disable_noatomic(temp_crtc, ctx);
> > > > +
> > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc,
> > > > bigjoiner_masters) {
> > > > + intel_crtc_disable_noatomic(temp_crtc, ctx);
> > > > + kill_bigjoiner_slave_noatomic(temp_crtc);
> > > > + }
> > > > +}
> > > > +
> > > > static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > > > struct drm_modeset_acquire_ctx *ctx)
> > > > {
> > > > @@ -299,10 +404,11 @@ static void intel_sanitize_crtc(struct intel_crtc
> > > > *crtc,
> > > > /*
> > > > * Adjust the state of the output pipe according to whether we
> > > > have
> > > > * active connectors/encoders.
> > > > + * TODO: Add support for MST
> > > > */
> > > > if (crtc_state->hw.active && !intel_crtc_has_encoders(crtc) &&
> > > > !intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > - intel_crtc_disable_noatomic(crtc, ctx);
> > > > + disable_crtc_with_slaves(crtc, ctx);
> > >
> > > I'd like to keep the _noatomic() in the name.
> >
> > Ok, will change this.
> >
> > >
> > > > }
> > > >
> > > > static bool has_bogus_dpll_config(const struct intel_crtc_state
> > > > *crtc_state)
> > > > --
> > > > 2.37.2
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
