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@xxxxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
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);
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
