On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:

For vendors drivers which pass an already allocated and initialized encoder especially for cases where the encoder hardware is shared OR the writeback encoder shares the resources with the rest of the display pipeline introduce a new API, drm_writeback_connector_init_with_encoder() which expects an initialized encoder as a parameter and only sets up the writeback connector.

changes in v5:

• reorder this change to come before in the series to avoid incorrect functionality in subsequent changes
• continue using struct drm_encoder instead of struct drm_encoder * and switch it in next change

Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com

Hi Abhinav,

---

drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------ include/drm/drm_writeback.h | 5 ++ 2 files changed, 106 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dc2ef12..abe78b9 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { .destroy = drm_encoder_cleanup, };

-/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

-int drm_writeback_connector_init(struct drm_device *dev,

• 		 struct drm_writeback_connector *wb_connector,
  

• 		 const struct drm_connector_funcs *con_funcs,
  

• 		 const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• 		 const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+static int drm_writeback_connector_setup(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

{ struct drm_property_blob *blob;

• struct drm_connector *connector = &wb_connector->base; struct drm_mode_config *config = &dev->mode_config;

• struct drm_connector *connector = &wb_connector->base;

Point of this reordering the statements is...?

int ret = create_writeback_properties(dev);

if (ret != 0) @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,

blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), formats);

• if (IS_ERR(blob))

• return PTR_ERR(blob);
  

• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• goto fail;
  

• if (IS_ERR(blob)) {

• ret = PTR_ERR(blob);
  

• return ret;
  

• }

I don't see why you have changed the earlier code to store the result of PTR_ERR into ret other than to help your debugging. I suggest that you keep the existing code that returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

connector->interlace_allowed = 0;

@@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail:

• drm_encoder_cleanup(&wb_connector->encoder);

-fail: drm_property_blob_put(blob); return ret; }

+/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs,
  

• const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+{

• int ret = 0;
• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• return ret;
  

• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• if (ret)

• drm_encoder_cleanup(&wb_connector->encoder);
  

• return ret;

+} EXPORT_SYMBOL(drm_writeback_connector_init);

+/**

• • drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties
• • using the encoder which already assigned and initialized
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values.
• • This function assumes that the drm_writebac_connector's encoder has already been

spelling: writeback

• • created and initialized before invoking this function.
• • In addition, this function also assumes that callers of this API will manage
• • assigning the encoder helper functions, possible_crtcs and any other encoder
• • specific operation which is otherwise handled by drm_writeback_connector_init().
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.

.... if they want to manage themselves the lifetime of the associated encoder.

We're not trying to replace drm_writeback_connector_init() function here, only to provide an alternative function to call for special cases.

• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

Where is the encoder parameter? Isn't that the reason why the function is called `_with_encoder`?

I think there might have been too many version of these patchsets and things are starting to be confusing. Please revisit the series without rushing and come up with a plan of action. My understanding of watching this series has been that you're trying to come up with a function that does *connector* initialisation but skips the *encoder* initialisation because it might have been already done by the driver. The idea will be then to have a function `drm_writeback_connector_init_with_encoder()` that does *all* the work that `drm_writeback_connector_init()` does at the moment minus the encoder initialisation part. Then `drm_writeback_connector_init()` only initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`. There is no need to have the `drm_writeback_connector_setup()` function at all.

Best regards, Liviu

+{

• int ret = 0;
• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• return ret;

+} +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);

int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) { diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index db6214f..0093bab 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -152,6 +152,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs);

+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• 		struct drm_writeback_connector *wb_connector,
  

• 		const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• 		int n_formats);
  

int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);

-- 2.7.4

On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:

Hi Liviu

Hello,

Thanks for the review.

On 3/23/2022 9:46 AM, Liviu Dudau wrote:

...

On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:

...

For vendors drivers which pass an already allocated and initialized encoder especially for cases where the encoder hardware is shared OR the writeback encoder shares the resources with the rest of the display pipeline introduce a new API, drm_writeback_connector_init_with_encoder() which expects an initialized encoder as a parameter and only sets up the writeback connector.

changes in v5:

• reorder this change to come before in the series to avoid incorrect functionality in subsequent changes
• continue using struct drm_encoder instead of struct drm_encoder * and switch it in next change

Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com

Hi Abhinav,

...

---

drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------ include/drm/drm_writeback.h | 5 ++ 2 files changed, 106 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dc2ef12..abe78b9 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { .destroy = drm_encoder_cleanup, }; -/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

-int drm_writeback_connector_init(struct drm_device *dev,

• 		 struct drm_writeback_connector *wb_connector,
  

• 		 const struct drm_connector_funcs *con_funcs,
  

• 		 const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• 		 const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+static int drm_writeback_connector_setup(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

  { struct drm_property_blob *blob;

• struct drm_connector *connector = &wb_connector->base; struct drm_mode_config *config = &dev->mode_config;

• struct drm_connector *connector = &wb_connector->base;

Point of this reordering the statements is...?

This diff can be avoided. There was no reason to reorder this. I will remove this re-order.

...

...

int ret = create_writeback_properties(dev); if (ret != 0) @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev, blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), formats);

• if (IS_ERR(blob))

• return PTR_ERR(blob);
  

• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• goto fail;
  

• if (IS_ERR(blob)) {

• ret = PTR_ERR(blob);
  

• return ret;
  

• }

I don't see why you have changed the earlier code to store the result of PTR_ERR into ret other than to help your debugging. I suggest that you keep the existing code that returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

Sure, i can fix this for a smaller diff stat.

...

...

connector->interlace_allowed = 0; @@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail:

• drm_encoder_cleanup(&wb_connector->encoder);

-fail: drm_property_blob_put(blob); return ret; }

+/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs,
  

• const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+{

• int ret = 0;
• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• return ret;
  

• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• if (ret)

• drm_encoder_cleanup(&wb_connector->encoder);
  

• return ret;

+} EXPORT_SYMBOL(drm_writeback_connector_init); +/**

• • drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties
• • using the encoder which already assigned and initialized
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values.
• • This function assumes that the drm_writebac_connector's encoder has already been

spelling: writeback

Ack. will fix this.

...

...

• • created and initialized before invoking this function.
• • In addition, this function also assumes that callers of this API will manage
• • assigning the encoder helper functions, possible_crtcs and any other encoder
• • specific operation which is otherwise handled by drm_writeback_connector_init().
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.

.... if they want to manage themselves the lifetime of the associated encoder.

We're not trying to replace drm_writeback_connector_init() function here, only to provide an alternative function to call for special cases.

Yes, we are trying to provide an alternative function call for special case where the encoder is a shared encoder and/or where the resources of the writeback encoder are shared with other hardware blocks.

I can add that comment to this function's doc.

...

...

• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

Where is the encoder parameter? Isn't that the reason why the function is called `_with_encoder`?

The encoder parameter is skipped here because this function assumes that wb_connector->encoder has already been initialized, setup with functions and its possible_crts have already been set prior to calling this function like the vc4 example shows. So this function doesnt need an explicit encoder parameter. Let me know if thats a concern.

...

I think there might have been too many version of these patchsets and things are starting to be confusing. Please revisit the series without rushing and come up with a plan of action. My understanding of watching this series has been that you're trying to come up with a function that does *connector* initialisation but skips the *encoder* initialisation because it might have been already done by the driver. The idea will be then to have a function `drm_writeback_connector_init_with_encoder()` that does *all* the work that `drm_writeback_connector_init()` does at the moment minus the encoder initialisation part. Then `drm_writeback_connector_init()` only initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`. There is no need to have the `drm_writeback_connector_setup()` function at all.

Best regards, Liviu

I agree there have been a 4 revisions prior to this because of the way this affects the existing writeback drivers. So initial revision was a bit intrusive into other drivers (which was just mostly a take over from the previous patchset posted by the Co-developer ) and since rev3 we have decided to have a separate API drm_writeback_connector_init_with_encoder() so that existing clients are unaffected and it works seamlessly under the hood.

Only clients which already embed an encoder (vc4) and the new ones which have special encoder requirements like the MSM driver for which I am preparing these changes for will use the new API.

Apologies for the revisions, but thanks to some great feedback from you and laurent its shaping up nicely and reaching its conclusion I feel.

I think it is only natural that there will be some iterations. If I remember correctly, the initial writeback series has something like 13 revisions :)

So i think this revision is pretty close to being clean thanks to the feedback you gave on rev4. Between rev4 and this one I didnt drastically change the design other than re-ordering the changes to avoid the intermediate patches having an incorrect state for the vc4 encoder. So all your comments related to the encoder_cleanup() and vc4's encoder not getting initialized would have gotten addressed but overall concept remained same.

You are right, we are trying to come up with a function which does connector initialization but skips the encoder part because that has already been done in the client side of this API ( hence _with_encoder() name ). The "_with_encoder" indicates that the caller already has an encoder for the writeback connector which is being passed so there is no need to pass the encoder again.

I thought this addresses all the concerns posted in the previous series.

So are you suggesting something like below?

1. rename drm_writeback_connector_setup() to

drm_writeback_connector_init_with_encoder() (essentially thats what will end up happening )

Correct. Without the pointless reordering of code it should be about 3 lines of code that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).

2. Inside drm_writeback_connector_init() check:

drm_writeback_connector_init(.....) { if (!wb_conn->encoder) initialize the encoder

No, the assumption of drm_writeback_connector_init() is that we will provide the encoder, so no need to check that one is already provided. What you could do is:

WARN_ON(wb_conn->encoder);

before we overwrite the encoder. That way we will get a nice warning in the kernel log if someone tries to call drm_writeback_connector_init() with a preset encoder.

call drm_writeback_**_init_with_encoder() }

This will also work but have the foll concerns/questions:

1. Will drm_writeback_connector_init_with_encoder() be exported if we change

as per your suggestion or will all clients continue to use drm_writeback_connector_init() only? We wanted to have a separate function for new clients.

Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.

2. How will we detect that encoder needs to be initialized without it being

a pointer which happens in the later change. So ordering becomes an issue.

I think the init problem is simple. You either call drm_writeback_connector_init() and the framework provides you with an encoder, or you call drm_writeback_connector_init_with_encoder() and the framework will use yours. The problems will show up on the cleanup and exit codes, where we need to be able to skip the cleanup if the encoder pointer is not the internal one. I think a simple

if (connector->encoder == &connector->internal_encoder) do_encoder_cleanup_work_here()

should work.

Thats why I thought this is the best way to address the comments and keep the functionality intact with each change.

Let me know if I have misunderstood some comment or idea.

I hope that with these clarifications we are both on the same page.

Best regards, Liviu

...

...

+{

• int ret = 0;
• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• return ret;

+} +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);

• int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) {

diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index db6214f..0093bab 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -152,6 +152,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs); +int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• 		struct drm_writeback_connector *wb_connector,
  

• 		const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• 		int n_formats);
  

• int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);

-- 2.7.4

On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:

Hi Liviu

Hello,

Thanks for the response.

On 3/24/2022 3:12 AM, Liviu Dudau wrote:

...

On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hello,

...

Thanks for the review.

On 3/23/2022 9:46 AM, Liviu Dudau wrote:

...

On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:

...

For vendors drivers which pass an already allocated and initialized encoder especially for cases where the encoder hardware is shared OR the writeback encoder shares the resources with the rest of the display pipeline introduce a new API, drm_writeback_connector_init_with_encoder() which expects an initialized encoder as a parameter and only sets up the writeback connector.

changes in v5:

• reorder this change to come before in the series to avoid incorrect functionality in subsequent changes
• continue using struct drm_encoder instead of struct drm_encoder * and switch it in next change

Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com

Hi Abhinav,

...

---

drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------ include/drm/drm_writeback.h | 5 ++ 2 files changed, 106 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dc2ef12..abe78b9 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { .destroy = drm_encoder_cleanup, }; -/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

-int drm_writeback_connector_init(struct drm_device *dev,

• 		 struct drm_writeback_connector *wb_connector,
  

• 		 const struct drm_connector_funcs *con_funcs,
  

• 		 const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• 		 const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+static int drm_writeback_connector_setup(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

  { struct drm_property_blob *blob;

• struct drm_connector *connector = &wb_connector->base; struct drm_mode_config *config = &dev->mode_config;

• struct drm_connector *connector = &wb_connector->base;

Point of this reordering the statements is...?

This diff can be avoided. There was no reason to reorder this. I will remove this re-order.

...

...

int ret = create_writeback_properties(dev);
if (ret != 0)

@@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev, blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), formats);

• if (IS_ERR(blob))

• return PTR_ERR(blob);
  

• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• goto fail;
  

• if (IS_ERR(blob)) {

• ret = PTR_ERR(blob);
  

• return ret;
  

• }

I don't see why you have changed the earlier code to store the result of PTR_ERR into ret other than to help your debugging. I suggest that you keep the existing code that returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

Sure, i can fix this for a smaller diff stat.

...

...

connector->interlace_allowed = 0;

@@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail:

• drm_encoder_cleanup(&wb_connector->encoder);

-fail: drm_property_blob_put(blob); return ret; }

+/**

• • drm_writeback_connector_init - Initialize a writeback connector and its properties
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • @possible_crtcs: possible crtcs for the internal writeback encoder
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values. It will also create an internal encoder associated with the
• • drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
• • the encoder helper.
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.
• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs,
  

• const struct drm_encoder_helper_funcs *enc_helper_funcs,
  

• const u32 *formats, int n_formats, uint32_t possible_crtcs)
  

+{

• int ret = 0;
• drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
• wb_connector->encoder.possible_crtcs = possible_crtcs;
• ret = drm_encoder_init(dev, &wb_connector->encoder,

• 	       &drm_writeback_encoder_funcs,
  

• 	       DRM_MODE_ENCODER_VIRTUAL, NULL);
  

• if (ret)

• return ret;
  

• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• if (ret)

• drm_encoder_cleanup(&wb_connector->encoder);
  

• return ret;

+} EXPORT_SYMBOL(drm_writeback_connector_init); +/**

• • drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties
• • using the encoder which already assigned and initialized
• • @dev: DRM device
• • @wb_connector: Writeback connector to initialize
• • @con_funcs: Connector funcs vtable
• • @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
• • @formats: Array of supported pixel formats for the writeback engine
• • @n_formats: Length of the formats array
• • This function creates the writeback-connector-specific properties if they
• • have not been already created, initializes the connector as
• • type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
• • values.
• • This function assumes that the drm_writebac_connector's encoder has already been

spelling: writeback

Ack. will fix this.

...

...

• • created and initialized before invoking this function.
• • In addition, this function also assumes that callers of this API will manage
• • assigning the encoder helper functions, possible_crtcs and any other encoder
• • specific operation which is otherwise handled by drm_writeback_connector_init().
• • Drivers should always use this function instead of drm_connector_init() to
• • set up writeback connectors.

.... if they want to manage themselves the lifetime of the associated encoder.

We're not trying to replace drm_writeback_connector_init() function here, only to provide an alternative function to call for special cases.

Yes, we are trying to provide an alternative function call for special case where the encoder is a shared encoder and/or where the resources of the writeback encoder are shared with other hardware blocks.

I can add that comment to this function's doc.

...

...

• • Returns: 0 on success, or a negative error code
• */

+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• struct drm_writeback_connector *wb_connector,
  

• const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• int n_formats)
  

Where is the encoder parameter? Isn't that the reason why the function is called `_with_encoder`?

The encoder parameter is skipped here because this function assumes that wb_connector->encoder has already been initialized, setup with functions and its possible_crts have already been set prior to calling this function like the vc4 example shows. So this function doesnt need an explicit encoder parameter. Let me know if thats a concern.

...

I think there might have been too many version of these patchsets and things are starting to be confusing. Please revisit the series without rushing and come up with a plan of action. My understanding of watching this series has been that you're trying to come up with a function that does *connector* initialisation but skips the *encoder* initialisation because it might have been already done by the driver. The idea will be then to have a function `drm_writeback_connector_init_with_encoder()` that does *all* the work that `drm_writeback_connector_init()` does at the moment minus the encoder initialisation part. Then `drm_writeback_connector_init()` only initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`. There is no need to have the `drm_writeback_connector_setup()` function at all.

Best regards, Liviu

I agree there have been a 4 revisions prior to this because of the way this affects the existing writeback drivers. So initial revision was a bit intrusive into other drivers (which was just mostly a take over from the previous patchset posted by the Co-developer ) and since rev3 we have decided to have a separate API drm_writeback_connector_init_with_encoder() so that existing clients are unaffected and it works seamlessly under the hood.

Only clients which already embed an encoder (vc4) and the new ones which have special encoder requirements like the MSM driver for which I am preparing these changes for will use the new API.

Apologies for the revisions, but thanks to some great feedback from you and laurent its shaping up nicely and reaching its conclusion I feel.

I think it is only natural that there will be some iterations. If I remember correctly, the initial writeback series has something like 13 revisions :)

...

So i think this revision is pretty close to being clean thanks to the feedback you gave on rev4. Between rev4 and this one I didnt drastically change the design other than re-ordering the changes to avoid the intermediate patches having an incorrect state for the vc4 encoder. So all your comments related to the encoder_cleanup() and vc4's encoder not getting initialized would have gotten addressed but overall concept remained same.

You are right, we are trying to come up with a function which does connector initialization but skips the encoder part because that has already been done in the client side of this API ( hence _with_encoder() name ). The "_with_encoder" indicates that the caller already has an encoder for the writeback connector which is being passed so there is no need to pass the encoder again.

I thought this addresses all the concerns posted in the previous series.

So are you suggesting something like below?

1. rename drm_writeback_connector_setup() to

drm_writeback_connector_init_with_encoder() (essentially thats what will end up happening )

Correct. Without the pointless reordering of code it should be about 3 lines of code that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).

But isnt thats how it already looks.

int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs) { int ret = 0;

wb_connector->encoder = &wb_connector->internal_encoder;

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder,
               &drm_writeback_encoder_funcs,
               DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
    return ret;

ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs,

formats, n_formats);

So the only change you are requesting is that, instead of having a new drm_writeback_connector_setup(), just call drm_writeback_connector_init_with_encoder().

It will essentially look like

int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs) { int ret = 0;

wb_connector->encoder = &wb_connector->internal_encoder;

(1)

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder,
               &drm_writeback_encoder_funcs,
               DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
    return ret;

ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,

con_funcs, formats, n_formats);

Yes, this is exactly what I had in mind.

drm_writeback_connector_init_with_encoder() will still not have an encoder parameter because of what I wrote previously.

But in your patch drm_writeback_connector_init_with_encoder() still has an encoder_funcs pointer which is useless, as the expectations are (AFAII) that the whole encoder init dance has already happened. And while you have a point that you can set the encoder pointer in the connector before calling drm_writeback_connector_init_with_encoder() I think it would be easier to read if you pass the encoder explicitly in the parameter list and skip the assignment in (1) and do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not wrong, I just think we should be explicit so that code is easier to read.

Hope this is what you had in mind as well.

...

...

2. Inside drm_writeback_connector_init() check:

drm_writeback_connector_init(.....) { if (!wb_conn->encoder) initialize the encoder

No, the assumption of drm_writeback_connector_init() is that we will provide the encoder, so no need to check that one is already provided. What you could do is:

  WARN_ON(wb_conn->encoder);

Got it, i will add a warning inside drm_writeback_connector_init() to emphasize that its only meant for cases where an encoder is not provided.

...

before we overwrite the encoder. That way we will get a nice warning in the kernel log if someone tries to call drm_writeback_connector_init() with a preset encoder.

...

call drm_writeback_**_init_with_encoder() 				

}

This will also work but have the foll concerns/questions:

1. Will drm_writeback_connector_init_with_encoder() be exported if we change

as per your suggestion or will all clients continue to use drm_writeback_connector_init() only? We wanted to have a separate function for new clients.

Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.

Alright, so vc4 and new vendors which provide their own encoder will use this one. So no changes to the rest of the series.

...

...

2. How will we detect that encoder needs to be initialized without it being

a pointer which happens in the later change. So ordering becomes an issue.

I think the init problem is simple. You either call drm_writeback_connector_init() and the framework provides you with an encoder, or you call drm_writeback_connector_init_with_encoder() and the framework will use yours. The problems will show up on the cleanup and exit codes, where we need to be able to skip the cleanup if the encoder pointer is not the internal one. I think a simple

 if (connector->encoder == &connector->internal_encoder)
       do_encoder_cleanup_work_here()

should work.

Sorry, I am missing something here.

Even in this current patch, the drm_encoder_cleanup() is done only inside drm_writeback_connector_init() where internal_encoder is used.

For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and expect the caller to do it like vc4 does in the next patch.

So why do we need such a condition?

You're right. I was thinking that at cleanup time we also need to do work with the right encoder, but that should accomplished by passing the right .destroy hook in the drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the initialisation correctly it should all work fine, please disregard my comments.

Best regards, Liviu

...

...

Thats why I thought this is the best way to address the comments and keep the functionality intact with each change.

Let me know if I have misunderstood some comment or idea.

I hope that with these clarifications we are both on the same page.

Best regards, Liviu

...

...

...

+{

• int ret = 0;
• ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,

• 	n_formats);
  

• return ret;

+} +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);

• int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) {

diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index db6214f..0093bab 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -152,6 +152,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs); +int drm_writeback_connector_init_with_encoder(struct drm_device *dev,

• 		struct drm_writeback_connector *wb_connector,
  

• 		const struct drm_connector_funcs *con_funcs, const u32 *formats,
  

• 		int n_formats);
  

• int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);

-- 2.7.4

Hi Liviu

Gentle reminder ... Can you please help to clarify the last set of questions so that I can work on the next version?

Thanks

Abhinav

On 3/25/2022 9:31 AM, Abhinav Kumar wrote:

Hi Liviu

On 3/25/2022 3:19 AM, Liviu Dudau wrote:

...

On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hello,

...

Thanks for the response.

On 3/24/2022 3:12 AM, Liviu Dudau wrote:

...

On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hello,

...

Thanks for the review.

On 3/23/2022 9:46 AM, Liviu Dudau wrote:

...

On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote: > For vendors drivers which pass an already allocated and > initialized encoder especially for cases where the encoder > hardware is shared OR the writeback encoder shares the resources > with the rest of the display pipeline introduce a new API, > drm_writeback_connector_init_with_encoder() which expects > an initialized encoder as a parameter and only sets up the > writeback connector. > > changes in v5: >     - reorder this change to come before in the series >       to avoid incorrect functionality in subsequent changes >     - continue using struct drm_encoder instead of >       struct drm_encoder * and switch it in next change > > Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com

Hi Abhinav,

> --- >     drivers/gpu/drm/drm_writeback.c | 143 > ++++++++++++++++++++++++++++------------ >     include/drm/drm_writeback.h     |   5 ++ >     2 files changed, 106 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c > b/drivers/gpu/drm/drm_writeback.c > index dc2ef12..abe78b9 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs > drm_writeback_encoder_funcs = { >         .destroy = drm_encoder_cleanup, >     }; > -/** > - * drm_writeback_connector_init - Initialize a writeback > connector and its properties > - * @dev: DRM device > - * @wb_connector: Writeback connector to initialize > - * @con_funcs: Connector funcs vtable > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by > the internal encoder > - * @formats: Array of supported pixel formats for the writeback > engine > - * @n_formats: Length of the formats array > - * @possible_crtcs: possible crtcs for the internal writeback > encoder > - * > - * This function creates the writeback-connector-specific > properties if they > - * have not been already created, initializes the connector as > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes > the property > - * values. It will also create an internal encoder associated > with the > - * drm_writeback_connector and set it to use the > @enc_helper_funcs vtable for > - * the encoder helper. > - * > - * Drivers should always use this function instead of > drm_connector_init() to > - * set up writeback connectors. > - * > - * Returns: 0 on success, or a negative error code > - */ > -int drm_writeback_connector_init(struct drm_device *dev, > -                 struct drm_writeback_connector *wb_connector, > -                 const struct drm_connector_funcs *con_funcs, > -                 const struct drm_encoder_helper_funcs > *enc_helper_funcs, > -                 const u32 *formats, int n_formats, uint32_t > possible_crtcs) > +static int drm_writeback_connector_setup(struct drm_device *dev, > +        struct drm_writeback_connector *wb_connector, > +        const struct drm_connector_funcs *con_funcs, const u32 > *formats, > +        int n_formats) >     { >         struct drm_property_blob *blob; > -    struct drm_connector *connector = &wb_connector->base; >         struct drm_mode_config *config = &dev->mode_config; > +    struct drm_connector *connector = &wb_connector->base; > +

Point of this reordering the statements is...?

This diff can be avoided. There was no reason to reorder this. I will remove this re-order.

...

>         int ret = create_writeback_properties(dev); >         if (ret != 0) > @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct > drm_device *dev, >         blob = drm_property_create_blob(dev, n_formats * > sizeof(*formats), >                         formats); > -    if (IS_ERR(blob)) > -        return PTR_ERR(blob); > - > -    drm_encoder_helper_add(&wb_connector->encoder, > enc_helper_funcs); > - > -    wb_connector->encoder.possible_crtcs = possible_crtcs; > - > -    ret = drm_encoder_init(dev, &wb_connector->encoder, > -                   &drm_writeback_encoder_funcs, > -                   DRM_MODE_ENCODER_VIRTUAL, NULL); > -    if (ret) > -        goto fail; > +    if (IS_ERR(blob)) { > +        ret = PTR_ERR(blob); > +        return ret; > +    }

I don't see why you have changed the earlier code to store the result of PTR_ERR into ret other than to help your debugging. I suggest that you keep the existing code that returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

Sure, i can fix this for a smaller diff stat.

...

>         connector->interlace_allowed = 0; > @@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct > drm_device *dev, >     attach_fail: >         drm_connector_cleanup(connector); >     connector_fail: > -    drm_encoder_cleanup(&wb_connector->encoder); > -fail: >         drm_property_blob_put(blob); >         return ret; >     } > + > +/** > + * drm_writeback_connector_init - Initialize a writeback > connector and its properties > + * @dev: DRM device > + * @wb_connector: Writeback connector to initialize > + * @con_funcs: Connector funcs vtable > + * @enc_helper_funcs: Encoder helper funcs vtable to be used by > the internal encoder > + * @formats: Array of supported pixel formats for the writeback > engine > + * @n_formats: Length of the formats array > + * @possible_crtcs: possible crtcs for the internal writeback > encoder > + * > + * This function creates the writeback-connector-specific > properties if they > + * have not been already created, initializes the connector as > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes > the property > + * values. It will also create an internal encoder associated > with the > + * drm_writeback_connector and set it to use the > @enc_helper_funcs vtable for > + * the encoder helper. > + * > + * Drivers should always use this function instead of > drm_connector_init() to > + * set up writeback connectors. > + * > + * Returns: 0 on success, or a negative error code > + */ > +int drm_writeback_connector_init(struct drm_device *dev, > +        struct drm_writeback_connector *wb_connector, > +        const struct drm_connector_funcs *con_funcs, > +        const struct drm_encoder_helper_funcs *enc_helper_funcs, > +        const u32 *formats, int n_formats, uint32_t possible_crtcs) > +{ > +    int ret = 0; > + > +    drm_encoder_helper_add(&wb_connector->encoder, > enc_helper_funcs); > + > +    wb_connector->encoder.possible_crtcs = possible_crtcs; > + > +    ret = drm_encoder_init(dev, &wb_connector->encoder, > +                   &drm_writeback_encoder_funcs, > +                   DRM_MODE_ENCODER_VIRTUAL, NULL); > +    if (ret) > +        return ret; > + > +    ret = drm_writeback_connector_setup(dev, wb_connector, > con_funcs, formats, > +            n_formats); > + > +    if (ret) > +        drm_encoder_cleanup(&wb_connector->encoder); > + > +    return ret; > +} >     EXPORT_SYMBOL(drm_writeback_connector_init); > +/** > + * drm_writeback_connector_init_with_encoder - Initialize a > writeback connector and its properties > + * using the encoder which already assigned and initialized > + * > + * @dev: DRM device > + * @wb_connector: Writeback connector to initialize > + * @con_funcs: Connector funcs vtable > + * @enc_helper_funcs: Encoder helper funcs vtable to be used by > the internal encoder > + * @formats: Array of supported pixel formats for the writeback > engine > + * @n_formats: Length of the formats array > + * > + * This function creates the writeback-connector-specific > properties if they > + * have not been already created, initializes the connector as > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes > the property > + * values. > + * > + * This function assumes that the drm_writebac_connector's > encoder has already been

spelling: writeback

Ack. will fix this.

...

> + * created and initialized before invoking this function. > + * > + * In addition, this function also assumes that callers of this > API will manage > + * assigning the encoder helper functions, possible_crtcs and > any other encoder > + * specific operation which is otherwise handled by > drm_writeback_connector_init(). > + * > + * Drivers should always use this function instead of > drm_connector_init() to > + * set up writeback connectors.

.... if they want to manage themselves the lifetime of the associated encoder.

We're not trying to replace drm_writeback_connector_init() function here, only to provide an alternative function to call for special cases.

Yes, we are trying to provide an alternative function call for special case where the encoder is a shared encoder and/or where the resources of the writeback encoder are shared with other hardware blocks.

I can add that comment to this function's doc.

...

> + * > + * Returns: 0 on success, or a negative error code > + */ > +int drm_writeback_connector_init_with_encoder(struct drm_device > *dev, > +        struct drm_writeback_connector *wb_connector, > +        const struct drm_connector_funcs *con_funcs, const u32 > *formats, > +        int n_formats)

Where is the encoder parameter? Isn't that the reason why the function is called `_with_encoder`?

The encoder parameter is skipped here because this function assumes that wb_connector->encoder has already been initialized, setup with functions and its possible_crts have already been set prior to calling this function like the vc4 example shows. So this function doesnt need an explicit encoder parameter. Let me know if thats a concern.

...

I think there might have been too many version of these patchsets and things are starting to be confusing. Please revisit the series without rushing and come up with a plan of action. My understanding of watching this series has been that you're trying to come up with a function that does *connector* initialisation but skips the *encoder* initialisation because it might have been already done by the driver. The idea will be then to have a function `drm_writeback_connector_init_with_encoder()` that does *all* the work that `drm_writeback_connector_init()` does at the moment minus the encoder initialisation part. Then `drm_writeback_connector_init()` only initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`. There is no need to have the `drm_writeback_connector_setup()` function at all.

Best regards, Liviu

I agree there have been a 4 revisions prior to this because of the way this affects the existing writeback drivers. So initial revision was a bit intrusive into other drivers (which was just mostly a take over from the previous patchset posted by the Co-developer ) and since rev3 we have decided to have a separate API drm_writeback_connector_init_with_encoder() so that existing clients are unaffected and it works seamlessly under the hood.

Only clients which already embed an encoder (vc4) and the new ones which have special encoder requirements like the MSM driver for which I am preparing these changes for will use the new API.

Apologies for the revisions, but thanks to some great feedback from you and laurent its shaping up nicely and reaching its conclusion I feel.

I think it is only natural that there will be some iterations. If I remember correctly, the initial writeback series has something like 13 revisions :)

...

So i think this revision is pretty close to being clean thanks to the feedback you gave on rev4. Between rev4 and this one I didnt drastically change the design other than re-ordering the changes to avoid the intermediate patches having an incorrect state for the vc4 encoder. So all your comments related to the encoder_cleanup() and vc4's encoder not getting initialized would have gotten addressed but overall concept remained same.

You are right, we are trying to come up with a function which does connector initialization but skips the encoder part because that has already been done in the client side of this API ( hence _with_encoder() name ). The "_with_encoder" indicates that the caller already has an encoder for the writeback connector which is being passed so there is no need to pass the encoder again.

I thought this addresses all the concerns posted in the previous series.

So are you suggesting something like below?

1. rename drm_writeback_connector_setup() to

drm_writeback_connector_init_with_encoder() (essentially thats what will end up happening )

Correct. Without the pointless reordering of code it should be about 3 lines of code that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).

But isnt thats how it already looks.

int drm_writeback_connector_init(struct drm_device *dev,          struct drm_writeback_connector *wb_connector,          const struct drm_connector_funcs *con_funcs,          const struct drm_encoder_helper_funcs *enc_helper_funcs,          const u32 *formats, int n_formats, uint32_t possible_crtcs) {      int ret = 0;

wb_connector->encoder = &wb_connector->internal_encoder;

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder,                     &drm_writeback_encoder_funcs,                     DRM_MODE_ENCODER_VIRTUAL, NULL);      if (ret)          return ret;

ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,              n_formats);

So the only change you are requesting is that, instead of having a new drm_writeback_connector_setup(), just call drm_writeback_connector_init_with_encoder().

It will essentially look like

int drm_writeback_connector_init(struct drm_device *dev,          struct drm_writeback_connector *wb_connector,          const struct drm_connector_funcs *con_funcs,          const struct drm_encoder_helper_funcs *enc_helper_funcs,          const u32 *formats, int n_formats, uint32_t possible_crtcs) {      int ret = 0;

wb_connector->encoder = &wb_connector->internal_encoder;

(1)

...

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder,                     &drm_writeback_encoder_funcs,                     DRM_MODE_ENCODER_VIRTUAL, NULL);      if (ret)          return ret;

ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, con_funcs, formats,              n_formats);

Yes, this is exactly what I had in mind.

Thank you for confirming.

...

...

drm_writeback_connector_init_with_encoder() will still not have an encoder parameter because of what I wrote previously.

But in your patch drm_writeback_connector_init_with_encoder() still has an encoder_funcs pointer which is useless, as the expectations are (AFAII) that the whole encoder init dance has already happened. And while you have a point that you can set the encoder pointer in the connector before calling drm_writeback_connector_init_with_encoder() I think it would be easier to read if you pass the encoder explicitly in the parameter list and skip the assignment in (1) and do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not wrong, I just think we should be explicit so that code is easier to read.

Sorry, but I am missing something here.

Where is the encoder_funcs pointer in drm_writeback_connector_init_with_encoder()? It only has drm_connector_funcs pointer. Because, like I mentioned in the earlier comment, the callers of this API would have already setup the encoder with the required functions/possible_crtcs etc.

int drm_writeback_connector_init_with_encoder(struct drm_device *dev,         struct drm_writeback_connector *wb_connector,         const struct drm_connector_funcs *con_funcs, const u32 *formats,         int n_formats) {     int ret = 0;

ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,             n_formats);

return ret; }

So are you suggesting I pass the drm_encoder as a parameter to drm_writeback_connector_init_with_encoder as well?

That has two potential issues:

1. If we move just the assignment of wb_connector->encoder to inside

drm_writeback_connector_init_with_encoder, we cannot do these before calling drm_writeback_connector_init_with_encoder() as wb_connector->encoder will be NULL for the cases which use internal_encoder

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder,                    &drm_writeback_encoder_funcs,                    DRM_MODE_ENCODER_VIRTUAL, NULL);

2. We can only do these changes once wb_connector->encoder has been

changed to a pointer which happens in patch 4 of this series https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5

I was told to split the functionality of adding the new API in a separate patch by laurent.

To keep changes independent and functionally correct I thought this is the best split.

So the cleanup you are suggesting can be done in patch 4 of this series but still I am not sure how to get around concern (1) because the encoder has to be fully initialized before attaching to a wb_connector.

...

...

Hope this is what you had in mind as well.

...

...

2. Inside drm_writeback_connector_init() check:

drm_writeback_connector_init(.....) {      if (!wb_conn->encoder)     initialize the encoder

No, the assumption of drm_writeback_connector_init() is that we will provide the encoder, so no need to check that one is already provided. What you could do is:

WARN_ON(wb_conn->encoder);

Got it, i will add a warning inside drm_writeback_connector_init() to emphasize that its only meant for cases where an encoder is not provided.

...

before we overwrite the encoder. That way we will get a nice warning in the kernel log if someone tries to call drm_writeback_connector_init() with a preset encoder.

...

call drm_writeback_**_init_with_encoder() }

This will also work but have the foll concerns/questions:

1. Will drm_writeback_connector_init_with_encoder() be exported if

we change as per your suggestion or will all clients continue to use drm_writeback_connector_init() only? We wanted to have a separate function for new clients.

Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.

Alright, so vc4 and new vendors which provide their own encoder will use this one. So no changes to the rest of the series.

...

...

2. How will we detect that encoder needs to be initialized without

it being a pointer which happens in the later change. So ordering becomes an issue.

I think the init problem is simple. You either call drm_writeback_connector_init() and the framework provides you with an encoder, or you call drm_writeback_connector_init_with_encoder() and the framework will use yours. The problems will show up on the cleanup and exit codes, where we need to be able to skip the cleanup if the encoder pointer is not the internal one. I think a simple

if (connector->encoder == &connector->internal_encoder)             do_encoder_cleanup_work_here()

should work.

Sorry, I am missing something here.

Even in this current patch, the drm_encoder_cleanup() is done only inside drm_writeback_connector_init() where internal_encoder is used.

For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and expect the caller to do it like vc4 does in the next patch.

So why do we need such a condition?

You're right. I was thinking that at cleanup time we also need to do work with the right encoder, but that should accomplished by passing the right .destroy hook in the drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the initialisation correctly it should all work fine, please disregard my comments.

Best regards, Liviu

...

...

...

Thats why I thought this is the best way to address the comments and keep the functionality intact with each change.

Let me know if I have misunderstood some comment or idea.

I hope that with these clarifications we are both on the same page.

Best regards, Liviu

...

...

> +{ > +    int ret = 0; > + > +    ret = drm_writeback_connector_setup(dev, wb_connector, > con_funcs, formats, > +            n_formats); > + > +    return ret; > +} > +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > + >     int drm_writeback_set_fb(struct drm_connector_state *conn_state, >                  struct drm_framebuffer *fb) >     { > diff --git a/include/drm/drm_writeback.h > b/include/drm/drm_writeback.h > index db6214f..0093bab 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -152,6 +152,11 @@ int drm_writeback_connector_init(struct > drm_device *dev, >                      const struct drm_encoder_helper_funcs > *enc_helper_funcs, >                      const u32 *formats, int n_formats, uint32_t > possible_crtcs); > +int drm_writeback_connector_init_with_encoder(struct drm_device > *dev, > +                struct drm_writeback_connector *wb_connector, > +                const struct drm_connector_funcs *con_funcs, > const u32 *formats, > +                int n_formats); > + >     int drm_writeback_set_fb(struct drm_connector_state *conn_state, >                  struct drm_framebuffer *fb); > -- > 2.7.4 >

Hi Liviu

Thanks for the response.

Let me fix up and spin a new version.

Abhinav

On 3/31/2022 3:01 AM, Liviu Dudau wrote:

On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hi Abhinav,

Sorry for the delay, got busy with other things at the beginning of the week.

...

On 3/25/2022 3:19 AM, Liviu Dudau wrote:

...

On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hello,

...

Thanks for the response.

On 3/24/2022 3:12 AM, Liviu Dudau wrote:

...

On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:

...

Hi Liviu

Hello,

...

Thanks for the review.

On 3/23/2022 9:46 AM, Liviu Dudau wrote: > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote: >> For vendors drivers which pass an already allocated and >> initialized encoder especially for cases where the encoder >> hardware is shared OR the writeback encoder shares the resources >> with the rest of the display pipeline introduce a new API, >> drm_writeback_connector_init_with_encoder() which expects >> an initialized encoder as a parameter and only sets up the >> writeback connector. >> >> changes in v5: >> - reorder this change to come before in the series >> to avoid incorrect functionality in subsequent changes >> - continue using struct drm_encoder instead of >> struct drm_encoder * and switch it in next change >> >> Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com > > Hi Abhinav, > >> --- >> drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------ >> include/drm/drm_writeback.h | 5 ++ >> 2 files changed, 106 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c >> index dc2ef12..abe78b9 100644 >> --- a/drivers/gpu/drm/drm_writeback.c >> +++ b/drivers/gpu/drm/drm_writeback.c >> @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { >> .destroy = drm_encoder_cleanup, >> }; >> -/** >> - * drm_writeback_connector_init - Initialize a writeback connector and its properties >> - * @dev: DRM device >> - * @wb_connector: Writeback connector to initialize >> - * @con_funcs: Connector funcs vtable >> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder >> - * @formats: Array of supported pixel formats for the writeback engine >> - * @n_formats: Length of the formats array >> - * @possible_crtcs: possible crtcs for the internal writeback encoder >> - * >> - * This function creates the writeback-connector-specific properties if they >> - * have not been already created, initializes the connector as >> - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property >> - * values. It will also create an internal encoder associated with the >> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for >> - * the encoder helper. >> - * >> - * Drivers should always use this function instead of drm_connector_init() to >> - * set up writeback connectors. >> - * >> - * Returns: 0 on success, or a negative error code >> - */ >> -int drm_writeback_connector_init(struct drm_device *dev, >> - struct drm_writeback_connector *wb_connector, >> - const struct drm_connector_funcs *con_funcs, >> - const struct drm_encoder_helper_funcs *enc_helper_funcs, >> - const u32 *formats, int n_formats, uint32_t possible_crtcs) >> +static int drm_writeback_connector_setup(struct drm_device *dev, >> + struct drm_writeback_connector *wb_connector, >> + const struct drm_connector_funcs *con_funcs, const u32 *formats, >> + int n_formats) >> { >> struct drm_property_blob *blob; >> - struct drm_connector *connector = &wb_connector->base; >> struct drm_mode_config *config = &dev->mode_config; >> + struct drm_connector *connector = &wb_connector->base; >> + > > Point of this reordering the statements is...? This diff can be avoided. There was no reason to reorder this. I will remove this re-order. > >> int ret = create_writeback_properties(dev); >> if (ret != 0) >> @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev, >> blob = drm_property_create_blob(dev, n_formats * sizeof(*formats), >> formats); >> - if (IS_ERR(blob)) >> - return PTR_ERR(blob); >> - >> - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); >> - >> - wb_connector->encoder.possible_crtcs = possible_crtcs; >> - >> - ret = drm_encoder_init(dev, &wb_connector->encoder, >> - &drm_writeback_encoder_funcs, >> - DRM_MODE_ENCODER_VIRTUAL, NULL); >> - if (ret) >> - goto fail; >> + if (IS_ERR(blob)) { >> + ret = PTR_ERR(blob); >> + return ret; >> + } > > I don't see why you have changed the earlier code to store the result of PTR_ERR into > ret other than to help your debugging. I suggest that you keep the existing code that > returns PTR_ERR(blob) directly and you will have a nicer diff stat as well. Sure, i can fix this for a smaller diff stat. > >> connector->interlace_allowed = 0; >> @@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device *dev, >> attach_fail: >> drm_connector_cleanup(connector); >> connector_fail: >> - drm_encoder_cleanup(&wb_connector->encoder); >> -fail: >> drm_property_blob_put(blob); >> return ret; >> } >> + >> +/** >> + * drm_writeback_connector_init - Initialize a writeback connector and its properties >> + * @dev: DRM device >> + * @wb_connector: Writeback connector to initialize >> + * @con_funcs: Connector funcs vtable >> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder >> + * @formats: Array of supported pixel formats for the writeback engine >> + * @n_formats: Length of the formats array >> + * @possible_crtcs: possible crtcs for the internal writeback encoder >> + * >> + * This function creates the writeback-connector-specific properties if they >> + * have not been already created, initializes the connector as >> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property >> + * values. It will also create an internal encoder associated with the >> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for >> + * the encoder helper. >> + * >> + * Drivers should always use this function instead of drm_connector_init() to >> + * set up writeback connectors. >> + * >> + * Returns: 0 on success, or a negative error code >> + */ >> +int drm_writeback_connector_init(struct drm_device *dev, >> + struct drm_writeback_connector *wb_connector, >> + const struct drm_connector_funcs *con_funcs, >> + const struct drm_encoder_helper_funcs *enc_helper_funcs, >> + const u32 *formats, int n_formats, uint32_t possible_crtcs) >> +{ >> + int ret = 0; >> + >> + drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); >> + >> + wb_connector->encoder.possible_crtcs = possible_crtcs; >> + >> + ret = drm_encoder_init(dev, &wb_connector->encoder, >> + &drm_writeback_encoder_funcs, >> + DRM_MODE_ENCODER_VIRTUAL, NULL); >> + if (ret) >> + return ret; >> + >> + ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats, >> + n_formats); >> + >> + if (ret) >> + drm_encoder_cleanup(&wb_connector->encoder); >> + >> + return ret; >> +} >> EXPORT_SYMBOL(drm_writeback_connector_init); >> +/** >> + * drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties >> + * using the encoder which already assigned and initialized >> + * >> + * @dev: DRM device >> + * @wb_connector: Writeback connector to initialize >> + * @con_funcs: Connector funcs vtable >> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder

This is the encoder helper funcs that I was talking about. Maybe it is better if you send a new, cleaner version and we can continue the conversation/review there?

Best regards, Liviu

The doc is wrong here, I will fix this and spin a new version.

...

...

...

...

...

>> + * @formats: Array of supported pixel formats for the writeback engine >> + * @n_formats: Length of the formats array >> + * >> + * This function creates the writeback-connector-specific properties if they >> + * have not been already created, initializes the connector as >> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property >> + * values. >> + * >> + * This function assumes that the drm_writebac_connector's encoder has already been > > spelling: writeback Ack. will fix this. > >> + * created and initialized before invoking this function. >> + * >> + * In addition, this function also assumes that callers of this API will manage >> + * assigning the encoder helper functions, possible_crtcs and any other encoder >> + * specific operation which is otherwise handled by drm_writeback_connector_init(). >> + * >> + * Drivers should always use this function instead of drm_connector_init() to >> + * set up writeback connectors. > > .... if they want to manage themselves the lifetime of the associated encoder. > > We're not trying to replace drm_writeback_connector_init() function here, only to > provide an alternative function to call for special cases.

Yes, we are trying to provide an alternative function call for special case where the encoder is a shared encoder and/or where the resources of the writeback encoder are shared with other hardware blocks.

I can add that comment to this function's doc.

> >> + * >> + * Returns: 0 on success, or a negative error code >> + */ >> +int drm_writeback_connector_init_with_encoder(struct drm_device *dev, >> + struct drm_writeback_connector *wb_connector, >> + const struct drm_connector_funcs *con_funcs, const u32 *formats, >> + int n_formats) > > Where is the encoder parameter? Isn't that the reason why the function is called > `_with_encoder`? The encoder parameter is skipped here because this function assumes that wb_connector->encoder has already been initialized, setup with functions and its possible_crts have already been set prior to calling this function like the vc4 example shows. So this function doesnt need an explicit encoder parameter. Let me know if thats a concern. > > I think there might have been too many version of these patchsets and things are > starting to be confusing. Please revisit the series without rushing and come up with > a plan of action. My understanding of watching this series has been that you're > trying to come up with a function that does *connector* initialisation but skips the > *encoder* initialisation because it might have been already done by the driver. The > idea will be then to have a function `drm_writeback_connector_init_with_encoder()` > that does *all* the work that `drm_writeback_connector_init()` does at the moment minus > the encoder initialisation part. Then `drm_writeback_connector_init()` only > initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`. > There is no need to have the `drm_writeback_connector_setup()` function at all. > > Best regards, > Liviu >

I agree there have been a 4 revisions prior to this because of the way this affects the existing writeback drivers. So initial revision was a bit intrusive into other drivers (which was just mostly a take over from the previous patchset posted by the Co-developer ) and since rev3 we have decided to have a separate API drm_writeback_connector_init_with_encoder() so that existing clients are unaffected and it works seamlessly under the hood.

Only clients which already embed an encoder (vc4) and the new ones which have special encoder requirements like the MSM driver for which I am preparing these changes for will use the new API.

Apologies for the revisions, but thanks to some great feedback from you and laurent its shaping up nicely and reaching its conclusion I feel.

I think it is only natural that there will be some iterations. If I remember correctly, the initial writeback series has something like 13 revisions :)

...

So i think this revision is pretty close to being clean thanks to the feedback you gave on rev4. Between rev4 and this one I didnt drastically change the design other than re-ordering the changes to avoid the intermediate patches having an incorrect state for the vc4 encoder. So all your comments related to the encoder_cleanup() and vc4's encoder not getting initialized would have gotten addressed but overall concept remained same.

You are right, we are trying to come up with a function which does connector initialization but skips the encoder part because that has already been done in the client side of this API ( hence _with_encoder() name ). The "_with_encoder" indicates that the caller already has an encoder for the writeback connector which is being passed so there is no need to pass the encoder again.

I thought this addresses all the concerns posted in the previous series.

So are you suggesting something like below?

1. rename drm_writeback_connector_setup() to

drm_writeback_connector_init_with_encoder() (essentially thats what will end up happening )

Correct. Without the pointless reordering of code it should be about 3 lines of code that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).

But isnt thats how it already looks.

int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs) { int ret = 0;

  wb_connector->encoder = &wb_connector->internal_encoder;

  drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

  wb_connector->encoder->possible_crtcs = possible_crtcs;

  ret = drm_encoder_init(dev, wb_connector->encoder,
                 &drm_writeback_encoder_funcs,
                 DRM_MODE_ENCODER_VIRTUAL, NULL);
  if (ret)
      return ret;

  ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs,

formats, n_formats);

So the only change you are requesting is that, instead of having a new drm_writeback_connector_setup(), just call drm_writeback_connector_init_with_encoder().

It will essentially look like

int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats, uint32_t possible_crtcs) { int ret = 0;

  wb_connector->encoder = &wb_connector->internal_encoder;

(1)

...

  drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

  wb_connector->encoder->possible_crtcs = possible_crtcs;

  ret = drm_encoder_init(dev, wb_connector->encoder,
                 &drm_writeback_encoder_funcs,
                 DRM_MODE_ENCODER_VIRTUAL, NULL);
  if (ret)
      return ret;

  ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,

con_funcs, formats, n_formats);

Yes, this is exactly what I had in mind.

Thank you for confirming.

...

...

drm_writeback_connector_init_with_encoder() will still not have an encoder parameter because of what I wrote previously.

But in your patch drm_writeback_connector_init_with_encoder() still has an encoder_funcs pointer which is useless, as the expectations are (AFAII) that the whole encoder init dance has already happened. And while you have a point that you can set the encoder pointer in the connector before calling drm_writeback_connector_init_with_encoder() I think it would be easier to read if you pass the encoder explicitly in the parameter list and skip the assignment in (1) and do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not wrong, I just think we should be explicit so that code is easier to read.

Sorry, but I am missing something here.

Where is the encoder_funcs pointer in drm_writeback_connector_init_with_encoder()? It only has drm_connector_funcs pointer. Because, like I mentioned in the earlier comment, the callers of this API would have already setup the encoder with the required functions/possible_crtcs etc.

int drm_writeback_connector_init_with_encoder(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, const u32 *formats, int n_formats) { int ret = 0;

 ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs,

formats, n_formats);

 return ret;

}

So are you suggesting I pass the drm_encoder as a parameter to drm_writeback_connector_init_with_encoder as well?

That has two potential issues:

1. If we move just the assignment of wb_connector->encoder to inside

drm_writeback_connector_init_with_encoder, we cannot do these before calling drm_writeback_connector_init_with_encoder() as wb_connector->encoder will be NULL for the cases which use internal_encoder

drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);

wb_connector->encoder->possible_crtcs = possible_crtcs;

ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL);

Where can you not do this? Inside drm_writeback_connector_init_with_encoder() you don't need to do that, as the function is intended to initialise a writeback connector using a provided encoder that has been already initialised. Inside drm_writeback_connector_init() you will call these functions using the internal encoder pointer that you have added in the series.

...

2. We can only do these changes once wb_connector->encoder has been changed

to a pointer which happens in patch 4 of this series https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5

I was told to split the functionality of adding the new API in a separate patch by laurent.

To keep changes independent and functionally correct I thought this is the best split.

So the cleanup you are suggesting can be done in patch 4 of this series but still I am not sure how to get around concern (1) because the encoder has to be fully initialized before attaching to a wb_connector.

I think if you keep the vc4 patch in patch 4 and move the rest in here it will make more sense. Again, I think a new series cleaned up might help others to review as well (specially Laurent).

Best regards, Liviu

...

...

...

Hope this is what you had in mind as well.

...

...

2. Inside drm_writeback_connector_init() check:

drm_writeback_connector_init(.....) { if (!wb_conn->encoder) initialize the encoder

No, the assumption of drm_writeback_connector_init() is that we will provide the encoder, so no need to check that one is already provided. What you could do is:

    WARN_ON(wb_conn->encoder);

Got it, i will add a warning inside drm_writeback_connector_init() to emphasize that its only meant for cases where an encoder is not provided.

...

before we overwrite the encoder. That way we will get a nice warning in the kernel log if someone tries to call drm_writeback_connector_init() with a preset encoder.

...

  call drm_writeback_**_init_with_encoder() 				

}

This will also work but have the foll concerns/questions:

1. Will drm_writeback_connector_init_with_encoder() be exported if we change

as per your suggestion or will all clients continue to use drm_writeback_connector_init() only? We wanted to have a separate function for new clients.

Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.

Alright, so vc4 and new vendors which provide their own encoder will use this one. So no changes to the rest of the series.

...

...

2. How will we detect that encoder needs to be initialized without it being

a pointer which happens in the later change. So ordering becomes an issue.

I think the init problem is simple. You either call drm_writeback_connector_init() and the framework provides you with an encoder, or you call drm_writeback_connector_init_with_encoder() and the framework will use yours. The problems will show up on the cleanup and exit codes, where we need to be able to skip the cleanup if the encoder pointer is not the internal one. I think a simple

   if (connector->encoder == &connector->internal_encoder)
         do_encoder_cleanup_work_here()

should work.

Sorry, I am missing something here.

Even in this current patch, the drm_encoder_cleanup() is done only inside drm_writeback_connector_init() where internal_encoder is used.

For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and expect the caller to do it like vc4 does in the next patch.

So why do we need such a condition?

You're right. I was thinking that at cleanup time we also need to do work with the right encoder, but that should accomplished by passing the right .destroy hook in the drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the initialisation correctly it should all work fine, please disregard my comments.

Best regards, Liviu

...

...

...

Thats why I thought this is the best way to address the comments and keep the functionality intact with each change.

Let me know if I have misunderstood some comment or idea.

I hope that with these clarifications we are both on the same page.

Best regards, Liviu

...

> >> +{ >> + int ret = 0; >> + >> + ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats, >> + n_formats); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); >> + >> int drm_writeback_set_fb(struct drm_connector_state *conn_state, >> struct drm_framebuffer *fb) >> { >> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h >> index db6214f..0093bab 100644 >> --- a/include/drm/drm_writeback.h >> +++ b/include/drm/drm_writeback.h >> @@ -152,6 +152,11 @@ int drm_writeback_connector_init(struct drm_device *dev, >> const struct drm_encoder_helper_funcs *enc_helper_funcs, >> const u32 *formats, int n_formats, uint32_t possible_crtcs); >> +int drm_writeback_connector_init_with_encoder(struct drm_device *dev, >> + struct drm_writeback_connector *wb_connector, >> + const struct drm_connector_funcs *con_funcs, const u32 *formats, >> + int n_formats); >> + >> int drm_writeback_set_fb(struct drm_connector_state *conn_state, >> struct drm_framebuffer *fb); >> -- >> 2.7.4 >> >