On Tue, Sep 19, 2023 at 09:14:27PM +0300, Imre Deak wrote:
> On Tue, Sep 19, 2023 at 06:44:00PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 14, 2023 at 10:26:45PM +0300, Imre Deak wrote:
> > > Recompute the state of all CRTCs on an FDI link during a modeset that
> > > may be affected by the modeset of other CRTCs on the same link. This
> > > ensures that each CRTC on the link maximizes its BW use (after another
> > > CRTC is disabled).
> > >
> > > In practice this means recomputing pipe B's config on IVB if pipe C gets
> > > disabled.
> > >
> > > v2:
> > > - Add the change recomputing affected CRTC states in a separate patch.
> > > (Ville)
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 4 ++
> > > drivers/gpu/drm/i915/display/intel_fdi.c | 53 ++++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_fdi.h | 1 +
> > > 3 files changed, 58 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index aad16dcceb788..31297a333f50e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6210,6 +6210,10 @@ int intel_atomic_check_config(struct intel_atomic_state *state,
> > > if (ret)
> > > return ret;
> > >
> > > + ret = intel_fdi_add_affected_crtcs(state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > if (!intel_crtc_needs_modeset(new_crtc_state)) {
> > > if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fdi.c b/drivers/gpu/drm/i915/display/intel_fdi.c
> > > index ad01915a4a39b..d723ae7e10d71 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fdi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fdi.c
> > > @@ -120,6 +120,59 @@ void intel_fdi_link_train(struct intel_crtc *crtc,
> > > dev_priv->display.funcs.fdi->fdi_link_train(crtc, crtc_state);
> > > }
> > >
> > > +/**
> > > + * intel_fdi_add_affected_crtcs - add CRTCs on FDI affected by other modeset CRTCs
> > > + * @state: intel atomic state
> > > + *
> > > + * Add a CRTC using FDI to @state if changing another CRTC's FDI BW usage is
> > > + * known to affect the available FDI BW for the former CRTC. In practice this
> > > + * means adding CRTC B on IVYBRIDGE if its use of FDI lanes is limited (by
> > > + * CRTC C) and CRTC C is getting disabled.
> > > + *
> > > + * Returns 0 in case of success, or a negative error code otherwise.
> > > + */
> > > +int intel_fdi_add_affected_crtcs(struct intel_atomic_state *state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > + struct intel_crtc_state *old_crtc_state;
> > > + struct intel_crtc_state *new_crtc_state;
> >
> > Both look like they can be const.
>
> Ok.
>
> > > + struct intel_crtc *crtc;
> > > +
> > > + if (!IS_IVYBRIDGE(i915))
> >
> > Should also check num_pipes==3 since pipe C can (at least in theory)
> > be fused off.
>
> Yes, but thought that pipe C's new_crtc_state would be NULL then.
Ah, crtc itself would be NULL then :/
> Can make the check more explicit in any case.
>
> > > + return 0;
> > > +
> > > + crtc = intel_crtc_for_pipe(i915, PIPE_C);
> > > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > > +
> > > + if (!new_crtc_state)
> > > + return 0;
> > > +
> > > + old_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> >
> > old vs. new mixup
>
> Err, yes thanks for catching it.
>
> > > +
> > > + if (!old_crtc_state->fdi_lanes)
> > > + return 0;
> > > +
> > > + if (!intel_crtc_needs_modeset(new_crtc_state))
> > > + return 0;
> > > +
> > > + if (new_crtc_state->uapi.enable)
> > > + return 0;
> >
> > We could be switching pipe C from driving a PCH port to driving
> > the CPU eDP port. So this check seems a bit wrong.
>
> Yes, didn't think of that case. I'll change it to:
>
> encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
> if (new_crtc_state->uapi.enable && encoder->port != PORT_A)
> return 0;
>
> > > +
> > > + crtc = intel_crtc_for_pipe(i915, PIPE_B);
> > > + new_crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> > > +
> > > + if (IS_ERR(new_crtc_state))
> > > + return PTR_ERR(old_crtc_state);
> > > +
> > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > > + if (!old_crtc_state->fdi_lanes)
> > > + return 0;
> > > +
> > > + return intel_modeset_pipes_in_mask_early(state,
> > > + "FDI link BW decrease on pipe C",
> > > + BIT(PIPE_B));
> > > +}
> > > +
> > > /* units of 100MHz */
> > > static int pipe_required_fdi_lanes(struct intel_crtc_state *crtc_state)
> > > {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fdi.h b/drivers/gpu/drm/i915/display/intel_fdi.h
> > > index 129444c580f27..eb02b967bb440 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fdi.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_fdi.h
> > > @@ -14,6 +14,7 @@ struct intel_crtc_state;
> > > struct intel_encoder;
> > > struct intel_link_bw_limits;
> > >
> > > +int intel_fdi_add_affected_crtcs(struct intel_atomic_state *state);
> > > int intel_fdi_link_freq(struct drm_i915_private *i915,
> > > const struct intel_crtc_state *pipe_config);
> > > int ilk_fdi_compute_config(struct intel_crtc *intel_crtc,
> > > --
> > > 2.37.2
> >
> > --
> > Ville Syrjälä
> > Intel
