Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-31 Thread Abhinav Kumar

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 


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 = _connector->base;
struct drm_mode_config *config = >mode_config;
+   struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, _connector->encoder,
-  _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 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-31 Thread Liviu Dudau
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 
> > > > > > 
> > > > > > 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 = _connector->base;
> > > > > > >   struct drm_mode_config *config = >mode_config;
> > > > > > > + struct drm_connector *connector = _connector->base;
> > > > > > > +
> > > > > > 
> > > > > > Point of this reordering the statements 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-29 Thread Abhinav Kumar

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 


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 = _connector->base;
    struct drm_mode_config *config = >mode_config;
+    struct drm_connector *connector = _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(_connector->encoder, 
enc_helper_funcs);

-
-    wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-    ret = drm_encoder_init(dev, _connector->encoder,
-   _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);

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-25 Thread Abhinav Kumar

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 


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 = _connector->base;
struct drm_mode_config *config = >mode_config;
+   struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, _connector->encoder,
-  _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:
-   

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-25 Thread Liviu Dudau
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 
> > > > 
> > > > 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 = _connector->base;
> > > > >   struct drm_mode_config *config = >mode_config;
> > > > > + struct drm_connector *connector = _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);
> > > > > -
> > > > > - 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-24 Thread Abhinav Kumar

Hi Liviu

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 


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 = _connector->base;
struct drm_mode_config *config = >mode_config;
+   struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, _connector->encoder,
-  _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(_connector->encoder);
-fail:
drm_property_blob_put(blob);
return ret;
   }
+
+/**
+ * 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-24 Thread Liviu Dudau
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 
> > 
> > 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 = _connector->base;
> > >   struct drm_mode_config *config = >mode_config;
> > > + struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
> > > -
> > > - wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > -
> > > - ret = drm_encoder_init(dev, _connector->encoder,
> > > -_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 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-23 Thread Abhinav Kumar

Hi Liviu

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 


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 = _connector->base;
struct drm_mode_config *config = >mode_config;
+   struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, _connector->encoder,
-  _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(_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
+ * 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-23 Thread Liviu Dudau
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 

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 = _connector->base;
>   struct drm_mode_config *config = >mode_config;
> + struct drm_connector *connector = _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(_connector->encoder, enc_helper_funcs);
> -
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> -
> - ret = drm_encoder_init(dev, _connector->encoder,
> -_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(_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 

[PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-21 Thread Abhinav Kumar
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 
---
 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 = _connector->base;
struct drm_mode_config *config = >mode_config;
+   struct drm_connector *connector = _connector->base;
+
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(_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, _connector->encoder,
-  _writeback_encoder_funcs,
-  DRM_MODE_ENCODER_VIRTUAL, NULL);
-   if (ret)
-   goto fail;
+   if (IS_ERR(blob)) {
+   ret = PTR_ERR(blob);
+   return ret;
+   }
 
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(_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
+ *