There are some vendor drivers for which the writeback encoder shares hardware resources such as clocks and interrupts with the rest of the display pipeline. In addition, there can be use-cases where the writeback encoder could be a shared encoder between the physical display path and the writeback path.
To accommodate for such cases, change the drm_writeback_connector to accept a pointer to drm_encoder.
For existing users of drm_writeback_connector there will not be any change in functionality due to this change.
This approach was first posted by Suraj Kandpal here [1] for both encoder and connector. But after discussions [2], the consensus was reached to split this change for the drm_encoder first and the drm_connector part can be reworked in a subsequent change later.
Validation of this change was done using igt_writeback tests on MSM based RB5 board using the changes posted here [3].
For all other chipsets, these changes were compile-tested.
[1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-... [2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-... [3] https://patchwork.freedesktop.org/series/99724/
Abhinav Kumar (6): drm: allow real encoder to be passed for drm_writeback_connector drm/komeda: use drm_encoder pointer for drm_writeback_connector drm/vkms: use drm_encoder pointer for drm_writeback_connector drm/vc4: use drm_encoder pointer for drm_writeback_connector drm/rcar_du: use drm_encoder pointer for drm_writeback_connector drm/malidp: use drm_encoder pointer for drm_writeback_connector
drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 4 +++- drivers/gpu/drm/arm/malidp_mw.c | 3 ++- drivers/gpu/drm/drm_writeback.c | 8 ++++---- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- drivers/gpu/drm/vc4/vc4_txp.c | 14 ++++++++++---- drivers/gpu/drm/vkms/vkms_writeback.c | 4 +++- include/drm/drm_writeback.h | 13 +++++++++++-- 7 files changed, 35 insertions(+), 14 deletions(-)
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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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) @@ -204,7 +204,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 +233,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; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function. + * + * For some vendor drivers, the hardware resources are shared between + * writeback encoder and rest of the display pipeline. + * To accommodate such cases, encoder is a handle to the real encoder + * hardware. + * + * For current existing writeback users, this shall continue to be the + * embedded encoder for the writeback connector. + * */ - struct drm_encoder encoder; + struct drm_encoder *encoder;
/** * @pixel_formats_blob_ptr:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
if (ret)@@ -204,7 +204,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 +233,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; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:-- 2.7.4
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place.
Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ?
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
if (ret)@@ -204,7 +204,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 +233,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; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:
Hi Dmitry and Laurent
On 3/11/2022 12:05 AM, Laurent Pinchart wrote:
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place.
Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ?
Yes, I have tested this with the MSM driver which provides an encoder and yes it absolutely fits the purpose.
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
You are both right. We can skip the drm_encoder_init for drivers which have already provided an encoder.
The only issue I was facing with that is some of the drivers for example the below one, access the "wb_conn->encoder.possible_crtcs" before the call to drm_writeback_connector_init().
198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats));
If we allocate the encoder within drm_writeback_connector_init(), do you suggest I modify the drivers to move the usage of possible_crtcs after the drm_writeback_connector_init() call to avoid NULL ptr crash?
if (ret)@@ -204,7 +204,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 +233,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);diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:
On Fri, 11 Mar 2022 at 20:09, Abhinav Kumar quic_abhinavk@quicinc.com wrote:
Hi Dmitry and Laurent
On 3/11/2022 12:05 AM, Laurent Pinchart wrote:
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place.
Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ?
Yes, I have tested this with the MSM driver which provides an encoder and yes it absolutely fits the purpose.
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
You are both right. We can skip the drm_encoder_init for drivers which have already provided an encoder.
The only issue I was facing with that is some of the drivers for example the below one, access the "wb_conn->encoder.possible_crtcs" before the call to drm_writeback_connector_init().
Yes. please do.
198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats));
If we allocate the encoder within drm_writeback_connector_init(), do you suggest I modify the drivers to move the usage of possible_crtcs after the drm_writeback_connector_init() call to avoid NULL ptr crash?
if (ret)@@ -204,7 +204,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 +233,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);diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:
On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place.
Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ?
Also, can we not force all drivers to do this setup that don't need it? We have a ton of kms drivers, forcing unnecessary busiwork on drivers is really not good. -Daniel
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
if (ret)@@ -204,7 +204,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 +233,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; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:-- Regards,
Laurent Pinchart
Hi Daniel
On 3/17/2022 3:01 AM, Daniel Vetter wrote:
On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar quic_abhinavk@quicinc.com 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.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ 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 the encoder is provided by a separate driver, it might use a different set of encoder funcs.
More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place.
Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ?
Also, can we not force all drivers to do this setup that don't need it? We have a ton of kms drivers, forcing unnecessary busiwork on drivers is really not good. -Daniel
No, there will not be any requirement forced for existing drivers. A new API will be exposed for new clients which setup the encoder.
I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add()
if (ret)@@ -204,7 +204,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 +233,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);diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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 * @drm_writeback_connector control the behaviour of the @encoder * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function.** For some vendor drivers, the hardware resources are shared between* writeback encoder and rest of the display pipeline.* To accommodate such cases, encoder is a handle to the real encoder* hardware.** For current existing writeback users, this shall continue to be the* embedded encoder for the writeback connector.* */
struct drm_encoder encoder;
struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr:-- Regards,
Laurent Pinchart
Make changes to komeda driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c index e465cc4..9dd5f25 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c @@ -155,7 +155,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = &kwb_conn->base; - wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base)); + wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL); + + wb_conn->encoder->possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, kwb_conn->wb_layer->layer_type,
Make changes to vkms driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/vkms/vkms_writeback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227..adf6961 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -139,8 +139,10 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = { int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector; + struct vkms_output *output = &vkmsdev->output;
- vkmsdev->output.wb_connector.encoder.possible_crtcs = 1; + wb->encoder = &output->encoder; + output.wb_connector.encoder->possible_crtcs = 1; drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
return drm_writeback_connector_init(&vkmsdev->drm, wb,
Make changes to vc4 driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/vc4/vc4_txp.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 9809ca3..7b76968 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(&txp->connector.base, + wb_conn = &txp->connector; + wb_conn->encoder = &txp->drm_enc; + + drm_connector_helper_add(&wb_conn->base, &vc4_txp_connector_helper_funcs); - ret = drm_writeback_connector_init(drm, &txp->connector, + ret = drm_writeback_connector_init(drm, wb_conn, &vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs, drm_fmts, ARRAY_SIZE(drm_fmts)); @@ -506,7 +512,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;
- encoder = &txp->connector.encoder; + encoder = txp->connector.encoder; encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..03930ad 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); + wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL); + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Hi Abhinav
Thank you for the patch.
On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..03930ad 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
- wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
Where is this freed ?
- wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Hi Laurent
On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
Hi Abhinav
Thank you for the patch.
On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..03930ad 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
- wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
Where is this freed ?
You are right, this isnt. Looking more into this, it seems like moving the allocation of encoder to drm_writeback.c for cases which dont pass a real encoder is much better so that I will not have to add alloc() / free() code for all the vendor driver changes which is what I originally thought in my RFC but changed my mind because of below.
- wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
Do you think we can just move usage of wb_conn->encoder->possible_crtcs just right after drm_writeback_connector_init() so that it wont crash?
198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats)); 212 }
drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Hi Abhinav
On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..03930ad 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
- wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
Where is this freed ?
You are right, this isnt. Looking more into this, it seems like moving the allocation of encoder to drm_writeback.c for cases which dont pass a real encoder is much better so that I will not have to add alloc() / free() code for all the vendor driver changes which is what I originally thought in my RFC but changed my mind because of below.
Yes, I think that would be better indeed. You could even skip the dynamic allocation, you could have
struct drm_encoder *encoder; struct drm_encoder internal_encoder;
in drm_writeback_connector, and set
wb->encoder = &wb->internal_encoder;
when the user doesn't pass an encoder.
- wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
Do you think we can just move usage of wb_conn->encoder->possible_crtcs just right after drm_writeback_connector_init() so that it wont crash?
How about adding a possible_crtcs argument to drm_writeback_connector_init() (to cover existing use cases), and adding a new drm_writeback_connector_init_with_encoder() that would get an encoder pointer (and expect possible_crtcs, as well as all the other appropriate encoder fields, having been initialized) ?
198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats)); 212 }
drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Hi Laurent
Thank you for your inputs.
I liked all the suggestions, hence I have incorporated those and pushed a v2.
Thanks
Abhinav
On 3/13/2022 7:50 AM, Laurent Pinchart wrote:
Hi Abhinav
On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..03930ad 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
- wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
Where is this freed ?
You are right, this isnt. Looking more into this, it seems like moving the allocation of encoder to drm_writeback.c for cases which dont pass a real encoder is much better so that I will not have to add alloc() / free() code for all the vendor driver changes which is what I originally thought in my RFC but changed my mind because of below.
Yes, I think that would be better indeed. You could even skip the dynamic allocation, you could have
struct drm_encoder *encoder; struct drm_encoder internal_encoder;
in drm_writeback_connector, and set
wb->encoder = &wb->internal_encoder;
when the user doesn't pass an encoder.
- wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
Do you think we can just move usage of wb_conn->encoder->possible_crtcs just right after drm_writeback_connector_init() so that it wont crash?
How about adding a possible_crtcs argument to drm_writeback_connector_init() (to cover existing use cases), and adding a new drm_writeback_connector_init_with_encoder() that would get an encoder pointer (and expect possible_crtcs, as well as all the other appropriate encoder fields, having been initialized) ?
198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats)); 212 }
drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Make changes to malidp driver to start using drm_encoder pointer for drm_writeback_connector.
Co-developed-by: Kandpal Suraj suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/arm/malidp_mw.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index f5847a7..0bd5b78 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -212,7 +212,8 @@ int malidp_mw_connector_init(struct drm_device *drm) if (!malidp->dev->hw->enable_memwrite) return 0;
- malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc); + malidp->mw_connector.encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL); + malidp->mw_connector.encoder->possible_crtcs = 1 << drm_crtc_index(&malidp->crtc); drm_connector_helper_add(&malidp->mw_connector.base, &malidp_mw_connector_helper_funcs);
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Daniel Vetter -
Dmitry Baryshkov -
Laurent Pinchart