[Freedreno] [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 at 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 at 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 at intel.com>
> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk at 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