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.
>> - 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.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
