From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com ([192.55.52.151]:53750 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726250AbeLDMpK (ORCPT ); Tue, 4 Dec 2018 07:45:10 -0500 From: Jani Nikula To: Stanislav Lisovskiy , dri-devel@lists.freedesktop.org Cc: ville.syrjala@intel.com, intel-gfx@lists.freedesktop.org, martin.peres@intel.com, stable@vger.kernel.org, stanislav.lisovskiy@intel.com Subject: Re: [PATCH v3] Return only active connectors for get_resources ioctl In-Reply-To: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> References: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> Date: Tue, 04 Dec 2018 14:45:28 +0200 Message-ID: <87va49795j.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: stable-owner@vger.kernel.org List-ID: On Thu, 29 Nov 2018, Stanislav Lisovskiy wrote: > Currently kernel might allocate different connector ids > for the same outputs in case of DP MST, which seems to > confuse userspace. There are can be different connector > ids in the list, which could be assigned to the same > output, while being in different states. > This results in issues, like external displays staying > blank after quick unplugging and plugging back(bug #106250). > Returning only active DP connectors fixes the issue. Some nitpicks here... The patch subject lacks a prefix. > > v2: Removed caps from the title > > v3: Refactored initial condition to look more compact. > Thanks to Chris Wilson for giving a hint. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106250 > Signed-off-by: Stanislav Lisovskiy > --- > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..3e2cd959a66a 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -143,22 +143,28 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > drm_connector_list_iter_begin(dev, &conn_iter); > count = 0; > connector_id = u64_to_user_ptr(card_res->connector_id_ptr); > + DRM_DEBUG_KMS("GetResources: writing connectors start"); I think the added debugs here are too verbose. You should probably separate the debugs from the actual change here. Note also that the logging functions already add the function name, so "GetResources:" is redundant. Also "\n" is missing. > drm_for_each_connector_iter(connector, &conn_iter) { > /* only expose writeback connectors if userspace understands them */ > if (!file_priv->writeback_connectors && > (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)) > continue; > > + if (READ_ONCE(connector->registration_state) != DRM_CONNECTOR_REGISTERED) > + continue; > + > if (drm_lease_held(file_priv, connector->base.id)) { > if (count < card_res->count_connectors && > put_user(connector->base.id, connector_id + count)) { > drm_connector_list_iter_end(&conn_iter); > return -EFAULT; > } > + DRM_DEBUG_KMS("GetResources: connector %s", connector->name); Please look around, the connector logging is fairly consistent throughout. BR, Jani. > count++; > } > } > card_res->count_connectors = count; > + DRM_DEBUG_KMS("GetResources: writing connectors end - count %d", count); > drm_connector_list_iter_end(&conn_iter); > > return ret; -- Jani Nikula, Intel Open Source Graphics Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v3] Return only active connectors for get_resources ioctl Date: Tue, 04 Dec 2018 14:45:28 +0200 Message-ID: <87va49795j.fsf@intel.com> References: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> Sender: stable-owner@vger.kernel.org To: dri-devel@lists.freedesktop.org Cc: ville.syrjala@intel.com, intel-gfx@lists.freedesktop.org, martin.peres@intel.com, stable@vger.kernel.org, stanislav.lisovskiy@intel.com List-Id: dri-devel@lists.freedesktop.org On Thu, 29 Nov 2018, Stanislav Lisovskiy wrote: > Currently kernel might allocate different connector ids > for the same outputs in case of DP MST, which seems to > confuse userspace. There are can be different connector > ids in the list, which could be assigned to the same > output, while being in different states. > This results in issues, like external displays staying > blank after quick unplugging and plugging back(bug #106250). > Returning only active DP connectors fixes the issue. Some nitpicks here... The patch subject lacks a prefix. > > v2: Removed caps from the title > > v3: Refactored initial condition to look more compact. > Thanks to Chris Wilson for giving a hint. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106250 > Signed-off-by: Stanislav Lisovskiy > --- > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..3e2cd959a66a 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -143,22 +143,28 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > drm_connector_list_iter_begin(dev, &conn_iter); > count = 0; > connector_id = u64_to_user_ptr(card_res->connector_id_ptr); > + DRM_DEBUG_KMS("GetResources: writing connectors start"); I think the added debugs here are too verbose. You should probably separate the debugs from the actual change here. Note also that the logging functions already add the function name, so "GetResources:" is redundant. Also "\n" is missing. > drm_for_each_connector_iter(connector, &conn_iter) { > /* only expose writeback connectors if userspace understands them */ > if (!file_priv->writeback_connectors && > (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)) > continue; > > + if (READ_ONCE(connector->registration_state) != DRM_CONNECTOR_REGISTERED) > + continue; > + > if (drm_lease_held(file_priv, connector->base.id)) { > if (count < card_res->count_connectors && > put_user(connector->base.id, connector_id + count)) { > drm_connector_list_iter_end(&conn_iter); > return -EFAULT; > } > + DRM_DEBUG_KMS("GetResources: connector %s", connector->name); Please look around, the connector logging is fairly consistent throughout. BR, Jani. > count++; > } > } > card_res->count_connectors = count; > + DRM_DEBUG_KMS("GetResources: writing connectors end - count %d", count); > drm_connector_list_iter_end(&conn_iter); > > return ret; -- Jani Nikula, Intel Open Source Graphics Center