[v3,16/16] drm: Replace mode->export_head with a boolean

Commit Message

Ville Syrjälä April 28, 2020, 5:19 p.m. UTC

From: Ville Syrjälä <[e-mail]>

In order to shrink drm_display_mode below the magic two cacheline
mark in 64bit we need to shrink it by another 8 bytes. The easiest
thing to eliminate is the 'export_head' list head which is only
used during the getconnector ioctl to temporarly track which modes
on the connector's mode list are to be exposed and which are to
remain hidden.

We can simply replace the list head with a boolean which we use
to tag the modes that are to be exposed. If we make sure to clear
the tags after we're done with them we don't even need an extra
loop over the modes to reset the tags at the start of the
getconnector ioctl.

Conveniently we already have a hole for the boolean left
behind by the removal of mode->private_flags. The final size
of the struct is now 112 bytes on 32bit and 120 bytes on 64bit.

The downside is that drm_mode_expose_to_userspace() gets to
iterate a few more modes. It already was O(n^2), now it's
a slightly worse O(n^2).

Another alternative would be a temp bitmask so we wouldn't have
to have anything in the mode struct itself. The main issue is
how large of a bitmask do we need? I guess we could allocate
it dynamically but that means an extra kcalloc() and an extra
loop through the modes to count them first (or grow the bitmask
with krealloc() as needed).

CC: Sam Ravnborg <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Emil Velikov <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/drm_connector.c | 45 +++++++++++++++++++++++----------
 include/drm/drm_modes.h         | 24 ++++++++----------
 2 files changed, 43 insertions(+), 26 deletions(-)

Comments

Emil Velikov April 30, 2020, 1:50 p.m. UTC | #1

Hi Ville

I don't fully grok the i915 changes to provide meaningful review.
There are couple of small comments below, but regardless of those

Patches 01-11 and 14-16 are:
Reviewed-by: Emil Velikov <[e-mail]>

On Tue, 28 Apr 2020 at 18:20, Ville Syrjala
<[e-mail]> wrote:

> The downside is that drm_mode_expose_to_userspace() gets to
> iterate a few more modes. It already was O(n^2), now it's
> a slightly worse O(n^2).
>
Personally I'd drop the O() sentence, or change it to
It already was O(n^2), now it's slightly worse O((n+y)^2).

> Another alternative would be a temp bitmask so we wouldn't have
> to have anything in the mode struct itself. The main issue is
> how large of a bitmask do we need? I guess we could allocate
> it dynamically but that means an extra kcalloc() and an extra
> loop through the modes to count them first (or grow the bitmask
> with krealloc() as needed).
>
If the walk is even remotely close to being an issue, we could
consider the bitmask.
I don't think that's the case yet.


Hmm the original code never discards any entries from export_head.
I wonder if there's some corner case where we could end with an "old"
mode showing in the list?

For example:
- creates a user mode via drmModeCreatePropertyBlob()
- calls drmModeGetConnector() and sees their mode
- optional (?) does a modeset to and away from said mode
- removes their blob drmModeDestroyPropertyBlob()
- calls drmModeGetConnector() and still sees their removed mode.

If this is a bug (?) that we care about, we might want to add an igt
test for it.
Conversely, if this is a behaviour we want to keep this patch needs some work.

HTH

Emil

Ville Syrjälä Sept. 1, 2020, 10:58 a.m. UTC | #2

On Thu, Apr 30, 2020 at 02:50:52PM +0100, Emil Velikov wrote:
> Hi Ville
> 
> I don't fully grok the i915 changes to provide meaningful review.
> There are couple of small comments below, but regardless of those

Sorry, forgot to reply to this in a timely manner.

> 
> Patches 01-11 and 14-16 are:
> Reviewed-by: Emil Velikov <[e-mail]>
> 
> On Tue, 28 Apr 2020 at 18:20, Ville Syrjala
> <[e-mail]> wrote:
> 
> > The downside is that drm_mode_expose_to_userspace() gets to
> > iterate a few more modes. It already was O(n^2), now it's
> > a slightly worse O(n^2).
> >
> Personally I'd drop the O() sentence, or change it to
> It already was O(n^2), now it's slightly worse O((n+y)^2).

Dropped.

> 
> > Another alternative would be a temp bitmask so we wouldn't have
> > to have anything in the mode struct itself. The main issue is
> > how large of a bitmask do we need? I guess we could allocate
> > it dynamically but that means an extra kcalloc() and an extra
> > loop through the modes to count them first (or grow the bitmask
> > with krealloc() as needed).
> >
> If the walk is even remotely close to being an issue, we could
> consider the bitmask.
> I don't think that's the case yet.
> 
> 
> Hmm the original code never discards any entries from export_head.
> I wonder if there's some corner case where we could end with an "old"
> mode showing in the list?

No. export_list starts out empty so only the modes we explicitly add
to the list can be reached. Thus any dangling pointers in some other
mode's export_head are of no concern.

Pushed the last few patches to drm-misc-next. Thanks for the reviews
everyone.

> 
> For example:
> - creates a user mode via drmModeCreatePropertyBlob()
> - calls drmModeGetConnector() and sees their mode
> - optional (?) does a modeset to and away from said mode
> - removes their blob drmModeDestroyPropertyBlob()
> - calls drmModeGetConnector() and still sees their removed mode.
> 
> If this is a bug (?) that we care about, we might want to add an igt
> test for it.
> Conversely, if this is a behaviour we want to keep this patch needs some work.
> 
> HTH
> 
> Emil

Patch

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b1099e1251a2..7e719b08564d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2198,7 +2198,7 @@  static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
 
 static bool
 drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
-			     const struct list_head *export_list,
+			     const struct list_head *modes,
 			     const struct drm_file *file_priv)
 {
 	/*
@@ -2214,15 +2214,17 @@  drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
 	 * while preparing the list of user-modes.
 	 */
 	if (!file_priv->aspect_ratio_allowed) {
-		struct drm_display_mode *mode_itr;
+		const struct drm_display_mode *mode_itr;
 
-		list_for_each_entry(mode_itr, export_list, export_head)
-			if (drm_mode_match(mode_itr, mode,
+		list_for_each_entry(mode_itr, modes, head) {
+			if (mode_itr->expose_to_userspace &&
+			    drm_mode_match(mode_itr, mode,
 					   DRM_MODE_MATCH_TIMINGS |
 					   DRM_MODE_MATCH_CLOCK |
 					   DRM_MODE_MATCH_FLAGS |
 					   DRM_MODE_MATCH_3D_FLAGS))
 				return false;
+		}
 	}
 
 	return true;
@@ -2242,7 +2244,6 @@  int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_mode_modeinfo u_mode;
 	struct drm_mode_modeinfo __user *mode_ptr;
 	uint32_t __user *encoder_ptr;
-	LIST_HEAD(export_list);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
@@ -2286,25 +2287,30 @@  int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->connection = connector->status;
 
 	/* delayed so we get modes regardless of pre-fill_modes state */
-	list_for_each_entry(mode, &connector->modes, head)
-		if (drm_mode_expose_to_userspace(mode, &export_list,
+	list_for_each_entry(mode, &connector->modes, head) {
+		WARN_ON(mode->expose_to_userspace);
+
+		if (drm_mode_expose_to_userspace(mode, &connector->modes,
 						 file_priv)) {
-			list_add_tail(&mode->export_head, &export_list);
+			mode->expose_to_userspace = true;
 			mode_count++;
 		}
+	}
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
 	 * needed, and the 2nd time to fill it.
-	 * The modes that need to be exposed to the user are maintained in the
-	 * 'export_list'. When the ioctl is called first time to determine the,
-	 * space, the export_list gets filled, to find the no.of modes. In the
-	 * 2nd time, the user modes are filled, one by one from the export_list.
 	 */
 	if ((out_resp->count_modes >= mode_count) && mode_count) {
 		copied = 0;
 		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
-		list_for_each_entry(mode, &export_list, export_head) {
+		list_for_each_entry(mode, &connector->modes, head) {
+			if (!mode->expose_to_userspace)
+				continue;
+
+			/* Clear the tag for the next time around */
+			mode->expose_to_userspace = false;
+
 			drm_mode_convert_to_umode(&u_mode, mode);
 			/*
 			 * Reset aspect ratio flags of user-mode, if modes with
@@ -2315,13 +2321,26 @@  int drm_mode_getconnector(struct drm_device *dev, void *data,
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
 				ret = -EFAULT;
+
+				/*
+				 * Clear the tag for the rest of
+				 * the modes for the next time around.
+				 */
+				list_for_each_entry_continue(mode, &connector->modes, head)
+					mode->expose_to_userspace = false;
+
 				mutex_unlock(&dev->mode_config.mutex);
 
 				goto out;
 			}
 			copied++;
 		}
+	} else {
+		/* Clear the tag for the next time around */
+		list_for_each_entry(mode, &connector->modes, head)
+			mode->expose_to_userspace = false;
 	}
+
 	out_resp->count_modes = mode_count;
 	mutex_unlock(&dev->mode_config.mutex);
 
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 1e97138a9b8c..ac0589aab23e 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -348,6 +348,17 @@  struct drm_display_mode {
 	 */
 	u8 type;
 
+	/**
+	 * @expose_to_userspace:
+	 *
+	 * Indicates whether the mode is to be exposed to the userspace.
+	 * This is to maintain a set of exposed modes while preparing
+	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of
+	 * this only lies in the ioctl function, and is not to be used
+	 * outside the function.
+	 */
+	bool expose_to_userspace;
+
 	/**
 	 * @head:
 	 *
@@ -355,19 +366,6 @@  struct drm_display_mode {
 	 */
 	struct list_head head;
 
-	/**
-	 * @export_head:
-	 *
-	 * struct list_head for modes to be exposed to the userspace.
-	 * This is to maintain a list of exposed modes while preparing
-	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
-	 * list_head only lies in the ioctl function, and is not expected to be
-	 * used outside the function.
-	 * Once used, the stale pointers are not reset, but left as it is, to
-	 * avoid overhead of protecting it by mode_config.mutex.
-	 */
-	struct list_head export_head;
-
 	/**
 	 * @name:
 	 *