On Wed, 2017-01-25 at 07:15 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> > Use the added helpers to track MST link bandwidth for atomic modesets.
> > Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> > enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> > Similarly, link bw is released during ->atomic_check() with the connector
> > helper callback ->atomic_release() when CRTCs are disabled.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++++-
> > drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
> > 2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index dd34d21..0d42173 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -385,8 +385,15 @@ mode_fixup(struct drm_atomic_state *state)
> >
> > WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
> >
> > - if (!conn_state->crtc || !conn_state->best_encoder)
> > + if (!conn_state->crtc || !conn_state->best_encoder) {
> > + const struct drm_connector_helper_funcs *conn_funcs;
> > +
> > + conn_funcs = connector->helper_private;
> > + if (conn_funcs->atomic_release)
> > + conn_funcs->atomic_release(connector,
> > + conn_state);
>
> I wonder whether this won't surprise drivers: The way you coded this
> release only gets called when the connector is fully disabled. But it does
> not get called when you atomically switch it from one crtc to the other
> (in which case you also might want to release resources, before allocating
> new ones). I think that case of directly switching stuff is even a bug in
> your current atomic tracking code ...
>
> We also need to extract the release calls into a separate loop which runs
> _before_ we allocate any resources. Otherwise if you do an atomic commit
> where you disable connector2 and enable connector1 and they can't run both
> at the same time it'll fail, because we'll release the vcpi for connector2
> only after we've tried to get it for connnector1.
>
Yeah, you are right. I thought of this problem and then forgot about it.
Is there any igt test to test the switching?
-DK
> > continue;
> > + }
> >
> > crtc_state = drm_atomic_get_existing_crtc_state(state,
> > conn_state->crtc);
>
> Misplaced hunk, this needs to be in the patch that adds the
> ->atomic_release callback.
>
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 2f57a56..ee5fedb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > int lane_count, slots;
> > const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > int mst_pbn;
> > + struct drm_dp_mst_topology_state *topology_state;
> >
> > pipe_config->has_pch_encoder = false;
> > bpp = 24;
> > @@ -65,7 +66,17 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
> >
> > pipe_config->pbn = mst_pbn;
> > - slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> > + topology_state = drm_atomic_get_mst_topology_state(state,
> > + &intel_dp->mst_mgr);
> > + if (topology_state == NULL)
> > + return false;
> > +
> > + slots = drm_dp_atomic_find_vcpi_slots(topology_state, connector->port,
> > + mst_pbn);
>
> Can we merge the get_mst_topology_state and find_vcpi_slots functions
> together? Would be less code in drivers ...
>
> > + if (slots < 0) {
> > + DRM_DEBUG_KMS("not enough link bw for this mode\n");
> > + return false;
> > + }
>
> And then also stuff the error checking in there, and just have a return
> -EINVAL when there's not enough bw.
>
> I think this should be together with the previous patch, to wire up the
> entire mst state tracking for i915 at the same time. Atm the previous
> patch just wires up dead code, which is a bit backwards.
> -Daniel
>
> >
> > intel_link_compute_m_n(bpp, lane_count,
> > adjusted_mode->crtc_clock,
> > --
> > 2.7.4
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
