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/
changes in v4: - split the changes more according to functionality
Abhinav Kumar (4): drm: allow passing possible_crtcs to drm_writeback_connector_init() drm: allow real encoder to be passed for drm_writeback_connector drm: introduce drm_writeback_connector_init_with_encoder() API drm/vc4: change vc4 driver to use drm_writeback_connector_init_with_encoder()
.../drm/arm/display/komeda/komeda_wb_connector.c | 3 +- drivers/gpu/drm/arm/malidp_mw.c | 4 +- drivers/gpu/drm/drm_writeback.c | 143 +++++++++++++++------ drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 +- drivers/gpu/drm/vc4/vc4_txp.c | 36 ++++-- drivers/gpu/drm/vkms/vkms_writeback.c | 4 +- include/drm/drm_writeback.h | 25 +++- 7 files changed, 161 insertions(+), 58 deletions(-)
Clients of drm_writeback_connector_init() initialize the possible_crtcs and then invoke the call to this API.
To simplify things, allow passing possible_crtcs as a parameter to drm_writeback_connector_init() and make changes to the other drm drivers to make them compatible with this change.
changes in v4: - keep only changes related to possible_crtcs - add line breaks after ARRAY_SIZE - stop using temporary variables for possible_crtcs
Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- drivers/gpu/drm/drm_writeback.c | 6 +++++- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- drivers/gpu/drm/vc4/vc4_txp.c | 3 ++- drivers/gpu/drm/vkms/vkms_writeback.c | 4 ++-- include/drm/drm_writeback.h | 2 +- 7 files changed, 15 insertions(+), 11 deletions(-)
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..40774e6 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,6 @@ 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));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, kwb_conn->wb_layer->layer_type, @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, err = drm_writeback_connector_init(&kms->base, wb_conn, &komeda_wb_connector_funcs, &komeda_wb_encoder_helper_funcs, - formats, n_formats); + formats, n_formats, BIT(drm_crtc_index(&kcrtc->base))); komeda_put_fourcc_list(formats); if (err) { kfree(kwb_conn); diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index f5847a7..e54921d 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -212,7 +212,6 @@ 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); drm_connector_helper_add(&malidp->mw_connector.base, &malidp_mw_connector_helper_funcs);
@@ -223,7 +222,8 @@ int malidp_mw_connector_init(struct drm_device *drm) ret = drm_writeback_connector_init(drm, &malidp->mw_connector, &malidp_mw_connector_funcs, &malidp_mw_encoder_helper_funcs, - formats, n_formats); + formats, n_formats, + (1 << drm_crtc_index(&malidp->crtc))); kfree(formats); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..dc2ef12 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -157,6 +157,7 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder * @formats: Array of supported pixel formats for the writeback engine * @n_formats: Length of the formats array + * @possible_crtcs: possible crtcs for the internal writeback encoder * * This function creates the writeback-connector-specific properties if they * have not been already created, initializes the connector as @@ -174,7 +175,7 @@ int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, - const u32 *formats, int n_formats) + const u32 *formats, int n_formats, uint32_t possible_crtcs) { struct drm_property_blob *blob; struct drm_connector *connector = &wb_connector->base; @@ -190,6 +191,9 @@ int drm_writeback_connector_init(struct drm_device *dev, return PTR_ERR(blob);
drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); + + wb_connector->encoder.possible_crtcs = possible_crtcs; + ret = drm_encoder_init(dev, &wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..fcfb0b3 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,6 @@ 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); drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
@@ -208,7 +207,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, &rcar_du_wb_conn_funcs, &rcar_du_wb_enc_helper_funcs, writeback_formats, - ARRAY_SIZE(writeback_formats)); + ARRAY_SIZE(writeback_formats), + (1 << drm_crtc_index(&rcrtc->crtc))); }
void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 9809ca3..3447eb6 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -497,7 +497,8 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) ret = drm_writeback_connector_init(drm, &txp->connector, &vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs, - drm_fmts, ARRAY_SIZE(drm_fmts)); + drm_fmts, ARRAY_SIZE(drm_fmts), + 0); if (ret) return ret;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227..6d01e55 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -140,12 +140,12 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
- vkmsdev->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, &vkms_wb_connector_funcs, &vkms_wb_encoder_helper_funcs, vkms_wb_formats, - ARRAY_SIZE(vkms_wb_formats)); + ARRAY_SIZE(vkms_wb_formats), + 1); } diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..db6214f 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -150,7 +150,7 @@ int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, - const u32 *formats, int n_formats); + const u32 *formats, int n_formats, uint32_t possible_crtcs);
int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);
Hi,
On Thu, Mar 17, 2022 at 06:45:33PM -0700, Abhinav Kumar wrote:
Clients of drm_writeback_connector_init() initialize the possible_crtcs and then invoke the call to this API.
To simplify things, allow passing possible_crtcs as a parameter to drm_writeback_connector_init() and make changes to the other drm drivers to make them compatible with this change.
changes in v4: - keep only changes related to possible_crtcs - add line breaks after ARRAY_SIZE - stop using temporary variables for possible_crtcs
Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- drivers/gpu/drm/drm_writeback.c | 6 +++++- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- drivers/gpu/drm/vc4/vc4_txp.c | 3 ++- drivers/gpu/drm/vkms/vkms_writeback.c | 4 ++-- include/drm/drm_writeback.h | 2 +- 7 files changed, 15 insertions(+), 11 deletions(-)
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..40774e6 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,6 @@ 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));
formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl, kwb_conn->wb_layer->layer_type,
@@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms, err = drm_writeback_connector_init(&kms->base, wb_conn, &komeda_wb_connector_funcs, &komeda_wb_encoder_helper_funcs,
formats, n_formats);
formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index f5847a7..e54921d 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -212,7 +212,6 @@ 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); drm_connector_helper_add(&malidp->mw_connector.base, &malidp_mw_connector_helper_funcs);
@@ -223,7 +222,8 @@ int malidp_mw_connector_init(struct drm_device *drm) ret = drm_writeback_connector_init(drm, &malidp->mw_connector, &malidp_mw_connector_funcs, &malidp_mw_encoder_helper_funcs,
formats, n_formats);
formats, n_formats,(1 << drm_crtc_index(&malidp->crtc)));diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..dc2ef12 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -157,6 +157,7 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
- @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
- @formats: Array of supported pixel formats for the writeback engine
- @n_formats: Length of the formats array
- @possible_crtcs: possible crtcs for the internal writeback encoder
- This function creates the writeback-connector-specific properties if they
- have not been already created, initializes the connector as
@@ -174,7 +175,7 @@ int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs,
const u32 *formats, int n_formats)
const u32 *formats, int n_formats, uint32_t possible_crtcs){ struct drm_property_blob *blob; struct drm_connector *connector = &wb_connector->base; @@ -190,6 +191,9 @@ int drm_writeback_connector_init(struct drm_device *dev, return PTR_ERR(blob);
drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, &wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL);
For all of the above changes:
Acked-by: Liviu Dudau liviu.dudau@arm.com
Best regards, Liviu
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d125..fcfb0b3 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,7 +200,6 @@ 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); drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
@@ -208,7 +207,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, &rcar_du_wb_conn_funcs, &rcar_du_wb_enc_helper_funcs, writeback_formats,
ARRAY_SIZE(writeback_formats));
ARRAY_SIZE(writeback_formats),(1 << drm_crtc_index(&rcrtc->crtc)));}
void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 9809ca3..3447eb6 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -497,7 +497,8 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) ret = drm_writeback_connector_init(drm, &txp->connector, &vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
drm_fmts, ARRAY_SIZE(drm_fmts));
drm_fmts, ARRAY_SIZE(drm_fmts),0);diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 8694227..6d01e55 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -140,12 +140,12 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev) { struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
vkmsdev->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, &vkms_wb_connector_funcs, &vkms_wb_encoder_helper_funcs, vkms_wb_formats,
ARRAY_SIZE(vkms_wb_formats));
ARRAY_SIZE(vkms_wb_formats),1);} diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..db6214f 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -150,7 +150,7 @@ int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs,
const u32 *formats, int n_formats);
const u32 *formats, int n_formats, uint32_t possible_crtcs);int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb); -- 2.7.4
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 suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- 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(&wb_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 = &wb_connector->internal_encoder;
- ret = drm_encoder_init(dev, &wb_connector->encoder, + wb_connector->encoder->possible_crtcs = possible_crtcs; + + ret = drm_encoder_init(dev, wb_connector->encoder, &drm_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, - &wb_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(&wb_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(&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), @@ -507,7 +513,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, 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 * @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;
/** + * @internal_encoder: internal encoder used by writeback when + * a real encoder is not provided by the vendor drm drivers. + * @encoder will be assigned to this for those cases. + */ + struct drm_encoder internal_encoder; + /** * @pixel_formats_blob_ptr: * * DRM blob property data for the pixel formats list on writeback
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 suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
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(&wb_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 = &wb_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, &wb_connector->encoder,
- wb_connector->encoder->possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, wb_connector->encoder, &drm_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,
&wb_connector->encoder);
wb_connector->encoder);@@ -237,7 +239,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/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(&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),
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 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,
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
- @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;
/**
* @internal_encoder: internal encoder used by writeback when* a real encoder is not provided by the vendor drm drivers.* @encoder will be assigned to this for those cases.*/struct drm_encoder internal_encoder;
/**
- @pixel_formats_blob_ptr:
- DRM blob property data for the pixel formats list on writeback
-- 2.7.4
You need to come up with a bit more sophisticated algorithm for allowing drivers to pass on a connector with a pre-allocated encoder. If the encoder is pre-populated then you need to decide if drm_writeback_connector_init() will initialise the encoder too or if it's going to assume that it has been already initialised (in which case the error path where we call drm_encoder_cleanup() also needs to know if we're using the internal encoder or not and act accordingly).
Otherwise the code looks pretty good, thanks for the improvements.
Best regards, Liviu
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 suraj.kandpal@intel.com Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com
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(&wb_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 = &wb_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, &wb_connector->encoder,
- wb_connector->encoder->possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, wb_connector->encoder, &drm_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,
&wb_connector->encoder);
wb_connector->encoder);@@ -237,7 +239,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/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(&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),
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 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,
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
- @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;
/**
* @internal_encoder: internal encoder used by writeback when* a real encoder is not provided by the vendor drm drivers.* @encoder will be assigned to this for those cases.*/struct drm_encoder internal_encoder;
/**
- @pixel_formats_blob_ptr:
- DRM blob property data for the pixel formats list on writeback
-- 2.7.4
You need to come up with a bit more sophisticated algorithm for allowing drivers to pass on a connector with a pre-allocated encoder. If the encoder is pre-populated then you need to decide if drm_writeback_connector_init() will initialise the encoder too or if it's going to assume that it has been already initialised (in which case the error path where we call drm_encoder_cleanup() also needs to know if we're using the internal encoder or not and act accordingly).
Otherwise the code looks pretty good, thanks for the improvements.
Best regards, Liviu
For vendors drivers which pass an already allocated and initialized encoder especially for cases where the encoder hardware is shared OR the writeback encoder shares the resources with the rest of the display pipeline introduce a new API, drm_writeback_connector_init_with_encoder() which expects an initialized encoder as a parameter and only sets up the writeback connector.
changes in v4: - removed the possible_crtcs part
Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/drm_writeback.c | 147 ++++++++++++++++++++++++++++------------ include/drm/drm_writeback.h | 5 ++ 2 files changed, 108 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index a4c17d6..d0672f5 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { .destroy = drm_encoder_cleanup, };
-/** - * drm_writeback_connector_init - Initialize a writeback connector and its properties - * @dev: DRM device - * @wb_connector: Writeback connector to initialize - * @con_funcs: Connector funcs vtable - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder - * @formats: Array of supported pixel formats for the writeback engine - * @n_formats: Length of the formats array - * @possible_crtcs: possible crtcs for the internal writeback encoder - * - * This function creates the writeback-connector-specific properties if they - * have not been already created, initializes the connector as - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property - * values. It will also create an internal encoder associated with the - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for - * the encoder helper. - * - * Drivers should always use this function instead of drm_connector_init() to - * set up writeback connectors. - * - * Returns: 0 on success, or a negative error code - */ -int drm_writeback_connector_init(struct drm_device *dev, - struct drm_writeback_connector *wb_connector, - const struct drm_connector_funcs *con_funcs, - const struct drm_encoder_helper_funcs *enc_helper_funcs, - const u32 *formats, int n_formats, uint32_t possible_crtcs) +static int drm_writeback_connector_setup(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, const u32 *formats, + int n_formats) { struct drm_property_blob *blob; - struct drm_connector *connector = &wb_connector->base; struct drm_mode_config *config = &dev->mode_config; + struct drm_connector *connector = &wb_connector->base; + int ret = create_writeback_properties(dev);
if (ret != 0) @@ -187,20 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), formats); - if (IS_ERR(blob)) - return PTR_ERR(blob); - - drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); - - wb_connector->encoder = &wb_connector->internal_encoder; - - wb_connector->encoder->possible_crtcs = possible_crtcs; - - ret = drm_encoder_init(dev, wb_connector->encoder, - &drm_writeback_encoder_funcs, - DRM_MODE_ENCODER_VIRTUAL, NULL); - if (ret) - goto fail; + if (IS_ERR(blob)) { + ret = PTR_ERR(blob); + return ret; + }
connector->interlace_allowed = 0;
@@ -239,13 +207,104 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail: - drm_encoder_cleanup(wb_connector->encoder); -fail: drm_property_blob_put(blob); return ret; } + +/** + * drm_writeback_connector_init - Initialize a writeback connector and its properties + * @dev: DRM device + * @wb_connector: Writeback connector to initialize + * @con_funcs: Connector funcs vtable + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder + * @formats: Array of supported pixel formats for the writeback engine + * @n_formats: Length of the formats array + * @possible_crtcs: possible crtcs for the internal writeback encoder + * + * This function creates the writeback-connector-specific properties if they + * have not been already created, initializes the connector as + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property + * values. It will also create an internal encoder associated with the + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for + * the encoder helper. + * + * Drivers should always use this function instead of drm_connector_init() to + * set up writeback connectors. + * + * Returns: 0 on success, or a negative error code + */ +int drm_writeback_connector_init(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, + const struct drm_encoder_helper_funcs *enc_helper_funcs, + const u32 *formats, int n_formats, uint32_t possible_crtcs) +{ + int ret = 0; + + wb_connector->encoder = &wb_connector->internal_encoder; + + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); + + wb_connector->encoder->possible_crtcs = possible_crtcs; + + ret = drm_encoder_init(dev, wb_connector->encoder, + &drm_writeback_encoder_funcs, + DRM_MODE_ENCODER_VIRTUAL, NULL); + if (ret) + return ret; + + ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats, + n_formats); + + if (ret) + drm_encoder_cleanup(wb_connector->encoder); + + return ret; +} EXPORT_SYMBOL(drm_writeback_connector_init);
+/** + * drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties + * using the encoder which already assigned and initialized + * + * @dev: DRM device + * @wb_connector: Writeback connector to initialize + * @con_funcs: Connector funcs vtable + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder + * @formats: Array of supported pixel formats for the writeback engine + * @n_formats: Length of the formats array + * + * This function creates the writeback-connector-specific properties if they + * have not been already created, initializes the connector as + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property + * values. + * + * This function assumes that the drm_writebac_connector's encoder has already been + * created and initialized before invoking this function. + * + * In addition, this function also assumes that callers of this API will manage + * assigning the encoder helper functions, possible_crtcs and any other encoder + * specific operation which is otherwise handled by drm_writeback_connector_init(). + * + * Drivers should always use this function instead of drm_connector_init() to + * set up writeback connectors. + * + * Returns: 0 on success, or a negative error code + */ +int drm_writeback_connector_init_with_encoder(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, const u32 *formats, + int n_formats) +{ + int ret = 0; + + ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats, + n_formats); + + return ret; +} +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); + int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) { diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index c525b60..ed35c3d 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -166,6 +166,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs);
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, const u32 *formats, + int n_formats); + int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);
vc4 driver currently embeds the drm_encoder into struct vc4_txp and later on uses container_of to retrieve the vc4_txp from the drm_encoder.
Since drm_encoder has now been made a pointer inside drm_writeback_connector, make vc4 driver use the new API so that the embedded encoder model can be retained in the driver and there is no change in functionality.
changes in v4: - none
Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com --- drivers/gpu/drm/vc4/vc4_txp.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 341a9be5..3d24ef5 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -370,6 +370,10 @@ static const struct drm_encoder_helper_funcs vc4_txp_encoder_helper_funcs = { .disable = vc4_txp_encoder_disable, };
+static const struct drm_encoder_funcs vc4_txp_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + static int vc4_txp_enable_vblank(struct drm_crtc *crtc) { return 0; @@ -498,15 +502,24 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) wb_conn = &txp->connector; wb_conn->encoder = &txp->drm_enc;
+ drm_encoder_helper_add(wb_conn->encoder, &vc4_txp_encoder_helper_funcs); + + ret = drm_encoder_init(drm, wb_conn->encoder, + &vc4_txp_encoder_funcs, + DRM_MODE_ENCODER_VIRTUAL, NULL); + + if (ret) + return ret; + drm_connector_helper_add(&wb_conn->base, &vc4_txp_connector_helper_funcs); - ret = drm_writeback_connector_init(drm, wb_conn, - &vc4_txp_connector_funcs, - &vc4_txp_encoder_helper_funcs, - drm_fmts, ARRAY_SIZE(drm_fmts), - 0); - if (ret) + + ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, + &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts)); + if (ret) { + drm_encoder_cleanup(wb_conn->encoder); return ret; + }
ret = vc4_crtc_init(drm, vc4_crtc, &vc4_txp_crtc_funcs, &vc4_txp_crtc_helper_funcs);
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Liviu Dudau