> -----Original Message-----
> From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On Behalf Of Ville
> Syrjälä
> Sent: Friday, January 31, 2020 5:28 PM
> To: Laxminarayan Bharadiya, Pankaj <pankaj.laxminarayan.bharad...@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
