On Fri, 11 Mar 2022 at 20:09, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxx>
> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >>> ---
> >>> 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
