Re: [PATCH 1/6] drm: add writeback pointers to drm_connector

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Kandpal Suraj <suraj.kandpal@intel.com>
Cc: carsten.haitzler@arm.com, jani.nikula@intel.com,
	intel-gfx@lists.freedesktop.org, quic_abhinavk@quicinc.com,
	dri-devel@lists.freedesktop.org,
	Laurent.pinchart@ideasonboard.com, arun.r.murthy@intel.com
Subject: Re: [PATCH 1/6] drm: add writeback pointers to drm_connector
Date: Wed, 2 Feb 2022 13:28:25 +0300	[thread overview]
Message-ID: <CAA8EJppf5XL5+NSD+oksW9Zo1DKboPT5KBZwLmyF44q61wkxQA@mail.gmail.com> (raw)
In-Reply-To: <20220202085429.22261-2-suraj.kandpal@intel.com>

On Wed, 2 Feb 2022 at 11:46, Kandpal Suraj <suraj.kandpal@intel.com> wrote:
>
> Changing drm_connector and drm_encoder feilds to pointers in
> drm_writeback_connector as the elements of struct
> drm_writeback_connector are:
> struct drm_writeback_connector {
>         struct drm_connector base;
>         struct drm_encoder encoder;
> Similarly the elements of intel_encoder and intel_connector
> are:
> struct intel_encoder {
>         struct drm_encoder base;
>
> struct intel_connector {
>         struct drm_connector base;
>
> The function drm_writeback_connector_init() will initialize the
> drm_connector and drm_encoder and attach them as well.
> Since the drm_connector/encoder are both struct in
> drm_writeback_connector and intel_connector/encoder, we need
> one of them to be a pointer so we can reference them or else we
> will be pointing to 2 seprate instances.

Could you please clarify, why do you want to use intel_connector for
the writeback connector?
I can see a usecase for sharing an encoder between the display and
writback pipelines (and if I'm not mistaken, this is the case for
Abhinav's case).
However, sharing the hardware-backed connector and writeback connector
sounds like a sign of a loose abstraction for me.

Please correct me if I'm wrong and Intel driver would really benefit
from reusing intel_connector as a base for drm_writeback_connector.

> Usually the struct defined in drm framework pointing to any
> struct will be pointer and allocating them and initialization
> will be done with the users.
> Like struct drm_connector and drm_encoder are part of drm
> framework and the users of these such as i915 have included them
> in their struct intel_connector and intel_encoder. Likewise
> struct drm_writeback_connector is a special connector and hence
> is not a user of drm_connector and hence this should be pointers.
>
> Adding drm_writeback_connector to drm_connector so that
> writeback_connector can be fetched from drm_connector as the previous
> container_of method won't work due to change in the feilds of
> drm_connector and drm_encoder in drm_writeback_connector.
>
> Note:The corresponding ripple effect due to the above changes namely in
> two drivers as I can see it komeda and vkms have been dealt with in the
> upcoming patches of this series.
>
> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 19 ++++++++++---------
>  include/drm/drm_connector.h     |  3 +++
>  include/drm/drm_writeback.h     |  6 +++---
>  3 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dccf4504f1bb..47238db42363 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
>         struct drm_writeback_connector *wb_connector =
>                 fence_to_wb_connector(fence);
>
> -       return wb_connector->base.dev->driver->name;
> +       return wb_connector->base->dev->driver->name;
>  }
>
>  static const char *
> @@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>                                  const u32 *formats, int n_formats)
>  {
>         struct drm_property_blob *blob;
> -       struct drm_connector *connector = &wb_connector->base;
> +       struct drm_connector *connector = wb_connector->base;
>         struct drm_mode_config *config = &dev->mode_config;
>         int ret = create_writeback_properties(dev);
>
> @@ -189,14 +189,15 @@ 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)
>                 goto fail;
>
>         connector->interlace_allowed = 0;
> +       connector->wb_connector = wb_connector;
>
>         ret = drm_connector_init(dev, connector, con_funcs,
>                                  DRM_MODE_CONNECTOR_WRITEBACK);
> @@ -204,7 +205,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 +234,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;
> @@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job)
>  {
>         struct drm_writeback_connector *connector = job->connector;
>         const struct drm_connector_helper_funcs *funcs =
> -               connector->base.helper_private;
> +               connector->base->helper_private;
>         int ret;
>
>         if (funcs->prepare_writeback_job) {
> @@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job)
>  {
>         struct drm_writeback_connector *connector = job->connector;
>         const struct drm_connector_helper_funcs *funcs =
> -               connector->base.helper_private;
> +               connector->base->helper_private;
>
>         if (job->prepared && funcs->cleanup_writeback_job)
>                 funcs->cleanup_writeback_job(connector, job);
> @@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
>  {
>         struct dma_fence *fence;
>
> -       if (WARN_ON(wb_connector->base.connector_type !=
> +       if (WARN_ON(wb_connector->base->connector_type !=
>                     DRM_MODE_CONNECTOR_WRITEBACK))
>                 return NULL;
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 64cf5f88c05b..fa06faeb7844 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -44,6 +44,7 @@ struct drm_printer;
>  struct drm_privacy_screen;
>  struct edid;
>  struct i2c_adapter;
> +struct drm_writeback_connector;
>
>  enum drm_connector_force {
>         DRM_FORCE_UNSPECIFIED,
> @@ -1539,6 +1540,8 @@ struct drm_connector {
>          */
>         struct drm_encoder *encoder;
>
> +       struct drm_writeback_connector *wb_connector;
> +
>  #define MAX_ELD_BYTES  128
>         /** @eld: EDID-like data, if present */
>         uint8_t eld[MAX_ELD_BYTES];
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 9697d2714d2a..078c9907219c 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -22,7 +22,7 @@ struct drm_writeback_connector {
>         /**
>          * @base: base drm_connector object
>          */
> -       struct drm_connector base;
> +       struct drm_connector *base;
>
>         /**
>          * @encoder: Internal encoder used by the connector to fulfill
> @@ -31,7 +31,7 @@ struct drm_writeback_connector {
>          * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>          * function.
>          */
> -       struct drm_encoder encoder;
> +       struct drm_encoder *encoder;
>
>         /**
>          * @pixel_formats_blob_ptr:
> @@ -143,7 +143,7 @@ struct drm_writeback_job {
>  static inline struct drm_writeback_connector *
>  drm_connector_to_writeback(struct drm_connector *connector)
>  {
> -       return container_of(connector, struct drm_writeback_connector, base);
> +       return connector->wb_connector;
>  }
>
>  int drm_writeback_connector_init(struct drm_device *dev,
> --
> 2.17.1
>

-- 
With best wishes
Dmitry