Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

On 4/25/2022 10:32 AM, Laurent Pinchart wrote:
On Mon, Apr 25, 2022 at 01:50:43PM +0300, Dmitry Baryshkov wrote:
On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart wrote:
On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
On Fri, Apr 22, 2022 at 04:06:38PM -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.

For existing clients, drm_writeback_connector_init() will use
an internal_encoder under the hood and hence no changes will
be needed.

changes in v7:
     - move this change before the vc4 change in the series
       to minimize the changes to vendor drivers in drm core
       changes

Why is this needed ? The drm_writeback_connector functions don't need
the drm_encoder after drm_writeback_connector_init() (or
drm_writeback_connector_init_with_encoder()) returns.


Sorry I didnt follow this comment. This change log is incorrect, so
after changing the previous change in the series and modifying this, no
further changes are needed to vc4, so I decided to drop the next change.
So this change log is incorrect. I can remove this.

Is that what you meant?

So till the previous change, the only user of
drm_writeback_connector_init_with_encoder() was
drm_writeback_connector_init() which was still passing its own
internal_encoder.

Only if the wb_connector->encoder is changed to a pointer, other vendor
drivers can pass their own encoder to
drm_writeback_connector_init_with_encoder().

Hence you are right that drm_writeback_connector functions do not need
drm_encoder after init() returns, but till this change is done, other
vendor drivers cannot directly call
drm_writeback_connector_init_with_encoder() because the encoder will not
be valid till then.

Users of drm_writeback_connector_init_with_encoder() handle the encoder
themselves, they can simply ignore drm_writeback_connector.encoder. The
documentation of the encoder field needs to be updated though (I'd do so
in the previous patch), to clearly mention that the field is valid only
when using drm_writeback_connector_init(), not when calling
drm_writeback_connector_init_with_encoder().

If we allow it to be unitialized, it might end with hard-to-trace
bugs, additional conditions, etc.
In my opnion we should:
  - either make drm_writeback_connector.encoder a valid pointer
  - or drop the field completely.

And up to some point I'd vote for the second option. The code using
internal_encoder can continue using it directly. The code using
drm_writeback_connector_init_with_encoder() will manage encoder on
their own. We will loose a single entry point for wb's encoder, but do
we really need it? (Frankly speaking I didn't check.)

As far as I understand, we went for the second option as Abhinav dropped
this patch for the next version.


I dropped the patch as there was no agreement yet and I didnt want to 
block the series as its not really needed now but thats not option 2 
because wb_conn->encoder field remains there, just that its unused for 
users using drm_writeback_connector_init_with_encoder().

I guess what Dmitry is suggesting is just drop that 
wb_connector->encoder field completely.

Hope this clarifies it.

Signed-off-by: Abhinav Kumar <quic_abhin...@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
---
   drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
   drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
   include/drm/drm_writeback.h     | 22 ++++++++++++++++++++--
   3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c
b/drivers/gpu/drm/drm_writeback.c
index 92658ad..0538674 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
drm_device *dev,
   {
       int ret = 0;
-    drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
+    drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
-    wb_connector->encoder.possible_crtcs = possible_crtcs;
+    wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
-    ret = drm_encoder_init(dev, &wb_connector->encoder,
+    ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
                      &drm_writeback_encoder_funcs,
                      DRM_MODE_ENCODER_VIRTUAL, NULL);
       if (ret)
           return ret;
-    ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
&wb_connector->encoder,
-            con_funcs, formats, n_formats);
+    ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+            &wb_connector->internal_encoder, con_funcs, formats, n_formats);
       if (ret)
-        drm_encoder_cleanup(&wb_connector->encoder);
+        drm_encoder_cleanup(&wb_connector->internal_encoder);
       return ret;
   }
@@ -239,6 +239,12 @@ int drm_writeback_connector_init_with_encoder(struct 
drm_device *dev,
       struct drm_mode_config *config = &dev->mode_config;
       int ret = create_writeback_properties(dev);
+    /*
+     * Assign the encoder passed to this API to the wb_connector's encoder.
+     * For drm_writeback_connector_init(), this shall be the internal_encoder
+     */
+    wb_connector->encoder = enc;
+
       if (ret != 0)
           return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3447eb6..7e063a9 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -159,7 +159,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, connector.internal_encoder);
   }
   static inline struct vc4_txp *connector_to_vc4_txp(struct  drm_connector 
*conn)
@@ -507,7 +507,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 bb306fa..3fbae9d 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,13 +25,31 @@ 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;
+
+    /**
+     * @internal_encoder: internal encoder used by writeback when
+     * drm_writeback_connector_init() is used.
+     * @encoder will be assigned to this for those cases
+     *
+     * This will be unused when drm_writeback_connector_init_with_encoder()
+     * is used.
        */
-    struct drm_encoder encoder;
+    struct drm_encoder internal_encoder;
       /**
        * @pixel_formats_blob_ptr: