Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Tue, 5 Apr 2022 09:53:57 -0700

Hi Laurent

On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
Hi Rob and Abhinav,

On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
On Thu, Mar 31, 2022 at 05:12:13PM -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 v6:
       - assign the encoder inside
         drm_writeback_connector_init_with_encoder() for
         better readability
       - improve some documentation for internal encoder

Co-developed-by: Kandpal Suraj <suraj.kandpal@xxxxxxxxx>
Signed-off-by: Kandpal Suraj <suraj.kandpal@xxxxxxxxx>
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
  drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
  include/drm/drm_writeback.h     | 21 +++++++++++++++++++--

Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.

But this *is* API refactoring ;-)

Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?

I can move all the changes done in vc4 except below part to the change 
3/4 itself because that way I can show usage of vc4->drm_enc with the 
new API. Let me know if that works.

The only part which will remain is the below one:

@@ -523,7 +525,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);

Since i dont know vc4 driver very well, I was not sure of a good way to 
decouple this dependency.

Let me know if that works.

Thanks

Abhinav


  3 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,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;
  }
@@ -238,6 +238,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 5e53f02..a9b4f83 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)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)

       wb_conn = &txp->connector;

-     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
+     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);

-     ret = drm_encoder_init(drm, &wb_conn->encoder,
+     ret = drm_encoder_init(drm, &txp->drm_enc,
                       &vc4_txp_encoder_funcs,
                       DRM_MODE_ENCODER_VIRTUAL, NULL);

@@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
       drm_connector_helper_add(&wb_conn->base,
                                &vc4_txp_connector_helper_funcs);

-     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
+     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
                       &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
       if (ret) {
-             drm_encoder_cleanup(&wb_conn->encoder);
+             drm_encoder_cleanup(&txp->drm_enc);
               return ret;
       }

@@ -523,7 +525,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 4795024..3f5c330 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,15 +25,32 @@ 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
+      * 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 internal_encoder;
+     /**
        * @pixel_formats_blob_ptr:
        *
        * DRM blob property data for the pixel formats list on writeback







