Re: [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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 ?


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);
  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:




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux