Re: [Intel-gfx] RFC: pipe writeback design for i915

* Re: [Intel-gfx] RFC: pipe writeback design for i915
@ 2020-02-04  8:05 Bharadiya,Pankaj
  2020-02-04 23:43 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Bharadiya,Pankaj @ 2020-02-04  8:05 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> Sent: Friday, January 31, 2020 5:28 PM
> To: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] RFC: pipe writeback design for i915
> 
> On Fri, Jan 31, 2020 at 12:00:39PM +0530, Bharadiya,Pankaj wrote:
> > I am exploring the way of implementing the pipe writeback feature in 
> > i915 and would like to get early feedback on design.

[snip]

> > 
> > 1# Extend the intel_connector to support writeback
> > --------------------------------------------------
> > 
> > drm_writeback connector is of drm_connector type and intel_connector 
> > is also of drm_connector type.
> > 
> >   +-----------------------------------------------------------------------------+
> >   |                                     |                                       |
> >   | struct drm_writeback_connector {    |    struct intel_connector {           |
> >   |         struct drm_connector base;  |            struct drm_connector base; |
> >   |         .                           |            .                          |
> >   |         .                           |            .                          |
> >   |         .                           |            .                          |
> >   | };                                  |    };                                 |
> >   |                                     |                                       |
> >   
> > +---------------------------------------------------------------------
> > --------+
> 
> That's a bit unfortunate. We like to use intel_connector quite a bit in
> i915 so having two different types is going to be a pita. Ideally I guess the writeback connector shouldn't be a drm_connector at all and instead it would just provide some kind of thing to embed into the driver's connector struct. But that would mean the writeback helpers would need some other way to get at that data rather than just container_of().

I am thinking of the following -

- Modify the struct drm_writeback_connector accept drm_connector pointer (*base)
- Add new member in struct drm_connector to save struct drm_writeback_connector
  pointer so that drm_writeback_connector can be found using given a drm_connector.
- Modify existing drivers (rcar_du, arm/malidp, arm/komeda, vc4) which are
  implementing drm_writeback to adapt to this new change.

Here is the example patch I came with -

----------------------

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 43d9e3bb3a94..cb4434baa2eb 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 *
@@ -178,7 +178,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);
 
@@ -198,6 +198,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 		goto fail;
 
 	connector->interlace_allowed = 0;
+	connector->wb_connector = wb_connector;
 
 	ret = drm_connector_init(dev, connector, con_funcs,
 				 DRM_MODE_CONNECTOR_WRITEBACK);
@@ -264,7 +265,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) {
@@ -316,7 +317,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);
@@ -402,7 +403,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 2113500b4075..edd153f1815e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -42,6 +42,7 @@ struct drm_property_blob;
 struct drm_printer;
 struct edid;
 struct i2c_adapter;
+struct drm_writeback_connector;
 
 enum drm_connector_force {
 	DRM_FORCE_UNSPECIFIED,
@@ -1315,6 +1316,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 777c14c847f0..51a94c6a4ae3 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -16,7 +16,7 @@
 #include <linux/workqueue.h>
 
 struct drm_writeback_connector {
-	struct drm_connector base;
+	struct drm_connector *base;
 
 	/**
 	 * @encoder: Internal encoder used by the connector to fulfill
@@ -134,7 +134,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,

---------------------


With this, we should be able to extend intel_connector to support writeback.

struct intel_connector {
        struct drm_connector base;
+	struct drm_writeback_connector wb_conn;
.
.
.
}

Example usage:
	struct intel_connector *intel_connector;
	intel_connector = intel_connector_alloc();

	intel_connector->wb_conn.base = &intel_connector->base;

	/* Initialize writeback connector */
	drm_writeback_connector_init(...,&intel_connector->wb_conn, ...);


What do you think?

Thanks,
Pankaj 

> 
> --
> Ville Syrjälä
> Intel
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki 
> 
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 5+ messages in thread