[PATCH 0/6] Allow drm_writeback_connector to accept pointer to drm_encoder

[PATCH 0/6] Allow drm_writeback_connector to accept pointer to drm_encoder

[Abhinav Kumar]

There are some vendor drivers for which the writeback encoder shares
hardware resources such as clocks and interrupts with the rest of the
display pipeline. In addition, there can be use-cases where the
writeback encoder could be a shared encoder between the physical display
path and the writeback path.

To accommodate for such cases, change the drm_writeback_connector to
accept a pointer to drm_encoder.

For existing users of drm_writeback_connector there will not be any
change in functionality due to this change.

This approach was first posted by Suraj Kandpal here [1] for both
encoder and connector. But after discussions [2], the consensus was
reached to split this change for the drm_encoder first and the
drm_connector part can be reworked in a subsequent change later.

Validation of this change was done using igt_writeback tests on
MSM based RB5 board using the changes posted here [3].

For all other chipsets, these changes were compile-tested.

[1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-suraj.kandpal@intel.com/
[2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/
[3] https://patchwork.freedesktop.org/series/99724/

Abhinav Kumar (6):
  drm: allow real encoder to be passed for drm_writeback_connector
  drm/komeda: use drm_encoder pointer for drm_writeback_connector
  drm/vkms: use drm_encoder pointer for drm_writeback_connector
  drm/vc4: use drm_encoder pointer for drm_writeback_connector
  drm/rcar_du: use drm_encoder pointer for drm_writeback_connector
  drm/malidp: use drm_encoder pointer for drm_writeback_connector

 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c |  4 +++-
 drivers/gpu/drm/arm/malidp_mw.c                          |  3 ++-
 drivers/gpu/drm/drm_writeback.c                          |  8 ++++----
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c              |  3 ++-
 drivers/gpu/drm/vc4/vc4_txp.c                            | 14 ++++++++++----
 drivers/gpu/drm/vkms/vkms_writeback.c                    |  4 +++-
 include/drm/drm_writeback.h                              | 13 +++++++++++--
 7 files changed, 35 insertions(+), 14 deletions(-)

-- 
2.7.4

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

[Abhinav Kumar]

For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.

In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.

To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/drm_writeback.c |  8 ++++----
 include/drm/drm_writeback.h     | 13 +++++++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..4dad687 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
 	if (IS_ERR(blob))
 		return PTR_ERR(blob);
 
-	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
-	ret = drm_encoder_init(dev, &wb_connector->encoder,
+	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+	ret = drm_encoder_init(dev, wb_connector->encoder,
 			       &drm_writeback_encoder_funcs,
 			       DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret)
@@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 		goto connector_fail;
 
 	ret = drm_connector_attach_encoder(connector,
-						&wb_connector->encoder);
+						wb_connector->encoder);
 	if (ret)
 		goto attach_fail;
 
@@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 attach_fail:
 	drm_connector_cleanup(connector);
 connector_fail:
-	drm_encoder_cleanup(&wb_connector->encoder);
+	drm_encoder_cleanup(wb_connector->encoder);
 fail:
 	drm_property_blob_put(blob);
 	return ret;
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 9697d27..0ba266e 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,13 +25,22 @@ struct drm_writeback_connector {
 	struct drm_connector base;
 
 	/**
-	 * @encoder: Internal encoder used by the connector to fulfill
+	 * @encoder: handle to drm_encoder used by the connector to fulfill
 	 * the DRM framework requirements. The users of the
 	 * @drm_writeback_connector control the behaviour of the @encoder
 	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
 	 * function.
+	 *
+	 * For some vendor drivers, the hardware resources are shared between
+	 * writeback encoder and rest of the display pipeline.
+	 * To accommodate such cases, encoder is a handle to the real encoder
+	 * hardware.
+	 *
+	 * For current existing writeback users, this shall continue to be the
+	 * embedded encoder for the writeback connector.
+	 *
 	 */
-	struct drm_encoder encoder;
+	struct drm_encoder *encoder;
 
 	/**
 	 * @pixel_formats_blob_ptr:
-- 
2.7.4

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

[Dmitry Baryshkov]

On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
>
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
>
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
>
> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/drm_writeback.c |  8 ++++----
>  include/drm/drm_writeback.h     | 13 +++++++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dccf4504..4dad687 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
>         if (IS_ERR(blob))
>                 return PTR_ERR(blob);
>
> -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> -       ret = drm_encoder_init(dev, &wb_connector->encoder,
> +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> +       ret = drm_encoder_init(dev, wb_connector->encoder,
>                                &drm_writeback_encoder_funcs,
>                                DRM_MODE_ENCODER_VIRTUAL, NULL);

If the encoder is provided by a separate driver, it might use a
different set of encoder funcs.

I'd suggest checking whether the wb_connector->encoder is NULL here.
If it is, allocate one using drmm_kzalloc and init it.
If it is not NULL, assume that it has been initialized already, so
skip the drm_encoder_init() and just call the drm_encoder_helper_add()

>         if (ret)
> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>                 goto connector_fail;
>
>         ret = drm_connector_attach_encoder(connector,
> -                                               &wb_connector->encoder);
> +                                               wb_connector->encoder);
>         if (ret)
>                 goto attach_fail;
>
> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  attach_fail:
>         drm_connector_cleanup(connector);
>  connector_fail:
> -       drm_encoder_cleanup(&wb_connector->encoder);
> +       drm_encoder_cleanup(wb_connector->encoder);
>  fail:
>         drm_property_blob_put(blob);
>         return ret;
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 9697d27..0ba266e 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -25,13 +25,22 @@ struct drm_writeback_connector {
>         struct drm_connector base;
>
>         /**
> -        * @encoder: Internal encoder used by the connector to fulfill
> +        * @encoder: handle to drm_encoder used by the connector to fulfill
>          * the DRM framework requirements. The users of the
>          * @drm_writeback_connector control the behaviour of the @encoder
>          * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>          * function.
> +        *
> +        * For some vendor drivers, the hardware resources are shared between
> +        * writeback encoder and rest of the display pipeline.
> +        * To accommodate such cases, encoder is a handle to the real encoder
> +        * hardware.
> +        *
> +        * For current existing writeback users, this shall continue to be the
> +        * embedded encoder for the writeback connector.
> +        *
>          */
> -       struct drm_encoder encoder;
> +       struct drm_encoder *encoder;
>
>         /**
>          * @pixel_formats_blob_ptr:
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

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

[Laurent Pinchart]

On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
> On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> > For some vendor driver implementations, display hardware can
> > be shared between the encoder used for writeback and the physical
> > display.
> >
> > In addition resources such as clocks and interrupts can
> > also be shared between writeback and the real encoder.
> >
> > To accommodate such vendor drivers and hardware, allow
> > real encoder to be passed for drm_writeback_connector.
> >
> > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c |  8 ++++----
> >  include/drm/drm_writeback.h     | 13 +++++++++++--
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index dccf4504..4dad687 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >         if (IS_ERR(blob))
> >                 return PTR_ERR(blob);
> >
> > -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > -       ret = drm_encoder_init(dev, &wb_connector->encoder,
> > +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > +       ret = drm_encoder_init(dev, wb_connector->encoder,
> >                                &drm_writeback_encoder_funcs,
> >                                DRM_MODE_ENCODER_VIRTUAL, NULL);
> 
> If the encoder is provided by a separate driver, it might use a
> different set of encoder funcs.

More than that, if the encoder is provided externally but doesn't have
custom operations, I don't really see the point of having an external
encoder in the first place.

Has this series been tested with a driver that needs to provide an
encoder, to make sure it fits the purpose ?

> I'd suggest checking whether the wb_connector->encoder is NULL here.
> If it is, allocate one using drmm_kzalloc and init it.
> If it is not NULL, assume that it has been initialized already, so
> skip the drm_encoder_init() and just call the drm_encoder_helper_add()
> 
> >         if (ret)
> > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >                 goto connector_fail;
> >
> >         ret = drm_connector_attach_encoder(connector,
> > -                                               &wb_connector->encoder);
> > +                                               wb_connector->encoder);
> >         if (ret)
> >                 goto attach_fail;
> >
> > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >  attach_fail:
> >         drm_connector_cleanup(connector);
> >  connector_fail:
> > -       drm_encoder_cleanup(&wb_connector->encoder);
> > +       drm_encoder_cleanup(wb_connector->encoder);
> >  fail:
> >         drm_property_blob_put(blob);
> >         return ret;
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index 9697d27..0ba266e 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -25,13 +25,22 @@ struct drm_writeback_connector {
> >         struct drm_connector base;
> >
> >         /**
> > -        * @encoder: Internal encoder used by the connector to fulfill
> > +        * @encoder: handle to drm_encoder used by the connector to fulfill
> >          * the DRM framework requirements. The users of the
> >          * @drm_writeback_connector control the behaviour of the @encoder
> >          * by passing the @enc_funcs parameter to drm_writeback_connector_init()
> >          * function.
> > +        *
> > +        * For some vendor drivers, the hardware resources are shared between
> > +        * writeback encoder and rest of the display pipeline.
> > +        * To accommodate such cases, encoder is a handle to the real encoder
> > +        * hardware.
> > +        *
> > +        * For current existing writeback users, this shall continue to be the
> > +        * embedded encoder for the writeback connector.
> > +        *
> >          */
> > -       struct drm_encoder encoder;
> > +       struct drm_encoder *encoder;
> >
> >         /**
> >          * @pixel_formats_blob_ptr:

-- 
Regards,

Laurent Pinchart

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

[Abhinav Kumar]

Hi Dmitry and Laurent

On 3/11/2022 12:05 AM, Laurent Pinchart wrote:
> On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
>> On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>
>>> For some vendor driver implementations, display hardware can
>>> be shared between the encoder used for writeback and the physical
>>> display.
>>>
>>> In addition resources such as clocks and interrupts can
>>> also be shared between writeback and the real encoder.
>>>
>>> To accommodate such vendor drivers and hardware, allow
>>> real encoder to be passed for drm_writeback_connector.
>>>
>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/drm_writeback.c |  8 ++++----
>>>   include/drm/drm_writeback.h     | 13 +++++++++++--
>>>   2 files changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>> index dccf4504..4dad687 100644
>>> --- a/drivers/gpu/drm/drm_writeback.c
>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>          if (IS_ERR(blob))
>>>                  return PTR_ERR(blob);
>>>
>>> -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>>> -       ret = drm_encoder_init(dev, &wb_connector->encoder,
>>> +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>>> +       ret = drm_encoder_init(dev, wb_connector->encoder,
>>>                                 &drm_writeback_encoder_funcs,
>>>                                 DRM_MODE_ENCODER_VIRTUAL, NULL);
>>
>> If the encoder is provided by a separate driver, it might use a
>> different set of encoder funcs.
> 
> More than that, if the encoder is provided externally but doesn't have
> custom operations, I don't really see the point of having an external
> encoder in the first place.
> 
> Has this series been tested with a driver that needs to provide an
> encoder, to make sure it fits the purpose ?
> 

Yes, I have tested this with the MSM driver which provides an encoder
and yes it absolutely fits the purpose.


>> I'd suggest checking whether the wb_connector->encoder is NULL here.
>> If it is, allocate one using drmm_kzalloc and init it.
>> If it is not NULL, assume that it has been initialized already, so
>> skip the drm_encoder_init() and just call the drm_encoder_helper_add()

You are both right. We can skip the drm_encoder_init for drivers which 
have already provided an encoder.

The only issue I was facing with that is some of the drivers for example 
the below one, access the "wb_conn->encoder.possible_crtcs" before the 
call to drm_writeback_connector_init().

198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
199 			   struct rcar_du_crtc *rcrtc)
200 {
201 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
202
203 	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
204 	drm_connector_helper_add(&wb_conn->base,
205 				 &rcar_du_wb_conn_helper_funcs);
206
207 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
208 					    &rcar_du_wb_conn_funcs,
209 					    &rcar_du_wb_enc_helper_funcs,
210 					    writeback_formats,
211 					    ARRAY_SIZE(writeback_formats));

If we allocate the encoder within drm_writeback_connector_init(), do you 
suggest I modify the drivers to move the usage of possible_crtcs after 
the drm_writeback_connector_init() call to avoid NULL ptr crash?


>>
>>>          if (ret)
>>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>                  goto connector_fail;
>>>
>>>          ret = drm_connector_attach_encoder(connector,
>>> -                                               &wb_connector->encoder);
>>> +                                               wb_connector->encoder);
>>>          if (ret)
>>>                  goto attach_fail;
>>>
>>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>   attach_fail:
>>>          drm_connector_cleanup(connector);
>>>   connector_fail:
>>> -       drm_encoder_cleanup(&wb_connector->encoder);
>>> +       drm_encoder_cleanup(wb_connector->encoder);
>>>   fail:
>>>          drm_property_blob_put(blob);
>>>          return ret;
>>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>>> index 9697d27..0ba266e 100644
>>> --- a/include/drm/drm_writeback.h
>>> +++ b/include/drm/drm_writeback.h
>>> @@ -25,13 +25,22 @@ struct drm_writeback_connector {
>>>          struct drm_connector base;
>>>
>>>          /**
>>> -        * @encoder: Internal encoder used by the connector to fulfill
>>> +        * @encoder: handle to drm_encoder used by the connector to fulfill
>>>           * the DRM framework requirements. The users of the
>>>           * @drm_writeback_connector control the behaviour of the @encoder
>>>           * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>>>           * function.
>>> +        *
>>> +        * For some vendor drivers, the hardware resources are shared between
>>> +        * writeback encoder and rest of the display pipeline.
>>> +        * To accommodate such cases, encoder is a handle to the real encoder
>>> +        * hardware.
>>> +        *
>>> +        * For current existing writeback users, this shall continue to be the
>>> +        * embedded encoder for the writeback connector.
>>> +        *
>>>           */
>>> -       struct drm_encoder encoder;
>>> +       struct drm_encoder *encoder;
>>>
>>>          /**
>>>           * @pixel_formats_blob_ptr:
> 

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

[Dmitry Baryshkov]

On Fri, 11 Mar 2022 at 20:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Dmitry and Laurent
>
> On 3/11/2022 12:05 AM, Laurent Pinchart wrote:
> > On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
> >> On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>
> >>> For some vendor driver implementations, display hardware can
> >>> be shared between the encoder used for writeback and the physical
> >>> display.
> >>>
> >>> In addition resources such as clocks and interrupts can
> >>> also be shared between writeback and the real encoder.
> >>>
> >>> To accommodate such vendor drivers and hardware, allow
> >>> real encoder to be passed for drm_writeback_connector.
> >>>
> >>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>> ---
> >>>   drivers/gpu/drm/drm_writeback.c |  8 ++++----
> >>>   include/drm/drm_writeback.h     | 13 +++++++++++--
> >>>   2 files changed, 15 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> >>> index dccf4504..4dad687 100644
> >>> --- a/drivers/gpu/drm/drm_writeback.c
> >>> +++ b/drivers/gpu/drm/drm_writeback.c
> >>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >>>          if (IS_ERR(blob))
> >>>                  return PTR_ERR(blob);
> >>>
> >>> -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> >>> -       ret = drm_encoder_init(dev, &wb_connector->encoder,
> >>> +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> >>> +       ret = drm_encoder_init(dev, wb_connector->encoder,
> >>>                                 &drm_writeback_encoder_funcs,
> >>>                                 DRM_MODE_ENCODER_VIRTUAL, NULL);
> >>
> >> If the encoder is provided by a separate driver, it might use a
> >> different set of encoder funcs.
> >
> > More than that, if the encoder is provided externally but doesn't have
> > custom operations, I don't really see the point of having an external
> > encoder in the first place.
> >
> > Has this series been tested with a driver that needs to provide an
> > encoder, to make sure it fits the purpose ?
> >
>
> Yes, I have tested this with the MSM driver which provides an encoder
> and yes it absolutely fits the purpose.
>
>
> >> I'd suggest checking whether the wb_connector->encoder is NULL here.
> >> If it is, allocate one using drmm_kzalloc and init it.
> >> If it is not NULL, assume that it has been initialized already, so
> >> skip the drm_encoder_init() and just call the drm_encoder_helper_add()
>
> You are both right. We can skip the drm_encoder_init for drivers which
> have already provided an encoder.
>
> The only issue I was facing with that is some of the drivers for example
> the below one, access the "wb_conn->encoder.possible_crtcs" before the
> call to drm_writeback_connector_init().

Yes. please do.

>
> 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> 199                        struct rcar_du_crtc *rcrtc)
> 200 {
> 201     struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> 202
> 203     wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> 204     drm_connector_helper_add(&wb_conn->base,
> 205                              &rcar_du_wb_conn_helper_funcs);
> 206
> 207     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> 208                                         &rcar_du_wb_conn_funcs,
> 209                                         &rcar_du_wb_enc_helper_funcs,
> 210                                         writeback_formats,
> 211                                         ARRAY_SIZE(writeback_formats));
>
> If we allocate the encoder within drm_writeback_connector_init(), do you
> suggest I modify the drivers to move the usage of possible_crtcs after
> the drm_writeback_connector_init() call to avoid NULL ptr crash?
>
>
> >>
> >>>          if (ret)
> >>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >>>                  goto connector_fail;
> >>>
> >>>          ret = drm_connector_attach_encoder(connector,
> >>> -                                               &wb_connector->encoder);
> >>> +                                               wb_connector->encoder);
> >>>          if (ret)
> >>>                  goto attach_fail;
> >>>
> >>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >>>   attach_fail:
> >>>          drm_connector_cleanup(connector);
> >>>   connector_fail:
> >>> -       drm_encoder_cleanup(&wb_connector->encoder);
> >>> +       drm_encoder_cleanup(wb_connector->encoder);
> >>>   fail:
> >>>          drm_property_blob_put(blob);
> >>>          return ret;
> >>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> >>> index 9697d27..0ba266e 100644
> >>> --- a/include/drm/drm_writeback.h
> >>> +++ b/include/drm/drm_writeback.h
> >>> @@ -25,13 +25,22 @@ struct drm_writeback_connector {
> >>>          struct drm_connector base;
> >>>
> >>>          /**
> >>> -        * @encoder: Internal encoder used by the connector to fulfill
> >>> +        * @encoder: handle to drm_encoder used by the connector to fulfill
> >>>           * the DRM framework requirements. The users of the
> >>>           * @drm_writeback_connector control the behaviour of the @encoder
> >>>           * by passing the @enc_funcs parameter to drm_writeback_connector_init()
> >>>           * function.
> >>> +        *
> >>> +        * For some vendor drivers, the hardware resources are shared between
> >>> +        * writeback encoder and rest of the display pipeline.
> >>> +        * To accommodate such cases, encoder is a handle to the real encoder
> >>> +        * hardware.
> >>> +        *
> >>> +        * For current existing writeback users, this shall continue to be the
> >>> +        * embedded encoder for the writeback connector.
> >>> +        *
> >>>           */
> >>> -       struct drm_encoder encoder;
> >>> +       struct drm_encoder *encoder;
> >>>
> >>>          /**
> >>>           * @pixel_formats_blob_ptr:
> >



-- 
With best wishes
Dmitry

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

[Daniel Vetter]

On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:
> On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
> > On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >
> > > For some vendor driver implementations, display hardware can
> > > be shared between the encoder used for writeback and the physical
> > > display.
> > >
> > > In addition resources such as clocks and interrupts can
> > > also be shared between writeback and the real encoder.
> > >
> > > To accommodate such vendor drivers and hardware, allow
> > > real encoder to be passed for drm_writeback_connector.
> > >
> > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c |  8 ++++----
> > >  include/drm/drm_writeback.h     | 13 +++++++++++--
> > >  2 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index dccf4504..4dad687 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >         if (IS_ERR(blob))
> > >                 return PTR_ERR(blob);
> > >
> > > -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > > -       ret = drm_encoder_init(dev, &wb_connector->encoder,
> > > +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > > +       ret = drm_encoder_init(dev, wb_connector->encoder,
> > >                                &drm_writeback_encoder_funcs,
> > >                                DRM_MODE_ENCODER_VIRTUAL, NULL);
> > 
> > If the encoder is provided by a separate driver, it might use a
> > different set of encoder funcs.
> 
> More than that, if the encoder is provided externally but doesn't have
> custom operations, I don't really see the point of having an external
> encoder in the first place.
> 
> Has this series been tested with a driver that needs to provide an
> encoder, to make sure it fits the purpose ?

Also, can we not force all drivers to do this setup that don't need it? We
have a ton of kms drivers, forcing unnecessary busiwork on drivers is
really not good.
-Daniel

> 
> > I'd suggest checking whether the wb_connector->encoder is NULL here.
> > If it is, allocate one using drmm_kzalloc and init it.
> > If it is not NULL, assume that it has been initialized already, so
> > skip the drm_encoder_init() and just call the drm_encoder_helper_add()
> > 
> > >         if (ret)
> > > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >                 goto connector_fail;
> > >
> > >         ret = drm_connector_attach_encoder(connector,
> > > -                                               &wb_connector->encoder);
> > > +                                               wb_connector->encoder);
> > >         if (ret)
> > >                 goto attach_fail;
> > >
> > > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >  attach_fail:
> > >         drm_connector_cleanup(connector);
> > >  connector_fail:
> > > -       drm_encoder_cleanup(&wb_connector->encoder);
> > > +       drm_encoder_cleanup(wb_connector->encoder);
> > >  fail:
> > >         drm_property_blob_put(blob);
> > >         return ret;
> > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > > index 9697d27..0ba266e 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -25,13 +25,22 @@ struct drm_writeback_connector {
> > >         struct drm_connector base;
> > >
> > >         /**
> > > -        * @encoder: Internal encoder used by the connector to fulfill
> > > +        * @encoder: handle to drm_encoder used by the connector to fulfill
> > >          * the DRM framework requirements. The users of the
> > >          * @drm_writeback_connector control the behaviour of the @encoder
> > >          * by passing the @enc_funcs parameter to drm_writeback_connector_init()
> > >          * function.
> > > +        *
> > > +        * For some vendor drivers, the hardware resources are shared between
> > > +        * writeback encoder and rest of the display pipeline.
> > > +        * To accommodate such cases, encoder is a handle to the real encoder
> > > +        * hardware.
> > > +        *
> > > +        * For current existing writeback users, this shall continue to be the
> > > +        * embedded encoder for the writeback connector.
> > > +        *
> > >          */
> > > -       struct drm_encoder encoder;
> > > +       struct drm_encoder *encoder;
> > >
> > >         /**
> > >          * @pixel_formats_blob_ptr:
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Abhinav Kumar]

Hi Daniel

On 3/17/2022 3:01 AM, Daniel Vetter wrote:
> On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:
>> On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
>>> On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> For some vendor driver implementations, display hardware can
>>>> be shared between the encoder used for writeback and the physical
>>>> display.
>>>>
>>>> In addition resources such as clocks and interrupts can
>>>> also be shared between writeback and the real encoder.
>>>>
>>>> To accommodate such vendor drivers and hardware, allow
>>>> real encoder to be passed for drm_writeback_connector.
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_writeback.c |  8 ++++----
>>>>   include/drm/drm_writeback.h     | 13 +++++++++++--
>>>>   2 files changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>> index dccf4504..4dad687 100644
>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>          if (IS_ERR(blob))
>>>>                  return PTR_ERR(blob);
>>>>
>>>> -       drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>>>> -       ret = drm_encoder_init(dev, &wb_connector->encoder,
>>>> +       drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>>>> +       ret = drm_encoder_init(dev, wb_connector->encoder,
>>>>                                 &drm_writeback_encoder_funcs,
>>>>                                 DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>
>>> If the encoder is provided by a separate driver, it might use a
>>> different set of encoder funcs.
>>
>> More than that, if the encoder is provided externally but doesn't have
>> custom operations, I don't really see the point of having an external
>> encoder in the first place.
>>
>> Has this series been tested with a driver that needs to provide an
>> encoder, to make sure it fits the purpose ?
> 
> Also, can we not force all drivers to do this setup that don't need it? We
> have a ton of kms drivers, forcing unnecessary busiwork on drivers is
> really not good.
> -Daniel
No, there will not be any requirement forced for existing drivers.
A new API will be exposed for new clients which setup the encoder.
> 
>>
>>> I'd suggest checking whether the wb_connector->encoder is NULL here.
>>> If it is, allocate one using drmm_kzalloc and init it.
>>> If it is not NULL, assume that it has been initialized already, so
>>> skip the drm_encoder_init() and just call the drm_encoder_helper_add()
>>>
>>>>          if (ret)
>>>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>                  goto connector_fail;
>>>>
>>>>          ret = drm_connector_attach_encoder(connector,
>>>> -                                               &wb_connector->encoder);
>>>> +                                               wb_connector->encoder);
>>>>          if (ret)
>>>>                  goto attach_fail;
>>>>
>>>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>   attach_fail:
>>>>          drm_connector_cleanup(connector);
>>>>   connector_fail:
>>>> -       drm_encoder_cleanup(&wb_connector->encoder);
>>>> +       drm_encoder_cleanup(wb_connector->encoder);
>>>>   fail:
>>>>          drm_property_blob_put(blob);
>>>>          return ret;
>>>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>>>> index 9697d27..0ba266e 100644
>>>> --- a/include/drm/drm_writeback.h
>>>> +++ b/include/drm/drm_writeback.h
>>>> @@ -25,13 +25,22 @@ struct drm_writeback_connector {
>>>>          struct drm_connector base;
>>>>
>>>>          /**
>>>> -        * @encoder: Internal encoder used by the connector to fulfill
>>>> +        * @encoder: handle to drm_encoder used by the connector to fulfill
>>>>           * the DRM framework requirements. The users of the
>>>>           * @drm_writeback_connector control the behaviour of the @encoder
>>>>           * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>>>>           * function.
>>>> +        *
>>>> +        * For some vendor drivers, the hardware resources are shared between
>>>> +        * writeback encoder and rest of the display pipeline.
>>>> +        * To accommodate such cases, encoder is a handle to the real encoder
>>>> +        * hardware.
>>>> +        *
>>>> +        * For current existing writeback users, this shall continue to be the
>>>> +        * embedded encoder for the writeback connector.
>>>> +        *
>>>>           */
>>>> -       struct drm_encoder encoder;
>>>> +       struct drm_encoder *encoder;
>>>>
>>>>          /**
>>>>           * @pixel_formats_blob_ptr:
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
> 

[PATCH 2/6] drm/komeda: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Make changes to komeda driver to start using drm_encoder pointer
for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index e465cc4..9dd5f25 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -155,7 +155,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 	kwb_conn->wb_layer = kcrtc->master->wb_layer;
 
 	wb_conn = &kwb_conn->base;
-	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
+	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
+
+	wb_conn->encoder->possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
 
 	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
 					       kwb_conn->wb_layer->layer_type,
-- 
2.7.4

[PATCH 3/6] drm/vkms: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Make changes to vkms driver to start using drm_encoder pointer
for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/vkms/vkms_writeback.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227..adf6961 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -139,8 +139,10 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
 {
 	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+	struct vkms_output *output = &vkmsdev->output;
 
-	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
+	wb->encoder = &output->encoder;
+	output.wb_connector.encoder->possible_crtcs = 1;
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 
 	return drm_writeback_connector_init(&vkmsdev->drm, wb,
-- 
2.7.4

[PATCH 4/6] drm/vc4: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Make changes to vc4 driver to start using drm_encoder pointer
for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/vc4/vc4_txp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 9809ca3..7b76968 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
 
 	struct platform_device *pdev;
 
+	struct drm_encoder drm_enc;
+
 	struct drm_writeback_connector connector;
 
 	void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
 
 static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
 {
-	return container_of(encoder, struct vc4_txp, connector.encoder);
+	return container_of(encoder, struct vc4_txp, drm_enc);
 }
 
 static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	struct vc4_txp *txp;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
+	struct drm_writeback_connector *wb_conn;
 	int ret, irq;
 
 	irq = platform_get_irq(pdev, 0);
@@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	txp->regset.regs = txp_regs;
 	txp->regset.nregs = ARRAY_SIZE(txp_regs);
 
-	drm_connector_helper_add(&txp->connector.base,
+	wb_conn = &txp->connector;
+	wb_conn->encoder = &txp->drm_enc;
+
+	drm_connector_helper_add(&wb_conn->base,
 				 &vc4_txp_connector_helper_funcs);
-	ret = drm_writeback_connector_init(drm, &txp->connector,
+	ret = drm_writeback_connector_init(drm, wb_conn,
 					   &vc4_txp_connector_funcs,
 					   &vc4_txp_encoder_helper_funcs,
 					   drm_fmts, ARRAY_SIZE(drm_fmts));
@@ -506,7 +512,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	encoder = &txp->connector.encoder;
+	encoder = txp->connector.encoder;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
-- 
2.7.4

[PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Make changes to rcar_du driver to start using drm_encoder pointer
for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d125..03930ad 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
 {
 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
 
-	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
+	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
+	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
 	drm_connector_helper_add(&wb_conn->base,
 				 &rcar_du_wb_conn_helper_funcs);
 
-- 
2.7.4

Re: [PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav

Thank you for the patch.

On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
> Make changes to rcar_du driver to start using drm_encoder pointer
> for drm_writeback_connector.
> 
> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> index c79d125..03930ad 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>  {
>  	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>  
> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> +	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);

Where is this freed ?

> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>  	drm_connector_helper_add(&wb_conn->base,
>  				 &rcar_du_wb_conn_helper_funcs);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
> Hi Abhinav
> 
> Thank you for the patch.
> 
> On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
>> Make changes to rcar_du driver to start using drm_encoder pointer
>> for drm_writeback_connector.
>>
>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> index c79d125..03930ad 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>   {
>>   	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>>   
>> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> +	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
> 
> Where is this freed ?

You are right, this isnt. Looking more into this, it seems like moving 
the allocation of encoder to drm_writeback.c for cases which dont pass a 
real encoder is much better so that I will not have to add alloc() / 
free() code for all the vendor driver changes which is what I originally 
thought in my RFC but changed my mind because of below.

> 
>> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);

Do you think we can just move usage of wb_conn->encoder->possible_crtcs 
just right after drm_writeback_connector_init() so that it wont crash?

198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
199 			   struct rcar_du_crtc *rcrtc)
200 {
201 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
202
203 	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
204 	drm_connector_helper_add(&wb_conn->base,
205 				 &rcar_du_wb_conn_helper_funcs);
206
207 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
208 					    &rcar_du_wb_conn_funcs,
209 					    &rcar_du_wb_enc_helper_funcs,
210 					    writeback_formats,
211 					    ARRAY_SIZE(writeback_formats));
212 }

>>   	drm_connector_helper_add(&wb_conn->base,
>>   				 &rcar_du_wb_conn_helper_funcs);
>>   
> 

Re: [PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav

On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
> On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
> > On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
> >> Make changes to rcar_du driver to start using drm_encoder pointer
> >> for drm_writeback_connector.
> >>
> >> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> index c79d125..03930ad 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> >>   {
> >>   	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> >>   
> >> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> >> +	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
> > 
> > Where is this freed ?
> 
> You are right, this isnt. Looking more into this, it seems like moving 
> the allocation of encoder to drm_writeback.c for cases which dont pass a 
> real encoder is much better so that I will not have to add alloc() / 
> free() code for all the vendor driver changes which is what I originally 
> thought in my RFC but changed my mind because of below.

Yes, I think that would be better indeed. You could even skip the
dynamic allocation, you could have

	struct drm_encoder *encoder;
	struct drm_encoder internal_encoder;

in drm_writeback_connector, and set

	wb->encoder = &wb->internal_encoder;

when the user doesn't pass an encoder.

> >> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> 
> Do you think we can just move usage of wb_conn->encoder->possible_crtcs 
> just right after drm_writeback_connector_init() so that it wont crash?

How about adding a possible_crtcs argument to
drm_writeback_connector_init() (to cover existing use cases), and adding
a new drm_writeback_connector_init_with_encoder() that would get an
encoder pointer (and expect possible_crtcs, as well as all the other
appropriate encoder fields, having been initialized) ?

> 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> 199 			   struct rcar_du_crtc *rcrtc)
> 200 {
> 201 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> 202
> 203 	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> 204 	drm_connector_helper_add(&wb_conn->base,
> 205 				 &rcar_du_wb_conn_helper_funcs);
> 206
> 207 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> 208 					    &rcar_du_wb_conn_funcs,
> 209 					    &rcar_du_wb_enc_helper_funcs,
> 210 					    writeback_formats,
> 211 					    ARRAY_SIZE(writeback_formats));
> 212 }
> 
> >>   	drm_connector_helper_add(&wb_conn->base,
> >>   				 &rcar_du_wb_conn_helper_funcs);
> >>   

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] drm/rcar_du: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

Thank you for your inputs.

I liked all the suggestions, hence I have incorporated those and pushed 
a v2.

Thanks

Abhinav

On 3/13/2022 7:50 AM, Laurent Pinchart wrote:
> Hi Abhinav
> 
> On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
>> On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
>>> On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
>>>> Make changes to rcar_du driver to start using drm_encoder pointer
>>>> for drm_writeback_connector.
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> index c79d125..03930ad 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>>    {
>>>>    	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>>>>    
>>>> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>> +	wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
>>>
>>> Where is this freed ?
>>
>> You are right, this isnt. Looking more into this, it seems like moving
>> the allocation of encoder to drm_writeback.c for cases which dont pass a
>> real encoder is much better so that I will not have to add alloc() /
>> free() code for all the vendor driver changes which is what I originally
>> thought in my RFC but changed my mind because of below.
> 
> Yes, I think that would be better indeed. You could even skip the
> dynamic allocation, you could have
> 
> 	struct drm_encoder *encoder;
> 	struct drm_encoder internal_encoder;
> 
> in drm_writeback_connector, and set
> 
> 	wb->encoder = &wb->internal_encoder;
> 
> when the user doesn't pass an encoder.
> 
>>>> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>
>> Do you think we can just move usage of wb_conn->encoder->possible_crtcs
>> just right after drm_writeback_connector_init() so that it wont crash?
> 
> How about adding a possible_crtcs argument to
> drm_writeback_connector_init() (to cover existing use cases), and adding
> a new drm_writeback_connector_init_with_encoder() that would get an
> encoder pointer (and expect possible_crtcs, as well as all the other
> appropriate encoder fields, having been initialized) ?
> 
>> 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>> 199 			   struct rcar_du_crtc *rcrtc)
>> 200 {
>> 201 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>> 202
>> 203 	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> 204 	drm_connector_helper_add(&wb_conn->base,
>> 205 				 &rcar_du_wb_conn_helper_funcs);
>> 206
>> 207 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>> 208 					    &rcar_du_wb_conn_funcs,
>> 209 					    &rcar_du_wb_enc_helper_funcs,
>> 210 					    writeback_formats,
>> 211 					    ARRAY_SIZE(writeback_formats));
>> 212 }
>>
>>>>    	drm_connector_helper_add(&wb_conn->base,
>>>>    				 &rcar_du_wb_conn_helper_funcs);
>>>>    
> 

[PATCH 6/6] drm/malidp: use drm_encoder pointer for drm_writeback_connector

[Abhinav Kumar]

Make changes to malidp driver to start using drm_encoder pointer
for drm_writeback_connector.

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/arm/malidp_mw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index f5847a7..0bd5b78 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -212,7 +212,8 @@ int malidp_mw_connector_init(struct drm_device *drm)
 	if (!malidp->dev->hw->enable_memwrite)
 		return 0;
 
-	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+	malidp->mw_connector.encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
+	malidp->mw_connector.encoder->possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
 	drm_connector_helper_add(&malidp->mw_connector.base,
 				 &malidp_mw_connector_helper_funcs);
 
-- 
2.7.4