Re: [PATCH 1/3] drm: add writeback pointers to drm_connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



+ laurent on this

Hi Suraj

On 1/11/2022 2:17 AM, Kandpal, Suraj wrote:
Changing drm_connector and drm_encoder feilds to pointers in
drm_writeback_connector as the elements of struct
drm_writeback_connector are:
struct drm_writeback_connector {
	struct drm_connector base;
	struct drm_encoder encoder;
Similarly the elements of intel_encoder and intel_connector
are:
struct intel_encoder {
	struct drm_encoder base;

struct intel_connector {
	struct drm_connector base;

The function drm_writeback_connector_init() will initialize the
drm_connector and drm_encoder and attach them as well.
Since the drm_connector/encoder are both struct in
drm_writeback_connector and intel_connector/encoder, we need
one of them to be a pointer so we can reference them or else we
will be pointing to 2 seprate instances.

Usually the struct defined in drm framework pointing to any
struct will be pointer and allocating them and initialization
will be done with the users.
Like struct drm_connector and drm_encoder are part of drm
framework and the users of these such as i915 have included them
in their struct intel_connector and intel_encoder. Likewise
struct drm_writeback_connector is a special connector and hence
is not a user of drm_connector and hence this should be pointers.

Adding drm_writeback_connector to drm_connector so that
writeback_connector can be fetched from drm_connector as the previous
container_of method won't work due to change in the feilds of
drm_connector and drm_encoder in drm_writeback_connector.

Note:The corresponding ripple effect due to the above changes namely in
two drivers as I can see it komeda and vkms have been dealt with in the
upcoming patches of this series.

Signed-off-by: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>

Jani pointed me to this thread as i had posted something similar here : https://patchwork.freedesktop.org/patch/470296/ but since this thread was posted earlier, we can discuss further here.

Overall, its similar to what I had posted in the RFC and your commit text also covers my concerns too.

One question I have about your change is since you have changed wb_connector::encoder to be a pointer, i saw the other changes in the series but they do not allocate an encoder. Would this not affect the other drivers which are assuming that the encoder in wb_connector is
struct drm_encoder encoder and not struct drm_encoder* encoder.

Your changes fix the compilation issue but wouldnt this crash as encoder
wasnt allocated for other drivers.

---
  drivers/gpu/drm/drm_writeback.c | 19 ++++++++++---------
  include/drm/drm_connector.h     |  3 +++
  include/drm/drm_writeback.h     |  6 +++---
  3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504f1bb..47238db42363 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
  	struct drm_writeback_connector *wb_connector =
  		fence_to_wb_connector(fence);
- return wb_connector->base.dev->driver->name;
+	return wb_connector->base->dev->driver->name;
  }
static const char *
@@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
  				 const u32 *formats, int n_formats)
  {
  	struct drm_property_blob *blob;
-	struct drm_connector *connector = &wb_connector->base;
+	struct drm_connector *connector = wb_connector->base;
  	struct drm_mode_config *config = &dev->mode_config;
  	int ret = create_writeback_properties(dev);
@@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev,
  	if (IS_ERR(blob))
  		return PTR_ERR(blob);
- drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
-	ret = drm_encoder_init(dev, &wb_connector->encoder,
+	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+	ret = drm_encoder_init(dev, wb_connector->encoder,
  			       &drm_writeback_encoder_funcs,
  			       DRM_MODE_ENCODER_VIRTUAL, NULL);
  	if (ret)
  		goto fail;
connector->interlace_allowed = 0;
+	connector->wb_connector = wb_connector;
ret = drm_connector_init(dev, connector, con_funcs,
  				 DRM_MODE_CONNECTOR_WRITEBACK);
@@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
  		goto connector_fail;
ret = drm_connector_attach_encoder(connector,
-						&wb_connector->encoder);
+						wb_connector->encoder);
  	if (ret)
  		goto attach_fail;
@@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
  attach_fail:
  	drm_connector_cleanup(connector);
  connector_fail:
-	drm_encoder_cleanup(&wb_connector->encoder);
+	drm_encoder_cleanup(wb_connector->encoder);
  fail:
  	drm_property_blob_put(blob);
  	return ret;
@@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job)
  {
  	struct drm_writeback_connector *connector = job->connector;
  	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->base->helper_private;
  	int ret;
if (funcs->prepare_writeback_job) {
@@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job)
  {
  	struct drm_writeback_connector *connector = job->connector;
  	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->base->helper_private;
if (job->prepared && funcs->cleanup_writeback_job)
  		funcs->cleanup_writeback_job(connector, job);
@@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
  {
  	struct dma_fence *fence;
- if (WARN_ON(wb_connector->base.connector_type !=
+	if (WARN_ON(wb_connector->base->connector_type !=
  		    DRM_MODE_CONNECTOR_WRITEBACK))
  		return NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b501d0badaea..ec4657cfd7b9 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -44,6 +44,7 @@ struct drm_printer;
  struct drm_privacy_screen;
  struct edid;
  struct i2c_adapter;
+struct drm_writeback_connector;
enum drm_connector_force {
  	DRM_FORCE_UNSPECIFIED,
@@ -1533,6 +1534,8 @@ struct drm_connector {
  	 */
  	struct drm_encoder *encoder;
+ struct drm_writeback_connector *wb_connector;
+
  #define MAX_ELD_BYTES	128
  	/** @eld: EDID-like data, if present */
  	uint8_t eld[MAX_ELD_BYTES];
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 9697d2714d2a..078c9907219c 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -22,7 +22,7 @@ struct drm_writeback_connector {
  	/**
  	 * @base: base drm_connector object
  	 */
-	struct drm_connector base;
+	struct drm_connector *base;
/**
  	 * @encoder: Internal encoder used by the connector to fulfill
@@ -31,7 +31,7 @@ struct drm_writeback_connector {
  	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
  	 * function.
  	 */
-	struct drm_encoder encoder;
+	struct drm_encoder *encoder;
/**
  	 * @pixel_formats_blob_ptr:
@@ -143,7 +143,7 @@ struct drm_writeback_job {
  static inline struct drm_writeback_connector *
  drm_connector_to_writeback(struct drm_connector *connector)
  {
-	return container_of(connector, struct drm_writeback_connector, base);
+	return connector->wb_connector;
  }
int drm_writeback_connector_init(struct drm_device *dev,



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux