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: