diff mbox series

[v7,4/7] drm: writeback: Create an helper for drm_writeback_connector initialization

Message ID 20250113-google-vkms-managed-v7-4-4f81d1893e0b@bootlin.com (mailing list archive)
State New
Headers show
Series drm/vkms: Switch all vkms object to DRM managed objects | expand

Commit Message

Louis Chauvet Jan. 13, 2025, 5:09 p.m. UTC
As the old drm and the new drmm variants of drm_writeback_connector
requires almost the same initialization, create an internal helper to do
most of the initialization work.

Currently there is no cleanup function for writeback connectors. To allows
implementation of drmm variant of writeback connector, create a cleanup
function that can be used to properly remove all the writeback-specific
properties and allocations.

This also introduce an helper to cleanup only the drm_writeback_connector
properties, so it can be used during initialization to cleanup in case of
failure.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/drm_writeback.c | 130 ++++++++++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 26 deletions(-)

Comments

kernel test robot Jan. 15, 2025, 1:22 a.m. UTC | #1
Hi Louis,

kernel test robot noticed the following build warnings:

[auto build test WARNING on f8a2397baf041a5cee408b082334bb09c7e161df]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/drm-vkms-Switch-to-managed-for-connector/20250114-011112
base:   f8a2397baf041a5cee408b082334bb09c7e161df
patch link:    https://lore.kernel.org/r/20250113-google-vkms-managed-v7-4-4f81d1893e0b%40bootlin.com
patch subject: [PATCH v7 4/7] drm: writeback: Create an helper for drm_writeback_connector initialization
config: i386-randconfig-014-20250115 (https://download.01.org/0day-ci/archive/20250115/202501150938.a3MspUwj-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250115/202501150938.a3MspUwj-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/202501150938.a3MspUwj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_writeback.c: In function 'drm_writeback_connector_init_with_encoder':
>> drivers/gpu/drm/drm_writeback.c:321:35: warning: unused variable 'blob' [-Wunused-variable]
     321 |         struct drm_property_blob *blob;
         |                                   ^~~~
   drivers/gpu/drm/drm_writeback.c: At top level:
   drivers/gpu/drm/drm_writeback.c:348:13: warning: 'drm_writeback_connector_cleanup' defined but not used [-Wunused-function]
     348 | static void drm_writeback_connector_cleanup(struct drm_device *dev,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
>> drivers/gpu/drm/drm_writeback.c:243: warning: expecting prototype for drm_writeback_connector_init_with_encoder(). Prototype was for __drm_writeback_connector_init() instead


vim +/blob +321 drivers/gpu/drm/drm_writeback.c

   214	
   215	/**
   216	 * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
   217	 * a custom encoder
   218	 *
   219	 * @dev: DRM device
   220	 * @wb_connector: Writeback connector to initialize
   221	 * @enc: handle to the already initialized drm encoder
   222	 * @formats: Array of supported pixel formats for the writeback engine
   223	 * @n_formats: Length of the formats array
   224	 *
   225	 * This function creates the writeback-connector-specific properties if they
   226	 * have not been already created, initializes the connector as
   227	 * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
   228	 * values.
   229	 *
   230	 * This function assumes that the drm_writeback_connector's encoder has already been
   231	 * created and initialized before invoking this function.
   232	 *
   233	 * In addition, this function also assumes that callers of this API will manage
   234	 * assigning the encoder helper functions, possible_crtcs and any other encoder
   235	 * specific operation.
   236	 *
   237	 * Returns: 0 on success, or a negative error code
   238	 */
   239	static int __drm_writeback_connector_init(struct drm_device *dev,
   240						  struct drm_writeback_connector *wb_connector,
   241						  struct drm_encoder *enc, const u32 *formats,
   242						  int n_formats)
 > 243	{
   244		struct drm_connector *connector = &wb_connector->base;
   245		struct drm_mode_config *config = &dev->mode_config;
   246		struct drm_property_blob *blob;
   247		int ret = create_writeback_properties(dev);
   248	
   249		if (ret != 0)
   250			return ret;
   251	
   252		connector->interlace_allowed = 0;
   253	
   254		ret = drm_connector_attach_encoder(connector, enc);
   255		if (ret)
   256			return ret;
   257	
   258		blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
   259						formats);
   260		if (IS_ERR(blob))
   261			return PTR_ERR(blob);
   262	
   263		INIT_LIST_HEAD(&wb_connector->job_queue);
   264		spin_lock_init(&wb_connector->job_lock);
   265	
   266		wb_connector->fence_context = dma_fence_context_alloc(1);
   267		spin_lock_init(&wb_connector->fence_lock);
   268		snprintf(wb_connector->timeline_name,
   269			 sizeof(wb_connector->timeline_name),
   270			 "CONNECTOR:%d-%s", connector->base.id, connector->name);
   271	
   272		drm_object_attach_property(&connector->base,
   273					   config->writeback_out_fence_ptr_property, 0);
   274	
   275		drm_object_attach_property(&connector->base,
   276					   config->writeback_fb_id_property, 0);
   277	
   278		drm_object_attach_property(&connector->base,
   279					   config->writeback_pixel_formats_property,
   280					   blob->base.id);
   281		wb_connector->pixel_formats_blob_ptr = blob;
   282	
   283		return 0;
   284	}
   285	
   286	/**
   287	 * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
   288	 * a custom encoder
   289	 *
   290	 * @dev: DRM device
   291	 * @wb_connector: Writeback connector to initialize
   292	 * @enc: handle to the already initialized drm encoder
   293	 * @con_funcs: Connector funcs vtable
   294	 * @formats: Array of supported pixel formats for the writeback engine
   295	 * @n_formats: Length of the formats array
   296	 *
   297	 * This function creates the writeback-connector-specific properties if they
   298	 * have not been already created, initializes the connector as
   299	 * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
   300	 * values.
   301	 *
   302	 * This function assumes that the drm_writeback_connector's encoder has already been
   303	 * created and initialized before invoking this function.
   304	 *
   305	 * In addition, this function also assumes that callers of this API will manage
   306	 * assigning the encoder helper functions, possible_crtcs and any other encoder
   307	 * specific operation.
   308	 *
   309	 * Drivers should always use this function instead of drm_connector_init() to
   310	 * set up writeback connectors if they want to manage themselves the lifetime of the
   311	 * associated encoder.
   312	 *
   313	 * Returns: 0 on success, or a negative error code
   314	 */
   315	int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
   316						      struct drm_writeback_connector *wb_connector,
   317						      struct drm_encoder *enc,
   318						      const struct drm_connector_funcs *con_funcs,
   319						      const u32 *formats, int n_formats)
   320	{
 > 321		struct drm_property_blob *blob;
   322		struct drm_connector *connector = &wb_connector->base;
   323		int ret;
   324	
   325		ret = drm_connector_init(dev, connector, con_funcs,
   326					 DRM_MODE_CONNECTOR_WRITEBACK);
   327		if (ret)
   328			return ret;
   329	
   330		ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
   331						     n_formats);
   332		if (ret)
   333			drm_connector_cleanup(connector);
   334	
   335		return ret;
   336	}
   337	EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
   338
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 33a3c98a962d1ec49ac4b353902036cf74290ae6..494400b09796d37ed89145da45d5f1e029632de5 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -15,6 +15,7 @@ 
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_property.h>
 #include <drm/drm_writeback.h>
@@ -140,6 +141,22 @@  static int create_writeback_properties(struct drm_device *dev)
 	return 0;
 }
 
+static void delete_writeback_properties(struct drm_device *dev)
+{
+	if (dev->mode_config.writeback_pixel_formats_property) {
+		drm_property_destroy(dev, dev->mode_config.writeback_pixel_formats_property);
+		dev->mode_config.writeback_pixel_formats_property = NULL;
+	}
+	if (dev->mode_config.writeback_out_fence_ptr_property) {
+		drm_property_destroy(dev, dev->mode_config.writeback_out_fence_ptr_property);
+		dev->mode_config.writeback_out_fence_ptr_property = NULL;
+	}
+	if (dev->mode_config.writeback_fb_id_property) {
+		drm_property_destroy(dev, dev->mode_config.writeback_fb_id_property);
+		dev->mode_config.writeback_fb_id_property = NULL;
+	}
+}
+
 static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
@@ -202,7 +219,6 @@  EXPORT_SYMBOL(drm_writeback_connector_init);
  * @dev: DRM device
  * @wb_connector: Writeback connector to initialize
  * @enc: handle to the already initialized drm encoder
- * @con_funcs: Connector funcs vtable
  * @formats: Array of supported pixel formats for the writeback engine
  * @n_formats: Length of the formats array
  *
@@ -218,41 +234,31 @@  EXPORT_SYMBOL(drm_writeback_connector_init);
  * assigning the encoder helper functions, possible_crtcs and any other encoder
  * specific operation.
  *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors if they want to manage themselves the lifetime of the
- * associated encoder.
- *
  * Returns: 0 on success, or a negative error code
  */
-int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
-		struct drm_writeback_connector *wb_connector, struct drm_encoder *enc,
-		const struct drm_connector_funcs *con_funcs, const u32 *formats,
-		int n_formats)
+static int __drm_writeback_connector_init(struct drm_device *dev,
+					  struct drm_writeback_connector *wb_connector,
+					  struct drm_encoder *enc, const u32 *formats,
+					  int n_formats)
 {
-	struct drm_property_blob *blob;
 	struct drm_connector *connector = &wb_connector->base;
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_property_blob *blob;
 	int ret = create_writeback_properties(dev);
 
 	if (ret != 0)
 		return ret;
 
-	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
-					formats);
-	if (IS_ERR(blob))
-		return PTR_ERR(blob);
-
-
 	connector->interlace_allowed = 0;
 
-	ret = drm_connector_init(dev, connector, con_funcs,
-				 DRM_MODE_CONNECTOR_WRITEBACK);
-	if (ret)
-		goto connector_fail;
-
 	ret = drm_connector_attach_encoder(connector, enc);
 	if (ret)
-		goto attach_fail;
+		return ret;
+
+	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
+					formats);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
 
 	INIT_LIST_HEAD(&wb_connector->job_queue);
 	spin_lock_init(&wb_connector->job_lock);
@@ -275,15 +281,87 @@  int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
 	wb_connector->pixel_formats_blob_ptr = blob;
 
 	return 0;
+}
+
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
+ * a custom encoder
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @enc: handle to the already initialized drm encoder
+ * @con_funcs: Connector funcs vtable
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * This function assumes that the drm_writeback_connector's encoder has already been
+ * created and initialized before invoking this function.
+ *
+ * In addition, this function also assumes that callers of this API will manage
+ * assigning the encoder helper functions, possible_crtcs and any other encoder
+ * specific operation.
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors if they want to manage themselves the lifetime of the
+ * associated encoder.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
+					      struct drm_writeback_connector *wb_connector,
+					      struct drm_encoder *enc,
+					      const struct drm_connector_funcs *con_funcs,
+					      const u32 *formats, int n_formats)
+{
+	struct drm_property_blob *blob;
+	struct drm_connector *connector = &wb_connector->base;
+	int ret;
+
+	ret = drm_connector_init(dev, connector, con_funcs,
+				 DRM_MODE_CONNECTOR_WRITEBACK);
+	if (ret)
+		return ret;
+
+	ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
+					     n_formats);
+	if (ret)
+		drm_connector_cleanup(connector);
 
-attach_fail:
-	drm_connector_cleanup(connector);
-connector_fail:
-	drm_property_blob_put(blob);
 	return ret;
 }
 EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
 
+/**
+ * drm_writeback_connector_cleanup - Cleanup the writeback connector
+ * @dev: DRM device
+ * @wb_connector: Pointer to the writeback connector to clean up
+ *
+ * This will decrement the reference counter of blobs and destroy properties. It
+ * will also clean the remaining jobs in this writeback connector. Caution: This helper will not
+ * clean up the attached encoder and the drm_connector.
+ */
+static void drm_writeback_connector_cleanup(struct drm_device *dev,
+					    struct drm_writeback_connector *wb_connector)
+{
+	unsigned long flags;
+	struct drm_writeback_job *pos, *n;
+
+	delete_writeback_properties(dev);
+	drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
+
+	spin_lock_irqsave(&wb_connector->job_lock, flags);
+	list_for_each_entry_safe(pos, n, &wb_connector->job_queue, list_entry) {
+		drm_writeback_cleanup_job(pos);
+		list_del(&pos->list_entry);
+	}
+	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
+}
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb)
 {