[PATCH v2 0/4] DRM: small improvements

[PATCH v2 0/4] DRM: small improvements

[Luca Ceresoli]

This series brings small improvements to the DRM documentation, logging and
a warning on an incorrect code path.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v2:
- Added patches 3 and 4
- Updated reviewed-by tags
- Link to v1: https://lore.kernel.org/r/20241018-drm-small-improvements-v1-0-cc316e1a98c9@bootlin.com

---
Luca Ceresoli (4):
      drm/drm_mode_object: fix typo in kerneldoc
      drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message
      drm/mode_object: add drm_mode_object_read_refcount()
      drm/connector: warn when cleaning up a refcounted connector

 drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
 drivers/gpu/drm/drm_connector.c     |  6 ++++++
 drivers/gpu/drm/drm_mode_object.c   | 20 ++++++++++++++++++++
 include/drm/drm_mode_object.h       |  3 ++-
 4 files changed, 31 insertions(+), 3 deletions(-)
---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241018-drm-small-improvements-1d104cc10280

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v2 1/4] drm/drm_mode_object: fix typo in kerneldoc

[Luca Ceresoli]

Remove unintended extra word.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 include/drm/drm_mode_object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index 08d7a7f0188fea79e2d8ad5ee6cc5044300f1a26..c68edbd126d04d51221f50aa2b4166475543b59f 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -35,7 +35,7 @@ struct drm_file;
  * @id: userspace visible identifier
  * @type: type of the object, one of DRM_MODE_OBJECT\_\*
  * @properties: properties attached to this object, including values
- * @refcount: reference count for objects which with dynamic lifetime
+ * @refcount: reference count for objects with dynamic lifetime
  * @free_cb: free function callback, only set for objects with dynamic lifetime
  *
  * Base structure for modeset objects visible to userspace. Objects can be

-- 
2.34.1

[PATCH v2 2/4] drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message

[Luca Ceresoli]

This message reports a mismatch between new_crtc_state->enable and
has_connectors, which should be either both true or both false. However it
does not mention which one is true and which is false, which can be useful
for debugging. Add the value of both avriables to the log message.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 43cdf39019a44537794cc5a519d139b0cb77073c..3c3bdef9bcf3c4ffcd861744f6607f317ab0c041 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -666,8 +666,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 
 		if (new_crtc_state->enable != has_connectors) {
-			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch\n",
-				       crtc->base.id, crtc->name);
+			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch (%d/%d)\n",
+				       crtc->base.id, crtc->name,
+				       new_crtc_state->enable, has_connectors);
 
 			return -EINVAL;
 		}

-- 
2.34.1

Re: [PATCH v2 2/4] drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message

[Dmitry Baryshkov]

On Wed, Nov 06, 2024 at 11:48:25AM +0100, Luca Ceresoli wrote:
> This message reports a mismatch between new_crtc_state->enable and
> has_connectors, which should be either both true or both false. However it
> does not mention which one is true and which is false, which can be useful
> for debugging. Add the value of both avriables to the log message.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v2 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[Luca Ceresoli]

Add a wrapper to kref_read() just like the ones already in place for
kref_get() and kref_put(). This will be used for sanity checks on object
lifetime.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_mode_object.c | 20 ++++++++++++++++++++
 include/drm/drm_mode_object.h     |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index df4cc0e8e263d5887a799cf1a61d998234be7158..f990cc7e9b5d3bda3453123593314fa1ea2bf923 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -217,6 +217,26 @@ void drm_mode_object_get(struct drm_mode_object *obj)
 }
 EXPORT_SYMBOL(drm_mode_object_get);
 
+/**
+ * drm_mode_object_get - read the refcount for a mode object
+ * @obj: DRM mode object
+ *
+ * This function returns the current object's refcount if it is a
+ * refcounted modeset object, or 0 on any other object.
+ */
+unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
+{
+	unsigned int refcount = 0;
+
+	if (obj->free_cb) {
+		refcount = kref_read(&obj->refcount);
+		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, refcount);
+	}
+
+	return refcount;
+}
+EXPORT_SYMBOL(drm_mode_object_read_refcount);
+
 /**
  * drm_object_attach_property - attach a property to a modeset object
  * @obj: drm modeset object
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index c68edbd126d04d51221f50aa2b4166475543b59f..3d2c739e703888bf4520c61594d480f128d50e56 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -123,6 +123,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 					     uint32_t id, uint32_t type);
 void drm_mode_object_get(struct drm_mode_object *obj);
 void drm_mode_object_put(struct drm_mode_object *obj);
+unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj);
 
 int drm_object_property_set_value(struct drm_mode_object *obj,
 				  struct drm_property *property,

-- 
2.34.1

Re: [PATCH v2 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[Jani Nikula]

On Wed, 06 Nov 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> Add a wrapper to kref_read() just like the ones already in place for
> kref_get() and kref_put(). This will be used for sanity checks on object
> lifetime.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_mode_object.c | 20 ++++++++++++++++++++
>  include/drm/drm_mode_object.h     |  1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index df4cc0e8e263d5887a799cf1a61d998234be7158..f990cc7e9b5d3bda3453123593314fa1ea2bf923 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -217,6 +217,26 @@ void drm_mode_object_get(struct drm_mode_object *obj)
>  }
>  EXPORT_SYMBOL(drm_mode_object_get);
>  
> +/**
> + * drm_mode_object_get - read the refcount for a mode object
> + * @obj: DRM mode object
> + *
> + * This function returns the current object's refcount if it is a
> + * refcounted modeset object, or 0 on any other object.

Returns: The current object's ...

> + */
> +unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
> +{
> +	unsigned int refcount = 0;
> +
> +	if (obj->free_cb) {
> +		refcount = kref_read(&obj->refcount);
> +		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, refcount);

Please ditch the debug.

> +	}
> +
> +	return refcount;
> +}
> +EXPORT_SYMBOL(drm_mode_object_read_refcount);
> +
>  /**
>   * drm_object_attach_property - attach a property to a modeset object
>   * @obj: drm modeset object
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index c68edbd126d04d51221f50aa2b4166475543b59f..3d2c739e703888bf4520c61594d480f128d50e56 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -123,6 +123,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  					     uint32_t id, uint32_t type);
>  void drm_mode_object_get(struct drm_mode_object *obj);
>  void drm_mode_object_put(struct drm_mode_object *obj);
> +unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj);
>  
>  int drm_object_property_set_value(struct drm_mode_object *obj,
>  				  struct drm_property *property,

-- 
Jani Nikula, Intel

Re: [PATCH v2 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[Luca Ceresoli]

Hello Jani,

On Wed, 06 Nov 2024 14:03:08 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Wed, 06 Nov 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > Add a wrapper to kref_read() just like the ones already in place for
> > kref_get() and kref_put(). This will be used for sanity checks on object
> > lifetime.
> >
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_mode_object.c | 20 ++++++++++++++++++++
> >  include/drm/drm_mode_object.h     |  1 +
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index df4cc0e8e263d5887a799cf1a61d998234be7158..f990cc7e9b5d3bda3453123593314fa1ea2bf923 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -217,6 +217,26 @@ void drm_mode_object_get(struct drm_mode_object *obj)
> >  }
> >  EXPORT_SYMBOL(drm_mode_object_get);
> >  
> > +/**
> > + * drm_mode_object_get - read the refcount for a mode object
> > + * @obj: DRM mode object
> > + *
> > + * This function returns the current object's refcount if it is a
> > + * refcounted modeset object, or 0 on any other object.  
> 
> Returns: The current object's ...
> 
> > + */
> > +unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
> > +{
> > +	unsigned int refcount = 0;
> > +
> > +	if (obj->free_cb) {
> > +		refcount = kref_read(&obj->refcount);
> > +		DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, refcount);  
> 
> Please ditch the debug.

OK, will do both in v3.

Luca
-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[kernel test robot]

Hi Luca,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 42f7652d3eb527d03665b09edac47f85fb600924]

url:    https://github.com/intel-lab-lkp/linux/commits/Luca-Ceresoli/drm-drm_mode_object-fix-typo-in-kerneldoc/20241106-185032
base:   42f7652d3eb527d03665b09edac47f85fb600924
patch link:    https://lore.kernel.org/r/20241106-drm-small-improvements-v2-3-f6e2aef86719%40bootlin.com
patch subject: [PATCH v2 3/4] drm/mode_object: add drm_mode_object_read_refcount()
config: arc-randconfig-002-20241106 (https://download.01.org/0day-ci/archive/20241107/202411070038.tZJzI7NC-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411070038.tZJzI7NC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411070038.tZJzI7NC-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_mode_object.c:228: warning: expecting prototype for drm_mode_object_get(). Prototype was for drm_mode_object_read_refcount() instead


vim +228 drivers/gpu/drm/drm_mode_object.c

   219	
   220	/**
   221	 * drm_mode_object_get - read the refcount for a mode object
   222	 * @obj: DRM mode object
   223	 *
   224	 * This function returns the current object's refcount if it is a
   225	 * refcounted modeset object, or 0 on any other object.
   226	 */
   227	unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
 > 228	{
   229		unsigned int refcount = 0;
   230	
   231		if (obj->free_cb) {
   232			refcount = kref_read(&obj->refcount);
   233			DRM_DEBUG("OBJ ID: %d (%d)\n", obj->id, refcount);
   234		}
   235	
   236		return refcount;
   237	}
   238	EXPORT_SYMBOL(drm_mode_object_read_refcount);
   239	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 4/4] drm/connector: warn when cleaning up a refcounted connector

[Luca Ceresoli]

Calling drm_connector_cleanup() should only be done via the free_cb =>
.destroy path, which cleans up the struct drm_connector only when the
refcount drops to zero.

A cleanup done with a refcount higher than 0 can result from buggy code,
e.g. by doing cleanup directly in the drivers teardown code. Serious
trouble can happen if this happens, so warn about it.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_connector.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fc35f47e2849ed6786d6223ac9c69e1c359fc648..e0bf9c490af43055de4caaee1580a4befbd608c5 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -624,6 +624,12 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode, *t;
 
+	/*
+	 * Cleanup must happen when the last ref is put, via the
+	 * drm_connector_free() callback.
+	 */
+	WARN_ON(drm_mode_object_read_refcount(&connector->base) != 0);
+
 	/* The connector should have been removed from userspace long before
 	 * it is finally destroyed.
 	 */

-- 
2.34.1

Re: [PATCH v2 4/4] drm/connector: warn when cleaning up a refcounted connector

[Dmitry Baryshkov]

On Wed, Nov 06, 2024 at 11:48:27AM +0100, Luca Ceresoli wrote:
> Calling drm_connector_cleanup() should only be done via the free_cb =>
> .destroy path, which cleans up the struct drm_connector only when the
> refcount drops to zero.
> 
> A cleanup done with a refcount higher than 0 can result from buggy code,
> e.g. by doing cleanup directly in the drivers teardown code. Serious
> trouble can happen if this happens, so warn about it.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry