| Message ID | 1485301777-3465-10-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > --- > 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. > 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 >
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@intel.com> > > --- > > 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 > > >
On Wed, Jan 25, 2017 at 09:00:02PM +0000, Pandiyan, Dhinakaran wrote: > 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@intel.com> > > > --- > > > 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? Not sure we have any direct mode-switching tests. Please chat with Mika Kahola and make sure we do have that test coverage. Mika is working on atomic test coverage, and having this would be good in general (not just for mst). -Daniel
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); continue; + } crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc); 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); + if (slots < 0) { + DRM_DEBUG_KMS("not enough link bw for this mode\n"); + return false; + } intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock,
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@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++++- drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-)