On Wed, Apr 27, 2016 at 11:54:35AM +1000, Dave Airlie wrote:
> On 22 April 2016 at 19:03, Daniel Vetter <daniel@xxxxxxxx> wrote:
> > On Fri, Apr 15, 2016 at 03:10:45PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <airlied@xxxxxxxxxx>
> >>
> >> Don't just free the connector when we get the destroy callback.
> >>
> >> Drop a reference to it, and set it's mst_port to NULL so
> >> no more mst work is done on it.
> >>
> >> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> >
> > Looks correct, but two comments for better api for shared helpers inline below.
> > -Daniel
> >
> >> ---
> >> drivers/gpu/drm/i915/intel_dp_mst.c | 46 ++++++++++++++++++-------------------
> >> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> >> 2 files changed, 24 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> index a2bd698..2e730b6 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> @@ -113,7 +113,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder)
> >>
> >> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >>
> >> - drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->port);
> >> + drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, intel_mst->connector->port);
> >>
> >> ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> >> if (ret) {
> >> @@ -138,10 +138,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder)
> >> /* and this can also fail */
> >> drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> >>
> >> - drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->port);
> >> + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, intel_mst->connector->port);
> >>
> >> intel_dp->active_mst_links--;
> >> - intel_mst->port = NULL;
> >> +
> >> + drm_connector_unreference(&intel_mst->connector->base);
> >> + intel_mst->connector = NULL;
> >> if (intel_dp->active_mst_links == 0) {
> >> intel_dig_port->base.post_disable(&intel_dig_port->base);
> >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >> @@ -181,7 +183,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> >> found->encoder = encoder;
> >>
> >> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >> - intel_mst->port = found->port;
> >> +
> >> + intel_mst->connector = found;
> >> + drm_connector_reference(&intel_mst->connector->base);
> >>
> >> if (intel_dp->active_mst_links == 0) {
> >> intel_prepare_ddi_buffer(&intel_dig_port->base);
> >> @@ -199,7 +203,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> >> }
> >>
> >> ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> >> - intel_mst->port,
> >> + intel_mst->connector->port,
> >> intel_crtc->config->pbn, &slots);
> >> if (ret == false) {
> >> DRM_ERROR("failed to allocate vcpi\n");
> >> @@ -248,7 +252,7 @@ static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
> >> {
> >> struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> >> *pipe = intel_mst->pipe;
> >> - if (intel_mst->port)
> >> + if (intel_mst->connector)
> >> return true;
> >> return false;
> >> }
> >> @@ -312,6 +316,11 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
> >> struct edid *edid;
> >> int ret;
> >>
> >> + if (!intel_connector->port || !intel_dp) {
> >> + ret = intel_connector_update_modes(connector, NULL);
> >> + return ret;
> >> + }
> >> +
> >> edid = drm_dp_mst_get_edid(connector, &intel_dp->mst_mgr, intel_connector->port);
> >> if (!edid)
> >> return 0;
> >> @@ -328,6 +337,8 @@ intel_dp_mst_detect(struct drm_connector *connector, bool force)
> >> struct intel_connector *intel_connector = to_intel_connector(connector);
> >> struct intel_dp *intel_dp = intel_connector->mst_port;
> >>
> >> + if (!intel_connector->port || !intel_dp)
> >> + return connector_status_disconnected;
> >> return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr, intel_connector->port);
> >
> > Should we push this change and the preceeding one into dp helpers, i.e.
> > make them cope with a null port? Otoh more work to fish out the
> > ->mst_mgr, so not sure.
>
> Actually they cope with a NULL port fine, so we can drop a bit of
> those two hunks,
>
> >
> > Hm ... isn't the mst_mgr a redundant argument, and we could figure that
> > out purely from the port? Would be a bit of refactoring, but I think the
> > shared code for this crucial bit of semantics (there's no way the mst port
> > is ever connected if it's unplugged) would be good.
>
> No the port isn't a reference counted object, it's just a point we
> revalidate in the
> mst code. The MST manager does't disappear. So you can't get from port
> to anything
> else until you check it's valid. You can't reference count port on the
> connector or
> else you just end up with circular references between the two.
Yeah I think this is beyond my understanding of the mst helpers. Really
should fix that sometimes ;-)
> >> - drm_modeset_unlock_all(dev);
> >> -
> >> intel_connector->unregister(intel_connector);
> >>
> >> drm_modeset_lock_all(dev);
> >> intel_connector_remove_from_fbdev(intel_connector);
> >> - drm_connector_cleanup(connector);
> >> + intel_connector->mst_port = NULL;
> >> drm_modeset_unlock_all(dev);
> >
> > Hm, ugly that we still need to grab modeset locks here. I'd like to avoid
> > that, since it could be a major stall. Maybe we need to have a separate
> > lock for the fbdev connector list, and then push the locking for that
> > (including refcounting) down into fbdev helpers?
>
> If you plug out an MST device a stall isn't going to be a major worry, you
> are going to get a modeset most likely straight away.
>
> So I think we should only address this problem when we have this problem.
Well it's more general unhappiness with how fbdev locking piggy-packs on
top of drm_modeset_lock_all(). At least ime reusing BKLs because they're
there already sooner or later leads to headaches - having separate locks
for separate things makes review easier. Avoid the stall is just a benefit
on top. Anyway, was just an idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
