Re: [PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector
Hi Livid
Thanks for your review.
All your comments are valid. I think I should re-order the patches like
you have suggested. That should address all comments.
Thanks
Abhinav
On 3/21/2022 10:24 AM, Liviu Dudau wrote:
On Thu, Mar 17, 2022 at 06:45:34PM -0700, Abhinav Kumar wrote:
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v4:
- split the possible_crtcs change and the parts which should
belong to the addition of new API to the next change
Co-developed-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 12 +++-
drivers/gpu/drm/vc4/vc4_txp.c | 14 ++
include/drm/drm_writeback.h | 18 --
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..a4c17d6 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -190,11 +190,13 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->encoder = _connector->internal_encoder;
You need to check here that the wb_connector doesn't have already an attached
encoder
before you overwrite the pointer with the internal encoder.
- ret = drm_encoder_init(dev, _connector->encoder,
+ wb_connector->encoder->possible_crtcs = possible_crtcs;
+
+ ret = drm_encoder_init(dev, wb_connector->encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
Here you have initialised the encoder pointed at by wb_connector->encoder,
which is
always wb_connector->internal_encoder with your code.
if (ret)
@@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
goto connector_fail;
ret = drm_connector_attach_encoder(connector,
- _connector->encoder);
+ wb_connector->encoder);
if (ret)
goto attach_fail;
@@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(wb_connector->encoder);
fail:
drm_property_blob_put(blob);
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3447eb6..341a9be5 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
struct vc4_txp *txp;
struct drm_crtc *crtc;
struct drm_encoder *encoder;
+ struct drm_writeback_connector *wb_conn;
int ret, irq;
irq = platform_get_irq(pdev, 0);
@@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
txp->regset.regs = txp_regs;
txp->regset.nregs = ARRAY_SIZE(txp_regs);
- drm_connector_helper_add(>connector.base,
+ wb_conn = >connector;
+ wb_conn->encoder = >drm_enc;
+
+ drm_connector_helper_add(_conn->base,
_txp_connector_helper_funcs);
- ret = drm_writeback_connector_init(drm, >connector,
+ ret = drm_writeback_connector_init(drm, wb_conn,
_txp_connector_funcs,
_txp_encoder_helper_funcs,
drm_fmts, ARRAY_SIZE(drm_fmts),
This call will never initialise the txp->drm_enc, as per my comments above.
However
if this was the intent, it's fine, but then you need to add a
drm_encoder_init() call
here for txp->drm_enc. Otherwise, you need to stop overwriting the pointer in
Re: [PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector
On Thu, Mar 17, 2022 at 06:45:34PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
>
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
>
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
>
> changes in v4:
> - split the possible_crtcs change and the parts which should
> belong to the addition of new API to the next change
>
> Co-developed-by: Kandpal Suraj
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/drm_writeback.c | 12 +++-
> drivers/gpu/drm/vc4/vc4_txp.c | 14 ++
> include/drm/drm_writeback.h | 18 --
> 3 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..a4c17d6 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -190,11 +190,13 @@ int drm_writeback_connector_init(struct drm_device *dev,
> if (IS_ERR(blob))
> return PTR_ERR(blob);
>
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->encoder = _connector->internal_encoder;
You need to check here that the wb_connector doesn't have already an attached
encoder
before you overwrite the pointer with the internal encoder.
>
> - ret = drm_encoder_init(dev, _connector->encoder,
> + wb_connector->encoder->possible_crtcs = possible_crtcs;
> +
> + ret = drm_encoder_init(dev, wb_connector->encoder,
> _writeback_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
Here you have initialised the encoder pointed at by wb_connector->encoder,
which is
always wb_connector->internal_encoder with your code.
> if (ret)
> @@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> goto connector_fail;
>
> ret = drm_connector_attach_encoder(connector,
> - _connector->encoder);
> + wb_connector->encoder);
> if (ret)
> goto attach_fail;
>
> @@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> attach_fail:
> drm_connector_cleanup(connector);
> connector_fail:
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(wb_connector->encoder);
> fail:
> drm_property_blob_put(blob);
> return ret;
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 3447eb6..341a9be5 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -151,6 +151,8 @@ struct vc4_txp {
>
> struct platform_device *pdev;
>
> + struct drm_encoder drm_enc;
> +
> struct drm_writeback_connector connector;
>
> void __iomem *regs;
> @@ -159,7 +161,7 @@ struct vc4_txp {
>
> static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
> {
> - return container_of(encoder, struct vc4_txp, connector.encoder);
> + return container_of(encoder, struct vc4_txp, drm_enc);
> }
>
> static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector
> *conn)
> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device
> *master, void *data)
> struct vc4_txp *txp;
> struct drm_crtc *crtc;
> struct drm_encoder *encoder;
> + struct drm_writeback_connector *wb_conn;
> int ret, irq;
>
> irq = platform_get_irq(pdev, 0);
> @@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct
> device *master, void *data)
> txp->regset.regs = txp_regs;
> txp->regset.nregs = ARRAY_SIZE(txp_regs);
>
> - drm_connector_helper_add(>connector.base,
> + wb_conn = >connector;
> + wb_conn->encoder = >drm_enc;
> +
> + drm_connector_helper_add(_conn->base,
>_txp_connector_helper_funcs);
> - ret = drm_writeback_connector_init(drm, >connector,
> + ret = drm_writeback_connector_init(drm, wb_conn,
> _txp_connector_funcs,
> _txp_encoder_helper_funcs,
> drm_fmts, ARRAY_SIZE(drm_fmts),
This call will never initialise the txp->drm_enc, as per my comments above.
However
if this was the intent, it's fine, but then you need to add a
drm_encoder_init() call
here for txp->drm_enc. Otherwise, you need to stop overwriting the pointer in
drm_writeback_connector_init().
> @@ -507,7 +513,7 @@ static int vc4_txp_bind(struct device *dev, struct
[PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v4:
- split the possible_crtcs change and the parts which should
belong to the addition of new API to the next change
Co-developed-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 12 +++-
drivers/gpu/drm/vc4/vc4_txp.c | 14 ++
include/drm/drm_writeback.h | 18 --
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..a4c17d6 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -190,11 +190,13 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->encoder = _connector->internal_encoder;
- ret = drm_encoder_init(dev, _connector->encoder,
+ wb_connector->encoder->possible_crtcs = possible_crtcs;
+
+ ret = drm_encoder_init(dev, wb_connector->encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
@@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
goto connector_fail;
ret = drm_connector_attach_encoder(connector,
- _connector->encoder);
+ wb_connector->encoder);
if (ret)
goto attach_fail;
@@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(wb_connector->encoder);
fail:
drm_property_blob_put(blob);
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3447eb6..341a9be5 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
struct vc4_txp *txp;
struct drm_crtc *crtc;
struct drm_encoder *encoder;
+ struct drm_writeback_connector *wb_conn;
int ret, irq;
irq = platform_get_irq(pdev, 0);
@@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
txp->regset.regs = txp_regs;
txp->regset.nregs = ARRAY_SIZE(txp_regs);
- drm_connector_helper_add(>connector.base,
+ wb_conn = >connector;
+ wb_conn->encoder = >drm_enc;
+
+ drm_connector_helper_add(_conn->base,
_txp_connector_helper_funcs);
- ret = drm_writeback_connector_init(drm, >connector,
+ ret = drm_writeback_connector_init(drm, wb_conn,
_txp_connector_funcs,
_txp_encoder_helper_funcs,
drm_fmts, ARRAY_SIZE(drm_fmts),
@@ -507,7 +513,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index db6214f..c525b60 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,15 +25,29 @@ struct drm_writeback_connector {
struct drm_connector base;
/**
-* @encoder: Internal encoder used by the connector to fulfill
+* @encoder: handle to drm_encoder used by the connector to fulfill
* the DRM framework requirements. The users of the
