[PATCH 00/28] Enable Pipe writeback

[PATCH 00/28] Enable Pipe writeback

[Suraj Kandpal]

This series aims to enable pipe writeback functionality on
ADLP where it has been tested. The plan is to slowly accomodate
all supported hardware after this functionality is tested on them.
This series also brings change to drm core but not in a drastic way.
We introduce a helper which lets drivers have their own preallocated
conenctor keeping the connector in drm_writeback_conenctor blank.
This lets driver have more control over their connector but still use
the drm core queues for job creation and signalling. Some new helpers
have been added to aid drivers so that derivation of drm_connector
from drm_writeback_connector and vice versa becomes easy for drivers
that will use this helper since it won't be as straight forward as
wb_conn->connector anymore. Driver not using these API will not be
affected in anyways.
This series enables the triggered captured mode where we need to
trigger a capture.

Cc: Harry Wetland <harry.wentland@amd.com>
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

Suraj Kandpal (28):
  drm/writeback: Add function that takes preallocated connector
  drm/writeback: Add a helper function to get writeback connector
  drm/writeback: Define function to get drm_connector from writeback
  drm/i915/writeback: Add writeback registers
  drm/i915/writeback: Add some preliminary writeback definitions
  drm/i915/writeback: Init writeback connector
  drm/i915/writeback: Add function for get_writeback_connector
  drm/i915/writeback: Define the get_connector_from_writeback hook
  drm/i915/writeback: Add function to get modes
  drm/i915/writeback: Add hook to check modes
  drm/i915/writeback: Define encoder->get_hw_state
  drm/i915/writeback: Fill encoder->get_config
  drm/i915/writeback: Add private structure for writeback job
  drm/i915/writeback: Define function for prepare and cleanup hooks
  drm/i915/writeback: Define compute_config for writeback
  drm/i915/writeback: Define function for connector function detect
  drm/i915/writeback: Define function to destroy writeback connector
  drm/i915/writeback: Add connector atomic check
  drm/i915/writeback: Add the enable sequence from writeback
  drm/i915/writeback: Add writeback to xe Makefile
  drm/i915/writeback: Define writeback frame capture function
  drm/i915/writeback: Configure WD_STRIDE reg
  drm/i915/writeback: Configure WD_SURF register
  drm/i915/writeback: Enable writeback interrupts
  drm/i915/writeback: Initialize writeback encoder.
  drm/i915/writeback: Define the disable sequence for writeback
  drm/i915/writeback: Make exception for writeback connector
  drm/i915/writeback: Modify state verify function

 drivers/gpu/drm/drm_writeback.c               | 123 +++-
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_acpi.c     |   1 +
 .../drm/i915/display/intel_crtc_state_dump.c  |   2 +-
 drivers/gpu/drm/i915/display/intel_display.c  | 178 +++--
 drivers/gpu/drm/i915/display/intel_display.h  |   4 +
 .../drm/i915/display/intel_display_debugfs.c  |   3 +
 .../drm/i915/display/intel_display_device.c   |  29 +-
 .../drm/i915/display/intel_display_device.h   |   2 +-
 .../gpu/drm/i915/display/intel_display_irq.c  |  10 +
 .../drm/i915/display/intel_display_limits.h   |   2 +
 .../drm/i915/display/intel_display_power.c    |   4 +
 .../drm/i915/display/intel_display_power.h    |   2 +
 .../gpu/drm/i915/display/intel_display_regs.h |   1 +
 .../drm/i915/display/intel_display_types.h    |   1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   3 +
 drivers/gpu/drm/i915/display/intel_opregion.c |   2 +-
 drivers/gpu/drm/i915/display/intel_pmdemand.c |   3 +
 drivers/gpu/drm/i915/display/intel_vdsc.c     |   4 +
 .../gpu/drm/i915/display/intel_writeback.c    | 686 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_writeback.h    |  23 +
 .../drm/i915/display/intel_writeback_reg.h    | 142 ++++
 drivers/gpu/drm/xe/Makefile                   |   1 +
 include/drm/drm_modeset_helper_vtables.h      |  59 ++
 include/drm/drm_writeback.h                   |  21 +-
 25 files changed, 1238 insertions(+), 69 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.h
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback_reg.h

-- 
2.34.1

[PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Suraj Kandpal]

Write a function that takes a preallocated drm_connector instead of
using the one allocated inside the drm writeback connector init
function.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/drm_writeback.c | 76 +++++++++++++++++++++++++++++++++
 include/drm/drm_writeback.h     |  7 +++
 2 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 95b8a2e4bda6..fa58eb0dc7bf 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drmm_writeback_connector_init);
 
+/*
+ * drm_writeback_connector_init_with_conn - Initialize a writeback connector with
+ * custom encoder and connector
+ *
+ * @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 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_conn(struct drm_device *dev, struct drm_connector *connector,
+				       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_mode_config *config = &dev->mode_config;
+	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;
+
+	INIT_LIST_HEAD(&wb_connector->job_queue);
+	spin_lock_init(&wb_connector->job_lock);
+
+	wb_connector->fence_context = dma_fence_context_alloc(1);
+	spin_lock_init(&wb_connector->fence_lock);
+	snprintf(wb_connector->timeline_name,
+		 sizeof(wb_connector->timeline_name),
+		 "CONNECTOR:%d-%s", connector->base.id, connector->name);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_out_fence_ptr_property, 0);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_fb_id_property, 0);
+
+	drm_object_attach_property(&connector->base,
+				   config->writeback_pixel_formats_property,
+				   blob->base.id);
+	wb_connector->pixel_formats_blob_ptr = blob;
+
+	return 0;
+
+connector_fail:
+	drm_property_blob_put(blob);
+	return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init_with_conn);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb)
 {
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index c380a7b8f55a..149744dbeef0 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -167,6 +167,13 @@ int drmm_writeback_connector_init(struct drm_device *dev,
 				  struct drm_encoder *enc,
 				  const u32 *formats, int n_formats);
 
+int
+drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connector *connector,
+				       struct drm_writeback_connector *wb_connector,
+				       struct drm_encoder *enc,
+				       const struct drm_connector_funcs *con_funcs,
+				       const u32 *formats, int n_formats);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb);
 
-- 
2.34.1

[PATCH 02/28] drm/writeback: Add a helper function to get writeback connector

[Suraj Kandpal]

Now that we can initialize a drm_writeback_connector without
having to initialize the drm_connector within it and leaving the
responsibility of initialising the drm_connector and maintaining
the association with drm_writeback_connector to it. This helper
hooks lets drivers return the drm_writeback_connector associated
with the give drm_connector.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/drm_writeback.c          | 14 ++++++
 include/drm/drm_modeset_helper_vtables.h | 59 ++++++++++++++++++++++++
 include/drm/drm_writeback.h              | 14 ++++--
 3 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index fa58eb0dc7bf..e9f7123270d6 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -107,6 +107,19 @@ static const struct dma_fence_ops drm_writeback_fence_ops = {
 	.get_timeline_name = drm_writeback_fence_get_timeline_name,
 };
 
+struct drm_writeback_connector *
+drm_connector_to_writeback(struct drm_connector *connector)
+{
+	const struct drm_connector_helper_funcs *funcs =
+		connector->helper_private;
+
+	if (funcs->get_writeback_connector)
+		return funcs->get_writeback_connector(connector);
+
+	return container_of(connector, struct drm_writeback_connector, base);
+}
+EXPORT_SYMBOL(drm_connector_to_writeback);
+
 static int create_writeback_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
@@ -443,6 +456,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
 				       struct drm_writeback_connector *wb_connector,
 				       struct drm_encoder *enc,
 				       const struct drm_connector_funcs *con_funcs,
+				       const struct drm_writeback_connector_helper_funcs *wb_funcs,
 				       const u32 *formats, int n_formats)
 {
 	struct drm_property_blob *blob;
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index ce7c7aeac887..6b89b33d2304 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -31,6 +31,7 @@
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_writeback.h>
 
 /**
  * DOC: overview
@@ -1179,6 +1180,25 @@ struct drm_connector_helper_funcs {
 	 *
 	 */
 	void (*disable_hpd)(struct drm_connector *connector);
+
+	/**
+	 * @get_writeback_connector:
+	 *
+	 * This callback is used by drivers to get the writeback connector in
+	 * case the init is done via drm_writeback_init_with_conn. Which means
+	 * the drivers don't have drm_connector embedded in drm_writeback_connector
+	 * so they need to send the associated writeback connector with this
+	 * function.
+	 *
+	 * This operation is optional.
+	 *
+	 * This is mainly called from drm_writeback_set_gb.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_writeback_connector assoiciated with the drm connector.
+	 */
+	struct drm_writeback_connector *(*get_writeback_connector)(struct drm_connector *connector);
 };
 
 /**
@@ -1192,6 +1212,45 @@ static inline void drm_connector_helper_add(struct drm_connector *connector,
 	connector->helper_private = funcs;
 }
 
+/**
+ * struct drm_writeback_connector_helper_funcs - helper operations for writeback
+ * connectors.
+ *
+ * These functions are used by the atomic and legacy modeset helpers and by the
+ * probe helpers.
+ */
+struct drm_writeback_connector_helper_funcs {
+	/**
+	 * @get_connector_from_writeback:
+	 *
+	 * This callback is used by drivers to get the drm_connector in
+	 * case the init is done via drm_writeback_init_with_conn. Which means
+	 * the drivers don't have drm_connector embedded in drm_writeback_connector
+	 * so they need to send the associated drm_connector with this
+	 * function.
+	 *
+	 * This operation is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_connector assoiciated with the drm_writeback_connector.
+	 */
+	struct drm_connector
+	*(*get_connector_from_writeback)(struct drm_writeback_connector *wbconnector);
+};
+
+/**
+ * drm_writeback_connector_helper_add - sets the helper vtable for a connector
+ * @wb_connector: DRM writeback connector
+ * @funcs: helper vtable to set for @wb_connector
+ */
+static inline void
+drm_writeback_connector_helper_add(struct drm_writeback_connector *wb_connector,
+				   const struct drm_writeback_connector_helper_funcs *funcs)
+{
+	wb_connector->helper_private = funcs;
+}
+
 /**
  * struct drm_plane_helper_funcs - helper operations for planes
  *
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 149744dbeef0..77c3c64c132d 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -84,6 +84,13 @@ struct drm_writeback_connector {
 	 * The name of the connector's fence timeline.
 	 */
 	char timeline_name[32];
+
+	/**
+	 * @helper_private:
+	 *
+	 * helper private funcs for writeback_connector
+	 */
+	const struct drm_writeback_connector_helper_funcs *helper_private;
 };
 
 /**
@@ -142,11 +149,7 @@ struct drm_writeback_job {
 	void *priv;
 };
 
-static inline struct drm_writeback_connector *
-drm_connector_to_writeback(struct drm_connector *connector)
-{
-	return container_of(connector, struct drm_writeback_connector, base);
-}
+struct drm_writeback_connector *drm_connector_to_writeback(struct drm_connector *connector);
 
 int drm_writeback_connector_init(struct drm_device *dev,
 				 struct drm_writeback_connector *wb_connector,
@@ -172,6 +175,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
 				       struct drm_writeback_connector *wb_connector,
 				       struct drm_encoder *enc,
 				       const struct drm_connector_funcs *con_funcs,
+				       const struct drm_writeback_connector_helper_funcs *wb_funcs,
 				       const u32 *formats, int n_formats);
 
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
-- 
2.34.1

[PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Suraj Kandpal]

Now that drm_connector may not always be embedded within
drm_writeback_connector, let's define a function which either uses
the writeback helper hook that returns the drm_connector associated
with the drm_writeback_connector or just return the drm_connector
embedded inside drm_writeback_connector if the helper hook is not
present. Lets use this function and call it whenever
drm_connector is required mostly when connector helper private funcs
need to be fetched.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/drm_writeback.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index e9f7123270d6..d610cb827975 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -120,6 +120,18 @@ drm_connector_to_writeback(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_to_writeback);
 
+static struct drm_connector *
+drm_connector_from_writeback(struct drm_writeback_connector *wb_connector)
+{
+	const struct drm_writeback_connector_helper_funcs *funcs =
+		wb_connector->helper_private;
+
+	if (funcs && funcs->get_connector_from_writeback)
+		return funcs->get_connector_from_writeback(wb_connector);
+
+	return &wb_connector->base;
+}
+
 static int create_writeback_properties(struct drm_device *dev)
 {
 	struct drm_property *prop;
@@ -478,6 +490,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
 	if (ret)
 		goto connector_fail;
 
+	drm_writeback_connector_helper_add(wb_connector, wb_funcs);
 	INIT_LIST_HEAD(&wb_connector->job_queue);
 	spin_lock_init(&wb_connector->job_lock);
 
@@ -527,13 +540,15 @@ int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 
 int drm_writeback_prepare_job(struct drm_writeback_job *job)
 {
-	struct drm_writeback_connector *connector = job->connector;
+	struct drm_writeback_connector *wb_connector = job->connector;
+	struct drm_connector *connector =
+		drm_connector_from_writeback(wb_connector);
 	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->helper_private;
 	int ret;
 
 	if (funcs->prepare_writeback_job) {
-		ret = funcs->prepare_writeback_job(connector, job);
+		ret = funcs->prepare_writeback_job(wb_connector, job);
 		if (ret < 0)
 			return ret;
 	}
@@ -579,12 +594,14 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
 
 void drm_writeback_cleanup_job(struct drm_writeback_job *job)
 {
-	struct drm_writeback_connector *connector = job->connector;
+	struct drm_writeback_connector *wb_connector = job->connector;
+	struct drm_connector *connector =
+		drm_connector_from_writeback(wb_connector);
 	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->helper_private;
 
 	if (job->prepared && funcs->cleanup_writeback_job)
-		funcs->cleanup_writeback_job(connector, job);
+		funcs->cleanup_writeback_job(wb_connector, job);
 
 	if (job->fb)
 		drm_framebuffer_put(job->fb);
@@ -665,9 +682,11 @@ EXPORT_SYMBOL(drm_writeback_signal_completion);
 struct dma_fence *
 drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
 {
+	struct drm_connector *connector =
+		drm_connector_from_writeback(wb_connector);
 	struct dma_fence *fence;
 
-	if (WARN_ON(wb_connector->base.connector_type !=
+	if (WARN_ON(connector->connector_type !=
 		    DRM_MODE_CONNECTOR_WRITEBACK))
 		return NULL;
 
-- 
2.34.1

[PATCH 04/28] drm/i915/writeback: Add writeback registers

[Suraj Kandpal]

Add writeback registers to its own file.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../drm/i915/display/intel_writeback_reg.h    | 136 ++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback_reg.h

diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
new file mode 100644
index 000000000000..ffe302ef3dd9
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
@@ -0,0 +1,136 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_WRITEBACK_REGS_H__
+#define __INTEL_WRITEBACK_REGS_H__
+
+#include "intel_display_reg_defs.h"
+
+/* WD 0 and 1 */
+#define TRANSCODER_WD0_OFFSET	0x6e000
+#define TRANSCODER_WD1_OFFSET	0x6d800
+
+/* WD 0 and 1 */
+#define PIPE_WD0_OFFSET		0x7e008
+#define PIPE_WD1_OFFSET		0x7d008
+
+/* Gen12 WD */
+#define _MMIO_WD(tc, wd0, wd1)	_MMIO_TRANS((tc) - TRANSCODER_WD_0, wd0, wd1)
+
+#define WD_TRANS_ENABLE		REG_BIT(31)
+#define WD_TRANS_STATE		REG_BIT(30)
+
+/* WD transcoder control */
+#define _WD_TRANS_FUNC_CTL_0	0x6e400
+#define _WD_TRANS_FUNC_CTL_1	0x6ec00
+#define WD_TRANS_FUNC_CTL(tc)	_MMIO_WD(tc,\
+				_WD_TRANS_FUNC_CTL_0,\
+				_WD_TRANS_FUNC_CTL_1)
+
+#define TRANS_WD_FUNC_ENABLE		REG_BIT(31)
+#define WD_TRIGGERED_CAP_MODE_ENABLE	REG_BIT(30)
+#define START_TRIGGER_FRAME		REG_BIT(29)
+#define STOP_TRIGGER_FRAME		REG_BIT(28)
+#define WD_INPUT_SELECT_MASK		REG_GENMASK(14, 12)
+#define WD_INPUT_PIPE_A			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 0)
+#define WD_INPUT_PIPE_B			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 5)
+#define WD_INPUT_PIPE_C			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 6)
+#define WD_INPUT_PIPE_D			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 7)
+#define WD_COLOR_MODE_MASK		REG_GENMASK(22, 20)
+#define WD_CONTROL_POINTERS             REG_GENMASK(19, 18)
+#define WD_DISABLE_POINTERS             REG_FIELD_PREP(WD_CONTROL_POINTERS, 3)
+#define WD_PIX_FMT_YUYV			REG_FIELD_PREP(WD_COLOR_MODE_MASK, 1)
+#define WD_PIX_FMT_XYUV8888		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 2)
+#define WD_PIX_FMT_XBGR8888		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 3)
+#define WD_PIX_FMT_Y410			REG_FIELD_PREP(WD_COLOR_MODE_MASK, 4)
+#define WD_PIX_FMT_YUV422		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 5)
+#define WD_PIX_FMT_XBGR2101010		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 6)
+#define WD_PIX_FMT_RGB565		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 7)
+#define WD_FRAME_NUMBER_MASK		REG_GENMASK(3, 0)
+#define WD_FRAME_NUMBER(n)		REG_FIELD_PREP(WD_FRAME_NUMBER_MASK, n)
+
+#define _WD_STRIDE_0			0x6e510
+#define _WD_STRIDE_1			0x6ed10
+#define WD_STRIDE(tc)			_MMIO_WD(tc,\
+					_WD_STRIDE_0,\
+					_WD_STRIDE_1)
+#define WD_STRIDE_MASK			REG_GENMASK(15, 6)
+
+#define _WD_STREAMCAP_CTL0		0x6e590
+#define _WD_STREAMCAP_CTL1		0x6ed90
+#define WD_STREAMCAP_CTL(tc)		_MMIO_WD(tc,\
+					_WD_STREAMCAP_CTL0,\
+					_WD_STREAMCAP_CTL1)
+
+#define WD_STREAM_CAP_MODE_EN		REG_BIT(31)
+#define WD_SLICING_STRAT_MASK		REG_GENMASK(25, 24)
+#define WD_SLICING_STRAT_1_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 0)
+#define WD_SLICING_STRAT_2_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 1)
+#define WD_SLICING_STRAT_4_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 2)
+#define WD_SLICING_STRAT_8_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 3)
+#define WD_STREAM_OVERRUN_STATUS	1
+
+#define _WD_SURF_0			0x6e514
+#define _WD_SURF_1			0x6ed14
+#define WD_SURF(tc)			_MMIO_WD(tc,\
+					_WD_SURF_0,\
+					_WD_SURF_1)
+
+#define _WD_IMR_0			0x6e560
+#define _WD_IMR_1			0x6ed60
+#define WD_IMR(tc)			_MMIO_WD(tc,\
+					_WD_IMR_0,\
+					_WD_IMR_1)
+#define WD_FRAME_COMPLETE_INT		REG_BIT(7)
+#define WD_GTT_FAULT_INT		REG_BIT(6)
+#define WD_VBLANK_INT			REG_BIT(5)
+#define WD_OVERRUN_INT			REG_BIT(4)
+#define WD_CAPTURING_INT		REG_BIT(3)
+#define WD_WRITE_COMPLETE_INT		REG_BIT(2)
+
+#define _WD_IIR_0			0x6e564
+#define _WD_IIR_1			0x6ed64
+#define WD_IIR(tc)			_MMIO_WD(tc,\
+					_WD_IIR_0,\
+					_WD_IIR_1)
+
+#define _WD_FRAME_STATUS_0		0x6e568
+#define _WD_FRAME_STATUS_1		0x6ed68
+#define WD_FRAME_STATUS(tc)		_MMIO_WD(tc,\
+					_WD_FRAME_STATUS_0,\
+					_WD_FRAME_STATUS_1)
+
+#define WD_FRAME_COMPLETE		REG_BIT(31)
+#define WD_STATE_MASK			REG_GENMASK(26, 24)
+#define WD_STATE_IDLE			REG_FIELD_PREP(WD_STATE_MASK, 0)
+#define WD_STATE_CAPSTART		REG_FIELD_PREP(WD_STATE_MASK, 1)
+#define WD_STATE_FRAME_START		REG_FIELD_PREP(WD_STATE_MASK, 2)
+#define WD_STATE_CAPACITIVE		REG_FIELD_PREP(WD_STATE_MASK, 3)
+#define WD_STATE_TG_DONE		REG_FIELD_PREP(WD_STATE_MASK, 4)
+#define WD_STATE_WDX_DONE		REG_FIELD_PREP(WD_STATE_MASK, 5)
+#define WD_STATE_QUICK_CAP		REG_FIELD_PREP(WD_STATE_MASK, 6)
+
+#define _WD_27_M_0			0x6e524
+#define _WD_27_M_1			0x6ed24
+#define WD_27_M(tc)			_MMIO_WD(tc,\
+					_WD_27_M_0,\
+					_WD_27_M_1)
+
+#define _WD_27_N_0			0x6e528
+
+/* Address looks wrong in bspec: */
+#define _WD_27_N_1			0x6ec28
+#define WD_27_N(tc)			_MMIO_WD(tc,\
+					_WD_27_N_0,\
+					_WD_27_N_1)
+
+#define _WD_TAIL_CFG_0			0x6e520
+#define _WD_TAIL_CFG_1			0x6ed20
+
+#define WD_TAIL_CFG(tc)			_MMIO_WD(tc,\
+					_WD_TAIL_CFG_0,\
+					_WD_TAIL_CFG_1)
+
+#endif /* __INTEL_WRITEBACK_REGS_H__ */
-- 
2.34.1

[PATCH 05/28] drm/i915/writeback: Add some preliminary writeback definitions

[Suraj Kandpal]

Add some preliminary definitions like, output type and transcoder
related to the writeback functionality.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_acpi.c     |  1 +
 .../drm/i915/display/intel_crtc_state_dump.c  |  2 +-
 drivers/gpu/drm/i915/display/intel_display.c  |  3 +-
 drivers/gpu/drm/i915/display/intel_display.h  |  4 +++
 .../drm/i915/display/intel_display_device.c   | 29 +++++++++++++++++--
 .../drm/i915/display/intel_display_device.h   |  2 +-
 .../drm/i915/display/intel_display_limits.h   |  2 ++
 .../drm/i915/display/intel_display_power.c    |  4 +++
 .../drm/i915/display/intel_display_power.h    |  2 ++
 .../drm/i915/display/intel_display_types.h    |  1 +
 10 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c
index 1addd6288241..4b1ec56299ca 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -255,6 +255,7 @@ static u32 acpi_display_type(struct intel_connector *connector)
 		break;
 	case DRM_MODE_CONNECTOR_Unknown:
 	case DRM_MODE_CONNECTOR_VIRTUAL:
+	case DRM_MODE_CONNECTOR_WRITEBACK:
 		display_type = ACPI_DISPLAY_TYPE_OTHER;
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 0c7f91046996..77e05d73ed92 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -53,7 +53,6 @@ intel_dump_infoframe(struct intel_display *display,
 }
 
 #define OUTPUT_TYPE(x) [INTEL_OUTPUT_ ## x] = #x
-
 static const char * const output_type_str[] = {
 	OUTPUT_TYPE(UNUSED),
 	OUTPUT_TYPE(ANALOG),
@@ -67,6 +66,7 @@ static const char * const output_type_str[] = {
 	OUTPUT_TYPE(DSI),
 	OUTPUT_TYPE(DDI),
 	OUTPUT_TYPE(DP_MST),
+	OUTPUT_TYPE(WRITEBACK),
 };
 
 #undef OUTPUT_TYPE
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 7035c1fc9033..c2d1156de8e9 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3716,7 +3716,8 @@ static u8 hsw_panel_transcoders(struct intel_display *display)
 	u8 panel_transcoder_mask = BIT(TRANSCODER_EDP);
 
 	if (DISPLAY_VER(display) >= 11)
-		panel_transcoder_mask |= BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1);
+		panel_transcoder_mask |= BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1) |
+				BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1);
 
 	return panel_transcoder_mask;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 37e2ab301a80..12b8f2cf0fc8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -65,6 +65,10 @@ static inline const char *transcoder_name(enum transcoder transcoder)
 		return "DSI A";
 	case TRANSCODER_DSI_C:
 		return "DSI C";
+	case TRANSCODER_WD_0:
+		return "WD 0";
+	case TRANSCODER_WD_1:
+		return "WD 1";
 	default:
 		return "<invalid>";
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
index 089cffabbad5..286de0187931 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.c
+++ b/drivers/gpu/drm/i915/display/intel_display_device.c
@@ -22,6 +22,7 @@
 #include "intel_display_types.h"
 #include "intel_fbc.h"
 #include "intel_step.h"
+#include "intel_writeback_reg.h"
 
 __diag_push();
 __diag_ignore_all("-Woverride-init", "Allow field initialization overrides for display info");
@@ -145,12 +146,16 @@ static const struct intel_display_device_info no_display = {};
 		[TRANSCODER_B] = PIPE_B_OFFSET, \
 		[TRANSCODER_C] = PIPE_C_OFFSET, \
 		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
+		[TRANSCODER_WD_0] = PIPE_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = PIPE_WD1_OFFSET, \
 	}, \
 	.trans_offsets = { \
 		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
 		[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
 		[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
 		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
+		[TRANSCODER_WD_0] = TRANSCODER_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = TRANSCODER_WD1_OFFSET, \
 	}
 
 #define CHV_PIPE_OFFSETS \
@@ -581,7 +586,8 @@ static const struct platform_desc hsw_desc = {
 		.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
 		.__runtime_defaults.cpu_transcoder_mask =
 		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
-		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP),
+		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) |
+		BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1),
 		.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D) | BIT(PORT_E),
 		.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
 	},
@@ -678,7 +684,8 @@ static const struct intel_display_device_info skl_display = {
 	.__runtime_defaults.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
 	.__runtime_defaults.cpu_transcoder_mask =
 	BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
-	BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP),
+	BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) |
+	BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1),
 	.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C) | BIT(PORT_D) | BIT(PORT_E),
 	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A),
 };
@@ -830,6 +837,7 @@ static const struct platform_desc cml_desc = {
 		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \
 		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \
 		BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \
+		BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_0), \
 	.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) | BIT(PORT_C)
 
 static const enum intel_step bxt_steppings[] = {
@@ -884,6 +892,8 @@ static const struct platform_desc glk_desc = {
 		[TRANSCODER_EDP] = PIPE_EDP_OFFSET, \
 		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
 		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+		[TRANSCODER_WD_0] = PIPE_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = PIPE_WD1_OFFSET, \
 	}, \
 	.trans_offsets = { \
 		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
@@ -892,6 +902,8 @@ static const struct platform_desc glk_desc = {
 		[TRANSCODER_EDP] = TRANSCODER_EDP_OFFSET, \
 		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
 		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+		[TRANSCODER_WD_0] = TRANSCODER_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = TRANSCODER_WD1_OFFSET, \
 	}, \
 	IVB_CURSOR_OFFSETS, \
 	ICL_COLORS, \
@@ -905,6 +917,7 @@ static const struct platform_desc glk_desc = {
 		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \
 		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \
 		BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1), \
+		BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1), \
 	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A)
 
 static const u16 icl_port_f_ids[] = {
@@ -975,6 +988,8 @@ static const struct platform_desc ehl_desc = {
 		[TRANSCODER_D] = PIPE_D_OFFSET, \
 		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
 		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
+		[TRANSCODER_WD_0] = PIPE_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = PIPE_WD1_OFFSET, \
 	}, \
 	.trans_offsets = { \
 		[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
@@ -983,6 +998,8 @@ static const struct platform_desc ehl_desc = {
 		[TRANSCODER_D] = TRANSCODER_D_OFFSET, \
 		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
 		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
+		[TRANSCODER_WD_0] = TRANSCODER_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = TRANSCODER_WD1_OFFSET, \
 	}, \
 	TGL_CURSOR_OFFSETS, \
 	ICL_COLORS, \
@@ -997,6 +1014,7 @@ static const struct platform_desc ehl_desc = {
 		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \
 		BIT(TRANSCODER_C) | BIT(TRANSCODER_D) | \
 		BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1), \
+		BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1), \
 	.__runtime_defaults.fbc_mask = BIT(INTEL_FBC_A)
 
 static const u16 tgl_uy_ids[] = {
@@ -1142,6 +1160,8 @@ static const struct platform_desc adl_s_desc = {
 		[TRANSCODER_D] = PIPE_D_OFFSET,					\
 		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET,				\
 		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET,				\
+		[TRANSCODER_WD_0] = PIPE_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = PIPE_WD1_OFFSET, \
 	},									\
 	.trans_offsets = {							\
 		[TRANSCODER_A] = TRANSCODER_A_OFFSET,				\
@@ -1150,6 +1170,8 @@ static const struct platform_desc adl_s_desc = {
 		[TRANSCODER_D] = TRANSCODER_D_OFFSET,				\
 		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET,			\
 		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET,			\
+		[TRANSCODER_WD_0] = TRANSCODER_WD0_OFFSET, \
+		[TRANSCODER_WD_1] = TRANSCODER_WD1_OFFSET, \
 	},									\
 	TGL_CURSOR_OFFSETS,							\
 										\
@@ -1169,7 +1191,8 @@ static const struct intel_display_device_info xe_lpd_display = {
 	.__runtime_defaults.cpu_transcoder_mask =
 		BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
 		BIT(TRANSCODER_C) | BIT(TRANSCODER_D) |
-		BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1),
+		BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1) |
+		BIT(TRANSCODER_WD_0) | BIT(TRANSCODER_WD_1),
 	.__runtime_defaults.port_mask = BIT(PORT_A) | BIT(PORT_B) |
 		BIT(PORT_TC1) | BIT(PORT_TC2) | BIT(PORT_TC3) | BIT(PORT_TC4),
 };
diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 4308822f0415..779043ee744b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -261,7 +261,7 @@ struct intel_display_runtime_info {
 	u32 rawclk_freq;
 
 	u8 pipe_mask;
-	u8 cpu_transcoder_mask;
+	u16 cpu_transcoder_mask;
 	u16 port_mask;
 
 	u8 num_sprites[I915_MAX_PIPES];
diff --git a/drivers/gpu/drm/i915/display/intel_display_limits.h b/drivers/gpu/drm/i915/display/intel_display_limits.h
index f0fa27e365ab..67978c1b71ad 100644
--- a/drivers/gpu/drm/i915/display/intel_display_limits.h
+++ b/drivers/gpu/drm/i915/display/intel_display_limits.h
@@ -45,6 +45,8 @@ enum transcoder {
 	TRANSCODER_DSI_1,
 	TRANSCODER_DSI_A = TRANSCODER_DSI_0,	/* legacy DSI */
 	TRANSCODER_DSI_C = TRANSCODER_DSI_1,	/* legacy DSI */
+	TRANSCODER_WD_0,
+	TRANSCODER_WD_1,
 
 	I915_MAX_TRANSCODERS
 };
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 273054c22325..bdc924a42369 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -76,6 +76,10 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "TRANSCODER_DSI_A";
 	case POWER_DOMAIN_TRANSCODER_DSI_C:
 		return "TRANSCODER_DSI_C";
+	case POWER_DOMAIN_TRANSCODER_WD_0:
+		return "TRANSCODER_WD_0";
+	case POWER_DOMAIN_TRANSCODER_WD_1:
+		return "TRANSCODER_WD_1";
 	case POWER_DOMAIN_TRANSCODER_VDSC_PW2:
 		return "TRANSCODER_VDSC_PW2";
 	case POWER_DOMAIN_PORT_DDI_LANES_A:
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index f8813b0e16df..056677d62e54 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -40,6 +40,8 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_TRANSCODER_DSI_A,
 	POWER_DOMAIN_TRANSCODER_DSI_C,
+	POWER_DOMAIN_TRANSCODER_WD_0,
+	POWER_DOMAIN_TRANSCODER_WD_1,
 
 	/* VDSC/joining for eDP/DSI transcoder (ICL) or pipe A (TGL) */
 	POWER_DOMAIN_TRANSCODER_VDSC_PW2,
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 8f8019d40d77..2064e2c2ec77 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -80,6 +80,7 @@ enum intel_output_type {
 	INTEL_OUTPUT_DSI = 9,
 	INTEL_OUTPUT_DDI = 10,
 	INTEL_OUTPUT_DP_MST = 11,
+	INTEL_OUTPUT_WRITEBACK = 12,
 };
 
 enum hdmi_force_audio {
-- 
2.34.1

[PATCH 06/28] drm/i915/writeback: Init writeback connector

[Suraj Kandpal]

Initialize writeback connector initialising the virtual encoder
and intel connector. We also allocate memory for drm_writeback_connector
but not the drm_connector within it due to a constraint
we need all connectors to be an intel_connector.
The writeback_format arrays is used to tell the user which
drm formats are supported by us.

Bspec: 49275
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 .../gpu/drm/i915/display/intel_writeback.c    | 129 ++++++++++++++++++
 .../gpu/drm/i915/display/intel_writeback.h    |  17 +++
 3 files changed, 147 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 853543443072..c9201f6111c3 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -295,6 +295,7 @@ i915-y += \
 	display/intel_vblank.o \
 	display/intel_vga.o \
 	display/intel_wm.o \
+	display/intel_writeback.o \
 	display/skl_scaler.o \
 	display/skl_universal_plane.o \
 	display/skl_watermark.o \
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
new file mode 100644
index 000000000000..f3843ac9b026
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#include <linux/slab.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_writeback.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_encoder.h>
+
+#include "i915_drv.h"
+#include "intel_atomic.h"
+#include "intel_de.h"
+#include "intel_display_types.h"
+#include "intel_display_driver.h"
+#include "intel_connector.h"
+#include "intel_writeback.h"
+
+struct intel_writeback_connector {
+	struct drm_writeback_connector base;
+	struct intel_encoder encoder;
+	struct intel_connector connector;
+	enum transcoder trans;
+	int frame_num;
+};
+
+static const u32 writeback_formats[] = {
+	DRM_FORMAT_XYUV8888,
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XVYU2101010,
+	DRM_FORMAT_VYUY,
+	DRM_FORMAT_XBGR2101010,
+};
+
+static int intel_writeback_connector_init(struct intel_connector *connector)
+{
+	struct intel_digital_connector_state *conn_state;
+
+	conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
+	if (!conn_state)
+		return -ENOMEM;
+
+	__drm_atomic_helper_connector_reset(&connector->base,
+					    &conn_state->base);
+	return 0;
+}
+
+static int
+intel_writeback_connector_alloc(struct intel_connector *connector)
+{
+	if (intel_writeback_connector_init(connector) < 0) {
+		kfree(connector);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+const struct drm_connector_funcs conn_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_connector_helper_funcs conn_helper_funcs = {
+};
+
+static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
+};
+
+int intel_writeback_init(struct intel_display *display)
+{
+	struct intel_encoder *encoder;
+	struct intel_writeback_connector *writeback_conn;
+	struct intel_connector *connector;
+	int ret;
+
+	writeback_conn = kzalloc(sizeof(*writeback_conn), GFP_KERNEL);
+	if (!writeback_conn)
+		return -ENOSPC;
+
+	encoder = &writeback_conn->encoder;
+	encoder->base.possible_crtcs = 0xf;
+	ret = drm_encoder_init(display->drm, &encoder->base,
+			       &drm_writeback_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret) {
+		kfree(writeback_conn);
+		return ret;
+	}
+
+	encoder->type = INTEL_OUTPUT_WRITEBACK;
+	encoder->pipe_mask = ~0;
+	encoder->cloneable = 0;
+
+	connector = &writeback_conn->connector;
+	intel_writeback_connector_alloc(connector);
+
+	connector->base.interlace_allowed = 0;
+	drm_connector_helper_add(&connector->base, &conn_helper_funcs);
+	ret = drm_writeback_connector_init_with_conn(display->drm, &connector->base,
+						     &writeback_conn->base,
+						     &encoder->base, &conn_funcs,
+						     &writeback_conn_helper_funcs,
+						     writeback_formats,
+						     ARRAY_SIZE(writeback_formats));
+	if (ret) {
+		intel_connector_free(connector);
+		drm_encoder_cleanup(&encoder->base);
+		kfree(&writeback_conn->encoder);
+		kfree(writeback_conn);
+		return ret;
+	}
+
+	intel_connector_attach_encoder(connector, encoder);
+	connector->get_hw_state = intel_connector_get_hw_state;
+	connector->base.status = connector_status_disconnected;
+	writeback_conn->frame_num = 1;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
new file mode 100644
index 000000000000..5911684cb81a
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_writeback.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef __INTEL_WRITEBACK_H__
+#define __INTEL_WRITEBACK_H__
+
+#include <linux/types.h>
+
+struct intel_display;
+struct intel_writeback_connector;
+
+int intel_writeback_init(struct intel_display *display);
+
+#endif /* __INTEL_WRITEBACK_H__ */
+
-- 
2.34.1

[PATCH 07/28] drm/i915/writeback: Add function for get_writeback_connector

[Suraj Kandpal]

Define function to get_writeback_connector which returns the
drm_writeback_connector given the drm_connector.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index f3843ac9b026..1ff078a191df 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -36,6 +36,13 @@ static const u32 writeback_formats[] = {
 	DRM_FORMAT_XBGR2101010,
 };
 
+static struct intel_writeback_connector
+*conn_to_intel_writeback_connector(struct intel_connector *connector)
+{
+	return container_of(connector, struct intel_writeback_connector,
+			    connector);
+}
+
 static int intel_writeback_connector_init(struct intel_connector *connector)
 {
 	struct intel_digital_connector_state *conn_state;
@@ -60,6 +67,16 @@ intel_writeback_connector_alloc(struct intel_connector *connector)
 	return 0;
 }
 
+static struct drm_writeback_connector *
+intel_get_writeback_connector(struct drm_connector *connector)
+{
+	struct intel_connector *conn = to_intel_connector(connector);
+	struct intel_writeback_connector *wb_conn =
+		conn_to_intel_writeback_connector(conn);
+
+	return &wb_conn->base;
+}
+
 static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
@@ -71,6 +88,7 @@ const struct drm_connector_funcs conn_funcs = {
 };
 
 static const struct drm_connector_helper_funcs conn_helper_funcs = {
+	.get_writeback_connector = intel_get_writeback_connector,
 };
 
 static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
-- 
2.34.1

[PATCH 08/28] drm/i915/writeback: Define the get_connector_from_writeback hook

[Suraj Kandpal]

Define the get_connector_from_writeback hook to get the drm_connector
from drm_writeback_connector.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 1ff078a191df..02f4910d7797 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -43,6 +43,13 @@ static struct intel_writeback_connector
 			    connector);
 }
 
+static struct intel_writeback_connector
+*to_intel_writeback_connector(struct drm_writeback_connector *wb_conn)
+{
+	return container_of(wb_conn, struct intel_writeback_connector,
+			    base);
+}
+
 static int intel_writeback_connector_init(struct intel_connector *connector)
 {
 	struct intel_digital_connector_state *conn_state;
@@ -77,6 +84,15 @@ intel_get_writeback_connector(struct drm_connector *connector)
 	return &wb_conn->base;
 }
 
+static struct drm_connector *
+intel_get_connector_from_writeback(struct drm_writeback_connector *connector)
+{
+	struct intel_writeback_connector *wb_conn =
+		to_intel_writeback_connector(connector);
+
+	return &wb_conn->connector.base;
+}
+
 static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
@@ -92,6 +108,7 @@ static const struct drm_connector_helper_funcs conn_helper_funcs = {
 };
 
 static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
+	.get_connector_from_writeback = intel_get_connector_from_writeback,
 };
 
 int intel_writeback_init(struct intel_display *display)
-- 
2.34.1

[PATCH 09/28] drm/i915/writeback: Add function to get modes

[Suraj Kandpal]

Add a function that get modes for writeback connector.
Since we have a restriction on supporting only 3840x2160 60Hz modes
at max we will create modes only up until that point.

Bspec: 49275
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 02f4910d7797..7df99771fd6f 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -10,6 +10,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_edid.h>
 
 #include "i915_drv.h"
 #include "intel_atomic.h"
@@ -74,6 +75,11 @@ intel_writeback_connector_alloc(struct intel_connector *connector)
 	return 0;
 }
 
+static int intel_writeback_get_modes(struct drm_connector *connector)
+{
+	return drm_add_modes_noedid(connector, 3840, 2160);
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -105,6 +111,7 @@ const struct drm_connector_funcs conn_funcs = {
 
 static const struct drm_connector_helper_funcs conn_helper_funcs = {
 	.get_writeback_connector = intel_get_writeback_connector,
+	.get_modes = intel_writeback_get_modes,
 };
 
 static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
-- 
2.34.1

[PATCH 10/28] drm/i915/writeback: Add hook to check modes

[Suraj Kandpal]

Add connector helper hooks to check if mode is valid or not.
We add this restriction to make sure mode is 3840x2160 60Hz.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 7df99771fd6f..ff5f15ce3f5f 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -75,6 +75,25 @@ intel_writeback_connector_alloc(struct intel_connector *connector)
 	return 0;
 }
 
+static enum drm_mode_status
+intel_writeback_mode_valid(struct drm_connector *_connector,
+			   const struct drm_display_mode *mode)
+{
+	int refresh_rate;
+
+	if (mode->hdisplay > 3840)
+		return MODE_H_ILLEGAL;
+
+	if (mode->vdisplay > 2160)
+		return MODE_V_ILLEGAL;
+
+	refresh_rate = drm_mode_vrefresh(mode);
+	if (refresh_rate > 60)
+		return MODE_BAD;
+
+	return MODE_OK;
+}
+
 static int intel_writeback_get_modes(struct drm_connector *connector)
 {
 	return drm_add_modes_noedid(connector, 3840, 2160);
@@ -112,6 +131,7 @@ const struct drm_connector_funcs conn_funcs = {
 static const struct drm_connector_helper_funcs conn_helper_funcs = {
 	.get_writeback_connector = intel_get_writeback_connector,
 	.get_modes = intel_writeback_get_modes,
+	.mode_valid = intel_writeback_mode_valid,
 };
 
 static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
-- 
2.34.1

[PATCH 11/28] drm/i915/writeback: Define encoder->get_hw_state

[Suraj Kandpal]

Define the get_hw_state function for encoder which
get's the encoder state, pipe config.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 48 +++++++++++++++++++
 .../drm/i915/display/intel_writeback_reg.h    |  3 ++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index ff5f15ce3f5f..91ca74de7652 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -19,6 +19,7 @@
 #include "intel_display_driver.h"
 #include "intel_connector.h"
 #include "intel_writeback.h"
+#include "intel_writeback_reg.h"
 
 struct intel_writeback_connector {
 	struct drm_writeback_connector base;
@@ -138,6 +139,52 @@ static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_f
 	.get_connector_from_writeback = intel_get_connector_from_writeback,
 };
 
+static bool
+intel_writeback_get_hw_state(struct intel_encoder *encoder,
+			     enum pipe *pipe)
+{
+	struct intel_display *display = to_intel_display(encoder);
+	u8 pipe_mask = 0;
+	u32 tmp;
+
+	/* TODO need to be done for both the wd transcoder */
+	tmp = intel_de_read(display,
+			    TRANSCONF_WD(TRANSCODER_WD_0));
+	if (!(tmp & WD_TRANS_ENABLE))
+		return false;
+
+	tmp = intel_de_read(display,
+			    WD_TRANS_FUNC_CTL(TRANSCODER_WD_0));
+
+	if (!(tmp & TRANS_WD_FUNC_ENABLE))
+		return false;
+
+	switch (tmp & WD_INPUT_SELECT_MASK) {
+	case WD_INPUT_PIPE_A:
+		pipe_mask |= BIT(PIPE_A);
+		break;
+	case WD_INPUT_PIPE_B:
+		pipe_mask |= BIT(PIPE_B);
+		break;
+	case WD_INPUT_PIPE_C:
+		pipe_mask |= BIT(PIPE_C);
+		break;
+	case WD_INPUT_PIPE_D:
+		pipe_mask |= BIT(PIPE_D);
+		break;
+	default:
+		MISSING_CASE(tmp & WD_INPUT_SELECT_MASK);
+		fallthrough;
+	}
+
+	if (pipe_mask == 0)
+		return false;
+
+	*pipe = ffs(pipe_mask) - 1;
+
+	return true;
+}
+
 int intel_writeback_init(struct intel_display *display)
 {
 	struct intel_encoder *encoder;
@@ -162,6 +209,7 @@ int intel_writeback_init(struct intel_display *display)
 	encoder->type = INTEL_OUTPUT_WRITEBACK;
 	encoder->pipe_mask = ~0;
 	encoder->cloneable = 0;
+	encoder->get_hw_state = intel_writeback_get_hw_state;
 
 	connector = &writeback_conn->connector;
 	intel_writeback_connector_alloc(connector);
diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
index ffe302ef3dd9..5e7c6c99d191 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback_reg.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
@@ -19,6 +19,9 @@
 /* Gen12 WD */
 #define _MMIO_WD(tc, wd0, wd1)	_MMIO_TRANS((tc) - TRANSCODER_WD_0, wd0, wd1)
 
+#define TRANSCONF_WD(tc)	_MMIO_WD(tc,\
+				PIPE_WD0_OFFSET,\
+				PIPE_WD1_OFFSET)
 #define WD_TRANS_ENABLE		REG_BIT(31)
 #define WD_TRANS_STATE		REG_BIT(30)
 
-- 
2.34.1

[PATCH 12/28] drm/i915/writeback: Fill encoder->get_config

[Suraj Kandpal]

Fill the encoder->get_config hook with relevant data which helps
verify state.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 91ca74de7652..674cc4ecf1b9 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -139,6 +139,14 @@ static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_f
 	.get_connector_from_writeback = intel_get_connector_from_writeback,
 };
 
+static void
+intel_writeback_get_config(struct intel_encoder *encoder,
+			   struct intel_crtc_state *crtc_state)
+{
+	crtc_state->output_types |= BIT(INTEL_OUTPUT_WRITEBACK);
+	crtc_state->output_format = INTEL_OUTPUT_FORMAT_RGB;
+}
+
 static bool
 intel_writeback_get_hw_state(struct intel_encoder *encoder,
 			     enum pipe *pipe)
@@ -209,6 +217,7 @@ int intel_writeback_init(struct intel_display *display)
 	encoder->type = INTEL_OUTPUT_WRITEBACK;
 	encoder->pipe_mask = ~0;
 	encoder->cloneable = 0;
+	encoder->get_config = intel_writeback_get_config;
 	encoder->get_hw_state = intel_writeback_get_hw_state;
 
 	connector = &writeback_conn->connector;
-- 
2.34.1

[PATCH 13/28] drm/i915/writeback: Add private structure for writeback job

[Suraj Kandpal]

Create intel_writeback_job to track drm_writback_job and other structure
we might need to complete the writeback job.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 674cc4ecf1b9..8d24b1ee0a2a 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -25,10 +25,17 @@ struct intel_writeback_connector {
 	struct drm_writeback_connector base;
 	struct intel_encoder encoder;
 	struct intel_connector connector;
+	struct intel_writeback_job *job;
 	enum transcoder trans;
 	int frame_num;
 };
 
+struct intel_writeback_job {
+	struct drm_framebuffer *fb;
+	struct drm_writeback_connector *wb_connector;
+	struct i915_vma *vma;
+};
+
 static const u32 writeback_formats[] = {
 	DRM_FORMAT_XYUV8888,
 	DRM_FORMAT_YUYV,
-- 
2.34.1

[PATCH 14/28] drm/i915/writeback: Define function for prepare and cleanup hooks

[Suraj Kandpal]

Define function for prepare and cleanup hooks which help map
and unmap drm framebuffer since we need these address to do
register writes in WD_SURF and WD_STRIDE register.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 8d24b1ee0a2a..f1570a638422 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/err.h>
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_writeback.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -11,13 +12,16 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 #include "i915_drv.h"
+#include "i915_vma.h"
 #include "intel_atomic.h"
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_display_driver.h"
 #include "intel_connector.h"
+#include "intel_fb_pin.h"
 #include "intel_writeback.h"
 #include "intel_writeback_reg.h"
 
@@ -107,6 +111,68 @@ static int intel_writeback_get_modes(struct drm_connector *connector)
 	return drm_add_modes_noedid(connector, 3840, 2160);
 }
 
+static int intel_writeback_prepare_job(struct drm_writeback_connector *wb_connector,
+				       struct drm_writeback_job *job)
+{
+	struct intel_writeback_connector *wb_conn =
+		to_intel_writeback_connector(wb_connector);
+	struct i915_vma *vma;
+	struct intel_writeback_job *wb_job;
+	unsigned long out_flags = 0;
+	const struct i915_gtt_view view = {
+		.type = I915_GTT_VIEW_NORMAL,
+	};
+	int ret;
+
+	if (!job->fb)
+		return 0;
+
+	if (job->fb->modifier != DRM_FORMAT_MOD_LINEAR)
+		return -EINVAL;
+
+	wb_job = kzalloc(sizeof(*wb_job), GFP_KERNEL);
+	if (!wb_job)
+		return -ENOMEM;
+
+	vma = intel_fb_pin_to_ggtt(job->fb, &view, 4 * 1024, 0, 0, true, &out_flags);
+	if (IS_ERR(vma)) {
+		drm_err(job->fb->dev, "Failed to map framebuffer: %d\n", ret);
+		ret = PTR_ERR(vma);
+		goto err;
+	}
+
+	wb_job->fb = job->fb;
+	wb_job->vma = vma;
+	wb_job->wb_connector = wb_connector;
+	drm_framebuffer_get(wb_job->fb);
+	job->priv = wb_job;
+	wb_conn->job = wb_job;
+
+	return 0;
+
+err:
+	kfree(wb_job);
+	return ret;
+}
+
+static void intel_writeback_cleanup_job(struct drm_writeback_connector *connector,
+					struct drm_writeback_job *job)
+{
+	struct intel_writeback_job *wb_job = job->priv;
+	struct i915_vma *vma;
+	unsigned long out_flags = 0;
+
+	if (!job->fb)
+		return;
+
+	vma = wb_job->vma;
+	wb_job->vma = NULL;
+	intel_fb_unpin_vma(vma, out_flags);
+	drm_framebuffer_put(wb_job->fb);
+	kfree(wb_job);
+	job->priv = NULL;
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -140,6 +206,8 @@ static const struct drm_connector_helper_funcs conn_helper_funcs = {
 	.get_writeback_connector = intel_get_writeback_connector,
 	.get_modes = intel_writeback_get_modes,
 	.mode_valid = intel_writeback_mode_valid,
+	.prepare_writeback_job = intel_writeback_prepare_job,
+	.cleanup_writeback_job = intel_writeback_cleanup_job,
 };
 
 static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_funcs = {
-- 
2.34.1

[PATCH 15/28] drm/i915/writeback: Define compute_config for writeback

[Suraj Kandpal]

Define the compute config function where we assign the output_type
and add the transcoder that needs to be used. We currently assign
one WD0 transcoder.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index f1570a638422..3ea09587c4c2 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -214,6 +214,25 @@ static const struct drm_writeback_connector_helper_funcs writeback_conn_helper_f
 	.get_connector_from_writeback = intel_get_connector_from_writeback,
 };
 
+static int
+intel_writeback_compute_config(struct intel_encoder *encoder,
+			       struct intel_crtc_state *pipe_config,
+			       struct drm_connector_state *conn_state)
+{
+	struct intel_display *display = to_intel_display(encoder);
+
+	if (!conn_state->writeback_job)
+		return 0;
+
+	if (HAS_TRANSCODER(display, TRANSCODER_WD_0))
+		pipe_config->cpu_transcoder = TRANSCODER_WD_0;
+
+	pipe_config->output_types |= BIT(INTEL_OUTPUT_WRITEBACK);
+	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
+
+	return 0;
+}
+
 static void
 intel_writeback_get_config(struct intel_encoder *encoder,
 			   struct intel_crtc_state *crtc_state)
@@ -294,6 +313,7 @@ int intel_writeback_init(struct intel_display *display)
 	encoder->cloneable = 0;
 	encoder->get_config = intel_writeback_get_config;
 	encoder->get_hw_state = intel_writeback_get_hw_state;
+	encoder->compute_config = intel_writeback_compute_config;
 
 	connector = &writeback_conn->connector;
 	intel_writeback_connector_alloc(connector);
-- 
2.34.1

[PATCH 16/28] drm/i915/writeback: Define function for connector function detect

[Suraj Kandpal]

detect function always returns connector_status_connected if
writeback connector has been initialized.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 3ea09587c4c2..def33191a89e 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -173,6 +173,13 @@ static void intel_writeback_cleanup_job(struct drm_writeback_connector *connecto
 	job->priv = NULL;
 }
 
+static enum drm_connector_status
+intel_writeback_detect(struct drm_connector *connector,
+		       bool force)
+{
+	return connector_status_connected;
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -197,6 +204,7 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
 };
 
 const struct drm_connector_funcs conn_funcs = {
+	.detect = intel_writeback_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-- 
2.34.1

[PATCH 17/28] drm/i915/writeback: Define function to destroy writeback connector

[Suraj Kandpal]

Define function to destroy the drm_writbeack_connector and
drm_connector associated with it.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index def33191a89e..9b2432d86d35 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -180,6 +180,12 @@ intel_writeback_detect(struct drm_connector *connector,
 	return connector_status_connected;
 }
 
+static void intel_writeback_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+	kfree(connector);
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -208,6 +214,7 @@ const struct drm_connector_funcs conn_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+	.destroy = intel_writeback_connector_destroy,
 };
 
 static const struct drm_connector_helper_funcs conn_helper_funcs = {
-- 
2.34.1

[PATCH 18/28] drm/i915/writeback: Add connector atomic check

[Suraj Kandpal]

Add connector helper function for atomic check which sets the
mode_changed bit and checks if pixel format of fb is valid or not.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 9b2432d86d35..7fb30cc61991 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -186,6 +186,54 @@ static void intel_writeback_connector_destroy(struct drm_connector *connector)
 	kfree(connector);
 }
 
+static int intel_writeback_check_format(u32 format)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(writeback_formats); i++) {
+		if (writeback_formats[i] == format)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int intel_writeback_atomic_check(struct drm_connector *connector,
+					struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state =
+		drm_atomic_get_new_connector_state(state, connector);
+	struct drm_crtc_state *crtc_state;
+	struct drm_framebuffer *fb;
+	int ret;
+
+	/* We return 0 since this is called while disabling writeback encoder */
+	if (!conn_state->crtc)
+		return 0;
+
+	/* We do not allow a blank commit when using writeback connector */
+	if (!conn_state->writeback_job)
+		return -EINVAL;
+
+	fb = conn_state->writeback_job->fb;
+	if (!fb)
+		return -EINVAL;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (fb->width != crtc_state->mode.hdisplay ||
+	    fb->height != crtc_state->mode.vdisplay)
+		return -EINVAL;
+
+	ret = intel_writeback_check_format(fb->format->format);
+	if (ret) {
+		drm_dbg_kms(connector->dev,
+			    "Unsupported drm format sent in writeback job\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -221,6 +269,7 @@ static const struct drm_connector_helper_funcs conn_helper_funcs = {
 	.get_writeback_connector = intel_get_writeback_connector,
 	.get_modes = intel_writeback_get_modes,
 	.mode_valid = intel_writeback_mode_valid,
+	.atomic_check = intel_writeback_atomic_check,
 	.prepare_writeback_job = intel_writeback_prepare_job,
 	.cleanup_writeback_job = intel_writeback_cleanup_job,
 };
-- 
2.34.1

[PATCH 19/28] drm/i915/writeback: Add the enable sequence from writeback

[Suraj Kandpal]

Add enable sequence for writeback, use encoder->enable hook to
enable the transcoder.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 104 ++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 7fb30cc61991..2db9ae7d810f 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -15,10 +15,13 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 
 #include "i915_drv.h"
+#include "i915_reg.h"
 #include "i915_vma.h"
 #include "intel_atomic.h"
+#include "intel_crtc.h"
 #include "intel_de.h"
 #include "intel_display_types.h"
+#include "intel_display_regs.h"
 #include "intel_display_driver.h"
 #include "intel_connector.h"
 #include "intel_fb_pin.h"
@@ -31,6 +34,7 @@ struct intel_writeback_connector {
 	struct intel_connector connector;
 	struct intel_writeback_job *job;
 	enum transcoder trans;
+	enum pipe pipe;
 	int frame_num;
 };
 
@@ -49,6 +53,12 @@ static const u32 writeback_formats[] = {
 	DRM_FORMAT_XBGR2101010,
 };
 
+static struct intel_writeback_connector
+*enc_to_intel_writeback_connector(struct intel_encoder *encoder)
+{
+	return container_of(encoder, struct intel_writeback_connector, encoder);
+}
+
 static struct intel_writeback_connector
 *conn_to_intel_writeback_connector(struct intel_connector *connector)
 {
@@ -234,6 +244,99 @@ static int intel_writeback_atomic_check(struct drm_connector *connector,
 	return 0;
 }
 
+static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
+					   struct intel_encoder *encoder,
+					   const struct intel_crtc_state *crtc_state,
+					   const struct drm_connector_state *conn_state)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct intel_writeback_connector *wb_conn =
+		enc_to_intel_writeback_connector(encoder);
+	struct intel_writeback_job *job = wb_conn->job;
+	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+	enum transcoder trans = crtc_state->cpu_transcoder;
+	struct intel_crtc *pipe_crtc;
+	struct drm_framebuffer *fb;
+	u32 val = 0, hactive, vactive;
+	int i = 0;
+
+	if (!conn_state->writeback_job)
+		return;
+
+	wb_conn->trans = trans;
+	wb_conn->pipe = crtc->pipe;
+	fb = job->fb;
+	hactive = adjusted_mode->hdisplay;
+	vactive = adjusted_mode->vdisplay;
+
+	/* Configure WD_STRIDE, WD_SURF and WD_TAIL_CFG */
+	/* Enable Planes, Pipes and Transcoder */
+	/* TRANSCODER TIMINGS and other transcoder setting*/
+	/* minimum hactive as per bspec: 64 pixels */
+	if (hactive < 64)
+		drm_err(display->drm, "hactive is less then 64 pixels\n");
+
+	intel_de_write(display, TRANS_HTOTAL(display, trans), HACTIVE(hactive - 1));
+	intel_de_write(display, TRANS_VTOTAL(display, trans), VACTIVE(vactive - 1));
+
+	val = 0;
+	/* 2f) Configure and enable TRANS_WD_FUNC_CTL */
+	switch (crtc->pipe) {
+	default:
+		fallthrough;
+	case PIPE_A:
+		val |= WD_INPUT_PIPE_A;
+		break;
+	case PIPE_B:
+		val |= WD_INPUT_PIPE_B;
+		break;
+	case PIPE_C:
+		val |= WD_INPUT_PIPE_C;
+		break;
+	case PIPE_D:
+		val |= WD_INPUT_PIPE_D;
+		break;
+	}
+
+	switch (fb->format->format) {
+	default:
+		fallthrough;
+	case DRM_FORMAT_YUYV:
+		val |= WD_PIX_FMT_YUYV;
+		break;
+	case DRM_FORMAT_XYUV8888:
+		val |= WD_PIX_FMT_XYUV8888;
+		break;
+	case DRM_FORMAT_XBGR8888:
+		val |= WD_PIX_FMT_XBGR8888;
+		break;
+	case DRM_FORMAT_XBGR2101010:
+		val |= WD_PIX_FMT_XBGR2101010;
+		break;
+	}
+
+	val |= TRANS_WD_FUNC_ENABLE | WD_TRIGGERED_CAP_MODE_ENABLE |
+		WD_DISABLE_POINTERS;
+	intel_de_write(display, WD_TRANS_FUNC_CTL(trans), val);
+
+	if (DISPLAY_VER(display) >= 13)
+		intel_de_rmw(display, PIPE_CHICKEN(crtc->pipe),
+			     UNDERRUN_RECOVERY_DISABLE_ADLP,
+			     UNDERRUN_RECOVERY_DISABLE_ADLP);
+
+	/*  Configure and enable TRANS_CONF */
+	intel_de_write(display, TRANSCONF_WD(trans), WD_TRANS_ENABLE);
+	intel_de_posting_read(display, TRANSCONF_WD(trans));
+
+	for_each_pipe_crtc_modeset_enable(display, pipe_crtc, crtc_state, i) {
+		const struct intel_crtc_state *pipe_crtc_state =
+			intel_atomic_get_new_crtc_state(state, pipe_crtc);
+
+		intel_crtc_vblank_on(pipe_crtc_state);
+	}
+}
+
 static struct drm_writeback_connector *
 intel_get_writeback_connector(struct drm_connector *connector)
 {
@@ -378,6 +481,7 @@ int intel_writeback_init(struct intel_display *display)
 	encoder->get_config = intel_writeback_get_config;
 	encoder->get_hw_state = intel_writeback_get_hw_state;
 	encoder->compute_config = intel_writeback_compute_config;
+	encoder->enable = intel_writeback_enable_encoder;
 
 	connector = &writeback_conn->connector;
 	intel_writeback_connector_alloc(connector);
-- 
2.34.1

[PATCH 20/28] drm/i915/writeback: Add writeback to xe Makefile

[Suraj Kandpal]

Add a intel_writeback.c to xe so that it builds for xe.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/xe/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 76f980475b2c..0551e75b2569 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -299,6 +299,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
 	i915-display/intel_vga.o \
 	i915-display/intel_vrr.o \
 	i915-display/intel_wm.o \
+	i915-display/intel_writeback.o \
 	i915-display/skl_scaler.o \
 	i915-display/skl_universal_plane.o \
 	i915-display/skl_watermark.o
-- 
2.34.1

[PATCH 21/28] drm/i915/writeback: Define writeback frame capture function

[Suraj Kandpal]

Define the commit function to be called at atomic_commit_tail
if drm_writeback_job is available. This function calls the
capture function and queues the job to be called later via
interrupt handler when the job is complete.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  |  3 ++
 .../gpu/drm/i915/display/intel_writeback.c    | 54 +++++++++++++++++++
 .../gpu/drm/i915/display/intel_writeback.h    |  4 ++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c2d1156de8e9..67ed3d6791b8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -125,6 +125,7 @@
 #include "intel_vga.h"
 #include "intel_vrr.h"
 #include "intel_wm.h"
+#include "intel_writeback.h"
 #include "skl_scaler.h"
 #include "skl_universal_plane.h"
 #include "skl_watermark.h"
@@ -7429,6 +7430,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	/* FIXME probably need to sequence this properly */
 	intel_program_dpkgc_latency(state);
 
+	intel_writeback_atomic_commit(state);
+
 	intel_wait_for_vblank_workers(state);
 
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 2db9ae7d810f..25e293cc5be4 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -244,6 +244,60 @@ static int intel_writeback_atomic_check(struct drm_connector *connector,
 	return 0;
 }
 
+static void intel_writeback_capture(struct intel_atomic_state *state,
+				    struct intel_connector *connector)
+{
+	struct intel_display *display = to_intel_display(connector);
+	struct intel_writeback_connector *wb_conn =
+		conn_to_intel_writeback_connector(connector);
+	enum transcoder trans = wb_conn->trans;
+	u32 val = 0;
+
+	val |= START_TRIGGER_FRAME | WD_FRAME_NUMBER(wb_conn->frame_num);
+	intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
+		     START_TRIGGER_FRAME | WD_FRAME_NUMBER_MASK,
+		     val);
+
+	if (intel_de_wait_for_set(display, WD_FRAME_STATUS(trans), WD_FRAME_COMPLETE,
+				  50)) {
+		drm_dbg_kms(display->drm,
+			    "Frame was not captured after triggering a capture\n");
+		intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
+			     STOP_TRIGGER_FRAME,
+			     STOP_TRIGGER_FRAME);
+	} else {
+		drm_writeback_signal_completion(&wb_conn->base, 0);
+		intel_de_write(display, WD_FRAME_STATUS(trans), WD_FRAME_COMPLETE);
+		wb_conn->frame_num++;
+		if (wb_conn->frame_num > 7)
+			wb_conn->frame_num = 1;
+		wb_conn->job = NULL;
+	}
+}
+
+void intel_writeback_atomic_commit(struct intel_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	int i;
+
+	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
+		struct intel_writeback_job *job;
+		struct intel_connector *intel_connector = to_intel_connector(connector);
+
+		if (!conn_state)
+			return;
+
+		if (conn_state->writeback_job && conn_state->writeback_job->fb) {
+			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
+			job = conn_state->writeback_job->priv;
+
+			drm_writeback_queue_job(job->wb_connector, conn_state);
+			intel_writeback_capture(state, intel_connector);
+		}
+	}
+}
+
 static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
 					   struct intel_encoder *encoder,
 					   const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
index 5911684cb81a..3c145cf73e20 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback.h
@@ -8,10 +8,14 @@
 
 #include <linux/types.h>
 
+#include "intel_display_types.h"
+
+struct intel_atomic_state;
 struct intel_display;
 struct intel_writeback_connector;
 
 int intel_writeback_init(struct intel_display *display);
+void intel_writeback_atomic_commit(struct intel_atomic_state *state);
 
 #endif /* __INTEL_WRITEBACK_H__ */
 
-- 
2.34.1

[PATCH 22/28] drm/i915/writeback: Configure WD_STRIDE reg

[Suraj Kandpal]

Write to the WD_STRIDE register using the appropriate calculation
based on the color mode and hactive.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 34 +++++++++++++++++++
 .../drm/i915/display/intel_writeback_reg.h    |  1 +
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 25e293cc5be4..c6c05bd3d298 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -244,15 +244,49 @@ static int intel_writeback_atomic_check(struct drm_connector *connector,
 	return 0;
 }
 
+static int
+get_color_mode_bpp(struct intel_display *display, u32 color_format)
+{
+	int bpp = 0;
+
+	switch (color_format) {
+	case DRM_FORMAT_XYUV8888:
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_XVYU2101010:
+		bpp = 4;
+		break;
+	default:
+		drm_err(display->drm, "Unsupported format for writeback\n");
+		break;
+	}
+
+	return bpp;
+}
+
 static void intel_writeback_capture(struct intel_atomic_state *state,
 				    struct intel_connector *connector)
 {
 	struct intel_display *display = to_intel_display(connector);
 	struct intel_writeback_connector *wb_conn =
 		conn_to_intel_writeback_connector(connector);
+	struct intel_crtc *crtc = intel_crtc_for_pipe(display, wb_conn->pipe);
+	struct intel_crtc_state *crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	const struct drm_display_mode *adjusted_mode =
+		&crtc_state->hw.adjusted_mode;
+	struct intel_writeback_job *wb_job = wb_conn->job;
 	enum transcoder trans = wb_conn->trans;
 	u32 val = 0;
+	int bpp;
 
+	bpp = get_color_mode_bpp(display, wb_job->fb->format->format);
+	val = DIV_ROUND_UP((adjusted_mode->hdisplay * bpp), 64);
+	intel_de_write(display, WD_STRIDE(trans), WD_STRIDE_VAL(val));
+
+	val = 0;
 	val |= START_TRIGGER_FRAME | WD_FRAME_NUMBER(wb_conn->frame_num);
 	intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
 		     START_TRIGGER_FRAME | WD_FRAME_NUMBER_MASK,
diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
index 5e7c6c99d191..f526af0f9aff 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback_reg.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
@@ -60,6 +60,7 @@
 					_WD_STRIDE_0,\
 					_WD_STRIDE_1)
 #define WD_STRIDE_MASK			REG_GENMASK(15, 6)
+#define WD_STRIDE_VAL(val)		REG_FIELD_PREP(WD_STRIDE_MASK, val)
 
 #define _WD_STREAMCAP_CTL0		0x6e590
 #define _WD_STREAMCAP_CTL1		0x6ed90
-- 
2.34.1

[PATCH 23/28] drm/i915/writeback: Configure WD_SURF register

[Suraj Kandpal]

Get the ggtt_offset of the drm_framebuffer which needs to be
written to the surface base address bits of WD_SURF register.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_writeback.c     | 3 +++
 drivers/gpu/drm/i915/display/intel_writeback_reg.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index c6c05bd3d298..0f26134beacd 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -286,6 +286,9 @@ static void intel_writeback_capture(struct intel_atomic_state *state,
 	val = DIV_ROUND_UP((adjusted_mode->hdisplay * bpp), 64);
 	intel_de_write(display, WD_STRIDE(trans), WD_STRIDE_VAL(val));
 
+	val = i915_ggtt_offset(wb_job->vma);
+	intel_de_write(display, WD_SURF(trans), val);
+
 	val = 0;
 	val |= START_TRIGGER_FRAME | WD_FRAME_NUMBER(wb_conn->frame_num);
 	intel_de_rmw(display, WD_TRANS_FUNC_CTL(trans),
diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
index f526af0f9aff..403f9b64015b 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback_reg.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
@@ -81,6 +81,8 @@
 #define WD_SURF(tc)			_MMIO_WD(tc,\
 					_WD_SURF_0,\
 					_WD_SURF_1)
+#define  WD_SURF_ADDR_MASK		REG_GENMASK(31, 12)
+#define  WD_SURF_ADDR(val)		REG_FIELD_PREP(WD_SURF_ADDR_MASK, val)
 
 #define _WD_IMR_0			0x6e560
 #define _WD_IMR_1			0x6ed60
-- 
2.34.1

[PATCH 24/28] drm/i915/writeback: Enable writeback interrupts

[Suraj Kandpal]

Enable writeback interrupts while enabling writeback
and define the isr handler and schedule work for later
to signal completion job.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_display_irq.c  | 10 ++++
 .../gpu/drm/i915/display/intel_display_regs.h |  1 +
 .../gpu/drm/i915/display/intel_writeback.c    | 51 +++++++++++++++++++
 .../gpu/drm/i915/display/intel_writeback.h    |  1 +
 4 files changed, 63 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
index fb25ec8adae3..0874afe839b2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
@@ -31,6 +31,8 @@
 #include "intel_psr.h"
 #include "intel_psr_regs.h"
 #include "intel_uncore.h"
+#include "intel_writeback.h"
+#include "intel_writeback_reg.h"
 
 static void
 intel_display_irq_regs_init(struct intel_display *display, struct i915_irq_regs regs,
@@ -1215,6 +1217,11 @@ gen8_de_misc_irq_handler(struct intel_display *display, u32 iir)
 		found = true;
 	}
 
+	if (iir & (GEN8_DE_MISC_WD0)) {
+		intel_writeback_isr_handler(display);
+		found = true;
+	}
+
 	if (iir & GEN8_DE_EDP_PSR) {
 		struct intel_encoder *encoder;
 		u32 psr_iir;
@@ -2251,6 +2258,9 @@ void gen8_de_irq_postinstall(struct intel_display *display)
 	if (DISPLAY_VER(display) < 11)
 		de_misc_masked |= GEN8_DE_MISC_GSE;
 
+	if (DISPLAY_VER(display) >= 13)
+		de_misc_masked |= GEN8_DE_MISC_WD0;
+
 	if (display->platform.geminilake || display->platform.broxton)
 		de_port_masked |= BXT_DE_PORT_GMBUS;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
index 7bd09d981cd2..fb748ae0634f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
@@ -1325,6 +1325,7 @@
 #define  XELPDP_RM_TIMEOUT		REG_BIT(29)
 #define  XELPDP_PMDEMAND_RSPTOUT_ERR	REG_BIT(27)
 #define  GEN8_DE_MISC_GSE		REG_BIT(27)
+#define  GEN8_DE_MISC_WD0		REG_BIT(23)
 #define  GEN8_DE_EDP_PSR		REG_BIT(19)
 #define  XELPDP_PMDEMAND_RSP		REG_BIT(3)
 #define  XE2LPD_DBUF_OVERLAP_DETECTED	REG_BIT(1)
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index 0f26134beacd..d66843fecd9a 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -13,6 +13,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_vblank.h>
 
 #include "i915_drv.h"
 #include "i915_reg.h"
@@ -23,6 +24,7 @@
 #include "intel_display_types.h"
 #include "intel_display_regs.h"
 #include "intel_display_driver.h"
+#include "intel_display_regs.h"
 #include "intel_connector.h"
 #include "intel_fb_pin.h"
 #include "intel_writeback.h"
@@ -335,6 +337,20 @@ void intel_writeback_atomic_commit(struct intel_atomic_state *state)
 	}
 }
 
+static void
+intel_writeback_enable_interrupts(struct intel_display *display,
+				  enum transcoder trans)
+{
+	u32 tmp;
+
+	tmp = intel_de_read(display, WD_IIR(trans));
+	intel_de_write_fw(display, WD_IIR(trans), tmp);
+
+	tmp = ~(WD_GTT_FAULT_INT | WD_WRITE_COMPLETE_INT |
+		WD_VBLANK_INT | WD_CAPTURING_INT);
+	intel_de_write(display, WD_IMR(trans), tmp);
+}
+
 static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
 					   struct intel_encoder *encoder,
 					   const struct intel_crtc_state *crtc_state,
@@ -360,6 +376,7 @@ static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
 	fb = job->fb;
 	hactive = adjusted_mode->hdisplay;
 	vactive = adjusted_mode->vdisplay;
+	intel_writeback_enable_interrupts(display, trans);
 
 	/* Configure WD_STRIDE, WD_SURF and WD_TAIL_CFG */
 	/* Enable Planes, Pipes and Transcoder */
@@ -545,6 +562,40 @@ intel_writeback_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+void intel_writeback_isr_handler(struct intel_display *display)
+{
+	struct intel_encoder *encoder;
+	struct intel_writeback_connector *wb_conn;
+	struct intel_crtc *crtc;
+	u32 iir;
+
+	for_each_intel_encoder(display->drm, encoder) {
+		if (encoder->type != INTEL_OUTPUT_WRITEBACK)
+			continue;
+
+		wb_conn = enc_to_intel_writeback_connector(encoder);
+		if (!wb_conn->job) {
+			drm_err(display->drm, "No writeback job for the connector\n");
+			continue;
+		}
+
+		crtc = intel_crtc_for_pipe(display, wb_conn->pipe);
+		iir = intel_de_read(display, WD_IIR(wb_conn->trans));
+		if (iir & WD_GTT_FAULT_INT)
+			drm_err(display->drm, " GTT fault during writeback\n");
+		if (iir & WD_WRITE_COMPLETE_INT)
+			drm_dbg_kms(display->drm, "Writeback job write completed\n");
+		if (iir & WD_VBLANK_INT) {
+			drm_crtc_handle_vblank(&crtc->base);
+			drm_dbg_kms(display->drm, "Writeback vblank raised\n");
+		}
+		if (iir & WD_CAPTURING_INT)
+			drm_dbg_kms(display->drm, "Writeback job capture has started\n");
+
+		intel_de_write(display, WD_IIR(wb_conn->trans), iir);
+	}
+}
+
 int intel_writeback_init(struct intel_display *display)
 {
 	struct intel_encoder *encoder;
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
index 3c145cf73e20..83a986753c4c 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback.h
@@ -16,6 +16,7 @@ struct intel_writeback_connector;
 
 int intel_writeback_init(struct intel_display *display);
 void intel_writeback_atomic_commit(struct intel_atomic_state *state);
+void intel_writeback_isr_handler(struct intel_display *display);
 
 #endif /* __INTEL_WRITEBACK_H__ */
 
-- 
2.34.1

[PATCH 25/28] drm/i915/writeback: Initialize writeback encoder.

[Suraj Kandpal]

Call the init function to initialize the writeback encoder
only for ADLP.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 67ed3d6791b8..dbde689713fe 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7895,6 +7895,9 @@ void intel_setup_outputs(struct intel_display *display)
 		intel_dvo_init(display);
 	}
 
+	if (DISPLAY_VER(display) == 13)
+		intel_writeback_init(display);
+
 	for_each_intel_encoder(display->drm, encoder) {
 		encoder->base.possible_crtcs =
 			intel_encoder_possible_crtcs(encoder);
-- 
2.34.1

[PATCH 26/28] drm/i915/writeback: Define the disable sequence for writeback

[Suraj Kandpal]

Define the disable sequence for a writeback encoder. We only disable
the encoder if no writeback job is pending, if it is then we just
need to disable the wd function so that values can be updated
accordingly.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/i915/display/intel_writeback.c    | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index d66843fecd9a..f8e940d91cb7 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -596,6 +596,33 @@ void intel_writeback_isr_handler(struct intel_display *display)
 	}
 }
 
+static void
+intel_writeback_disable_encoder(struct intel_atomic_state *state,
+				struct intel_encoder *encoder,
+				const struct intel_crtc_state *crtc_state,
+				const struct drm_connector_state *conn_state)
+{
+	struct intel_display *display = to_intel_display(encoder);
+	struct intel_writeback_connector *wb_conn =
+		enc_to_intel_writeback_connector(encoder);
+	struct intel_crtc *pipe_crtc;
+	int i = 0;
+
+	for_each_pipe_crtc_modeset_disable(display, pipe_crtc, crtc_state, i) {
+		const struct intel_crtc_state *old_pipe_crtc_state =
+			intel_atomic_get_old_crtc_state(state, pipe_crtc);
+
+		intel_crtc_vblank_off(old_pipe_crtc_state);
+	}
+
+	intel_de_rmw(display, TRANSCONF_WD(crtc_state->cpu_transcoder), WD_TRANS_ENABLE,
+		     REG_FIELD_PREP(WD_TRANS_ENABLE, 0));
+	intel_de_rmw(display, WD_TRANS_FUNC_CTL(crtc_state->cpu_transcoder),
+		     TRANS_WD_FUNC_ENABLE,
+		     REG_FIELD_PREP(TRANS_WD_FUNC_ENABLE, 0));
+	wb_conn->frame_num = 1;
+}
+
 int intel_writeback_init(struct intel_display *display)
 {
 	struct intel_encoder *encoder;
@@ -624,6 +651,7 @@ int intel_writeback_init(struct intel_display *display)
 	encoder->get_hw_state = intel_writeback_get_hw_state;
 	encoder->compute_config = intel_writeback_compute_config;
 	encoder->enable = intel_writeback_enable_encoder;
+	encoder->disable = intel_writeback_disable_encoder;
 
 	connector = &writeback_conn->connector;
 	intel_writeback_connector_alloc(connector);
-- 
2.34.1

[PATCH 27/28] drm/i915/writeback: Make exception for writeback connector

[Suraj Kandpal]

Writeback connector is a special connector as it does not actually
exist. This means a lot of checks and computations need to be skipped
and exceptions need to be made when it comes to this connector.
This commit contains all those changes for a writeback connector.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 109 +++++++++++++-----
 .../drm/i915/display/intel_display_debugfs.c  |   3 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   3 +
 drivers/gpu/drm/i915/display/intel_opregion.c |   2 +-
 drivers/gpu/drm/i915/display/intel_pmdemand.c |   3 +
 drivers/gpu/drm/i915/display/intel_vdsc.c     |   4 +
 .../gpu/drm/i915/display/intel_writeback.c    |   5 +
 .../gpu/drm/i915/display/intel_writeback.h    |   1 +
 8 files changed, 103 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dbde689713fe..953d14ac342d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -126,6 +126,7 @@
 #include "intel_vrr.h"
 #include "intel_wm.h"
 #include "intel_writeback.h"
+#include "intel_writeback_reg.h"
 #include "skl_scaler.h"
 #include "skl_universal_plane.h"
 #include "skl_watermark.h"
@@ -1637,6 +1638,10 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
 	}
 
 	intel_set_transcoder_timings(crtc_state);
+
+	if (intel_writeback_transcoder_is_wd(cpu_transcoder))
+		return;
+
 	if (HAS_VRR(display))
 		intel_vrr_set_transcoder_timings(crtc_state);
 
@@ -2652,6 +2657,10 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 	crtc_vblank_start = adjusted_mode->crtc_vblank_start;
 	crtc_vblank_end = adjusted_mode->crtc_vblank_end;
 
+	if (intel_writeback_transcoder_is_wd(cpu_transcoder)) {
+		return;
+	}
+
 	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
 		/* the chip adds 2 halflines automatically */
 		crtc_vtotal -= 1;
@@ -2840,6 +2849,15 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
 	struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
 	u32 tmp;
 
+	if (intel_writeback_transcoder_is_wd(cpu_transcoder)) {
+		tmp = intel_de_read(display, TRANS_HTOTAL(display, cpu_transcoder));
+		adjusted_mode->crtc_hdisplay = REG_FIELD_GET(HACTIVE_MASK, tmp) + 1;
+
+		tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
+		adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
+		return;
+	}
+
 	tmp = intel_de_read(display, TRANS_HTOTAL(display, cpu_transcoder));
 	adjusted_mode->crtc_hdisplay = REG_FIELD_GET(HACTIVE_MASK, tmp) + 1;
 	adjusted_mode->crtc_htotal = REG_FIELD_GET(HTOTAL_MASK, tmp) + 1;
@@ -3743,32 +3761,59 @@ static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
 		u32 tmp = 0;
 
 		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
-		with_intel_display_power_if_enabled(display, power_domain, wakeref)
-			tmp = intel_de_read(display,
-					    TRANS_DDI_FUNC_CTL(display, cpu_transcoder));
+		if (cpu_transcoder == TRANSCODER_WD_0 ||
+		    cpu_transcoder == TRANSCODER_WD_1) {
+			with_intel_display_power_if_enabled(display, power_domain, wakeref)
+				tmp = intel_de_read(display,
+						    WD_TRANS_FUNC_CTL(cpu_transcoder));
+			if (!(tmp & TRANS_WD_FUNC_ENABLE))
+				continue;
 
-		if (!(tmp & TRANS_DDI_FUNC_ENABLE))
-			continue;
+			switch (tmp & WD_INPUT_SELECT_MASK) {
+			case WD_INPUT_PIPE_A:
+				trans_pipe = PIPE_A;
+				break;
+			case WD_INPUT_PIPE_B:
+				trans_pipe = PIPE_B;
+				break;
+			case WD_INPUT_PIPE_C:
+				trans_pipe = PIPE_C;
+				break;
+			case WD_INPUT_PIPE_D:
+				trans_pipe = PIPE_D;
+				break;
+			default:
+				MISSING_CASE(tmp & WD_INPUT_SELECT_MASK);
+				break;
+			}
+		} else {
+			with_intel_display_power_if_enabled(display, power_domain, wakeref)
+				tmp = intel_de_read(display,
+						    TRANS_DDI_FUNC_CTL(display, cpu_transcoder));
 
-		switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
-		default:
-			drm_WARN(display->drm, 1,
-				 "unknown pipe linked to transcoder %s\n",
-				 transcoder_name(cpu_transcoder));
-			fallthrough;
-		case TRANS_DDI_EDP_INPUT_A_ONOFF:
-		case TRANS_DDI_EDP_INPUT_A_ON:
-			trans_pipe = PIPE_A;
-			break;
-		case TRANS_DDI_EDP_INPUT_B_ONOFF:
-			trans_pipe = PIPE_B;
-			break;
-		case TRANS_DDI_EDP_INPUT_C_ONOFF:
-			trans_pipe = PIPE_C;
-			break;
-		case TRANS_DDI_EDP_INPUT_D_ONOFF:
-			trans_pipe = PIPE_D;
-			break;
+			if (!(tmp & TRANS_DDI_FUNC_ENABLE))
+				continue;
+
+			switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
+			default:
+				drm_WARN(display->drm, 1,
+					 "unknown pipe linked to transcoder %s\n",
+					 transcoder_name(cpu_transcoder));
+				fallthrough;
+			case TRANS_DDI_EDP_INPUT_A_ONOFF:
+			case TRANS_DDI_EDP_INPUT_A_ON:
+				trans_pipe = PIPE_A;
+				break;
+			case TRANS_DDI_EDP_INPUT_B_ONOFF:
+				trans_pipe = PIPE_B;
+				break;
+			case TRANS_DDI_EDP_INPUT_C_ONOFF:
+				trans_pipe = PIPE_C;
+				break;
+			case TRANS_DDI_EDP_INPUT_D_ONOFF:
+				trans_pipe = PIPE_D;
+				break;
+			}
 		}
 
 		if (trans_pipe == crtc->pipe)
@@ -3857,6 +3902,13 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
 			pipe_config->pch_pfit.force_thru = true;
 	}
 
+	if (intel_writeback_transcoder_is_wd(pipe_config->cpu_transcoder)) {
+		tmp = intel_de_read(display,
+				    TRANSCONF_WD(pipe_config->cpu_transcoder));
+
+		return tmp & WD_TRANS_ENABLE;
+	}
+
 	tmp = intel_de_read(display,
 			    TRANSCONF(display, pipe_config->cpu_transcoder));
 
@@ -3952,7 +4004,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 	    DISPLAY_VER(display) >= 11)
 		intel_get_transcoder_timings(crtc, pipe_config);
 
-	if (transcoder_has_vrr(pipe_config))
+	if (!intel_writeback_transcoder_is_wd(pipe_config->cpu_transcoder) &&
+	    transcoder_has_vrr(pipe_config))
 		intel_vrr_get_config(pipe_config);
 
 	intel_get_pipe_src_size(crtc, pipe_config);
@@ -3965,6 +4018,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 			pipe_config->output_format = INTEL_OUTPUT_FORMAT_YCBCR444;
 		else
 			pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
+	} else if (intel_writeback_transcoder_is_wd(pipe_config->cpu_transcoder)) {
+		pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
 	} else {
 		pipe_config->output_format =
 			bdw_get_pipe_misc_output_format(crtc);
@@ -3991,6 +4046,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 	hsw_ips_get_config(pipe_config);
 
 	if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
+	    !intel_writeback_transcoder_is_wd(pipe_config->cpu_transcoder) &&
 	    !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
 		pipe_config->pixel_multiplier =
 			intel_de_read(display,
@@ -3999,7 +4055,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier = 1;
 	}
 
-	if (!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
+	if (!intel_writeback_transcoder_is_wd(pipe_config->cpu_transcoder) &&
+	    !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
 		tmp = intel_de_read(display, CHICKEN_TRANS(display, pipe_config->cpu_transcoder));
 
 		pipe_config->framestart_delay = REG_FIELD_GET(HSW_FRAME_START_DELAY_MASK, tmp) + 1;
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index ce3f9810c42d..8d680a69f773 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -203,6 +203,9 @@ static void intel_panel_info(struct seq_file *m,
 {
 	const struct drm_display_mode *fixed_mode;
 
+	if (connector->base.connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+		return;
+
 	if (list_empty(&connector->panel.fixed_modes))
 		return;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 33e0398120c8..390edbe90543 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -4381,6 +4381,9 @@ int intel_dpll_compute(struct intel_atomic_state *state,
 	struct intel_display *display = to_intel_display(state);
 	const struct intel_dpll_mgr *dpll_mgr = display->dpll.mgr;
 
+	if (encoder->type == INTEL_OUTPUT_WRITEBACK)
+		return 0;
+
 	if (drm_WARN_ON(display->drm, !dpll_mgr))
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 81efdb17fc0c..cee7d5ed230c 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -395,7 +395,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *encoder,
 	int ret;
 
 	/* don't care about old stuff for now */
-	if (!HAS_DDI(display))
+	if (!HAS_DDI(display) || encoder->type == INTEL_OUTPUT_WRITEBACK)
 		return 0;
 
 	/* Avoid port out of bounds checks if SWSCI isn't there. */
diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
index d806c15db7ce..add898fe2ac1 100644
--- a/drivers/gpu/drm/i915/display/intel_pmdemand.c
+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
@@ -155,6 +155,9 @@ intel_pmdemand_update_phys_mask(struct intel_display *display,
 	if (!encoder)
 		return;
 
+	if (encoder->type == INTEL_OUTPUT_WRITEBACK)
+		return;
+
 	if (intel_encoder_is_tc(encoder))
 		return;
 
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 8e799e225af1..3567e29f1cf9 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -20,6 +20,7 @@
 #include "intel_qp_tables.h"
 #include "intel_vdsc.h"
 #include "intel_vdsc_regs.h"
+#include "intel_writeback.h"
 
 bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state)
 {
@@ -989,6 +990,9 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
 	if (!intel_dsc_source_support(crtc_state))
 		return;
 
+	if (intel_writeback_transcoder_is_wd(cpu_transcoder))
+		return;
+
 	power_domain = intel_dsc_power_domain(crtc, cpu_transcoder);
 
 	wakeref = intel_display_power_get_if_enabled(display, power_domain);
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
index f8e940d91cb7..f4025a4518e4 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.c
+++ b/drivers/gpu/drm/i915/display/intel_writeback.c
@@ -55,6 +55,11 @@ static const u32 writeback_formats[] = {
 	DRM_FORMAT_XBGR2101010,
 };
 
+bool intel_writeback_transcoder_is_wd(enum transcoder transcoder)
+{
+	return transcoder == TRANSCODER_WD_0 || transcoder == TRANSCODER_WD_1;
+}
+
 static struct intel_writeback_connector
 *enc_to_intel_writeback_connector(struct intel_encoder *encoder)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
index 83a986753c4c..3a99a6526841 100644
--- a/drivers/gpu/drm/i915/display/intel_writeback.h
+++ b/drivers/gpu/drm/i915/display/intel_writeback.h
@@ -17,6 +17,7 @@ struct intel_writeback_connector;
 int intel_writeback_init(struct intel_display *display);
 void intel_writeback_atomic_commit(struct intel_atomic_state *state);
 void intel_writeback_isr_handler(struct intel_display *display);
+bool intel_writeback_transcoder_is_wd(enum transcoder transcoder);
 
 #endif /* __INTEL_WRITEBACK_H__ */
 
-- 
2.34.1

[PATCH 28/28] drm/i915/writeback: Modify state verify function

[Suraj Kandpal]

Modify the state verify functions to take into account the fact
that writeback does not need all the timings for it to be set.
Moreover there is no need for dpll state nor do we need to set
any sort of flags for it.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 60 ++++++++++++--------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 953d14ac342d..9be05995b268 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5064,6 +5064,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	struct drm_printer p;
 	u32 exclude_infoframes = 0;
 	bool ret = true;
+	bool is_writeback =
+		intel_crtc_has_type(current_config, INTEL_OUTPUT_WRITEBACK);
 
 	if (fastset)
 		p = drm_dbg_printer(display->drm, DRM_UT_KMS, NULL);
@@ -5179,20 +5181,25 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 } while (0)
 
 #define PIPE_CONF_CHECK_TIMINGS(name) do {     \
-	PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
-	PIPE_CONF_CHECK_I(name.crtc_htotal); \
-	PIPE_CONF_CHECK_I(name.crtc_hblank_start); \
-	PIPE_CONF_CHECK_I(name.crtc_hblank_end); \
-	PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
-	PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
-	PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
-	if (!fastset || !allow_vblank_delay_fastset(current_config)) \
-		PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
-	PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
-	PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
-	if (!fastset || !pipe_config->update_lrr) { \
-		PIPE_CONF_CHECK_I(name.crtc_vtotal); \
-		PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
+	if (is_writeback) { \
+		PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
+		PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
+	} else { \
+		PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
+		PIPE_CONF_CHECK_I(name.crtc_htotal); \
+		PIPE_CONF_CHECK_I(name.crtc_hblank_start); \
+		PIPE_CONF_CHECK_I(name.crtc_hblank_end); \
+		PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
+		PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
+		PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
+		if (!fastset || !allow_vblank_delay_fastset(current_config)) \
+			PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
+		PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
+		PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
+		if (!fastset || !pipe_config->update_lrr) { \
+			PIPE_CONF_CHECK_I(name.crtc_vtotal); \
+			PIPE_CONF_CHECK_I(name.crtc_vblank_end); \
+		} \
 	} \
 } while (0)
 
@@ -5321,10 +5328,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 
 	PIPE_CONF_CHECK_I(pixel_multiplier);
 
-	PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
-			      DRM_MODE_FLAG_INTERLACE);
+	if (!is_writeback)
+		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
+				      DRM_MODE_FLAG_INTERLACE);
 
-	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) {
+	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS) && !is_writeback) {
 		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
 				      DRM_MODE_FLAG_PHSYNC);
 		PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
@@ -5371,7 +5379,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 		PIPE_CONF_CHECK_RECT(pch_pfit.dst);
 
 		PIPE_CONF_CHECK_I(scaler_state.scaler_id);
-		PIPE_CONF_CHECK_I(pixel_rate);
+		if (!is_writeback)
+			PIPE_CONF_CHECK_I(pixel_rate);
 
 		PIPE_CONF_CHECK_X(gamma_mode);
 		if (display->platform.cherryview)
@@ -5394,28 +5403,31 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 
 	PIPE_CONF_CHECK_BOOL(double_wide);
 
-	if (display->dpll.mgr)
+	if (display->dpll.mgr && !is_writeback)
 		PIPE_CONF_CHECK_P(intel_dpll);
 
 	/* FIXME convert everything over the dpll_mgr */
-	if (display->dpll.mgr || HAS_GMCH(display))
+	if ((display->dpll.mgr || HAS_GMCH(display)) && !is_writeback)
 		PIPE_CONF_CHECK_PLL(dpll_hw_state);
 
 	/* FIXME convert MTL+ platforms over to dpll_mgr */
-	if (DISPLAY_VER(display) >= 14)
+	if (DISPLAY_VER(display) >= 14 && !is_writeback)
 		PIPE_CONF_CHECK_PLL_CX0(dpll_hw_state.cx0pll);
 
 	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
 	PIPE_CONF_CHECK_X(dsi_pll.div);
 
-	if (display->platform.g4x || DISPLAY_VER(display) >= 5)
+	if ((display->platform.g4x || DISPLAY_VER(display) >= 5) &&
+	    !is_writeback)
 		PIPE_CONF_CHECK_I(pipe_bpp);
 
-	if (!fastset || !pipe_config->update_m_n) {
+	if ((!fastset || !pipe_config->update_m_n) && !is_writeback) {
 		PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_clock);
 		PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_clock);
 	}
-	PIPE_CONF_CHECK_I(port_clock);
+
+	if (!is_writeback)
+		PIPE_CONF_CHECK_I(port_clock);
 
 	PIPE_CONF_CHECK_I(min_voltage_level);
 
-- 
2.34.1

Re: [PATCH 11/28] drm/i915/writeback: Define encoder->get_hw_state

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v6.16-rc7 next-20250725]
[cannot apply to linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-writeback-Add-function-that-takes-preallocated-connector/20250725-133017
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20250725050409.2687242-12-suraj.kandpal%40intel.com
patch subject: [PATCH 11/28] drm/i915/writeback: Define encoder->get_hw_state
config: i386-buildonly-randconfig-006-20250725 (https://download.01.org/0day-ci/archive/20250726/202507261936.cnkpq7bw-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250726/202507261936.cnkpq7bw-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/202507261936.cnkpq7bw-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_writeback.c:177:3: error: fallthrough annotation does not directly precede switch label
     177 |                 fallthrough;
         |                 ^
   include/linux/compiler_attributes.h:214:41: note: expanded from macro 'fallthrough'
     214 | # define fallthrough                    __attribute__((__fallthrough__))
         |                                         ^
   1 error generated.


vim +177 drivers/gpu/drm/i915/display/intel_writeback.c

   141	
   142	static bool
   143	intel_writeback_get_hw_state(struct intel_encoder *encoder,
   144				     enum pipe *pipe)
   145	{
   146		struct intel_display *display = to_intel_display(encoder);
   147		u8 pipe_mask = 0;
   148		u32 tmp;
   149	
   150		/* TODO need to be done for both the wd transcoder */
   151		tmp = intel_de_read(display,
   152				    TRANSCONF_WD(TRANSCODER_WD_0));
   153		if (!(tmp & WD_TRANS_ENABLE))
   154			return false;
   155	
   156		tmp = intel_de_read(display,
   157				    WD_TRANS_FUNC_CTL(TRANSCODER_WD_0));
   158	
   159		if (!(tmp & TRANS_WD_FUNC_ENABLE))
   160			return false;
   161	
   162		switch (tmp & WD_INPUT_SELECT_MASK) {
   163		case WD_INPUT_PIPE_A:
   164			pipe_mask |= BIT(PIPE_A);
   165			break;
   166		case WD_INPUT_PIPE_B:
   167			pipe_mask |= BIT(PIPE_B);
   168			break;
   169		case WD_INPUT_PIPE_C:
   170			pipe_mask |= BIT(PIPE_C);
   171			break;
   172		case WD_INPUT_PIPE_D:
   173			pipe_mask |= BIT(PIPE_D);
   174			break;
   175		default:
   176			MISSING_CASE(tmp & WD_INPUT_SELECT_MASK);
 > 177			fallthrough;
   178		}
   179	
   180		if (pipe_mask == 0)
   181			return false;
   182	
   183		*pipe = ffs(pipe_mask) - 1;
   184	
   185		return true;
   186	}
   187	

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

Re: [PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:42AM +0530, Suraj Kandpal wrote:
> Write a function that takes a preallocated drm_connector instead of
> using the one allocated inside the drm writeback connector init
> function.

Please start your commit message with describing the problem.

> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 76 +++++++++++++++++++++++++++++++++
>  include/drm/drm_writeback.h     |  7 +++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 95b8a2e4bda6..fa58eb0dc7bf 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drmm_writeback_connector_init);
>  
> +/*
> + * drm_writeback_connector_init_with_conn - Initialize a writeback connector with
> + * custom encoder and connector
> + *
> + * @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 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.

Why?

> + *
> + * 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_conn(struct drm_device *dev, struct drm_connector *connector,
> +				       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_mode_config *config = &dev->mode_config;
> +	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;

This function contans a lot of copy-paste from
__drm_writeback_connector_init(), which is obviously a no-go.

> +
> +	ret = drm_connector_init(dev, connector, con_funcs,
> +				 DRM_MODE_CONNECTOR_WRITEBACK);
> +	if (ret)
> +		goto connector_fail;
> +
> +	INIT_LIST_HEAD(&wb_connector->job_queue);
> +	spin_lock_init(&wb_connector->job_lock);
> +
> +	wb_connector->fence_context = dma_fence_context_alloc(1);
> +	spin_lock_init(&wb_connector->fence_lock);
> +	snprintf(wb_connector->timeline_name,
> +		 sizeof(wb_connector->timeline_name),
> +		 "CONNECTOR:%d-%s", connector->base.id, connector->name);
> +
> +	drm_object_attach_property(&connector->base,
> +				   config->writeback_out_fence_ptr_property, 0);
> +
> +	drm_object_attach_property(&connector->base,
> +				   config->writeback_fb_id_property, 0);
> +
> +	drm_object_attach_property(&connector->base,
> +				   config->writeback_pixel_formats_property,
> +				   blob->base.id);
> +	wb_connector->pixel_formats_blob_ptr = blob;
> +
> +	return 0;
> +
> +connector_fail:
> +	drm_property_blob_put(blob);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_writeback_connector_init_with_conn);
> +
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  			 struct drm_framebuffer *fb)
>  {
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index c380a7b8f55a..149744dbeef0 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -167,6 +167,13 @@ int drmm_writeback_connector_init(struct drm_device *dev,
>  				  struct drm_encoder *enc,
>  				  const u32 *formats, int n_formats);
>  
> +int
> +drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connector *connector,
> +				       struct drm_writeback_connector *wb_connector,
> +				       struct drm_encoder *enc,
> +				       const struct drm_connector_funcs *con_funcs,
> +				       const u32 *formats, int n_formats);
> +
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  			 struct drm_framebuffer *fb);
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 02/28] drm/writeback: Add a helper function to get writeback connector

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:43AM +0530, Suraj Kandpal wrote:
> Now that we can initialize a drm_writeback_connector without
> having to initialize the drm_connector within it and leaving the
> responsibility of initialising the drm_connector and maintaining
> the association with drm_writeback_connector to it. This helper
> hooks lets drivers return the drm_writeback_connector associated
> with the give drm_connector.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c          | 14 ++++++
>  include/drm/drm_modeset_helper_vtables.h | 59 ++++++++++++++++++++++++
>  include/drm/drm_writeback.h              | 14 ++++--
>  3 files changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index fa58eb0dc7bf..e9f7123270d6 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -107,6 +107,19 @@ static const struct dma_fence_ops drm_writeback_fence_ops = {
>  	.get_timeline_name = drm_writeback_fence_get_timeline_name,
>  };
>  
> +struct drm_writeback_connector *
> +drm_connector_to_writeback(struct drm_connector *connector)
> +{
> +	const struct drm_connector_helper_funcs *funcs =
> +		connector->helper_private;
> +
> +	if (funcs->get_writeback_connector)
> +		return funcs->get_writeback_connector(connector);
> +
> +	return container_of(connector, struct drm_writeback_connector, base);
> +}
> +EXPORT_SYMBOL(drm_connector_to_writeback);
> +
>  static int create_writeback_properties(struct drm_device *dev)
>  {
>  	struct drm_property *prop;
> @@ -443,6 +456,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
>  				       struct drm_writeback_connector *wb_connector,
>  				       struct drm_encoder *enc,
>  				       const struct drm_connector_funcs *con_funcs,
> +				       const struct drm_writeback_connector_helper_funcs *wb_funcs,
>  				       const u32 *formats, int n_formats)
>  {
>  	struct drm_property_blob *blob;
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index ce7c7aeac887..6b89b33d2304 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -31,6 +31,7 @@
>  
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
>  
>  /**
>   * DOC: overview
> @@ -1179,6 +1180,25 @@ struct drm_connector_helper_funcs {
>  	 *
>  	 */
>  	void (*disable_hpd)(struct drm_connector *connector);
> +
> +	/**
> +	 * @get_writeback_connector:
> +	 *
> +	 * This callback is used by drivers to get the writeback connector in
> +	 * case the init is done via drm_writeback_init_with_conn. Which means
> +	 * the drivers don't have drm_connector embedded in drm_writeback_connector
> +	 * so they need to send the associated writeback connector with this
> +	 * function.
> +	 *
> +	 * This operation is optional.
> +	 *
> +	 * This is mainly called from drm_writeback_set_gb.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * drm_writeback_connector assoiciated with the drm connector.
> +	 */
> +	struct drm_writeback_connector *(*get_writeback_connector)(struct drm_connector *connector);
>  };
>  
>  /**
> @@ -1192,6 +1212,45 @@ static inline void drm_connector_helper_add(struct drm_connector *connector,
>  	connector->helper_private = funcs;
>  }
>  
> +/**
> + * struct drm_writeback_connector_helper_funcs - helper operations for writeback
> + * connectors.
> + *
> + * These functions are used by the atomic and legacy modeset helpers and by the
> + * probe helpers.
> + */
> +struct drm_writeback_connector_helper_funcs {
> +	/**
> +	 * @get_connector_from_writeback:
> +	 *
> +	 * This callback is used by drivers to get the drm_connector in
> +	 * case the init is done via drm_writeback_init_with_conn. Which means
> +	 * the drivers don't have drm_connector embedded in drm_writeback_connector
> +	 * so they need to send the associated drm_connector with this
> +	 * function.
> +	 *
> +	 * This operation is optional.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * drm_connector assoiciated with the drm_writeback_connector.
> +	 */
> +	struct drm_connector
> +	*(*get_connector_from_writeback)(struct drm_writeback_connector *wbconnector);
> +};
> +
> +/**
> + * drm_writeback_connector_helper_add - sets the helper vtable for a connector
> + * @wb_connector: DRM writeback connector
> + * @funcs: helper vtable to set for @wb_connector
> + */
> +static inline void
> +drm_writeback_connector_helper_add(struct drm_writeback_connector *wb_connector,
> +				   const struct drm_writeback_connector_helper_funcs *funcs)
> +{
> +	wb_connector->helper_private = funcs;
> +}
> +
>  /**
>   * struct drm_plane_helper_funcs - helper operations for planes
>   *
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 149744dbeef0..77c3c64c132d 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -84,6 +84,13 @@ struct drm_writeback_connector {
>  	 * The name of the connector's fence timeline.
>  	 */
>  	char timeline_name[32];
> +
> +	/**
> +	 * @helper_private:
> +	 *
> +	 * helper private funcs for writeback_connector
> +	 */
> +	const struct drm_writeback_connector_helper_funcs *helper_private;
>  };

Unrelate to the commit? Also, where is this defined?

>  
>  /**
> @@ -142,11 +149,7 @@ struct drm_writeback_job {
>  	void *priv;
>  };
>  
> -static inline struct drm_writeback_connector *
> -drm_connector_to_writeback(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct drm_writeback_connector, base);
> -}
> +struct drm_writeback_connector *drm_connector_to_writeback(struct drm_connector *connector);
>  
>  int drm_writeback_connector_init(struct drm_device *dev,
>  				 struct drm_writeback_connector *wb_connector,
> @@ -172,6 +175,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
>  				       struct drm_writeback_connector *wb_connector,
>  				       struct drm_encoder *enc,
>  				       const struct drm_connector_funcs *con_funcs,
> +				       const struct drm_writeback_connector_helper_funcs *wb_funcs,
>  				       const u32 *formats, int n_formats);
>  
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:44AM +0530, Suraj Kandpal wrote:
> Now that drm_connector may not always be embedded within
> drm_writeback_connector, let's define a function which either uses
> the writeback helper hook that returns the drm_connector associated
> with the drm_writeback_connector or just return the drm_connector
> embedded inside drm_writeback_connector if the helper hook is not
> present. Lets use this function and call it whenever
> drm_connector is required mostly when connector helper private funcs
> need to be fetched.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index e9f7123270d6..d610cb827975 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_connector_to_writeback);
>  
> +static struct drm_connector *
> +drm_connector_from_writeback(struct drm_writeback_connector *wb_connector)
> +{
> +	const struct drm_writeback_connector_helper_funcs *funcs =
> +		wb_connector->helper_private;
> +
> +	if (funcs && funcs->get_connector_from_writeback)
> +		return funcs->get_connector_from_writeback(wb_connector);

The get_connector_from_writeback() and
drm_writeback_connector_helper_funcs should be moved to this patch.

However it feels really awkward to make drm_writeback_connector, which
is a wrapper around the drm_connector, to use some external DRM
connector. A quick grepping reveals API (which you missed) that expects
drm_writeback_connector.base to be a valid connector. And it would be
very hard to catch sunch an API later on.

If you want to use DRM framwework, while retaining
intel_connector for writeback connectors, I'd suggest following slightly
different path: extract common parts of drm_writeback_connector into a
common structure, and use it within the DRM core. Then provide functions
to fetch drm_connector from that struct or fetch it from drm_connector.


> +
> +	return &wb_connector->base;
> +}
> +
>  static int create_writeback_properties(struct drm_device *dev)
>  {
>  	struct drm_property *prop;
> @@ -478,6 +490,7 @@ drm_writeback_connector_init_with_conn(struct drm_device *dev, struct drm_connec
>  	if (ret)
>  		goto connector_fail;
>  
> +	drm_writeback_connector_helper_add(wb_connector, wb_funcs);
>  	INIT_LIST_HEAD(&wb_connector->job_queue);
>  	spin_lock_init(&wb_connector->job_lock);
>  
> @@ -527,13 +540,15 @@ int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  
>  int drm_writeback_prepare_job(struct drm_writeback_job *job)
>  {
> -	struct drm_writeback_connector *connector = job->connector;
> +	struct drm_writeback_connector *wb_connector = job->connector;
> +	struct drm_connector *connector =
> +		drm_connector_from_writeback(wb_connector);
>  	const struct drm_connector_helper_funcs *funcs =
> -		connector->base.helper_private;
> +		connector->helper_private;
>  	int ret;
>  
>  	if (funcs->prepare_writeback_job) {
> -		ret = funcs->prepare_writeback_job(connector, job);
> +		ret = funcs->prepare_writeback_job(wb_connector, job);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -579,12 +594,14 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
>  
>  void drm_writeback_cleanup_job(struct drm_writeback_job *job)
>  {
> -	struct drm_writeback_connector *connector = job->connector;
> +	struct drm_writeback_connector *wb_connector = job->connector;
> +	struct drm_connector *connector =
> +		drm_connector_from_writeback(wb_connector);
>  	const struct drm_connector_helper_funcs *funcs =
> -		connector->base.helper_private;
> +		connector->helper_private;
>  
>  	if (job->prepared && funcs->cleanup_writeback_job)
> -		funcs->cleanup_writeback_job(connector, job);
> +		funcs->cleanup_writeback_job(wb_connector, job);
>  
>  	if (job->fb)
>  		drm_framebuffer_put(job->fb);
> @@ -665,9 +682,11 @@ EXPORT_SYMBOL(drm_writeback_signal_completion);
>  struct dma_fence *
>  drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
>  {
> +	struct drm_connector *connector =
> +		drm_connector_from_writeback(wb_connector);
>  	struct dma_fence *fence;
>  
> -	if (WARN_ON(wb_connector->base.connector_type !=
> +	if (WARN_ON(connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_WRITEBACK))
>  		return NULL;
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 18/28] drm/i915/writeback: Add connector atomic check

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:59AM +0530, Suraj Kandpal wrote:
> Add connector helper function for atomic check which sets the
> mode_changed bit and checks if pixel format of fb is valid or not.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_writeback.c    | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> index 9b2432d86d35..7fb30cc61991 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -186,6 +186,54 @@ static void intel_writeback_connector_destroy(struct drm_connector *connector)
>  	kfree(connector);
>  }
>  
> +static int intel_writeback_check_format(u32 format)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(writeback_formats); i++) {
> +		if (writeback_formats[i] == format)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int intel_writeback_atomic_check(struct drm_connector *connector,
> +					struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *conn_state =
> +		drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_framebuffer *fb;
> +	int ret;
> +
> +	/* We return 0 since this is called while disabling writeback encoder */
> +	if (!conn_state->crtc)
> +		return 0;
> +
> +	/* We do not allow a blank commit when using writeback connector */
> +	if (!conn_state->writeback_job)
> +		return -EINVAL;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (!fb)
> +		return -EINVAL;

Other atomic_check functions return 0 if there is no writeback_job or no
FB. Please elaborate.

> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +	if (fb->width != crtc_state->mode.hdisplay ||
> +	    fb->height != crtc_state->mode.vdisplay)
> +		return -EINVAL;
> +
> +	ret = intel_writeback_check_format(fb->format->format);
> +	if (ret) {
> +		drm_dbg_kms(connector->dev,
> +			    "Unsupported drm format sent in writeback job\n");
> +		return ret;
> +	}

Missing a call to drm_atomic_helper_check_wb_connector_state(). Please
use the framework where available.

> +
> +	return 0;
> +}
> +
>  static struct drm_writeback_connector *
>  intel_get_writeback_connector(struct drm_connector *connector)
>  {
> @@ -221,6 +269,7 @@ static const struct drm_connector_helper_funcs conn_helper_funcs = {
>  	.get_writeback_connector = intel_get_writeback_connector,
>  	.get_modes = intel_writeback_get_modes,
>  	.mode_valid = intel_writeback_mode_valid,
> +	.atomic_check = intel_writeback_atomic_check,
>  	.prepare_writeback_job = intel_writeback_prepare_job,
>  	.cleanup_writeback_job = intel_writeback_cleanup_job,
>  };
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 00/28] Enable Pipe writeback

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:41AM +0530, Suraj Kandpal wrote:
> This series aims to enable pipe writeback functionality on
> ADLP where it has been tested. The plan is to slowly accomodate
> all supported hardware after this functionality is tested on them.
> This series also brings change to drm core but not in a drastic way.
> We introduce a helper which lets drivers have their own preallocated
> conenctor keeping the connector in drm_writeback_conenctor blank.
> This lets driver have more control over their connector but still use
> the drm core queues for job creation and signalling. Some new helpers
> have been added to aid drivers so that derivation of drm_connector
> from drm_writeback_connector and vice versa becomes easy for drivers
> that will use this helper since it won't be as straight forward as
> wb_conn->connector anymore. Driver not using these API will not be
> affected in anyways.
> This series enables the triggered captured mode where we need to
> trigger a capture.
> 
> Cc: Harry Wetland <harry.wentland@amd.com>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> 
> Suraj Kandpal (28):
>   drm/writeback: Add function that takes preallocated connector
>   drm/writeback: Add a helper function to get writeback connector
>   drm/writeback: Define function to get drm_connector from writeback
>   drm/i915/writeback: Add writeback registers
>   drm/i915/writeback: Add some preliminary writeback definitions
>   drm/i915/writeback: Init writeback connector
>   drm/i915/writeback: Add function for get_writeback_connector
>   drm/i915/writeback: Define the get_connector_from_writeback hook
>   drm/i915/writeback: Add function to get modes
>   drm/i915/writeback: Add hook to check modes
>   drm/i915/writeback: Define encoder->get_hw_state
>   drm/i915/writeback: Fill encoder->get_config
>   drm/i915/writeback: Add private structure for writeback job
>   drm/i915/writeback: Define function for prepare and cleanup hooks
>   drm/i915/writeback: Define compute_config for writeback
>   drm/i915/writeback: Define function for connector function detect
>   drm/i915/writeback: Define function to destroy writeback connector
>   drm/i915/writeback: Add connector atomic check

You are adding hooks one by one. Are you sure that the series is
bisectable? In other words, the driver must work (aka must not crash)
after each commit.

>   drm/i915/writeback: Add the enable sequence from writeback
>   drm/i915/writeback: Add writeback to xe Makefile
>   drm/i915/writeback: Define writeback frame capture function
>   drm/i915/writeback: Configure WD_STRIDE reg
>   drm/i915/writeback: Configure WD_SURF register
>   drm/i915/writeback: Enable writeback interrupts
>   drm/i915/writeback: Initialize writeback encoder.
>   drm/i915/writeback: Define the disable sequence for writeback
>   drm/i915/writeback: Make exception for writeback connector
>   drm/i915/writeback: Modify state verify function
> 
>  drivers/gpu/drm/drm_writeback.c               | 123 +++-
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/display/intel_acpi.c     |   1 +
>  .../drm/i915/display/intel_crtc_state_dump.c  |   2 +-
>  drivers/gpu/drm/i915/display/intel_display.c  | 178 +++--
>  drivers/gpu/drm/i915/display/intel_display.h  |   4 +
>  .../drm/i915/display/intel_display_debugfs.c  |   3 +
>  .../drm/i915/display/intel_display_device.c   |  29 +-
>  .../drm/i915/display/intel_display_device.h   |   2 +-
>  .../gpu/drm/i915/display/intel_display_irq.c  |  10 +
>  .../drm/i915/display/intel_display_limits.h   |   2 +
>  .../drm/i915/display/intel_display_power.c    |   4 +
>  .../drm/i915/display/intel_display_power.h    |   2 +
>  .../gpu/drm/i915/display/intel_display_regs.h |   1 +
>  .../drm/i915/display/intel_display_types.h    |   1 +
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   3 +
>  drivers/gpu/drm/i915/display/intel_opregion.c |   2 +-
>  drivers/gpu/drm/i915/display/intel_pmdemand.c |   3 +
>  drivers/gpu/drm/i915/display/intel_vdsc.c     |   4 +
>  .../gpu/drm/i915/display/intel_writeback.c    | 686 ++++++++++++++++++
>  .../gpu/drm/i915/display/intel_writeback.h    |  23 +
>  .../drm/i915/display/intel_writeback_reg.h    | 142 ++++
>  drivers/gpu/drm/xe/Makefile                   |   1 +
>  include/drm/drm_modeset_helper_vtables.h      |  59 ++
>  include/drm/drm_writeback.h                   |  21 +-
>  25 files changed, 1238 insertions(+), 69 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.h
>  create mode 100644 drivers/gpu/drm/i915/display/intel_writeback_reg.h
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 17/28] drm/i915/writeback: Define function to destroy writeback connector

[Dmitry Baryshkov]

On Fri, Jul 25, 2025 at 10:33:58AM +0530, Suraj Kandpal wrote:
> Define function to destroy the drm_writbeack_connector and
> drm_connector associated with it.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> index def33191a89e..9b2432d86d35 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -180,6 +180,12 @@ intel_writeback_detect(struct drm_connector *connector,
>  	return connector_status_connected;
>  }
>  
> +static void intel_writeback_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_cleanup(connector);
> +	kfree(connector);
> +}

Nice example of what I've written in my response to the cover letter:
without this commit we have a memory leak here, don't we?

> +
>  static struct drm_writeback_connector *
>  intel_get_writeback_connector(struct drm_connector *connector)
>  {
> @@ -208,6 +214,7 @@ const struct drm_connector_funcs conn_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +	.destroy = intel_writeback_connector_destroy,
>  };
>  
>  static const struct drm_connector_helper_funcs conn_helper_funcs = {
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 14/28] drm/i915/writeback: Define function for prepare and cleanup hooks

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v6.16-rc7 next-20250725]
[cannot apply to linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-writeback-Add-function-that-takes-preallocated-connector/20250725-133017
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20250725050409.2687242-15-suraj.kandpal%40intel.com
patch subject: [PATCH 14/28] drm/i915/writeback: Define function for prepare and cleanup hooks
config: i386-buildonly-randconfig-006-20250725 (https://download.01.org/0day-ci/archive/20250726/202507262142.wObleFKc-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250726/202507262142.wObleFKc-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/202507262142.wObleFKc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_writeback.c:139:60: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
     139 |                 drm_err(job->fb->dev, "Failed to map framebuffer: %d\n", ret);
         |                                                                          ^~~
   include/drm/drm_print.h:628:46: note: expanded from macro 'drm_err'
     628 |         __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
         |                                                     ^~~~~~~~~~~
   include/drm/drm_print.h:615:55: note: expanded from macro '__drm_printk'
     615 |         dev_##level##type(__drm_to_dev(drm), "[drm] " fmt, ##__VA_ARGS__)
         |                                                              ^~~~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                        ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_writeback.c:125:9: note: initialize the variable 'ret' to silence this warning
     125 |         int ret;
         |                ^
         |                 = 0
   drivers/gpu/drm/i915/display/intel_writeback.c:260:3: error: fallthrough annotation does not directly precede switch label
     260 |                 fallthrough;
         |                 ^
   include/linux/compiler_attributes.h:214:41: note: expanded from macro 'fallthrough'
     214 | # define fallthrough                    __attribute__((__fallthrough__))
         |                                         ^
   1 warning and 1 error generated.


vim +/ret +139 drivers/gpu/drm/i915/display/intel_writeback.c

   113	
   114	static int intel_writeback_prepare_job(struct drm_writeback_connector *wb_connector,
   115					       struct drm_writeback_job *job)
   116	{
   117		struct intel_writeback_connector *wb_conn =
   118			to_intel_writeback_connector(wb_connector);
   119		struct i915_vma *vma;
   120		struct intel_writeback_job *wb_job;
   121		unsigned long out_flags = 0;
   122		const struct i915_gtt_view view = {
   123			.type = I915_GTT_VIEW_NORMAL,
   124		};
   125		int ret;
   126	
   127		if (!job->fb)
   128			return 0;
   129	
   130		if (job->fb->modifier != DRM_FORMAT_MOD_LINEAR)
   131			return -EINVAL;
   132	
   133		wb_job = kzalloc(sizeof(*wb_job), GFP_KERNEL);
   134		if (!wb_job)
   135			return -ENOMEM;
   136	
   137		vma = intel_fb_pin_to_ggtt(job->fb, &view, 4 * 1024, 0, 0, true, &out_flags);
   138		if (IS_ERR(vma)) {
 > 139			drm_err(job->fb->dev, "Failed to map framebuffer: %d\n", ret);
   140			ret = PTR_ERR(vma);
   141			goto err;
   142		}
   143	
   144		wb_job->fb = job->fb;
   145		wb_job->vma = vma;
   146		wb_job->wb_connector = wb_connector;
   147		drm_framebuffer_get(wb_job->fb);
   148		job->priv = wb_job;
   149		wb_conn->job = wb_job;
   150	
   151		return 0;
   152	
   153	err:
   154		kfree(wb_job);
   155		return ret;
   156	}
   157	

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

Re: [PATCH 27/28] drm/i915/writeback: Make exception for writeback connector

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20250725]
[cannot apply to linus/master v6.16-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-writeback-Add-function-that-takes-preallocated-connector/20250725-133017
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20250725050409.2687242-28-suraj.kandpal%40intel.com
patch subject: [PATCH 27/28] drm/i915/writeback: Make exception for writeback connector
config: i386-buildonly-randconfig-006-20250725 (https://download.01.org/0day-ci/archive/20250726/202507262326.1J1VnzbD-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250726/202507262326.1J1VnzbD-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/202507262326.1J1VnzbD-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_display.c:3785:4: warning: variable 'trans_pipe' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
    3785 |                         default:
         |                         ^~~~~~~
   drivers/gpu/drm/i915/display/intel_display.c:3819:7: note: uninitialized use occurs here
    3819 |                 if (trans_pipe == crtc->pipe)
         |                     ^~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_display.c:3760:3: note: variable 'trans_pipe' is declared here
    3760 |                 enum pipe trans_pipe;
         |                 ^
   1 warning generated.


vim +/trans_pipe +3785 drivers/gpu/drm/i915/display/intel_display.c

  3743	
  3744	static u8 hsw_enabled_transcoders(struct intel_crtc *crtc)
  3745	{
  3746		struct intel_display *display = to_intel_display(crtc);
  3747		u8 panel_transcoder_mask = hsw_panel_transcoders(display);
  3748		enum transcoder cpu_transcoder;
  3749		u8 primary_pipe, secondary_pipes;
  3750		u8 enabled_transcoders = 0;
  3751	
  3752		/*
  3753		 * XXX: Do intel_display_power_get_if_enabled before reading this (for
  3754		 * consistency and less surprising code; it's in always on power).
  3755		 */
  3756		for_each_cpu_transcoder_masked(display, cpu_transcoder,
  3757					       panel_transcoder_mask) {
  3758			enum intel_display_power_domain power_domain;
  3759			intel_wakeref_t wakeref;
  3760			enum pipe trans_pipe;
  3761			u32 tmp = 0;
  3762	
  3763			power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
  3764			if (cpu_transcoder == TRANSCODER_WD_0 ||
  3765			    cpu_transcoder == TRANSCODER_WD_1) {
  3766				with_intel_display_power_if_enabled(display, power_domain, wakeref)
  3767					tmp = intel_de_read(display,
  3768							    WD_TRANS_FUNC_CTL(cpu_transcoder));
  3769				if (!(tmp & TRANS_WD_FUNC_ENABLE))
  3770					continue;
  3771	
  3772				switch (tmp & WD_INPUT_SELECT_MASK) {
  3773				case WD_INPUT_PIPE_A:
  3774					trans_pipe = PIPE_A;
  3775					break;
  3776				case WD_INPUT_PIPE_B:
  3777					trans_pipe = PIPE_B;
  3778					break;
  3779				case WD_INPUT_PIPE_C:
  3780					trans_pipe = PIPE_C;
  3781					break;
  3782				case WD_INPUT_PIPE_D:
  3783					trans_pipe = PIPE_D;
  3784					break;
> 3785				default:
  3786					MISSING_CASE(tmp & WD_INPUT_SELECT_MASK);
  3787					break;
  3788				}
  3789			} else {
  3790				with_intel_display_power_if_enabled(display, power_domain, wakeref)
  3791					tmp = intel_de_read(display,
  3792							    TRANS_DDI_FUNC_CTL(display, cpu_transcoder));
  3793	
  3794				if (!(tmp & TRANS_DDI_FUNC_ENABLE))
  3795					continue;
  3796	
  3797				switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
  3798				default:
  3799					drm_WARN(display->drm, 1,
  3800						 "unknown pipe linked to transcoder %s\n",
  3801						 transcoder_name(cpu_transcoder));
  3802					fallthrough;
  3803				case TRANS_DDI_EDP_INPUT_A_ONOFF:
  3804				case TRANS_DDI_EDP_INPUT_A_ON:
  3805					trans_pipe = PIPE_A;
  3806					break;
  3807				case TRANS_DDI_EDP_INPUT_B_ONOFF:
  3808					trans_pipe = PIPE_B;
  3809					break;
  3810				case TRANS_DDI_EDP_INPUT_C_ONOFF:
  3811					trans_pipe = PIPE_C;
  3812					break;
  3813				case TRANS_DDI_EDP_INPUT_D_ONOFF:
  3814					trans_pipe = PIPE_D;
  3815					break;
  3816				}
  3817			}
  3818	
  3819			if (trans_pipe == crtc->pipe)
  3820				enabled_transcoders |= BIT(cpu_transcoder);
  3821		}
  3822	
  3823		/* single pipe or joiner primary */
  3824		cpu_transcoder = (enum transcoder) crtc->pipe;
  3825		if (transcoder_ddi_func_is_enabled(display, cpu_transcoder))
  3826			enabled_transcoders |= BIT(cpu_transcoder);
  3827	
  3828		/* joiner secondary -> consider the primary pipe's transcoder as well */
  3829		enabled_joiner_pipes(display, crtc->pipe, &primary_pipe, &secondary_pipes);
  3830		if (secondary_pipes & BIT(crtc->pipe)) {
  3831			cpu_transcoder = (enum transcoder)ffs(primary_pipe) - 1;
  3832			if (transcoder_ddi_func_is_enabled(display, cpu_transcoder))
  3833				enabled_transcoders |= BIT(cpu_transcoder);
  3834		}
  3835	
  3836		return enabled_transcoders;
  3837	}
  3838	

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

RE: [PATCH 17/28] drm/i915/writeback: Define function to destroy writeback connector

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 6:11 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH 17/28] drm/i915/writeback: Define function to destroy
> writeback connector
> 
> On Fri, Jul 25, 2025 at 10:33:58AM +0530, Suraj Kandpal wrote:
> > Define function to destroy the drm_writbeack_connector and
> > drm_connector associated with it.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c
> > b/drivers/gpu/drm/i915/display/intel_writeback.c
> > index def33191a89e..9b2432d86d35 100644
> > --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> > +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> > @@ -180,6 +180,12 @@ intel_writeback_detect(struct drm_connector
> *connector,
> >  	return connector_status_connected;
> >  }
> >
> > +static void intel_writeback_connector_destroy(struct drm_connector
> > +*connector) {
> > +	drm_connector_cleanup(connector);
> > +	kfree(connector);
> > +}
> 
> Nice example of what I've written in my response to the cover letter:
> without this commit we have a memory leak here, don't we?

No we really don't none of this actually takes affect until the connector init is called which is way later 
So to answer your question this won't really cause a crash and is very bisectable

Regards,
Suraj Kandpal

> 
> > +
> >  static struct drm_writeback_connector *
> > intel_get_writeback_connector(struct drm_connector *connector)  { @@
> > -208,6 +214,7 @@ const struct drm_connector_funcs conn_funcs = {
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> >  	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
> >  	.atomic_destroy_state =
> drm_atomic_helper_connector_destroy_state,
> > +	.destroy = intel_writeback_connector_destroy,
> >  };
> >
> >  static const struct drm_connector_helper_funcs conn_helper_funcs = {
> > --
> > 2.34.1
> >
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 00/28] Enable Pipe writeback

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 6:10 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>; Harry Wetland <harry.wentland@amd.com>
> Subject: Re: [PATCH 00/28] Enable Pipe writeback
> 
> On Fri, Jul 25, 2025 at 10:33:41AM +0530, Suraj Kandpal wrote:
> > This series aims to enable pipe writeback functionality on ADLP where
> > it has been tested. The plan is to slowly accomodate all supported
> > hardware after this functionality is tested on them.
> > This series also brings change to drm core but not in a drastic way.
> > We introduce a helper which lets drivers have their own preallocated
> > conenctor keeping the connector in drm_writeback_conenctor blank.
> > This lets driver have more control over their connector but still use
> > the drm core queues for job creation and signalling. Some new helpers
> > have been added to aid drivers so that derivation of drm_connector
> > from drm_writeback_connector and vice versa becomes easy for drivers
> > that will use this helper since it won't be as straight forward as
> > wb_conn->connector anymore. Driver not using these API will not be
> > affected in anyways.
> > This series enables the triggered captured mode where we need to
> > trigger a capture.
> >
> > Cc: Harry Wetland <harry.wentland@amd.com>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> >
> > Suraj Kandpal (28):
> >   drm/writeback: Add function that takes preallocated connector
> >   drm/writeback: Add a helper function to get writeback connector
> >   drm/writeback: Define function to get drm_connector from writeback
> >   drm/i915/writeback: Add writeback registers
> >   drm/i915/writeback: Add some preliminary writeback definitions
> >   drm/i915/writeback: Init writeback connector
> >   drm/i915/writeback: Add function for get_writeback_connector
> >   drm/i915/writeback: Define the get_connector_from_writeback hook
> >   drm/i915/writeback: Add function to get modes
> >   drm/i915/writeback: Add hook to check modes
> >   drm/i915/writeback: Define encoder->get_hw_state
> >   drm/i915/writeback: Fill encoder->get_config
> >   drm/i915/writeback: Add private structure for writeback job
> >   drm/i915/writeback: Define function for prepare and cleanup hooks
> >   drm/i915/writeback: Define compute_config for writeback
> >   drm/i915/writeback: Define function for connector function detect
> >   drm/i915/writeback: Define function to destroy writeback connector
> >   drm/i915/writeback: Add connector atomic check
> 
> You are adding hooks one by one. Are you sure that the series is bisectable?
> In other words, the driver must work (aka must not crash) after each commit.
> 

Reply wont be a issue since the init call to connector is way later which will not cause bisection issue

Regards,
Suraj Kandpal

> >   drm/i915/writeback: Add the enable sequence from writeback
> >   drm/i915/writeback: Add writeback to xe Makefile
> >   drm/i915/writeback: Define writeback frame capture function
> >   drm/i915/writeback: Configure WD_STRIDE reg
> >   drm/i915/writeback: Configure WD_SURF register
> >   drm/i915/writeback: Enable writeback interrupts
> >   drm/i915/writeback: Initialize writeback encoder.
> >   drm/i915/writeback: Define the disable sequence for writeback
> >   drm/i915/writeback: Make exception for writeback connector
> >   drm/i915/writeback: Modify state verify function
> >
> >  drivers/gpu/drm/drm_writeback.c               | 123 +++-
> >  drivers/gpu/drm/i915/Makefile                 |   1 +
> >  drivers/gpu/drm/i915/display/intel_acpi.c     |   1 +
> >  .../drm/i915/display/intel_crtc_state_dump.c  |   2 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  | 178 +++--
> >  drivers/gpu/drm/i915/display/intel_display.h  |   4 +
> >  .../drm/i915/display/intel_display_debugfs.c  |   3 +
> >  .../drm/i915/display/intel_display_device.c   |  29 +-
> >  .../drm/i915/display/intel_display_device.h   |   2 +-
> >  .../gpu/drm/i915/display/intel_display_irq.c  |  10 +
> >  .../drm/i915/display/intel_display_limits.h   |   2 +
> >  .../drm/i915/display/intel_display_power.c    |   4 +
> >  .../drm/i915/display/intel_display_power.h    |   2 +
> >  .../gpu/drm/i915/display/intel_display_regs.h |   1 +
> >  .../drm/i915/display/intel_display_types.h    |   1 +
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |   3 +
> >  drivers/gpu/drm/i915/display/intel_opregion.c |   2 +-
> >  drivers/gpu/drm/i915/display/intel_pmdemand.c |   3 +
> >  drivers/gpu/drm/i915/display/intel_vdsc.c     |   4 +
> >  .../gpu/drm/i915/display/intel_writeback.c    | 686 ++++++++++++++++++
> >  .../gpu/drm/i915/display/intel_writeback.h    |  23 +
> >  .../drm/i915/display/intel_writeback_reg.h    | 142 ++++
> >  drivers/gpu/drm/xe/Makefile                   |   1 +
> >  include/drm/drm_modeset_helper_vtables.h      |  59 ++
> >  include/drm/drm_writeback.h                   |  21 +-
> >  25 files changed, 1238 insertions(+), 69 deletions(-)  create mode
> > 100644 drivers/gpu/drm/i915/display/intel_writeback.c
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_writeback.h
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_writeback_reg.h
> >
> > --
> > 2.34.1
> >
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 5:46 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes
> preallocated connector
> 
> On Fri, Jul 25, 2025 at 10:33:42AM +0530, Suraj Kandpal wrote:
> > Write a function that takes a preallocated drm_connector instead of
> > using the one allocated inside the drm writeback connector init
> > function.
> 
> Please start your commit message with describing the problem.
> 
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c | 76
> +++++++++++++++++++++++++++++++++
> >  include/drm/drm_writeback.h     |  7 +++
> >  2 files changed, 83 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..fa58eb0dc7bf
> > 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct
> > drm_device *dev,  }  EXPORT_SYMBOL(drmm_writeback_connector_init);
> >
> > +/*
> > + * drm_writeback_connector_init_with_conn - Initialize a writeback
> > +connector with
> > + * custom encoder and connector
> > + *
> > + * @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 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.
> 
> Why?

The problem would that not every want can have a drm_connector embedded inside the drm_writeback_connector
We have a restraint where all connectors need to be a intel connector and since the we are not allowed to make connector 
Inside the drm_connector into a pointer this gives a good alternative.

> 
> > + *
> > + * 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_conn(struct drm_device *dev, struct
> drm_connector *connector,
> > +				       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_mode_config *config = &dev->mode_config;
> > +	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;
> 
> This function contans a lot of copy-paste from
> __drm_writeback_connector_init(), which is obviously a no-go.

The whole point is the minore difference inbetween then and how it derives a lot of things from the
drm_writeback_connector because of which this looks like a similar function but is essentially different.

Regards,
Suraj Kandpal

> 
> > +
> > +	ret = drm_connector_init(dev, connector, con_funcs,
> > +				 DRM_MODE_CONNECTOR_WRITEBACK);
> > +	if (ret)
> > +		goto connector_fail;
> > +
> > +	INIT_LIST_HEAD(&wb_connector->job_queue);
> > +	spin_lock_init(&wb_connector->job_lock);
> > +
> > +	wb_connector->fence_context = dma_fence_context_alloc(1);
> > +	spin_lock_init(&wb_connector->fence_lock);
> > +	snprintf(wb_connector->timeline_name,
> > +		 sizeof(wb_connector->timeline_name),
> > +		 "CONNECTOR:%d-%s", connector->base.id, connector-
> >name);
> > +
> > +	drm_object_attach_property(&connector->base,
> > +				   config->writeback_out_fence_ptr_property,
> 0);
> > +
> > +	drm_object_attach_property(&connector->base,
> > +				   config->writeback_fb_id_property, 0);
> > +
> > +	drm_object_attach_property(&connector->base,
> > +				   config->writeback_pixel_formats_property,
> > +				   blob->base.id);
> > +	wb_connector->pixel_formats_blob_ptr = blob;
> > +
> > +	return 0;
> > +
> > +connector_fail:
> > +	drm_property_blob_put(blob);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_writeback_connector_init_with_conn);
> > +
> >  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> >  			 struct drm_framebuffer *fb)
> >  {
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index c380a7b8f55a..149744dbeef0 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -167,6 +167,13 @@ int drmm_writeback_connector_init(struct
> drm_device *dev,
> >  				  struct drm_encoder *enc,
> >  				  const u32 *formats, int n_formats);
> >
> > +int
> > +drm_writeback_connector_init_with_conn(struct drm_device *dev, struct
> drm_connector *connector,
> > +				       struct drm_writeback_connector
> *wb_connector,
> > +				       struct drm_encoder *enc,
> > +				       const struct drm_connector_funcs
> *con_funcs,
> > +				       const u32 *formats, int n_formats);
> > +
> >  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> >  			 struct drm_framebuffer *fb);
> >
> > --
> > 2.34.1
> >
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 02/28] drm/writeback: Add a helper function to get writeback connector

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 5:50 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH 02/28] drm/writeback: Add a helper function to get
> writeback connector
> 
> On Fri, Jul 25, 2025 at 10:33:43AM +0530, Suraj Kandpal wrote:
> > Now that we can initialize a drm_writeback_connector without having to
> > initialize the drm_connector within it and leaving the responsibility
> > of initialising the drm_connector and maintaining the association with
> > drm_writeback_connector to it. This helper hooks lets drivers return
> > the drm_writeback_connector associated with the give drm_connector.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c          | 14 ++++++
> >  include/drm/drm_modeset_helper_vtables.h | 59
> ++++++++++++++++++++++++
> >  include/drm/drm_writeback.h              | 14 ++++--
> >  3 files changed, 82 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c index fa58eb0dc7bf..e9f7123270d6
> > 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -107,6 +107,19 @@ static const struct dma_fence_ops
> drm_writeback_fence_ops = {
> >  	.get_timeline_name = drm_writeback_fence_get_timeline_name,
> >  };
> >
> > +struct drm_writeback_connector *
> > +drm_connector_to_writeback(struct drm_connector *connector) {
> > +	const struct drm_connector_helper_funcs *funcs =
> > +		connector->helper_private;
> > +
> > +	if (funcs->get_writeback_connector)
> > +		return funcs->get_writeback_connector(connector);
> > +
> > +	return container_of(connector, struct drm_writeback_connector,
> > +base); } EXPORT_SYMBOL(drm_connector_to_writeback);
> > +
> >  static int create_writeback_properties(struct drm_device *dev)  {
> >  	struct drm_property *prop;
> > @@ -443,6 +456,7 @@ drm_writeback_connector_init_with_conn(struct
> drm_device *dev, struct drm_connec
> >  				       struct drm_writeback_connector
> *wb_connector,
> >  				       struct drm_encoder *enc,
> >  				       const struct drm_connector_funcs
> *con_funcs,
> > +				       const struct
> drm_writeback_connector_helper_funcs
> > +*wb_funcs,
> >  				       const u32 *formats, int n_formats)  {
> >  	struct drm_property_blob *blob;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index ce7c7aeac887..6b89b33d2304 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -31,6 +31,7 @@
> >
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_writeback.h>
> >
> >  /**
> >   * DOC: overview
> > @@ -1179,6 +1180,25 @@ struct drm_connector_helper_funcs {
> >  	 *
> >  	 */
> >  	void (*disable_hpd)(struct drm_connector *connector);
> > +
> > +	/**
> > +	 * @get_writeback_connector:
> > +	 *
> > +	 * This callback is used by drivers to get the writeback connector in
> > +	 * case the init is done via drm_writeback_init_with_conn. Which
> means
> > +	 * the drivers don't have drm_connector embedded in
> drm_writeback_connector
> > +	 * so they need to send the associated writeback connector with this
> > +	 * function.
> > +	 *
> > +	 * This operation is optional.
> > +	 *
> > +	 * This is mainly called from drm_writeback_set_gb.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * drm_writeback_connector assoiciated with the drm connector.
> > +	 */
> > +	struct drm_writeback_connector *(*get_writeback_connector)(struct
> > +drm_connector *connector);
> >  };
> >
> >  /**
> > @@ -1192,6 +1212,45 @@ static inline void
> drm_connector_helper_add(struct drm_connector *connector,
> >  	connector->helper_private = funcs;
> >  }
> >
> > +/**
> > + * struct drm_writeback_connector_helper_funcs - helper operations
> > +for writeback
> > + * connectors.
> > + *
> > + * These functions are used by the atomic and legacy modeset helpers
> > +and by the
> > + * probe helpers.
> > + */
> > +struct drm_writeback_connector_helper_funcs {
> > +	/**
> > +	 * @get_connector_from_writeback:
> > +	 *
> > +	 * This callback is used by drivers to get the drm_connector in
> > +	 * case the init is done via drm_writeback_init_with_conn. Which
> means
> > +	 * the drivers don't have drm_connector embedded in
> drm_writeback_connector
> > +	 * so they need to send the associated drm_connector with this
> > +	 * function.
> > +	 *
> > +	 * This operation is optional.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * drm_connector assoiciated with the drm_writeback_connector.
> > +	 */
> > +	struct drm_connector
> > +	*(*get_connector_from_writeback)(struct drm_writeback_connector
> > +*wbconnector); };
> > +
> > +/**
> > + * drm_writeback_connector_helper_add - sets the helper vtable for a
> > +connector
> > + * @wb_connector: DRM writeback connector
> > + * @funcs: helper vtable to set for @wb_connector  */ static inline
> > +void drm_writeback_connector_helper_add(struct
> > +drm_writeback_connector *wb_connector,
> > +				   const struct
> drm_writeback_connector_helper_funcs *funcs) {
> > +	wb_connector->helper_private = funcs; }
> > +
> >  /**
> >   * struct drm_plane_helper_funcs - helper operations for planes
> >   *
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index 149744dbeef0..77c3c64c132d 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -84,6 +84,13 @@ struct drm_writeback_connector {
> >  	 * The name of the connector's fence timeline.
> >  	 */
> >  	char timeline_name[32];
> > +
> > +	/**
> > +	 * @helper_private:
> > +	 *
> > +	 * helper private funcs for writeback_connector
> > +	 */
> > +	const struct drm_writeback_connector_helper_funcs
> *helper_private;
> >  };
> 
> Unrelate to the commit? Also, where is this defined?

This is very much related to this commit and defined on top right here in this commit.

Regards,
Suraj Kandpal

> 
> >
> >  /**
> > @@ -142,11 +149,7 @@ struct drm_writeback_job {
> >  	void *priv;
> >  };
> >
> > -static inline struct drm_writeback_connector *
> > -drm_connector_to_writeback(struct drm_connector *connector) -{
> > -	return container_of(connector, struct drm_writeback_connector,
> base);
> > -}
> > +struct drm_writeback_connector *drm_connector_to_writeback(struct
> > +drm_connector *connector);
> >
> >  int drm_writeback_connector_init(struct drm_device *dev,
> >  				 struct drm_writeback_connector
> *wb_connector, @@ -172,6 +175,7
> > @@ drm_writeback_connector_init_with_conn(struct drm_device *dev,
> struct drm_connec
> >  				       struct drm_writeback_connector
> *wb_connector,
> >  				       struct drm_encoder *enc,
> >  				       const struct drm_connector_funcs
> *con_funcs,
> > +				       const struct
> drm_writeback_connector_helper_funcs
> > +*wb_funcs,
> >  				       const u32 *formats, int n_formats);
> >
> >  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > --
> > 2.34.1
> >
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 6:04 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH 03/28] drm/writeback: Define function to get
> drm_connector from writeback
> 
> On Fri, Jul 25, 2025 at 10:33:44AM +0530, Suraj Kandpal wrote:
> > Now that drm_connector may not always be embedded within
> > drm_writeback_connector, let's define a function which either uses the
> > writeback helper hook that returns the drm_connector associated with
> > the drm_writeback_connector or just return the drm_connector embedded
> > inside drm_writeback_connector if the helper hook is not present. Lets
> > use this function and call it whenever drm_connector is required
> > mostly when connector helper private funcs need to be fetched.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c | 33
> > ++++++++++++++++++++++++++-------
> >  1 file changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c index e9f7123270d6..d610cb827975
> > 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> drm_connector
> > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> >
> > +static struct drm_connector *
> > +drm_connector_from_writeback(struct drm_writeback_connector
> > +*wb_connector) {
> > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > +		wb_connector->helper_private;
> > +
> > +	if (funcs && funcs->get_connector_from_writeback)
> > +		return funcs-
> >get_connector_from_writeback(wb_connector);
> 
> The get_connector_from_writeback() and
> drm_writeback_connector_helper_funcs should be moved to this patch.

Want to keep them separate since they themselves introduce a lot of changes on of them
has use introducing a new writeback_helper_function structure.


> 
> However it feels really awkward to make drm_writeback_connector, which is
> a wrapper around the drm_connector, to use some external DRM connector.
> A quick grepping reveals API (which you missed) that expects
> drm_writeback_connector.base to be a valid connector. And it would be very
> hard to catch sunch an API later on.

Also seems like I did miss the fence_get_driver_name one which is an easy fix or
did you see anything else.
Really don't see any other problematic areas

> 
> If you want to use DRM framwework, while retaining intel_connector for
> writeback connectors, I'd suggest following slightly different path: extract
> common parts of drm_writeback_connector into a common structure, and
> use it within the DRM core. Then provide functions to fetch drm_connector
> from that struct or fetch it from drm_connector.

Causes a lot of changes in the drm_writeback_connector structure causing every other driver
Using this to change how they essentially call upon drm_writeback_connector. This API
was to provide more non invasive way to give everyone another alternative.

Regards,
Suraj Kandpal

> 
> 
> > +
> > +	return &wb_connector->base;
> > +}
> > +
> >  static int create_writeback_properties(struct drm_device *dev)  {
> >  	struct drm_property *prop;
> > @@ -478,6 +490,7 @@ drm_writeback_connector_init_with_conn(struct
> drm_device *dev, struct drm_connec
> >  	if (ret)
> >  		goto connector_fail;
> >
> > +	drm_writeback_connector_helper_add(wb_connector, wb_funcs);
> >  	INIT_LIST_HEAD(&wb_connector->job_queue);
> >  	spin_lock_init(&wb_connector->job_lock);
> >
> > @@ -527,13 +540,15 @@ int drm_writeback_set_fb(struct
> > drm_connector_state *conn_state,
> >
> >  int drm_writeback_prepare_job(struct drm_writeback_job *job)  {
> > -	struct drm_writeback_connector *connector = job->connector;
> > +	struct drm_writeback_connector *wb_connector = job->connector;
> > +	struct drm_connector *connector =
> > +		drm_connector_from_writeback(wb_connector);
> >  	const struct drm_connector_helper_funcs *funcs =
> > -		connector->base.helper_private;
> > +		connector->helper_private;
> >  	int ret;
> >
> >  	if (funcs->prepare_writeback_job) {
> > -		ret = funcs->prepare_writeback_job(connector, job);
> > +		ret = funcs->prepare_writeback_job(wb_connector, job);
> >  		if (ret < 0)
> >  			return ret;
> >  	}
> > @@ -579,12 +594,14 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
> >
> >  void drm_writeback_cleanup_job(struct drm_writeback_job *job)  {
> > -	struct drm_writeback_connector *connector = job->connector;
> > +	struct drm_writeback_connector *wb_connector = job->connector;
> > +	struct drm_connector *connector =
> > +		drm_connector_from_writeback(wb_connector);
> >  	const struct drm_connector_helper_funcs *funcs =
> > -		connector->base.helper_private;
> > +		connector->helper_private;
> >
> >  	if (job->prepared && funcs->cleanup_writeback_job)
> > -		funcs->cleanup_writeback_job(connector, job);
> > +		funcs->cleanup_writeback_job(wb_connector, job);
> >
> >  	if (job->fb)
> >  		drm_framebuffer_put(job->fb);
> > @@ -665,9 +682,11 @@
> EXPORT_SYMBOL(drm_writeback_signal_completion);
> >  struct dma_fence *
> >  drm_writeback_get_out_fence(struct drm_writeback_connector
> > *wb_connector)  {
> > +	struct drm_connector *connector =
> > +		drm_connector_from_writeback(wb_connector);
> >  	struct dma_fence *fence;
> >
> > -	if (WARN_ON(wb_connector->base.connector_type !=
> > +	if (WARN_ON(connector->connector_type !=
> >  		    DRM_MODE_CONNECTOR_WRITEBACK))
> >  		return NULL;
> >
> > --
> > 2.34.1
> >
> 
> --
> With best wishes
> Dmitry

Re: [PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Dmitry Baryshkov]

On Sat, Jul 26, 2025 at 04:41:29PM +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Sent: Saturday, July 26, 2025 5:46 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> > Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes
> > preallocated connector
> > 
> > On Fri, Jul 25, 2025 at 10:33:42AM +0530, Suraj Kandpal wrote:
> > > Write a function that takes a preallocated drm_connector instead of
> > > using the one allocated inside the drm writeback connector init
> > > function.
> > 
> > Please start your commit message with describing the problem.
> > 
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c | 76
> > +++++++++++++++++++++++++++++++++
> > >  include/drm/drm_writeback.h     |  7 +++
> > >  2 files changed, 83 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..fa58eb0dc7bf
> > > 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct
> > > drm_device *dev,  }  EXPORT_SYMBOL(drmm_writeback_connector_init);
> > >
> > > +/*
> > > + * drm_writeback_connector_init_with_conn - Initialize a writeback
> > > +connector with
> > > + * custom encoder and connector
> > > + *
> > > + * @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 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.
> > 
> > Why?
> 
> The problem would that not every want can have a drm_connector embedded inside the drm_writeback_connector
> We have a restraint where all connectors need to be a intel connector and since the we are not allowed to make connector 
> Inside the drm_connector into a pointer this gives a good alternative.

All of this needs to go to the commit message.

> 
> > 
> > > + *
> > > + * 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_conn(struct drm_device *dev, struct
> > drm_connector *connector,
> > > +				       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_mode_config *config = &dev->mode_config;
> > > +	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;
> > 
> > This function contans a lot of copy-paste from
> > __drm_writeback_connector_init(), which is obviously a no-go.
> 
> The whole point is the minore difference inbetween then and how it derives a lot of things from the
> drm_writeback_connector because of which this looks like a similar function but is essentially different.

It surely is. This means that you need to extract common code rather
than duplicate it.


-- 
With best wishes
Dmitry

Re: [PATCH 02/28] drm/writeback: Add a helper function to get writeback connector

[Dmitry Baryshkov]

On Sat, Jul 26, 2025 at 04:43:11PM +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Sent: Saturday, July 26, 2025 5:50 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> > Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [PATCH 02/28] drm/writeback: Add a helper function to get
> > writeback connector
> > 
> > On Fri, Jul 25, 2025 at 10:33:43AM +0530, Suraj Kandpal wrote:
> > > Now that we can initialize a drm_writeback_connector without having to
> > > initialize the drm_connector within it and leaving the responsibility
> > > of initialising the drm_connector and maintaining the association with
> > > drm_writeback_connector to it. This helper hooks lets drivers return
> > > the drm_writeback_connector associated with the give drm_connector.
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c          | 14 ++++++
> > >  include/drm/drm_modeset_helper_vtables.h | 59
> > ++++++++++++++++++++++++
> > >  include/drm/drm_writeback.h              | 14 ++++--
> > >  3 files changed, 82 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > b/drivers/gpu/drm/drm_writeback.c index fa58eb0dc7bf..e9f7123270d6
> > > 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -107,6 +107,19 @@ static const struct dma_fence_ops
> > drm_writeback_fence_ops = {
> > >  	.get_timeline_name = drm_writeback_fence_get_timeline_name,
> > >  };
> > >
> > > +struct drm_writeback_connector *
> > > +drm_connector_to_writeback(struct drm_connector *connector) {
> > > +	const struct drm_connector_helper_funcs *funcs =
> > > +		connector->helper_private;
> > > +
> > > +	if (funcs->get_writeback_connector)
> > > +		return funcs->get_writeback_connector(connector);
> > > +
> > > +	return container_of(connector, struct drm_writeback_connector,
> > > +base); } EXPORT_SYMBOL(drm_connector_to_writeback);
> > > +
> > >  static int create_writeback_properties(struct drm_device *dev)  {
> > >  	struct drm_property *prop;
> > > @@ -443,6 +456,7 @@ drm_writeback_connector_init_with_conn(struct
> > drm_device *dev, struct drm_connec
> > >  				       struct drm_writeback_connector
> > *wb_connector,
> > >  				       struct drm_encoder *enc,
> > >  				       const struct drm_connector_funcs
> > *con_funcs,
> > > +				       const struct
> > drm_writeback_connector_helper_funcs
> > > +*wb_funcs,
> > >  				       const u32 *formats, int n_formats)  {
> > >  	struct drm_property_blob *blob;
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > b/include/drm/drm_modeset_helper_vtables.h
> > > index ce7c7aeac887..6b89b33d2304 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -31,6 +31,7 @@
> > >
> > >  #include <drm/drm_crtc.h>
> > >  #include <drm/drm_encoder.h>
> > > +#include <drm/drm_writeback.h>
> > >
> > >  /**
> > >   * DOC: overview
> > > @@ -1179,6 +1180,25 @@ struct drm_connector_helper_funcs {
> > >  	 *
> > >  	 */
> > >  	void (*disable_hpd)(struct drm_connector *connector);
> > > +
> > > +	/**
> > > +	 * @get_writeback_connector:
> > > +	 *
> > > +	 * This callback is used by drivers to get the writeback connector in
> > > +	 * case the init is done via drm_writeback_init_with_conn. Which
> > means
> > > +	 * the drivers don't have drm_connector embedded in
> > drm_writeback_connector
> > > +	 * so they need to send the associated writeback connector with this
> > > +	 * function.
> > > +	 *
> > > +	 * This operation is optional.
> > > +	 *
> > > +	 * This is mainly called from drm_writeback_set_gb.
> > > +	 *
> > > +	 * RETURNS:
> > > +	 *
> > > +	 * drm_writeback_connector assoiciated with the drm connector.
> > > +	 */
> > > +	struct drm_writeback_connector *(*get_writeback_connector)(struct
> > > +drm_connector *connector);
> > >  };
> > >
> > >  /**
> > > @@ -1192,6 +1212,45 @@ static inline void
> > drm_connector_helper_add(struct drm_connector *connector,
> > >  	connector->helper_private = funcs;
> > >  }
> > >
> > > +/**
> > > + * struct drm_writeback_connector_helper_funcs - helper operations
> > > +for writeback
> > > + * connectors.
> > > + *
> > > + * These functions are used by the atomic and legacy modeset helpers
> > > +and by the
> > > + * probe helpers.
> > > + */
> > > +struct drm_writeback_connector_helper_funcs {
> > > +	/**
> > > +	 * @get_connector_from_writeback:
> > > +	 *
> > > +	 * This callback is used by drivers to get the drm_connector in
> > > +	 * case the init is done via drm_writeback_init_with_conn. Which
> > means
> > > +	 * the drivers don't have drm_connector embedded in
> > drm_writeback_connector
> > > +	 * so they need to send the associated drm_connector with this
> > > +	 * function.
> > > +	 *
> > > +	 * This operation is optional.
> > > +	 *
> > > +	 * RETURNS:
> > > +	 *
> > > +	 * drm_connector assoiciated with the drm_writeback_connector.
> > > +	 */
> > > +	struct drm_connector
> > > +	*(*get_connector_from_writeback)(struct drm_writeback_connector
> > > +*wbconnector); };
> > > +
> > > +/**
> > > + * drm_writeback_connector_helper_add - sets the helper vtable for a
> > > +connector
> > > + * @wb_connector: DRM writeback connector
> > > + * @funcs: helper vtable to set for @wb_connector  */ static inline
> > > +void drm_writeback_connector_helper_add(struct
> > > +drm_writeback_connector *wb_connector,
> > > +				   const struct
> > drm_writeback_connector_helper_funcs *funcs) {
> > > +	wb_connector->helper_private = funcs; }
> > > +
> > >  /**
> > >   * struct drm_plane_helper_funcs - helper operations for planes
> > >   *
> > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > > index 149744dbeef0..77c3c64c132d 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -84,6 +84,13 @@ struct drm_writeback_connector {
> > >  	 * The name of the connector's fence timeline.
> > >  	 */
> > >  	char timeline_name[32];
> > > +
> > > +	/**
> > > +	 * @helper_private:
> > > +	 *
> > > +	 * helper private funcs for writeback_connector
> > > +	 */
> > > +	const struct drm_writeback_connector_helper_funcs
> > *helper_private;
> > >  };
> > 
> > Unrelate to the commit? Also, where is this defined?
> 
> This is very much related to this commit and defined on top right here in this commit.

Then please split this patch into two.


-- 
With best wishes
Dmitry

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Dmitry Baryshkov]

On Sat, Jul 26, 2025 at 04:49:44PM +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Sent: Saturday, July 26, 2025 6:04 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> > Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [PATCH 03/28] drm/writeback: Define function to get
> > drm_connector from writeback

Please tune your mail client to insert smaller quotation headers. This
is just useless.

> > 
> > On Fri, Jul 25, 2025 at 10:33:44AM +0530, Suraj Kandpal wrote:
> > > Now that drm_connector may not always be embedded within
> > > drm_writeback_connector, let's define a function which either uses the
> > > writeback helper hook that returns the drm_connector associated with
> > > the drm_writeback_connector or just return the drm_connector embedded
> > > inside drm_writeback_connector if the helper hook is not present. Lets
> > > use this function and call it whenever drm_connector is required
> > > mostly when connector helper private funcs need to be fetched.
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c | 33
> > > ++++++++++++++++++++++++++-------
> > >  1 file changed, 26 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > b/drivers/gpu/drm/drm_writeback.c index e9f7123270d6..d610cb827975
> > > 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> > drm_connector
> > > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> > >
> > > +static struct drm_connector *
> > > +drm_connector_from_writeback(struct drm_writeback_connector
> > > +*wb_connector) {
> > > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > > +		wb_connector->helper_private;
> > > +
> > > +	if (funcs && funcs->get_connector_from_writeback)
> > > +		return funcs-
> > >get_connector_from_writeback(wb_connector);
> > 
> > The get_connector_from_writeback() and
> > drm_writeback_connector_helper_funcs should be moved to this patch.
> 
> Want to keep them separate since they themselves introduce a lot of changes on of them
> has use introducing a new writeback_helper_function structure.

Let's see how the series will take shape.

> 
> 
> > 
> > However it feels really awkward to make drm_writeback_connector, which is
> > a wrapper around the drm_connector, to use some external DRM connector.
> > A quick grepping reveals API (which you missed) that expects
> > drm_writeback_connector.base to be a valid connector. And it would be very
> > hard to catch sunch an API later on.
> 
> Also seems like I did miss the fence_get_driver_name one which is an easy fix or
> did you see anything else.
> Really don't see any other problematic areas

Yes, it was that function. However it is a nice example of how easy it
is to miss a call. Likewise anybody else changing the code might easily
not notice that Intel driver uses drm_writeback_connector in a strange
way.
> 
> > 
> > If you want to use DRM framwework, while retaining intel_connector for
> > writeback connectors, I'd suggest following slightly different path: extract
> > common parts of drm_writeback_connector into a common structure, and
> > use it within the DRM core. Then provide functions to fetch drm_connector
> > from that struct or fetch it from drm_connector.
> 
> Causes a lot of changes in the drm_writeback_connector structure causing every other driver
> Using this to change how they essentially call upon drm_writeback_connector. This API
> was to provide more non invasive way to give everyone another alternative.

Currently drm_writeback_connector is documented and implemented as being
a wrapper around drm_connector. You are changing that contract in a
non-intuitive way. I think there are several options on how to proceed:

- Clearly and loudly document that drm_writeback_connector is no longer
  a wrapper around drm_connector. Clearly document how to distinguish
  those two cases. In my opinion this is the worst option as it is
  significantly error-prone

- Make sure that the DRM framework can use writeback without
  drm_writeback_connector and them implement all necessary plumbing in
  the Intel driver. This can result in singnificant amount of code
  duplication, so I'd skip this option.

- Separate writeback parts of drm_writeback_connector into a struct,
  make drm_writeback_connector include drm_connector, new struct and
  most likely drm_encoder. Implement conversion callbacks (like you did
  in your patchset).

- Rework drm_writeback_connector and drm_connector in a similar way, but
  use writeback structure as a field inside drm_connector (similar to
  how we got the HDMI data). This saves you from having all conversion
  callbacks and makes it extensible for other drivers too. In fact you
  can use an anonymous union, making sure that HDMI and writeback
  structures take the same space in memory.

My preference is shifted towards the last option, it follows current
HDMI subclassing design and it doesn't add unnecessary complexity.

Yes, this requires reworking of all writeback drivers. Yes, it's a price
of having your own subclass of drm_connector. No, in my optionion,
leaving a semi-broken abstraction is not an option. Whatever path gets
implemented, it should be internally coherent.

-- 
With best wishes
Dmitry

Re: [PATCH 17/28] drm/i915/writeback: Define function to destroy writeback connector

[Dmitry Baryshkov]

On Sat, Jul 26, 2025 at 04:29:54PM +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Sent: Saturday, July 26, 2025 6:11 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> > Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Subject: Re: [PATCH 17/28] drm/i915/writeback: Define function to destroy
> > writeback connector
> > 
> > On Fri, Jul 25, 2025 at 10:33:58AM +0530, Suraj Kandpal wrote:
> > > Define function to destroy the drm_writbeack_connector and
> > > drm_connector associated with it.
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_writeback.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c
> > > b/drivers/gpu/drm/i915/display/intel_writeback.c
> > > index def33191a89e..9b2432d86d35 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> > > @@ -180,6 +180,12 @@ intel_writeback_detect(struct drm_connector
> > *connector,
> > >  	return connector_status_connected;
> > >  }
> > >
> > > +static void intel_writeback_connector_destroy(struct drm_connector
> > > +*connector) {
> > > +	drm_connector_cleanup(connector);
> > > +	kfree(connector);
> > > +}
> > 
> > Nice example of what I've written in my response to the cover letter:
> > without this commit we have a memory leak here, don't we?
> 
> No we really don't none of this actually takes affect until the connector init is called which is way later 
> So to answer your question this won't really cause a crash and is very bisectable

Ack, thanks. Then it's a fine way to implement the callbacks.

> 
> Regards,
> Suraj Kandpal
> 
> > 
> > > +
> > >  static struct drm_writeback_connector *
> > > intel_get_writeback_connector(struct drm_connector *connector)  { @@
> > > -208,6 +214,7 @@ const struct drm_connector_funcs conn_funcs = {
> > >  	.fill_modes = drm_helper_probe_single_connector_modes,
> > >  	.atomic_duplicate_state = intel_digital_connector_duplicate_state,
> > >  	.atomic_destroy_state =
> > drm_atomic_helper_connector_destroy_state,
> > > +	.destroy = intel_writeback_connector_destroy,
> > >  };
> > >
> > >  static const struct drm_connector_helper_funcs conn_helper_funcs = {
> > > --
> > > 2.34.1
> > >
> > 
> > --
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry

Re: [PATCH 04/28] drm/i915/writeback: Add writeback registers

[Murthy, Arun R]

On 25-07-2025 10:33, Suraj Kandpal wrote:
> Add writeback registers to its own file.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   .../drm/i915/display/intel_writeback_reg.h    | 136 ++++++++++++++++++
>   1 file changed, 136 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/display/intel_writeback_reg.h
>
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback_reg.h b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
> new file mode 100644
> index 000000000000..ffe302ef3dd9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_writeback_reg.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
After updating the copyright!

Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
--------------------

> +
> +#ifndef __INTEL_WRITEBACK_REGS_H__
> +#define __INTEL_WRITEBACK_REGS_H__
> +
> +#include "intel_display_reg_defs.h"
> +
> +/* WD 0 and 1 */
> +#define TRANSCODER_WD0_OFFSET	0x6e000
> +#define TRANSCODER_WD1_OFFSET	0x6d800
> +
> +/* WD 0 and 1 */
> +#define PIPE_WD0_OFFSET		0x7e008
> +#define PIPE_WD1_OFFSET		0x7d008
> +
> +/* Gen12 WD */
> +#define _MMIO_WD(tc, wd0, wd1)	_MMIO_TRANS((tc) - TRANSCODER_WD_0, wd0, wd1)
> +
> +#define WD_TRANS_ENABLE		REG_BIT(31)
> +#define WD_TRANS_STATE		REG_BIT(30)
> +
> +/* WD transcoder control */
> +#define _WD_TRANS_FUNC_CTL_0	0x6e400
> +#define _WD_TRANS_FUNC_CTL_1	0x6ec00
> +#define WD_TRANS_FUNC_CTL(tc)	_MMIO_WD(tc,\
> +				_WD_TRANS_FUNC_CTL_0,\
> +				_WD_TRANS_FUNC_CTL_1)
> +
> +#define TRANS_WD_FUNC_ENABLE		REG_BIT(31)
> +#define WD_TRIGGERED_CAP_MODE_ENABLE	REG_BIT(30)
> +#define START_TRIGGER_FRAME		REG_BIT(29)
> +#define STOP_TRIGGER_FRAME		REG_BIT(28)
> +#define WD_INPUT_SELECT_MASK		REG_GENMASK(14, 12)
> +#define WD_INPUT_PIPE_A			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 0)
> +#define WD_INPUT_PIPE_B			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 5)
> +#define WD_INPUT_PIPE_C			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 6)
> +#define WD_INPUT_PIPE_D			REG_FIELD_PREP(WD_INPUT_SELECT_MASK, 7)
> +#define WD_COLOR_MODE_MASK		REG_GENMASK(22, 20)
> +#define WD_CONTROL_POINTERS             REG_GENMASK(19, 18)
> +#define WD_DISABLE_POINTERS             REG_FIELD_PREP(WD_CONTROL_POINTERS, 3)
> +#define WD_PIX_FMT_YUYV			REG_FIELD_PREP(WD_COLOR_MODE_MASK, 1)
> +#define WD_PIX_FMT_XYUV8888		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 2)
> +#define WD_PIX_FMT_XBGR8888		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 3)
> +#define WD_PIX_FMT_Y410			REG_FIELD_PREP(WD_COLOR_MODE_MASK, 4)
> +#define WD_PIX_FMT_YUV422		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 5)
> +#define WD_PIX_FMT_XBGR2101010		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 6)
> +#define WD_PIX_FMT_RGB565		REG_FIELD_PREP(WD_COLOR_MODE_MASK, 7)
> +#define WD_FRAME_NUMBER_MASK		REG_GENMASK(3, 0)
> +#define WD_FRAME_NUMBER(n)		REG_FIELD_PREP(WD_FRAME_NUMBER_MASK, n)
> +
> +#define _WD_STRIDE_0			0x6e510
> +#define _WD_STRIDE_1			0x6ed10
> +#define WD_STRIDE(tc)			_MMIO_WD(tc,\
> +					_WD_STRIDE_0,\
> +					_WD_STRIDE_1)
> +#define WD_STRIDE_MASK			REG_GENMASK(15, 6)
> +
> +#define _WD_STREAMCAP_CTL0		0x6e590
> +#define _WD_STREAMCAP_CTL1		0x6ed90
> +#define WD_STREAMCAP_CTL(tc)		_MMIO_WD(tc,\
> +					_WD_STREAMCAP_CTL0,\
> +					_WD_STREAMCAP_CTL1)
> +
> +#define WD_STREAM_CAP_MODE_EN		REG_BIT(31)
> +#define WD_SLICING_STRAT_MASK		REG_GENMASK(25, 24)
> +#define WD_SLICING_STRAT_1_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 0)
> +#define WD_SLICING_STRAT_2_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 1)
> +#define WD_SLICING_STRAT_4_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 2)
> +#define WD_SLICING_STRAT_8_1		REG_FIELD_PREP(WD_SLICING_STRAT_MASK, 3)
> +#define WD_STREAM_OVERRUN_STATUS	1
> +
> +#define _WD_SURF_0			0x6e514
> +#define _WD_SURF_1			0x6ed14
> +#define WD_SURF(tc)			_MMIO_WD(tc,\
> +					_WD_SURF_0,\
> +					_WD_SURF_1)
> +
> +#define _WD_IMR_0			0x6e560
> +#define _WD_IMR_1			0x6ed60
> +#define WD_IMR(tc)			_MMIO_WD(tc,\
> +					_WD_IMR_0,\
> +					_WD_IMR_1)
> +#define WD_FRAME_COMPLETE_INT		REG_BIT(7)
> +#define WD_GTT_FAULT_INT		REG_BIT(6)
> +#define WD_VBLANK_INT			REG_BIT(5)
> +#define WD_OVERRUN_INT			REG_BIT(4)
> +#define WD_CAPTURING_INT		REG_BIT(3)
> +#define WD_WRITE_COMPLETE_INT		REG_BIT(2)
> +
> +#define _WD_IIR_0			0x6e564
> +#define _WD_IIR_1			0x6ed64
> +#define WD_IIR(tc)			_MMIO_WD(tc,\
> +					_WD_IIR_0,\
> +					_WD_IIR_1)
> +
> +#define _WD_FRAME_STATUS_0		0x6e568
> +#define _WD_FRAME_STATUS_1		0x6ed68
> +#define WD_FRAME_STATUS(tc)		_MMIO_WD(tc,\
> +					_WD_FRAME_STATUS_0,\
> +					_WD_FRAME_STATUS_1)
> +
> +#define WD_FRAME_COMPLETE		REG_BIT(31)
> +#define WD_STATE_MASK			REG_GENMASK(26, 24)
> +#define WD_STATE_IDLE			REG_FIELD_PREP(WD_STATE_MASK, 0)
> +#define WD_STATE_CAPSTART		REG_FIELD_PREP(WD_STATE_MASK, 1)
> +#define WD_STATE_FRAME_START		REG_FIELD_PREP(WD_STATE_MASK, 2)
> +#define WD_STATE_CAPACITIVE		REG_FIELD_PREP(WD_STATE_MASK, 3)
> +#define WD_STATE_TG_DONE		REG_FIELD_PREP(WD_STATE_MASK, 4)
> +#define WD_STATE_WDX_DONE		REG_FIELD_PREP(WD_STATE_MASK, 5)
> +#define WD_STATE_QUICK_CAP		REG_FIELD_PREP(WD_STATE_MASK, 6)
> +
> +#define _WD_27_M_0			0x6e524
> +#define _WD_27_M_1			0x6ed24
> +#define WD_27_M(tc)			_MMIO_WD(tc,\
> +					_WD_27_M_0,\
> +					_WD_27_M_1)
> +
> +#define _WD_27_N_0			0x6e528
> +
> +/* Address looks wrong in bspec: */
> +#define _WD_27_N_1			0x6ec28
> +#define WD_27_N(tc)			_MMIO_WD(tc,\
> +					_WD_27_N_0,\
> +					_WD_27_N_1)
> +
> +#define _WD_TAIL_CFG_0			0x6e520
> +#define _WD_TAIL_CFG_1			0x6ed20
> +
> +#define WD_TAIL_CFG(tc)			_MMIO_WD(tc,\
> +					_WD_TAIL_CFG_0,\
> +					_WD_TAIL_CFG_1)
> +
> +#endif /* __INTEL_WRITEBACK_REGS_H__ */

RE: [PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Sent: Sunday, July 27, 2025 9:03 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Murthy, Arun R <arun.r.murthy@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes preallocated
> connector
> 
> On Sat, Jul 26, 2025 at 04:41:29PM +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > Sent: Saturday, July 26, 2025 5:46 PM
> > > To: Kandpal, Suraj <suraj.kandpal@intel.com>
> > > Cc: dri-devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org;
> > > intel- gfx@lists.freedesktop.org; Nautiyal, Ankit K
> > > <ankit.k.nautiyal@intel.com>; Murthy, Arun R
> > > <arun.r.murthy@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> > > Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes
> > > preallocated connector
> > >
> > > On Fri, Jul 25, 2025 at 10:33:42AM +0530, Suraj Kandpal wrote:
> > > > Write a function that takes a preallocated drm_connector instead
> > > > of using the one allocated inside the drm writeback connector init
> > > > function.
> > >
> > > Please start your commit message with describing the problem.
> > >
> > > >
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_writeback.c | 76
> > > +++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_writeback.h     |  7 +++
> > > >  2 files changed, 83 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..fa58eb0dc7bf
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > @@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct
> > > > drm_device *dev,  }  EXPORT_SYMBOL(drmm_writeback_connector_init);
> > > >
> > > > +/*
> > > > + * drm_writeback_connector_init_with_conn - Initialize a
> > > > +writeback connector with
> > > > + * custom encoder and connector
> > > > + *
> > > > + * @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 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.
> > >
> > > Why?
> >
> > The problem would that not every want can have a drm_connector
> > embedded inside the drm_writeback_connector We have a restraint where
> > all connectors need to be a intel connector and since the we are not allowed
> to make connector Inside the drm_connector into a pointer this gives a good
> alternative.
> 
> All of this needs to go to the commit message.

Sure will get it there in the next revision.

> 
> >
> > >
> > > > + *
> > > > + * 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_conn(struct drm_device *dev,
> > > > +struct
> > > drm_connector *connector,
> > > > +				       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_mode_config *config = &dev->mode_config;
> > > > +	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;
> > >
> > > This function contans a lot of copy-paste from
> > > __drm_writeback_connector_init(), which is obviously a no-go.
> >
> > The whole point is the minore difference inbetween then and how it
> > derives a lot of things from the drm_writeback_connector because of which
> this looks like a similar function but is essentially different.
> 
> It surely is. This means that you need to extract common code rather than
> duplicate it.

Sure will find a more efficient way to do this in the next revision.

Regards,
Suraj Kandpal

> 
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 02/28] drm/writeback: Add a helper function to get writeback connector

[Kandpal, Suraj]

> > > > Now that we can initialize a drm_writeback_connector without
> > > > having to initialize the drm_connector within it and leaving the
> > > > responsibility of initialising the drm_connector and maintaining
> > > > the association with drm_writeback_connector to it. This helper
> > > > hooks lets drivers return the drm_writeback_connector associated with
> the give drm_connector.
> > > >
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_writeback.c          | 14 ++++++
> > > >  include/drm/drm_modeset_helper_vtables.h | 59
> > > ++++++++++++++++++++++++
> > > >  include/drm/drm_writeback.h              | 14 ++++--
> > > >  3 files changed, 82 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > b/drivers/gpu/drm/drm_writeback.c index fa58eb0dc7bf..e9f7123270d6
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > @@ -107,6 +107,19 @@ static const struct dma_fence_ops
> > > drm_writeback_fence_ops = {
> > > >  	.get_timeline_name = drm_writeback_fence_get_timeline_name,
> > > >  };
> > > >
> > > > +struct drm_writeback_connector *
> > > > +drm_connector_to_writeback(struct drm_connector *connector) {
> > > > +	const struct drm_connector_helper_funcs *funcs =
> > > > +		connector->helper_private;
> > > > +
> > > > +	if (funcs->get_writeback_connector)
> > > > +		return funcs->get_writeback_connector(connector);
> > > > +
> > > > +	return container_of(connector, struct drm_writeback_connector,
> > > > +base); } EXPORT_SYMBOL(drm_connector_to_writeback);
> > > > +
> > > >  static int create_writeback_properties(struct drm_device *dev)  {
> > > >  	struct drm_property *prop;
> > > > @@ -443,6 +456,7 @@ drm_writeback_connector_init_with_conn(struct
> > > drm_device *dev, struct drm_connec
> > > >  				       struct drm_writeback_connector
> > > *wb_connector,
> > > >  				       struct drm_encoder *enc,
> > > >  				       const struct drm_connector_funcs
> > > *con_funcs,
> > > > +				       const struct
> > > drm_writeback_connector_helper_funcs
> > > > +*wb_funcs,
> > > >  				       const u32 *formats, int n_formats)  {
> > > >  	struct drm_property_blob *blob;
> > > > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > > > b/include/drm/drm_modeset_helper_vtables.h
> > > > index ce7c7aeac887..6b89b33d2304 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -31,6 +31,7 @@
> > > >
> > > >  #include <drm/drm_crtc.h>
> > > >  #include <drm/drm_encoder.h>
> > > > +#include <drm/drm_writeback.h>
> > > >
> > > >  /**
> > > >   * DOC: overview
> > > > @@ -1179,6 +1180,25 @@ struct drm_connector_helper_funcs {
> > > >  	 *
> > > >  	 */
> > > >  	void (*disable_hpd)(struct drm_connector *connector);
> > > > +
> > > > +	/**
> > > > +	 * @get_writeback_connector:
> > > > +	 *
> > > > +	 * This callback is used by drivers to get the writeback connector in
> > > > +	 * case the init is done via drm_writeback_init_with_conn. Which
> > > means
> > > > +	 * the drivers don't have drm_connector embedded in
> > > drm_writeback_connector
> > > > +	 * so they need to send the associated writeback connector with this
> > > > +	 * function.
> > > > +	 *
> > > > +	 * This operation is optional.
> > > > +	 *
> > > > +	 * This is mainly called from drm_writeback_set_gb.
> > > > +	 *
> > > > +	 * RETURNS:
> > > > +	 *
> > > > +	 * drm_writeback_connector assoiciated with the drm connector.
> > > > +	 */
> > > > +	struct drm_writeback_connector
> > > > +*(*get_writeback_connector)(struct
> > > > +drm_connector *connector);
> > > >  };
> > > >
> > > >  /**
> > > > @@ -1192,6 +1212,45 @@ static inline void
> > > drm_connector_helper_add(struct drm_connector *connector,
> > > >  	connector->helper_private = funcs;  }
> > > >
> > > > +/**
> > > > + * struct drm_writeback_connector_helper_funcs - helper
> > > > +operations for writeback
> > > > + * connectors.
> > > > + *
> > > > + * These functions are used by the atomic and legacy modeset
> > > > +helpers and by the
> > > > + * probe helpers.
> > > > + */
> > > > +struct drm_writeback_connector_helper_funcs {
> > > > +	/**
> > > > +	 * @get_connector_from_writeback:
> > > > +	 *
> > > > +	 * This callback is used by drivers to get the drm_connector in
> > > > +	 * case the init is done via drm_writeback_init_with_conn. Which
> > > means
> > > > +	 * the drivers don't have drm_connector embedded in
> > > drm_writeback_connector
> > > > +	 * so they need to send the associated drm_connector with this
> > > > +	 * function.
> > > > +	 *
> > > > +	 * This operation is optional.
> > > > +	 *
> > > > +	 * RETURNS:
> > > > +	 *
> > > > +	 * drm_connector assoiciated with the drm_writeback_connector.
> > > > +	 */
> > > > +	struct drm_connector
> > > > +	*(*get_connector_from_writeback)(struct drm_writeback_connector
> > > > +*wbconnector); };
> > > > +
> > > > +/**
> > > > + * drm_writeback_connector_helper_add - sets the helper vtable
> > > > +for a connector
> > > > + * @wb_connector: DRM writeback connector
> > > > + * @funcs: helper vtable to set for @wb_connector  */ static
> > > > +inline void drm_writeback_connector_helper_add(struct
> > > > +drm_writeback_connector *wb_connector,
> > > > +				   const struct
> > > drm_writeback_connector_helper_funcs *funcs) {
> > > > +	wb_connector->helper_private = funcs; }
> > > > +
> > > >  /**
> > > >   * struct drm_plane_helper_funcs - helper operations for planes
> > > >   *
> > > > diff --git a/include/drm/drm_writeback.h
> > > > b/include/drm/drm_writeback.h index 149744dbeef0..77c3c64c132d
> > > > 100644
> > > > --- a/include/drm/drm_writeback.h
> > > > +++ b/include/drm/drm_writeback.h
> > > > @@ -84,6 +84,13 @@ struct drm_writeback_connector {
> > > >  	 * The name of the connector's fence timeline.
> > > >  	 */
> > > >  	char timeline_name[32];
> > > > +
> > > > +	/**
> > > > +	 * @helper_private:
> > > > +	 *
> > > > +	 * helper private funcs for writeback_connector
> > > > +	 */
> > > > +	const struct drm_writeback_connector_helper_funcs
> > > *helper_private;
> > > >  };
> > >
> > > Unrelate to the commit? Also, where is this defined?
> >
> > This is very much related to this commit and defined on top right here in this
> commit.
> 
> Then please split this patch into two.

Sure will do 

Regards,
Suraj Kandpal

> 
> 
> --
> With best wishes
> Dmitry

RE: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Kandpal, Suraj]

> Please tune your mail client to insert smaller quotation headers. This is just
> useless.
> 
> > >
> > > > Now that drm_connector may not always be embedded within
> > > > drm_writeback_connector, let's define a function which either uses
> > > > the writeback helper hook that returns the drm_connector
> > > > associated with the drm_writeback_connector or just return the
> > > > drm_connector embedded inside drm_writeback_connector if the
> > > > helper hook is not present. Lets use this function and call it
> > > > whenever drm_connector is required mostly when connector helper
> private funcs need to be fetched.
> > > >
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_writeback.c | 33
> > > > ++++++++++++++++++++++++++-------
> > > >  1 file changed, 26 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > b/drivers/gpu/drm/drm_writeback.c index e9f7123270d6..d610cb827975
> > > > 100644
> > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> > > drm_connector
> > > > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> > > >
> > > > +static struct drm_connector *
> > > > +drm_connector_from_writeback(struct drm_writeback_connector
> > > > +*wb_connector) {
> > > > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > > > +		wb_connector->helper_private;
> > > > +
> > > > +	if (funcs && funcs->get_connector_from_writeback)
> > > > +		return funcs-
> > > >get_connector_from_writeback(wb_connector);
> > >
> > > The get_connector_from_writeback() and
> > > drm_writeback_connector_helper_funcs should be moved to this patch.
> >
> > Want to keep them separate since they themselves introduce a lot of
> > changes on of them has use introducing a new writeback_helper_function
> structure.
> 
> Let's see how the series will take shape.
> 
> >
> >
> > >
> > > However it feels really awkward to make drm_writeback_connector,
> > > which is a wrapper around the drm_connector, to use some external DRM
> connector.
> > > A quick grepping reveals API (which you missed) that expects
> > > drm_writeback_connector.base to be a valid connector. And it would
> > > be very hard to catch sunch an API later on.
> >
> > Also seems like I did miss the fence_get_driver_name one which is an
> > easy fix or did you see anything else.
> > Really don't see any other problematic areas
> 
> Yes, it was that function. However it is a nice example of how easy it is to miss a
> call. Likewise anybody else changing the code might easily not notice that Intel
> driver uses drm_writeback_connector in a strange way.
> >
> > >
> > > If you want to use DRM framwework, while retaining intel_connector
> > > for writeback connectors, I'd suggest following slightly different
> > > path: extract common parts of drm_writeback_connector into a common
> > > structure, and use it within the DRM core. Then provide functions to
> > > fetch drm_connector from that struct or fetch it from drm_connector.
> >
> > Causes a lot of changes in the drm_writeback_connector structure
> > causing every other driver Using this to change how they essentially
> > call upon drm_writeback_connector. This API was to provide more non
> invasive way to give everyone another alternative.
> 
> Currently drm_writeback_connector is documented and implemented as being
> a wrapper around drm_connector. You are changing that contract in a non-
> intuitive way. I think there are several options on how to proceed:
> 
> - Clearly and loudly document that drm_writeback_connector is no longer
>   a wrapper around drm_connector. Clearly document how to distinguish
>   those two cases. In my opinion this is the worst option as it is
>   significantly error-prone
> 

I think this is already done when drm_writeback_connector_init_with_conn is
Defined

> - Make sure that the DRM framework can use writeback without
>   drm_writeback_connector and them implement all necessary plumbing in
>   the Intel driver. This can result in singnificant amount of code
>   duplication, so I'd skip this option.

Hmm Agreed.

> 
> - Separate writeback parts of drm_writeback_connector into a struct,
>   make drm_writeback_connector include drm_connector, new struct and
>   most likely drm_encoder. Implement conversion callbacks (like you did
>   in your patchset).

Again a lot of changes to other drivers which everyone will resist.
Something like this was tried previously with both encoder and connector
which was not accepted leading the patch series towards creation 
of the drm_writeback_connector_init_with_encoder.

> 
> - Rework drm_writeback_connector and drm_connector in a similar way, but
>   use writeback structure as a field inside drm_connector (similar to
>   how we got the HDMI data). This saves you from having all conversion
>   callbacks and makes it extensible for other drivers too. In fact you
>   can use an anonymous union, making sure that HDMI and writeback
>   structures take the same space in memory.

The idea of not having it inside drm_connector was that it's not a "real connector"
and we should not be treating it like one which makes me a little doubtful on if the
community will go for this.

> 
> My preference is shifted towards the last option, it follows current HDMI
> subclassing design and it doesn't add unnecessary complexity.
> 
> Yes, this requires reworking of all writeback drivers. Yes, it's a price of having
> your own subclass of drm_connector. No, in my optionion, leaving a semi-
> broken abstraction is not an option. Whatever path gets implemented, it should
> be internally coherent.

Well to be honest this has already been done with drm_encoder which is placed
Inside drm_writeback_connector with drm_writeback_connector_init_encoder
so this is not something very unintuitive. Also I feel messing with all other drivers by changing
writeback structure is the more error prone way to go about it. Also it will be understood that
drm_writeback_connector does not contain drm_connector to those using this API as it
will be documented. So its not really the semi-broken abstraction.

Regards,
Suraj Kandpal

> With best wishes
> Dmitry

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Dmitry Baryshkov]

On Fri, Aug 01, 2025 at 05:18:47AM +0000, Kandpal, Suraj wrote:
> > Please tune your mail client to insert smaller quotation headers. This is just
> > useless.
> > 
> > > >
> > > > > Now that drm_connector may not always be embedded within
> > > > > drm_writeback_connector, let's define a function which either uses
> > > > > the writeback helper hook that returns the drm_connector
> > > > > associated with the drm_writeback_connector or just return the
> > > > > drm_connector embedded inside drm_writeback_connector if the
> > > > > helper hook is not present. Lets use this function and call it
> > > > > whenever drm_connector is required mostly when connector helper
> > private funcs need to be fetched.
> > > > >
> > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_writeback.c | 33
> > > > > ++++++++++++++++++++++++++-------
> > > > >  1 file changed, 26 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > > b/drivers/gpu/drm/drm_writeback.c index e9f7123270d6..d610cb827975
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> > > > drm_connector
> > > > > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> > > > >
> > > > > +static struct drm_connector *
> > > > > +drm_connector_from_writeback(struct drm_writeback_connector
> > > > > +*wb_connector) {
> > > > > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > > > > +		wb_connector->helper_private;
> > > > > +
> > > > > +	if (funcs && funcs->get_connector_from_writeback)
> > > > > +		return funcs-
> > > > >get_connector_from_writeback(wb_connector);
> > > >
> > > > The get_connector_from_writeback() and
> > > > drm_writeback_connector_helper_funcs should be moved to this patch.
> > >
> > > Want to keep them separate since they themselves introduce a lot of
> > > changes on of them has use introducing a new writeback_helper_function
> > structure.
> > 
> > Let's see how the series will take shape.
> > 
> > >
> > >
> > > >
> > > > However it feels really awkward to make drm_writeback_connector,
> > > > which is a wrapper around the drm_connector, to use some external DRM
> > connector.
> > > > A quick grepping reveals API (which you missed) that expects
> > > > drm_writeback_connector.base to be a valid connector. And it would
> > > > be very hard to catch sunch an API later on.
> > >
> > > Also seems like I did miss the fence_get_driver_name one which is an
> > > easy fix or did you see anything else.
> > > Really don't see any other problematic areas
> > 
> > Yes, it was that function. However it is a nice example of how easy it is to miss a
> > call. Likewise anybody else changing the code might easily not notice that Intel
> > driver uses drm_writeback_connector in a strange way.
> > >
> > > >
> > > > If you want to use DRM framwework, while retaining intel_connector
> > > > for writeback connectors, I'd suggest following slightly different
> > > > path: extract common parts of drm_writeback_connector into a common
> > > > structure, and use it within the DRM core. Then provide functions to
> > > > fetch drm_connector from that struct or fetch it from drm_connector.
> > >
> > > Causes a lot of changes in the drm_writeback_connector structure
> > > causing every other driver Using this to change how they essentially
> > > call upon drm_writeback_connector. This API was to provide more non
> > invasive way to give everyone another alternative.
> > 
> > Currently drm_writeback_connector is documented and implemented as being
> > a wrapper around drm_connector. You are changing that contract in a non-
> > intuitive way. I think there are several options on how to proceed:
> > 
> > - Clearly and loudly document that drm_writeback_connector is no longer
> >   a wrapper around drm_connector. Clearly document how to distinguish
> >   those two cases. In my opinion this is the worst option as it is
> >   significantly error-prone
> > 
> 
> I think this is already done when drm_writeback_connector_init_with_conn is
> Defined

No. You also need to update drm_writeback_connector documentation, etc.

> 
> > - Make sure that the DRM framework can use writeback without
> >   drm_writeback_connector and them implement all necessary plumbing in
> >   the Intel driver. This can result in singnificant amount of code
> >   duplication, so I'd skip this option.
> 
> Hmm Agreed.
> 
> > 
> > - Separate writeback parts of drm_writeback_connector into a struct,
> >   make drm_writeback_connector include drm_connector, new struct and
> >   most likely drm_encoder. Implement conversion callbacks (like you did
> >   in your patchset).
> 
> Again a lot of changes to other drivers which everyone will resist.
> Something like this was tried previously with both encoder and connector
> which was not accepted leading the patch series towards creation 
> of the drm_writeback_connector_init_with_encoder.
> 
> > 
> > - Rework drm_writeback_connector and drm_connector in a similar way, but
> >   use writeback structure as a field inside drm_connector (similar to
> >   how we got the HDMI data). This saves you from having all conversion
> >   callbacks and makes it extensible for other drivers too. In fact you
> >   can use an anonymous union, making sure that HDMI and writeback
> >   structures take the same space in memory.
> 
> The idea of not having it inside drm_connector was that it's not a "real connector"
> and we should not be treating it like one which makes me a little doubtful on if the
> community will go for this.

Well... It is a "real" connector, otherwise e.g. Intel wouldn't have to
wrap it into an intel_connector structure. I think this is more of the
historical behaviour - to wrap the structure instead of adding data to
it. HDMI connector showed that it's much easier to add data, so I assume
it would be a preferred approach.

> 
> > 
> > My preference is shifted towards the last option, it follows current HDMI
> > subclassing design and it doesn't add unnecessary complexity.
> > 
> > Yes, this requires reworking of all writeback drivers. Yes, it's a price of having
> > your own subclass of drm_connector. No, in my optionion, leaving a semi-
> > broken abstraction is not an option. Whatever path gets implemented, it should
> > be internally coherent.
> 
> Well to be honest this has already been done with drm_encoder which is placed
> Inside drm_writeback_connector with drm_writeback_connector_init_encoder
> so this is not something very unintuitive. Also I feel messing with all other drivers by changing
> writeback structure is the more error prone way to go about it.

This is what we frequently have to do: change other drivers and depend
on developers testing them.

For the reference, currently only the following drivers implement
writeback. I think it's a pretty manageable change:

- AMDGPU
- ARM/Komeda
- ARM/Mali
- MSM/dpu1
- R-Car
- VC4
- VKMS

Yes, it requires some effort. But I think that it's better than just
making drm_connector part optional. 

> Also it will be understood that
> drm_writeback_connector does not contain drm_connector to those using this API as it
> will be documented. So its not really the semi-broken abstraction.

Thinking in OOP terms, the encoder is just a field in the struct.
drm_connector is a base class for drm_writeback_connector. By making it
optional, you are definitely semi-breaking the abstraction.



-- 
With best wishes
Dmitry

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Jani Nikula]

On Fri, 01 Aug 2025, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> Thinking in OOP terms, the encoder is just a field in the struct.
> drm_connector is a base class for drm_writeback_connector. By making it
> optional, you are definitely semi-breaking the abstraction.

The trouble is, in OOP terms, drm_connector is the "base class" for both
drm_writeback_connector and intel_connector. We're already stretching
what we can do with C.

Currently, it's always guaranteed all drm_connectors i915 ever sees are
embedded within intel_connector. Changing from one pointer to another is
trivial, guaranteed to work, and is never NULL if the source pointer is
non-NULL. This is a design that's been around for the longest time.

The current writeback implementation forces a different design by always
embedding drm_connector itself. We can't use a drm_connector that's
embedded within an intel_connector with it. If we want to have our own
stuff, we'd need an intel_writeback_connector wrapping
drm_writeback_connector, and it gets even more complicated with all the
interfaces that use intel_connector. It really shouldn't have to be this
way.

Using the current drm_writeback_connector in i915 requires careful
auditing of all drm_connector <-> intel_connector conversions, NULL
checks, and graceful error handling, also in places that have no
convenient way to return errors at all.

The OOP abstractions just break hard with C, we can't have multiple
inheritance, and IMO the pragmatic approach is to let *drivers* do what
they want, instead of having a midlayer helper design force something on
them.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Dmitry Baryshkov]

On Fri, Aug 01, 2025 at 02:57:48PM +0300, Jani Nikula wrote:
> On Fri, 01 Aug 2025, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> > Thinking in OOP terms, the encoder is just a field in the struct.
> > drm_connector is a base class for drm_writeback_connector. By making it
> > optional, you are definitely semi-breaking the abstraction.
> 
> The trouble is, in OOP terms, drm_connector is the "base class" for both
> drm_writeback_connector and intel_connector. We're already stretching
> what we can do with C.
> 
> Currently, it's always guaranteed all drm_connectors i915 ever sees are
> embedded within intel_connector. Changing from one pointer to another is
> trivial, guaranteed to work, and is never NULL if the source pointer is
> non-NULL. This is a design that's been around for the longest time.
> 
> The current writeback implementation forces a different design by always
> embedding drm_connector itself. We can't use a drm_connector that's
> embedded within an intel_connector with it. If we want to have our own
> stuff, we'd need an intel_writeback_connector wrapping
> drm_writeback_connector, and it gets even more complicated with all the
> interfaces that use intel_connector. It really shouldn't have to be this
> way.
> 
> Using the current drm_writeback_connector in i915 requires careful
> auditing of all drm_connector <-> intel_connector conversions, NULL
> checks, and graceful error handling, also in places that have no
> convenient way to return errors at all.
> 
> The OOP abstractions just break hard with C, we can't have multiple
> inheritance, and IMO the pragmatic approach is to let *drivers* do what
> they want, instead of having a midlayer helper design force something on
> them.

Yes. Basically, that's why I suggest refacoring drm_writeback_connector
to mvoe drm_connector_writeback into drm_connector itself (like it's done
for drm_connector_hdmi).

-- 
With best wishes
Dmitry

Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Jani Nikula]

On Fri, 01 Aug 2025, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> Yes. Basically, that's why I suggest refacoring drm_writeback_connector
> to mvoe drm_connector_writeback into drm_connector itself (like it's done
> for drm_connector_hdmi).

Ah, sorry, I think I missed that portion in my post-vacation email
catch-up.

BR,
Jani.

-- 
Jani Nikula, Intel

RE: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Kandpal, Suraj]

> Subject: Re: [PATCH 03/28] drm/writeback: Define function to get
> drm_connector from writeback
> 
> > > > >
> > > > > > Now that drm_connector may not always be embedded within
> > > > > > drm_writeback_connector, let's define a function which either
> > > > > > uses the writeback helper hook that returns the drm_connector
> > > > > > associated with the drm_writeback_connector or just return the
> > > > > > drm_connector embedded inside drm_writeback_connector if the
> > > > > > helper hook is not present. Lets use this function and call it
> > > > > > whenever drm_connector is required mostly when connector
> > > > > > helper
> > > private funcs need to be fetched.
> > > > > >
> > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_writeback.c | 33
> > > > > > ++++++++++++++++++++++++++-------
> > > > > >  1 file changed, 26 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > > > b/drivers/gpu/drm/drm_writeback.c index
> > > > > > e9f7123270d6..d610cb827975
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > @@ -120,6 +120,18 @@ drm_connector_to_writeback(struct
> > > > > drm_connector
> > > > > > *connector)  }  EXPORT_SYMBOL(drm_connector_to_writeback);
> > > > > >
> > > > > > +static struct drm_connector *
> > > > > > +drm_connector_from_writeback(struct drm_writeback_connector
> > > > > > +*wb_connector) {
> > > > > > +	const struct drm_writeback_connector_helper_funcs *funcs =
> > > > > > +		wb_connector->helper_private;
> > > > > > +
> > > > > > +	if (funcs && funcs->get_connector_from_writeback)
> > > > > > +		return funcs-
> > > > > >get_connector_from_writeback(wb_connector);
> > > > >
> > > > > The get_connector_from_writeback() and
> > > > > drm_writeback_connector_helper_funcs should be moved to this patch.
> > > >
> > > > Want to keep them separate since they themselves introduce a lot
> > > > of changes on of them has use introducing a new
> > > > writeback_helper_function
> > > structure.
> > >
> > > Let's see how the series will take shape.
> > >
> > > >
> > > >
> > > > >
> > > > > However it feels really awkward to make drm_writeback_connector,
> > > > > which is a wrapper around the drm_connector, to use some
> > > > > external DRM
> > > connector.
> > > > > A quick grepping reveals API (which you missed) that expects
> > > > > drm_writeback_connector.base to be a valid connector. And it
> > > > > would be very hard to catch sunch an API later on.
> > > >
> > > > Also seems like I did miss the fence_get_driver_name one which is
> > > > an easy fix or did you see anything else.
> > > > Really don't see any other problematic areas
> > >
> > > Yes, it was that function. However it is a nice example of how easy
> > > it is to miss a call. Likewise anybody else changing the code might
> > > easily not notice that Intel driver uses drm_writeback_connector in a
> strange way.
> > > >
> > > > >
> > > > > If you want to use DRM framwework, while retaining
> > > > > intel_connector for writeback connectors, I'd suggest following
> > > > > slightly different
> > > > > path: extract common parts of drm_writeback_connector into a
> > > > > common structure, and use it within the DRM core. Then provide
> > > > > functions to fetch drm_connector from that struct or fetch it from
> drm_connector.
> > > >
> > > > Causes a lot of changes in the drm_writeback_connector structure
> > > > causing every other driver Using this to change how they
> > > > essentially call upon drm_writeback_connector. This API was to
> > > > provide more non
> > > invasive way to give everyone another alternative.
> > >
> > > Currently drm_writeback_connector is documented and implemented as
> > > being a wrapper around drm_connector. You are changing that contract
> > > in a non- intuitive way. I think there are several options on how to proceed:
> > >
> > > - Clearly and loudly document that drm_writeback_connector is no longer
> > >   a wrapper around drm_connector. Clearly document how to distinguish
> > >   those two cases. In my opinion this is the worst option as it is
> > >   significantly error-prone
> > >
> >
> > I think this is already done when
> > drm_writeback_connector_init_with_conn is Defined
> 
> No. You also need to update drm_writeback_connector documentation, etc.
> 
> >
> > > - Make sure that the DRM framework can use writeback without
> > >   drm_writeback_connector and them implement all necessary plumbing in
> > >   the Intel driver. This can result in singnificant amount of code
> > >   duplication, so I'd skip this option.
> >
> > Hmm Agreed.
> >
> > >
> > > - Separate writeback parts of drm_writeback_connector into a struct,
> > >   make drm_writeback_connector include drm_connector, new struct and
> > >   most likely drm_encoder. Implement conversion callbacks (like you did
> > >   in your patchset).
> >
> > Again a lot of changes to other drivers which everyone will resist.
> > Something like this was tried previously with both encoder and
> > connector which was not accepted leading the patch series towards
> > creation of the drm_writeback_connector_init_with_encoder.
> >
> > >
> > > - Rework drm_writeback_connector and drm_connector in a similar way,
> but
> > >   use writeback structure as a field inside drm_connector (similar to
> > >   how we got the HDMI data). This saves you from having all conversion
> > >   callbacks and makes it extensible for other drivers too. In fact you
> > >   can use an anonymous union, making sure that HDMI and writeback
> > >   structures take the same space in memory.
> >
> > The idea of not having it inside drm_connector was that it's not a "real
> connector"
> > and we should not be treating it like one which makes me a little
> > doubtful on if the community will go for this.
> 
> Well... It is a "real" connector, otherwise e.g. Intel wouldn't have to wrap it into
> an intel_connector structure. I think this is more of the historical behaviour - to
> wrap the structure instead of adding data to it. HDMI connector showed that
> it's much easier to add data, so I assume it would be a preferred approach.

Sadly the drm_framework does not treat it as such and again as Jani had pointed out this should have used
drm_connector and drm_encoder as pointers within drm_writeback_connector personally I can go ahead with
designing either of the 3 designs available but let us get a consensus here on which design to go for since I had floated
a RFC for this design to get an idea of which design to go for before coming to this design and doing the final work.
Adding some more people here to get the discussion going.

Regards,
Suraj Kandpal

> 
> >
> > >
> > > My preference is shifted towards the last option, it follows current
> > > HDMI subclassing design and it doesn't add unnecessary complexity.
> > >
> > > Yes, this requires reworking of all writeback drivers. Yes, it's a
> > > price of having your own subclass of drm_connector. No, in my
> > > optionion, leaving a semi- broken abstraction is not an option.
> > > Whatever path gets implemented, it should be internally coherent.
> >
> > Well to be honest this has already been done with drm_encoder which is
> > placed Inside drm_writeback_connector with
> > drm_writeback_connector_init_encoder
> > so this is not something very unintuitive. Also I feel messing with
> > all other drivers by changing writeback structure is the more error prone way
> to go about it.
> 
> This is what we frequently have to do: change other drivers and depend on
> developers testing them.
> 
> For the reference, currently only the following drivers implement writeback. I
> think it's a pretty manageable change:
> 
> - AMDGPU
> - ARM/Komeda
> - ARM/Mali
> - MSM/dpu1
> - R-Car
> - VC4
> - VKMS
> 
> Yes, it requires some effort. But I think that it's better than just making
> drm_connector part optional.
> 
> > Also it will be understood that
> > drm_writeback_connector does not contain drm_connector to those using
> > this API as it will be documented. So its not really the semi-broken
> abstraction.
> 
> Thinking in OOP terms, the encoder is just a field in the struct.
> drm_connector is a base class for drm_writeback_connector. By making it
> optional, you are definitely semi-breaking the abstraction.
> 
> 
> 
> --
> With best wishes
> Dmitry