Re: [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector
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 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
Signed-off-by: Abhinav Kumar
---
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(_connector->encoder, enc_helper_funcs);
- ret = drm_encoder_init(dev, _connector->encoder,
+ drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+ ret = drm_encoder_init(dev, wb_connector->encoder,
_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,
- _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(_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
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
> > 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
> > > Signed-off-by: Abhinav Kumar
> > > ---
> > > 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(_connector->encoder, enc_helper_funcs);
> > > - ret = drm_encoder_init(dev, _connector->encoder,
> > > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > > + ret = drm_encoder_init(dev, wb_connector->encoder,
> > >_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,
> > > - _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(_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
On Fri, 11 Mar 2022 at 20:09, Abhinav Kumar 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
> >> 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
> >>> Signed-off-by: Abhinav Kumar
> >>> ---
> >>> 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(_connector->encoder, enc_helper_funcs);
> >>> - ret = drm_encoder_init(dev, _connector->encoder,
> >>> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> >>> + ret = drm_encoder_init(dev, wb_connector->encoder,
> >>> _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,
> 199struct rcar_du_crtc *rcrtc)
> 200 {
> 201 struct drm_writeback_connector *wb_conn = >writeback;
> 202
> 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(>crtc);
> 204 drm_connector_helper_add(_conn->base,
> 205 _du_wb_conn_helper_funcs);
> 206
> 207 return drm_writeback_connector_init(>ddev, wb_conn,
> 208 _du_wb_conn_funcs,
> 209 _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,
> >>> - _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(_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;
> >>>
> >>>
Re: [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector
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 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
Signed-off-by: Abhinav Kumar
---
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(_connector->encoder, enc_helper_funcs);
- ret = drm_encoder_init(dev, _connector->encoder,
+ drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+ ret = drm_encoder_init(dev, wb_connector->encoder,
_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,
199struct rcar_du_crtc *rcrtc)
200 {
201 struct drm_writeback_connector *wb_conn = >writeback;
202
203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(>crtc);
204 drm_connector_helper_add(_conn->base,
205 _du_wb_conn_helper_funcs);
206
207 return drm_writeback_connector_init(>ddev, wb_conn,
208 _du_wb_conn_funcs,
209 _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,
- _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(_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
Re: [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
> On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar wrote:
> >
> > For some vendor driver implementations, display hardware can
> > be shared between the encoder used for writeback and the physical
> > display.
> >
> > In addition resources such as clocks and interrupts can
> > also be shared between writeback and the real encoder.
> >
> > To accommodate such vendor drivers and hardware, allow
> > real encoder to be passed for drm_writeback_connector.
> >
> > Co-developed-by: Kandpal Suraj
> > Signed-off-by: Abhinav Kumar
> > ---
> > 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(_connector->encoder, enc_helper_funcs);
> > - ret = drm_encoder_init(dev, _connector->encoder,
> > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > + ret = drm_encoder_init(dev, wb_connector->encoder,
> >_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,
> > - _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(_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
On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar wrote:
>
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
>
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
>
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
>
> Co-developed-by: Kandpal Suraj
> Signed-off-by: Abhinav Kumar
> ---
> 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(_connector->encoder, enc_helper_funcs);
> - ret = drm_encoder_init(dev, _connector->encoder,
> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> + ret = drm_encoder_init(dev, wb_connector->encoder,
>_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,
> - _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(_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
