Re: [PATCH 14/15] drm/i915/mst: use reference counted connectors.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux