All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ville Syrjala <ville.syrjala@linux.intel.com>
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org, Thomas Zimmermann <tzimmermann@suse.de>
Subject: [PATCH v3 1/7] drm: Include the encoder itself in possible_clones
Date: Tue, 11 Feb 2020 18:22:02 +0200	[thread overview]
Message-ID: <20200211162208.16224-2-ville.syrjala@linux.intel.com> (raw)
In-Reply-To: <20200211162208.16224-1-ville.syrjala@linux.intel.com>

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs say possible_clones should always include the encoder itself.
Since most drivers don't want to deal with the complexities of cloning
let's allow them to set possible_clones=0 and instead we'll fix that
up in the core.

We can't put this special case into drm_encoder_init() because drivers
will have to fill up possible_clones after adding all the relevant
encoders. Otherwise they wouldn't know the proper encoder indexes to
use. So we'll just do it just before registering the device.

v2: Don't set the bit if possible_clones!=0 so that the
    validation (coming soon) will WARN (Thomas)
    Fix up the docs to allow possible_clones==0 (Daniel)
    .late_register() is too late, introduce drm_mode_config_validate()
    which gets called _before_ we register the char device (Daniel)

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_drv.c           |  3 +++
 drivers/gpu/drm/drm_mode_config.c   | 19 +++++++++++++++++++
 include/drm/drm_encoder.h           |  4 +++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 16f2413403aa..ace363b4f82b 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -82,6 +82,7 @@ int drm_mode_setcrtc(struct drm_device *dev,
 /* drm_mode_config.c */
 int drm_modeset_register_all(struct drm_device *dev);
 void drm_modeset_unregister_all(struct drm_device *dev);
+void drm_mode_config_validate(struct drm_device *dev);
 
 /* drm_modes.c */
 const char *drm_get_mode_status_name(enum drm_mode_status status);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7b1a628d1f6e..65a0acb79323 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -946,6 +946,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	struct drm_driver *driver = dev->driver;
 	int ret;
 
+	if (!driver->load)
+		drm_mode_config_validate(dev);
+
 	if (drm_dev_needs_global_mutex(dev))
 		mutex_lock(&drm_global_mutex);
 
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 08e6eff6a179..75e357c7e84d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -532,3 +532,22 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+/*
+ * For some reason we want the encoder itself included in
+ * possible_clones. Make life easy for drivers by allowing them
+ * to leave possible_clones unset if no cloning is possible.
+ */
+static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
+{
+	if (encoder->possible_clones == 0)
+		encoder->possible_clones = drm_encoder_mask(encoder);
+}
+
+void drm_mode_config_validate(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, dev)
+		fixup_encoder_possible_clones(encoder);
+}
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5623994b6e9e..22d6cdf729f1 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -159,7 +159,9 @@ struct drm_encoder {
 	 * encoders can be used in a cloned configuration, they both should have
 	 * each another bits set.
 	 *
-	 * In reality almost every driver gets this wrong.
+	 * As an exception to the above rule if the driver doesn't implement
+	 * any cloning it can leave @possible_clones set to 0. The core will
+	 * automagically fix this up by setting the bit for the encoder itself.
 	 *
 	 * Note that since encoder objects can't be hotplugged the assigned indices
 	 * are stable and hence known before registering all objects.
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: Ville Syrjala <ville.syrjala@linux.intel.com>
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org, Thomas Zimmermann <tzimmermann@suse.de>
Subject: [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones
Date: Tue, 11 Feb 2020 18:22:02 +0200	[thread overview]
Message-ID: <20200211162208.16224-2-ville.syrjala@linux.intel.com> (raw)
In-Reply-To: <20200211162208.16224-1-ville.syrjala@linux.intel.com>

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs say possible_clones should always include the encoder itself.
Since most drivers don't want to deal with the complexities of cloning
let's allow them to set possible_clones=0 and instead we'll fix that
up in the core.

We can't put this special case into drm_encoder_init() because drivers
will have to fill up possible_clones after adding all the relevant
encoders. Otherwise they wouldn't know the proper encoder indexes to
use. So we'll just do it just before registering the device.

v2: Don't set the bit if possible_clones!=0 so that the
    validation (coming soon) will WARN (Thomas)
    Fix up the docs to allow possible_clones==0 (Daniel)
    .late_register() is too late, introduce drm_mode_config_validate()
    which gets called _before_ we register the char device (Daniel)

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_drv.c           |  3 +++
 drivers/gpu/drm/drm_mode_config.c   | 19 +++++++++++++++++++
 include/drm/drm_encoder.h           |  4 +++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 16f2413403aa..ace363b4f82b 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -82,6 +82,7 @@ int drm_mode_setcrtc(struct drm_device *dev,
 /* drm_mode_config.c */
 int drm_modeset_register_all(struct drm_device *dev);
 void drm_modeset_unregister_all(struct drm_device *dev);
+void drm_mode_config_validate(struct drm_device *dev);
 
 /* drm_modes.c */
 const char *drm_get_mode_status_name(enum drm_mode_status status);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7b1a628d1f6e..65a0acb79323 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -946,6 +946,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	struct drm_driver *driver = dev->driver;
 	int ret;
 
+	if (!driver->load)
+		drm_mode_config_validate(dev);
+
 	if (drm_dev_needs_global_mutex(dev))
 		mutex_lock(&drm_global_mutex);
 
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 08e6eff6a179..75e357c7e84d 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -532,3 +532,22 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	drm_modeset_lock_fini(&dev->mode_config.connection_mutex);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+/*
+ * For some reason we want the encoder itself included in
+ * possible_clones. Make life easy for drivers by allowing them
+ * to leave possible_clones unset if no cloning is possible.
+ */
+static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
+{
+	if (encoder->possible_clones == 0)
+		encoder->possible_clones = drm_encoder_mask(encoder);
+}
+
+void drm_mode_config_validate(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, dev)
+		fixup_encoder_possible_clones(encoder);
+}
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5623994b6e9e..22d6cdf729f1 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -159,7 +159,9 @@ struct drm_encoder {
 	 * encoders can be used in a cloned configuration, they both should have
 	 * each another bits set.
 	 *
-	 * In reality almost every driver gets this wrong.
+	 * As an exception to the above rule if the driver doesn't implement
+	 * any cloning it can leave @possible_clones set to 0. The core will
+	 * automagically fix this up by setting the bit for the encoder itself.
 	 *
 	 * Note that since encoder objects can't be hotplugged the assigned indices
 	 * are stable and hence known before registering all objects.
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2020-02-11 16:22 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 16:22 [PATCH v3 0/7] drm: Try to fix encoder possible_clones/crtc Ville Syrjala
2020-02-11 16:22 ` [Intel-gfx] " Ville Syrjala
2020-02-11 16:22 ` Ville Syrjala [this message]
2020-02-11 16:22   ` [Intel-gfx] [PATCH v3 1/7] drm: Include the encoder itself in possible_clones Ville Syrjala
2020-02-11 16:58   ` Daniel Vetter
2020-02-11 16:58     ` [Intel-gfx] " Daniel Vetter
2020-02-11 16:22 ` [PATCH v3 2/7] drm/gma500: Sanitize possible_clones Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-11 16:22 ` [PATCH v3 3/7] drm/exynos: Use drm_encoder_mask() Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-17  2:27   ` Inki Dae
2020-02-17  2:27     ` [Intel-gfx] " Inki Dae
2020-02-25  0:32     ` Inki Dae
2020-02-11 16:22 ` [PATCH v3 4/7] drm/imx: Remove the bogus possible_clones setup Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-11 16:22 ` [PATCH v3 5/7] drm: Validate encoder->possible_clones Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-11 17:02   ` Daniel Vetter
2020-02-11 17:02     ` [Intel-gfx] " Daniel Vetter
2020-02-11 17:13     ` Ville Syrjälä
2020-02-11 17:13       ` [Intel-gfx] " Ville Syrjälä
2020-02-12  8:56       ` Daniel Vetter
2020-02-12  8:56         ` [Intel-gfx] " Daniel Vetter
2020-02-11 16:22 ` [PATCH v3 6/7] drm: Validate encoder->possible_crtcs Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-11 17:04   ` Daniel Vetter
2020-02-11 17:04     ` [Intel-gfx] " Daniel Vetter
2020-09-06 11:19     ` Jan Kiszka
2020-09-06 11:19       ` [Intel-gfx] " Jan Kiszka
2020-09-07  7:14       ` Daniel Vetter
2020-09-07  7:14         ` Daniel Vetter
2020-09-07  7:14         ` [Intel-gfx] " Daniel Vetter
2020-09-10 18:18         ` Deucher, Alexander
2020-09-10 18:18           ` Deucher, Alexander
2020-09-10 18:18           ` [Intel-gfx] " Deucher, Alexander
2020-09-29  9:36           ` Jan Kiszka
2020-09-29  9:36             ` Jan Kiszka
2020-09-29  9:36             ` [Intel-gfx] " Jan Kiszka
2020-09-29 20:04             ` Alex Deucher
2020-09-29 20:04               ` Alex Deucher
2020-09-29 20:04               ` [Intel-gfx] " Alex Deucher
2020-12-03 21:30               ` Alex Deucher
2020-12-03 21:30                 ` Alex Deucher
2020-12-03 21:30                 ` [Intel-gfx] " Alex Deucher
2020-12-09 13:17                 ` Daniel Vetter
2020-12-09 13:17                   ` Daniel Vetter
2020-12-09 13:17                   ` [Intel-gfx] " Daniel Vetter
2020-12-14 20:26                 ` Jan Kiszka
2020-12-14 20:26                   ` Jan Kiszka
2020-12-14 20:26                   ` [Intel-gfx] " Jan Kiszka
2020-02-11 16:22 ` [PATCH v3 7/7] drm: Allow drivers to leave encoder->possible_crtcs==0 Ville Syrjala
2020-02-11 16:22   ` [Intel-gfx] " Ville Syrjala
2020-02-11 17:05   ` Daniel Vetter
2020-02-11 17:05     ` [Intel-gfx] " Daniel Vetter
2020-02-11 17:14     ` Ville Syrjälä
2020-02-11 17:14       ` [Intel-gfx] " Ville Syrjälä
2020-02-12  9:07       ` Daniel Vetter
2020-02-12  9:07         ` [Intel-gfx] " Daniel Vetter
2020-02-12  9:08         ` Daniel Vetter
2020-02-12  9:08           ` [Intel-gfx] " Daniel Vetter
2020-03-18 16:44           ` Ville Syrjälä
2020-03-18 16:44             ` [Intel-gfx] " Ville Syrjälä
2020-12-03 22:16 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm: Try to fix encoder possible_clones/crtc (rev4) Patchwork

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=20200211162208.16224-2-ville.syrjala@linux.intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=tzimmermann@suse.de \
    /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: link
Be 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.