From: Jani Nikula <jani.nikula@linux.intel.com> To: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>, 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 Date: Tue, 04 Dec 2018 14:45:28 +0200 [thread overview] Message-ID: <87va49795j.fsf@intel.com> (raw) In-Reply-To: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> On Thu, 29 Nov 2018, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> 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 <stanislav.lisovskiy@intel.com> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Jani Nikula <jani.nikula@linux.intel.com> 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 Subject: Re: [PATCH v3] Return only active connectors for get_resources ioctl Date: Tue, 04 Dec 2018 14:45:28 +0200 [thread overview] Message-ID: <87va49795j.fsf@intel.com> (raw) In-Reply-To: <20181129110921.5832-1-stanislav.lisovskiy@intel.com> On Thu, 29 Nov 2018, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> 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 <stanislav.lisovskiy@intel.com> > --- > 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
next prev parent reply other threads:[~2018-12-04 12:45 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-29 11:09 [PATCH v3] Return only active connectors for get_resources ioctl Stanislav Lisovskiy 2018-11-29 11:21 ` Greg KH 2018-11-29 11:30 ` ✓ Fi.CI.BAT: success for Return only active connectors for get_resources ioctl (rev4) Patchwork 2018-11-29 19:06 ` [PATCH v3] Return only active connectors for get_resources ioctl Lyude Paul 2018-11-29 19:06 ` Lyude Paul 2018-11-29 21:00 ` ✓ Fi.CI.IGT: success for Return only active connectors for get_resources ioctl (rev4) Patchwork 2018-12-04 12:45 ` Jani Nikula [this message] 2018-12-04 12:45 ` [PATCH v3] Return only active connectors for get_resources ioctl Jani Nikula
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87va49795j.fsf@intel.com \ --to=jani.nikula@linux.intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=martin.peres@intel.com \ --cc=stable@vger.kernel.org \ --cc=stanislav.lisovskiy@intel.com \ --cc=ville.syrjala@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.