[Intel-gfx] [PATCH 0/6] drm writeback connector changes

[Intel-gfx] [PATCH 0/6] drm writeback connector changes

[Kandpal Suraj]

This patch series contains drm_writeback_connector structure change i.e 
change of drm_connector and drm_encoder fields to a pointer the reason
for which is explained in patch(1/6) drm: add writeback pointers to
drm_connector and the accompanying changes to the different drivers that
were effected by it.
I had perviously floated a patch series but missed some of the drivers
that were effected by the change hence refloating the patch series 

Kandpal Suraj (6):
  drm: add writeback pointers to drm_connector
  drm/arm/komeda : change driver to use drm_writeback_connector.base
    pointer
  drm/vkms: change vkms driver to use drm_writeback_connector.base
    pointer
  drm/vc4: vc4 driver changes to accommodate changes done in
    drm_writeback_connector structure
  drm/rcar_du: changes to rcar-du driver resulting from
    drm_writeback_connector structure changes
  drm/arm: changes to malidp driver resulting from
    drm_writeback_connector structure changes

 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  2 +-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 ++-
 .../arm/display/komeda/komeda_wb_connector.c  | 11 +++++-----
 drivers/gpu/drm/arm/malidp_crtc.c             |  2 +-
 drivers/gpu/drm/arm/malidp_drv.h              |  2 ++
 drivers/gpu/drm/arm/malidp_mw.c               | 12 +++++++----
 drivers/gpu/drm/drm_writeback.c               | 19 +++++++++---------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |  2 ++
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c   |  8 +++++---
 drivers/gpu/drm/vc4/vc4_txp.c                 | 20 +++++++++++++------
 drivers/gpu/drm/vkms/vkms_writeback.c         |  9 ++++++---
 include/drm/drm_connector.h                   |  3 +++
 include/drm/drm_writeback.h                   |  6 +++---
 13 files changed, 63 insertions(+), 36 deletions(-)

-- 
2.17.1

[Intel-gfx] [PATCH 1/6] drm: add writeback pointers to drm_connector

[Kandpal Suraj]

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.

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

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

[Dmitry Baryshkov]

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

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

[Kandpal, Suraj]

Hi Dmitry,
Thanks for your comments
 
> 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.
> 

The thing is drm_writeback_connector contains drm_connector and drm_encoder fields
which get initialized when we call drm_writeback_connector_init adding the connector
to the list of connectors for the drm_device.
Now if I decide not to wrap drm_writeback_connector with intel_connector the issue
is I'll have to add a lot of checks all over the driver to see if the drm_connector is actually
present inside intel_connector or not. I don’t see the point of not having drm_writeback_
connector as even though it’s a faux connector we still treat is as a connector and it would
be better for us to use intel_connector for writeback_connector.
Also the current drm_writeback_connector structure kind of restricts drivers consequently
dictating how they implement their writeback functionality, as a midlayer I don't feel that
would be the right approach as drivers should have the freedom to develop their own flow
to implement the functionality and use the midlayer as a helper to get that done. 

Best Regards,
Suraj Kandpal

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

[kernel test robot]

Hi Kandpal,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20220202/202202021914.BKeUA6Fh-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/57ad56d769873f62f87fe432243030ceb1678f87
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832
        git checkout 57ad56d769873f62f87fe432243030ceb1678f87
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/gpu/drm/arm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

Note: the linux-review/Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832 HEAD 75bbd0a8b1fb58f702279bfbc2fe2d74db8f7028 builds fine.
      It only hurts bisectability.

All errors (new ones prefixed by >>):

   drivers/gpu/drm/arm/malidp_crtc.c: In function 'malidp_crtc_atomic_check':
>> drivers/gpu/drm/arm/malidp_crtc.c:427:47: error: passing argument 1 of 'drm_connector_index' from incompatible pointer type [-Werror=incompatible-pointer-types]
     427 |                     (1 << drm_connector_index(&malidp->mw_connector.base)))
         |                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                               |
         |                                               struct drm_connector **
   In file included from include/drm/drm_modes.h:33,
                    from include/drm/drm_crtc.h:40,
                    from include/drm/drm_atomic.h:31,
                    from drivers/gpu/drm/arm/malidp_crtc.c:14:
   include/drm/drm_connector.h:1679:76: note: expected 'const struct drm_connector *' but argument is of type 'struct drm_connector **'
    1679 | static inline unsigned int drm_connector_index(const struct drm_connector *connector)
         |                                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
   cc1: some warnings being treated as errors
--
   drivers/gpu/drm/arm/malidp_mw.c: In function 'malidp_mw_connector_init':
>> drivers/gpu/drm/arm/malidp_mw.c:215:37: error: 'malidp->mw_connector.encoder' is a pointer; did you mean to use '->'?
     215 |         malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
         |                                     ^
         |                                     ->
>> drivers/gpu/drm/arm/malidp_mw.c:216:34: error: passing argument 1 of 'drm_connector_helper_add' from incompatible pointer type [-Werror=incompatible-pointer-types]
     216 |         drm_connector_helper_add(&malidp->mw_connector.base,
         |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                  |
         |                                  struct drm_connector **
   In file included from include/drm/drm_atomic_helper.h:32,
                    from drivers/gpu/drm/arm/malidp_mw.c:10:
   include/drm/drm_modeset_helper_vtables.h:1153:67: note: expected 'struct drm_connector *' but argument is of type 'struct drm_connector **'
    1153 | static inline void drm_connector_helper_add(struct drm_connector *connector,
         |                                             ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
   drivers/gpu/drm/arm/malidp_mw.c: In function 'malidp_mw_atomic_commit':
>> drivers/gpu/drm/arm/malidp_mw.c:239:63: error: 'mw_conn->base' is a pointer; did you mean to use '->'?
     239 |         struct drm_connector_state *conn_state = mw_conn->base.state;
         |                                                               ^
         |                                                               ->
   cc1: some warnings being treated as errors


vim +/drm_connector_index +427 drivers/gpu/drm/arm/malidp_crtc.c

28ce675b74742c Mihail Atanassov 2017-02-13  338  
ad49f8602fe889 Liviu Dudau      2016-03-07  339  static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
29b77ad7b9ca8c Maxime Ripard    2020-10-28  340  				    struct drm_atomic_state *state)
ad49f8602fe889 Liviu Dudau      2016-03-07  341  {
29b77ad7b9ca8c Maxime Ripard    2020-10-28  342  	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
29b77ad7b9ca8c Maxime Ripard    2020-10-28  343  									  crtc);
ad49f8602fe889 Liviu Dudau      2016-03-07  344  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
ad49f8602fe889 Liviu Dudau      2016-03-07  345  	struct malidp_hw_device *hwdev = malidp->dev;
ad49f8602fe889 Liviu Dudau      2016-03-07  346  	struct drm_plane *plane;
ad49f8602fe889 Liviu Dudau      2016-03-07  347  	const struct drm_plane_state *pstate;
ad49f8602fe889 Liviu Dudau      2016-03-07  348  	u32 rot_mem_free, rot_mem_usable;
ad49f8602fe889 Liviu Dudau      2016-03-07  349  	int rotated_planes = 0;
6954f24588ebdd Mihail Atanassov 2017-02-13  350  	int ret;
ad49f8602fe889 Liviu Dudau      2016-03-07  351  
ad49f8602fe889 Liviu Dudau      2016-03-07  352  	/*
ad49f8602fe889 Liviu Dudau      2016-03-07  353  	 * check if there is enough rotation memory available for planes
66da13a519b331 Liviu Dudau      2018-10-02  354  	 * that need 90° and 270° rotion or planes that are compressed.
66da13a519b331 Liviu Dudau      2018-10-02  355  	 * Each plane has set its required memory size in the ->plane_check()
66da13a519b331 Liviu Dudau      2018-10-02  356  	 * callback, here we only make sure that the sums are less that the
66da13a519b331 Liviu Dudau      2018-10-02  357  	 * total usable memory.
ad49f8602fe889 Liviu Dudau      2016-03-07  358  	 *
ad49f8602fe889 Liviu Dudau      2016-03-07  359  	 * The rotation memory allocation algorithm (for each plane):
66da13a519b331 Liviu Dudau      2018-10-02  360  	 *  a. If no more rotated or compressed planes exist, all remaining
66da13a519b331 Liviu Dudau      2018-10-02  361  	 *     rotate memory in the bank is available for use by the plane.
66da13a519b331 Liviu Dudau      2018-10-02  362  	 *  b. If other rotated or compressed planes exist, and plane's
66da13a519b331 Liviu Dudau      2018-10-02  363  	 *     layer ID is DE_VIDEO1, it can use all the memory from first bank
66da13a519b331 Liviu Dudau      2018-10-02  364  	 *     if secondary rotation memory bank is available, otherwise it can
ad49f8602fe889 Liviu Dudau      2016-03-07  365  	 *     use up to half the bank's memory.
66da13a519b331 Liviu Dudau      2018-10-02  366  	 *  c. If other rotated or compressed planes exist, and plane's layer ID
66da13a519b331 Liviu Dudau      2018-10-02  367  	 *     is not DE_VIDEO1, it can use half of the available memory.
ad49f8602fe889 Liviu Dudau      2016-03-07  368  	 *
ad49f8602fe889 Liviu Dudau      2016-03-07  369  	 * Note: this algorithm assumes that the order in which the planes are
ad49f8602fe889 Liviu Dudau      2016-03-07  370  	 * checked always has DE_VIDEO1 plane first in the list if it is
ad49f8602fe889 Liviu Dudau      2016-03-07  371  	 * rotated. Because that is how we create the planes in the first
ad49f8602fe889 Liviu Dudau      2016-03-07  372  	 * place, under current DRM version things work, but if ever the order
ad49f8602fe889 Liviu Dudau      2016-03-07  373  	 * in which drm_atomic_crtc_state_for_each_plane() iterates over planes
ad49f8602fe889 Liviu Dudau      2016-03-07  374  	 * changes, we need to pre-sort the planes before validation.
ad49f8602fe889 Liviu Dudau      2016-03-07  375  	 */
ad49f8602fe889 Liviu Dudau      2016-03-07  376  
ad49f8602fe889 Liviu Dudau      2016-03-07  377  	/* first count the number of rotated planes */
29b77ad7b9ca8c Maxime Ripard    2020-10-28  378  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
66da13a519b331 Liviu Dudau      2018-10-02  379  		struct drm_framebuffer *fb = pstate->fb;
66da13a519b331 Liviu Dudau      2018-10-02  380  
66da13a519b331 Liviu Dudau      2018-10-02  381  		if ((pstate->rotation & MALIDP_ROTATED_MASK) || fb->modifier)
ad49f8602fe889 Liviu Dudau      2016-03-07  382  			rotated_planes++;
ad49f8602fe889 Liviu Dudau      2016-03-07  383  	}
ad49f8602fe889 Liviu Dudau      2016-03-07  384  
ad49f8602fe889 Liviu Dudau      2016-03-07  385  	rot_mem_free = hwdev->rotation_memory[0];
ad49f8602fe889 Liviu Dudau      2016-03-07  386  	/*
ad49f8602fe889 Liviu Dudau      2016-03-07  387  	 * if we have more than 1 plane using rotation memory, use the second
ad49f8602fe889 Liviu Dudau      2016-03-07  388  	 * block of rotation memory as well
ad49f8602fe889 Liviu Dudau      2016-03-07  389  	 */
ad49f8602fe889 Liviu Dudau      2016-03-07  390  	if (rotated_planes > 1)
ad49f8602fe889 Liviu Dudau      2016-03-07  391  		rot_mem_free += hwdev->rotation_memory[1];
ad49f8602fe889 Liviu Dudau      2016-03-07  392  
ad49f8602fe889 Liviu Dudau      2016-03-07  393  	/* now validate the rotation memory requirements */
29b77ad7b9ca8c Maxime Ripard    2020-10-28  394  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
ad49f8602fe889 Liviu Dudau      2016-03-07  395  		struct malidp_plane *mp = to_malidp_plane(plane);
ad49f8602fe889 Liviu Dudau      2016-03-07  396  		struct malidp_plane_state *ms = to_malidp_plane_state(pstate);
66da13a519b331 Liviu Dudau      2018-10-02  397  		struct drm_framebuffer *fb = pstate->fb;
ad49f8602fe889 Liviu Dudau      2016-03-07  398  
66da13a519b331 Liviu Dudau      2018-10-02  399  		if ((pstate->rotation & MALIDP_ROTATED_MASK) || fb->modifier) {
ad49f8602fe889 Liviu Dudau      2016-03-07  400  			/* process current plane */
ad49f8602fe889 Liviu Dudau      2016-03-07  401  			rotated_planes--;
ad49f8602fe889 Liviu Dudau      2016-03-07  402  
ad49f8602fe889 Liviu Dudau      2016-03-07  403  			if (!rotated_planes) {
ad49f8602fe889 Liviu Dudau      2016-03-07  404  				/* no more rotated planes, we can use what's left */
ad49f8602fe889 Liviu Dudau      2016-03-07  405  				rot_mem_usable = rot_mem_free;
ad49f8602fe889 Liviu Dudau      2016-03-07  406  			} else {
ad49f8602fe889 Liviu Dudau      2016-03-07  407  				if ((mp->layer->id != DE_VIDEO1) ||
ad49f8602fe889 Liviu Dudau      2016-03-07  408  				    (hwdev->rotation_memory[1] == 0))
ad49f8602fe889 Liviu Dudau      2016-03-07  409  					rot_mem_usable = rot_mem_free / 2;
ad49f8602fe889 Liviu Dudau      2016-03-07  410  				else
ad49f8602fe889 Liviu Dudau      2016-03-07  411  					rot_mem_usable = hwdev->rotation_memory[0];
ad49f8602fe889 Liviu Dudau      2016-03-07  412  			}
ad49f8602fe889 Liviu Dudau      2016-03-07  413  
ad49f8602fe889 Liviu Dudau      2016-03-07  414  			rot_mem_free -= rot_mem_usable;
ad49f8602fe889 Liviu Dudau      2016-03-07  415  
ad49f8602fe889 Liviu Dudau      2016-03-07  416  			if (ms->rotmem_size > rot_mem_usable)
ad49f8602fe889 Liviu Dudau      2016-03-07  417  				return -EINVAL;
ad49f8602fe889 Liviu Dudau      2016-03-07  418  		}
ad49f8602fe889 Liviu Dudau      2016-03-07  419  	}
ad49f8602fe889 Liviu Dudau      2016-03-07  420  
8cbc5caf36ef7a Brian Starkey    2017-11-02  421  	/* If only the writeback routing has changed, we don't need a modeset */
29b77ad7b9ca8c Maxime Ripard    2020-10-28  422  	if (crtc_state->connectors_changed) {
8cbc5caf36ef7a Brian Starkey    2017-11-02  423  		u32 old_mask = crtc->state->connector_mask;
29b77ad7b9ca8c Maxime Ripard    2020-10-28  424  		u32 new_mask = crtc_state->connector_mask;
8cbc5caf36ef7a Brian Starkey    2017-11-02  425  
8cbc5caf36ef7a Brian Starkey    2017-11-02  426  		if ((old_mask ^ new_mask) ==
8cbc5caf36ef7a Brian Starkey    2017-11-02 @427  		    (1 << drm_connector_index(&malidp->mw_connector.base)))
29b77ad7b9ca8c Maxime Ripard    2020-10-28  428  			crtc_state->connectors_changed = false;
8cbc5caf36ef7a Brian Starkey    2017-11-02  429  	}
8cbc5caf36ef7a Brian Starkey    2017-11-02  430  
29b77ad7b9ca8c Maxime Ripard    2020-10-28  431  	ret = malidp_crtc_atomic_check_gamma(crtc, crtc_state);
29b77ad7b9ca8c Maxime Ripard    2020-10-28  432  	ret = ret ? ret : malidp_crtc_atomic_check_ctm(crtc, crtc_state);
29b77ad7b9ca8c Maxime Ripard    2020-10-28  433  	ret = ret ? ret : malidp_crtc_atomic_check_scaling(crtc, crtc_state);
6954f24588ebdd Mihail Atanassov 2017-02-13  434  
6954f24588ebdd Mihail Atanassov 2017-02-13  435  	return ret;
ad49f8602fe889 Liviu Dudau      2016-03-07  436  }
ad49f8602fe889 Liviu Dudau      2016-03-07  437  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

Hi Kandpal,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: i386-randconfig-a013-20220131 (https://download.01.org/0day-ci/archive/20220203/202202030437.kmrDD39E-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6b1e844b69f15bb7dffaf9365cd2b355d2eb7579)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/57ad56d769873f62f87fe432243030ceb1678f87
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832
        git checkout 57ad56d769873f62f87fe432243030ceb1678f87
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

Note: the linux-review/Kandpal-Suraj/drm-writeback-connector-changes/20220202-164832 HEAD 75bbd0a8b1fb58f702279bfbc2fe2d74db8f7028 builds fine.
      It only hurts bisectability.

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/vkms/vkms_writeback.c:117:56: error: member reference type 'struct drm_connector *' is a pointer; did you mean to use '->'?
           struct drm_connector_state *conn_state = wb_conn->base.state;
                                                    ~~~~~~~~~~~~~^
                                                                 ->
>> drivers/gpu/drm/vkms/vkms_writeback.c:143:38: error: member reference type 'struct drm_encoder *' is a pointer; did you mean to use '->'?
           vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
                                               ->
>> drivers/gpu/drm/vkms/vkms_writeback.c:144:27: error: incompatible pointer types passing 'struct drm_connector **' to parameter of type 'struct drm_connector *'; remove & [-Werror,-Wincompatible-pointer-types]
           drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
                                    ^~~~~~~~~
   include/drm/drm_modeset_helper_vtables.h:1153:67: note: passing argument to parameter 'connector' here
   static inline void drm_connector_helper_add(struct drm_connector *connector,
                                                                     ^
   3 errors generated.


vim +117 drivers/gpu/drm/vkms/vkms_writeback.c

dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  108  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  109  static void vkms_wb_atomic_commit(struct drm_connector *conn,
eca22edb37d29f Maxime Ripard    2020-11-18  110  				  struct drm_atomic_state *state)
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  111  {
eca22edb37d29f Maxime Ripard    2020-11-18  112  	struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
eca22edb37d29f Maxime Ripard    2020-11-18  113  											 conn);
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  114  	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  115  	struct vkms_output *output = &vkmsdev->output;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  116  	struct drm_writeback_connector *wb_conn = &output->wb_connector;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30 @117  	struct drm_connector_state *conn_state = wb_conn->base.state;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  118  	struct vkms_crtc_state *crtc_state = output->composer_state;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  119  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  120  	if (!conn_state)
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  121  		return;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  122  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  123  	vkms_set_composer(&vkmsdev->output, true);
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  124  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  125  	spin_lock_irq(&output->composer_lock);
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  126  	crtc_state->active_writeback = conn_state->writeback_job->priv;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  127  	crtc_state->wb_pending = true;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  128  	spin_unlock_irq(&output->composer_lock);
eca22edb37d29f Maxime Ripard    2020-11-18  129  	drm_writeback_queue_job(wb_conn, connector_state);
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  130  }
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  131  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  132  static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  133  	.get_modes = vkms_wb_connector_get_modes,
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  134  	.prepare_writeback_job = vkms_wb_prepare_job,
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  135  	.cleanup_writeback_job = vkms_wb_cleanup_job,
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  136  	.atomic_commit = vkms_wb_atomic_commit,
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  137  };
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  138  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  139  int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  140  {
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  141  	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30  142  
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30 @143  	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
dbd9d80c1b2e03 Rodrigo Siqueira 2020-08-30 @144  	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[Intel-gfx] [PATCH 2/6] drm/arm/komeda : change driver to use drm_writeback_connector.base pointer

[Kandpal Suraj]

Making changes to komeda driver because we had to change
drm_writeback_connector.base into a pointer the reason for which is
expained in the Patch (drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>

Reviewed-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c      |  2 +-
 drivers/gpu/drm/arm/display/komeda/komeda_kms.h       |  3 ++-
 .../gpu/drm/arm/display/komeda/komeda_wb_connector.c  | 11 ++++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..eb37f41c1790 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc,
 	if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
 		komeda_pipeline_update(slave, old->state);
 
-	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
+	conn_st = wb_conn ? wb_conn->base.base->state : NULL;
 	if (conn_st && conn_st->writeback_job)
 		drm_writeback_queue_job(&wb_conn->base, conn_st);
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..8d83883a1d99 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -53,6 +53,7 @@ struct komeda_plane_state {
  * struct komeda_wb_connector
  */
 struct komeda_wb_connector {
+	struct drm_connector conn;
 	/** @base: &drm_writeback_connector */
 	struct drm_writeback_connector base;
 
@@ -136,7 +137,7 @@ struct komeda_kms_dev {
 static inline bool is_writeback_only(struct drm_crtc_state *st)
 {
 	struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn;
-	struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL;
+	struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
 
 	return conn && (st->connector_mask == BIT(drm_connector_index(conn)));
 }
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 e465cc4879c9..2c3dec59fd88 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder,
 		return -EINVAL;
 	}
 
-	wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer;
+	wb_layer = to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
 
 	/*
 	 * No need for a full modested when the only connector changed is the
@@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector *connector,
 static void komeda_wb_connector_destroy(struct drm_connector *connector)
 {
 	drm_connector_cleanup(connector);
-	kfree(to_kconn(to_wb_conn(connector)));
+	kfree(to_kconn(drm_connector_to_writeback(connector)));
 }
 
 static const struct drm_connector_funcs komeda_wb_connector_funcs = {
@@ -155,7 +155,8 @@ 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->base = &kwb_conn->conn;
+	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,
@@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 		return err;
 	}
 
-	drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs);
+	drm_connector_helper_add(wb_conn->base, &komeda_wb_conn_helper_funcs);
 
-	info = &kwb_conn->base.base.display_info;
+	info = &kwb_conn->base.base->display_info;
 	info->bpc = __fls(kcrtc->master->improc->supported_color_depths);
 	info->color_formats = kcrtc->master->improc->supported_color_formats;
 
-- 
2.17.1

[Intel-gfx] [PATCH 3/6] drm/vkms: change vkms driver to use drm_writeback_connector.base pointer

[Kandpal Suraj]

Changing vkms driver to accomadate the change of
drm_writeback_connector.base to a pointer the reason for which is
explained in the Patch(drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/vkms/vkms_writeback.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227f555f..374431471f49 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -114,7 +114,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_writeback_connector *wb_conn = &output->wb_connector;
-	struct drm_connector_state *conn_state = wb_conn->base.state;
+	struct drm_connector_state *conn_state = wb_conn->base->state;
 	struct vkms_crtc_state *crtc_state = output->composer_state;
 
 	if (!conn_state)
@@ -139,9 +139,12 @@ 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;
-	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
+	wb->base = &output->connector;
+	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,
 					    &vkms_wb_connector_funcs,
-- 
2.17.1

[Intel-gfx] [PATCH 4/6] drm/vc4: vc4 driver changes to accommodate changes done in drm_writeback_connector structure

[Kandpal Suraj]

Changing vc4 driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/vc4/vc4_txp.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 9809ca3e2945..9882569d147c 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,10 @@ struct vc4_txp {
 
 	struct platform_device *pdev;
 
+	struct drm_connector drm_conn;
+
+	struct drm_encoder drm_enc;
+
 	struct drm_writeback_connector connector;
 
 	void __iomem *regs;
@@ -159,12 +163,12 @@ 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)
 {
-	return container_of(conn, struct vc4_txp, connector.base);
+	return container_of(conn, struct vc4_txp, drm_conn);
 }
 
 static const struct debugfs_reg32 txp_regs[] = {
@@ -467,6 +471,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);
@@ -491,10 +496,13 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	txp->regset.base = txp->regs;
 	txp->regset.regs = txp_regs;
 	txp->regset.nregs = ARRAY_SIZE(txp_regs);
+	wb_conn = &txp->connector;
+	wb_conn->base = &txp->drm_conn;
+	wb_conn->encoder = &txp->drm_enc;
 
-	drm_connector_helper_add(&txp->connector.base,
+	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 +514,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,
@@ -529,7 +537,7 @@ static void vc4_txp_unbind(struct device *dev, struct device *master,
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_txp *txp = dev_get_drvdata(dev);
 
-	vc4_txp_connector_destroy(&txp->connector.base);
+	vc4_txp_connector_destroy(txp->connector.base);
 
 	vc4->txp = NULL;
 }
-- 
2.17.1

[Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal Suraj]

Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
 	const char *const *sources;
 	unsigned int sources_count;
 
+	struct drm_connector connector;
+	struct drm_encoder encoder;
 	struct drm_writeback_connector writeback;
 };
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d1259e49b..5b1e83380c47 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -200,8 +200,10 @@ 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);
-	drm_connector_helper_add(&wb_conn->base,
+	wb_conn->base = &rcrtc->connector;
+	wb_conn->encoder = &rcrtc->encoder;
+	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
+	drm_connector_helper_add(wb_conn->base,
 				 &rcar_du_wb_conn_helper_funcs);
 
 	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
@@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
 	struct drm_framebuffer *fb;
 	unsigned int i;
 
-	state = rcrtc->writeback.base.state;
+	state = rcrtc->writeback.base->state;
 	if (!state || !state->writeback_job)
 		return;
 
-- 
2.17.1

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Kandpal,

Thank you for the patch.

On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> Changing rcar_du driver to accomadate the change of
> drm_writeback_connector.base and drm_writeback_connector.encoder
> to a pointer the reason for which is explained in the
> Patch(drm: add writeback pointers to drm_connector).
> 
> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 66e8839db708..68f387a04502 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>  	const char *const *sources;
>  	unsigned int sources_count;
>  
> +	struct drm_connector connector;
> +	struct drm_encoder encoder;

Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.

Nack.

>  	struct drm_writeback_connector writeback;
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> index c79d1259e49b..5b1e83380c47 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> @@ -200,8 +200,10 @@ 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);
> -	drm_connector_helper_add(&wb_conn->base,
> +	wb_conn->base = &rcrtc->connector;
> +	wb_conn->encoder = &rcrtc->encoder;
> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> +	drm_connector_helper_add(wb_conn->base,
>  				 &rcar_du_wb_conn_helper_funcs);
>  
>  	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>  	struct drm_framebuffer *fb;
>  	unsigned int i;
>  
> -	state = rcrtc->writeback.base.state;
> +	state = rcrtc->writeback.base->state;
>  	if (!state || !state->writeback_job)
>  		return;
>  

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Jani Nikula]

On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Kandpal,
>
> Thank you for the patch.
>
> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>> Changing rcar_du driver to accomadate the change of
>> drm_writeback_connector.base and drm_writeback_connector.encoder
>> to a pointer the reason for which is explained in the
>> Patch(drm: add writeback pointers to drm_connector).
>> 
>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> index 66e8839db708..68f387a04502 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>  	const char *const *sources;
>>  	unsigned int sources_count;
>>  
>> +	struct drm_connector connector;
>> +	struct drm_encoder encoder;
>
> Those fields are, at best, poorly named. Furthermore, there's no need in
> this driver or in other drivers using drm_writeback_connector to create
> an encoder or connector manually. Let's not polute all drivers because
> i915 doesn't have its abstractions right.

i915 uses the quite common model for struct inheritance:

	struct intel_connector {
		struct drm_connector base;
		/* ... */
	}

Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.

We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.

It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.

So I think drm_writeback_connector should *not* use the inheritance
abstraction because it's a midlayer that should leave that option to the
drivers. I think drm_writeback_connector needs to be changed to
accommodate that, and, unfortunately, it means current writeback users
need to be changed as well.

I am not sure, however, if the series at hand is the right
approach. Perhaps writeback can be modified to allocate the stuff for
you if you prefer it that way, as long as the drm_connector is not
embedded in struct drm_writeback_connector.


BR,
Jani.


>
> Nack.
>
>>  	struct drm_writeback_connector writeback;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> index c79d1259e49b..5b1e83380c47 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> @@ -200,8 +200,10 @@ 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);
>> -	drm_connector_helper_add(&wb_conn->base,
>> +	wb_conn->base = &rcrtc->connector;
>> +	wb_conn->encoder = &rcrtc->encoder;
>> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> +	drm_connector_helper_add(wb_conn->base,
>>  				 &rcar_du_wb_conn_helper_funcs);
>>  
>>  	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>>  	struct drm_framebuffer *fb;
>>  	unsigned int i;
>>  
>> -	state = rcrtc->writeback.base.state;
>> +	state = rcrtc->writeback.base->state;
>>  	if (!state || !state->writeback_job)
>>  		return;
>>  

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Jani,

On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >> Changing rcar_du driver to accomadate the change of
> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> >> to a pointer the reason for which is explained in the
> >> Patch(drm: add writeback pointers to drm_connector).
> >> 
> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> index 66e8839db708..68f387a04502 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >>  	const char *const *sources;
> >>  	unsigned int sources_count;
> >>  
> >> +	struct drm_connector connector;
> >> +	struct drm_encoder encoder;
> >
> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > this driver or in other drivers using drm_writeback_connector to create
> > an encoder or connector manually. Let's not polute all drivers because
> > i915 doesn't have its abstractions right.
> 
> i915 uses the quite common model for struct inheritance:
> 
> 	struct intel_connector {
> 		struct drm_connector base;
> 		/* ... */
> 	}
> 
> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> radeon, tilcdc, and vboxvideo.
> 
> We could argue about the relative merits of that abstraction, but I
> think the bottom line is that it's popular and the drivers using it are
> not going to be persuaded to move away from it.

Nobody said inheritance is bad.

> It's no coincidence that the drivers who've implemented writeback so far
> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> because the drm_writeback_connector midlayer does, forcing the issue.

Are you sure it's not a coincidence ? :-)

The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.

> So I think drm_writeback_connector should *not* use the inheritance
> abstraction because it's a midlayer that should leave that option to the
> drivers. I think drm_writeback_connector needs to be changed to
> accommodate that, and, unfortunately, it means current writeback users
> need to be changed as well.
> 
> I am not sure, however, if the series at hand is the right
> approach. Perhaps writeback can be modified to allocate the stuff for
> you if you prefer it that way, as long as the drm_connector is not
> embedded in struct drm_writeback_connector.
> 
> > Nack.
> >
> >>  	struct drm_writeback_connector writeback;
> >>  };
> >>  
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> index c79d1259e49b..5b1e83380c47 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> @@ -200,8 +200,10 @@ 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);
> >> -	drm_connector_helper_add(&wb_conn->base,
> >> +	wb_conn->base = &rcrtc->connector;
> >> +	wb_conn->encoder = &rcrtc->encoder;
> >> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> >> +	drm_connector_helper_add(wb_conn->base,
> >>  				 &rcar_du_wb_conn_helper_funcs);
> >>  
> >>  	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> >>  	struct drm_framebuffer *fb;
> >>  	unsigned int i;
> >>  
> >> -	state = rcrtc->writeback.base.state;
> >> +	state = rcrtc->writeback.base->state;
> >>  	if (!state || !state->writeback_job)
> >>  		return;
> >>  

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Jani Nikula]

On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Jani,
>
> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>> >> Changing rcar_du driver to accomadate the change of
>> >> drm_writeback_connector.base and drm_writeback_connector.encoder
>> >> to a pointer the reason for which is explained in the
>> >> Patch(drm: add writeback pointers to drm_connector).
>> >> 
>> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>> >>  2 files changed, 7 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> index 66e8839db708..68f387a04502 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>> >>  	const char *const *sources;
>> >>  	unsigned int sources_count;
>> >>  
>> >> +	struct drm_connector connector;
>> >> +	struct drm_encoder encoder;
>> >
>> > Those fields are, at best, poorly named. Furthermore, there's no need in
>> > this driver or in other drivers using drm_writeback_connector to create
>> > an encoder or connector manually. Let's not polute all drivers because
>> > i915 doesn't have its abstractions right.
>> 
>> i915 uses the quite common model for struct inheritance:
>> 
>> 	struct intel_connector {
>> 		struct drm_connector base;
>> 		/* ... */
>> 	}
>> 
>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>> radeon, tilcdc, and vboxvideo.
>> 
>> We could argue about the relative merits of that abstraction, but I
>> think the bottom line is that it's popular and the drivers using it are
>> not going to be persuaded to move away from it.
>
> Nobody said inheritance is bad.
>
>> It's no coincidence that the drivers who've implemented writeback so far
>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>> because the drm_writeback_connector midlayer does, forcing the issue.
>
> Are you sure it's not a coincidence ? :-)
>
> The encoder and especially connector created by drm_writeback_connector
> are there only because KMS requires a drm_encoder and a drm_connector to
> be exposed to userspace (and I could argue that using a connector for
> writeback is a hack, but that won't change). The connector is "virtual",
> I still fail to see why i915 or any other driver would need to wrap it
> into something else. The whole point of the drm_writeback_connector
> abstraction is that drivers do not have to manage the writeback
> drm_connector manually, they shouldn't touch it at all.

The thing is, drm_writeback_connector_init() calling
drm_connector_init() on the drm_connector embedded in
drm_writeback_connector leads to that connector being added to the
drm_device's list of connectors. Ditto for the encoder.

All the driver code that handles drm_connectors would need to take into
account they might not be embedded in intel_connector. Throughout the
driver. Ditto for the encoders.

The point is, you can't initialize a connector or an encoder for a
drm_device in isolation of the rest of the driver, even if it were
supposed to be hidden away.

BR,
Jani.

>
>> So I think drm_writeback_connector should *not* use the inheritance
>> abstraction because it's a midlayer that should leave that option to the
>> drivers. I think drm_writeback_connector needs to be changed to
>> accommodate that, and, unfortunately, it means current writeback users
>> need to be changed as well.
>> 
>> I am not sure, however, if the series at hand is the right
>> approach. Perhaps writeback can be modified to allocate the stuff for
>> you if you prefer it that way, as long as the drm_connector is not
>> embedded in struct drm_writeback_connector.
>> 
>> > Nack.
>> >
>> >>  	struct drm_writeback_connector writeback;
>> >>  };
>> >>  
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> index c79d1259e49b..5b1e83380c47 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> >> @@ -200,8 +200,10 @@ 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);
>> >> -	drm_connector_helper_add(&wb_conn->base,
>> >> +	wb_conn->base = &rcrtc->connector;
>> >> +	wb_conn->encoder = &rcrtc->encoder;
>> >> +	wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> >> +	drm_connector_helper_add(wb_conn->base,
>> >>  				 &rcar_du_wb_conn_helper_funcs);
>> >>  
>> >>  	return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>> >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>> >>  	struct drm_framebuffer *fb;
>> >>  	unsigned int i;
>> >>  
>> >> -	state = rcrtc->writeback.base.state;
>> >> +	state = rcrtc->writeback.base->state;
>> >>  	if (!state || !state->writeback_job)
>> >>  		return;
>> >>  

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Rob Clark]

On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Jani,
> >
> > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >> >> Changing rcar_du driver to accomadate the change of
> >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> >> >> to a pointer the reason for which is explained in the
> >> >> Patch(drm: add writeback pointers to drm_connector).
> >> >>
> >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> >> index 66e8839db708..68f387a04502 100644
> >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >> >>   const char *const *sources;
> >> >>   unsigned int sources_count;
> >> >>
> >> >> + struct drm_connector connector;
> >> >> + struct drm_encoder encoder;
> >> >
> >> > Those fields are, at best, poorly named. Furthermore, there's no need in
> >> > this driver or in other drivers using drm_writeback_connector to create
> >> > an encoder or connector manually. Let's not polute all drivers because
> >> > i915 doesn't have its abstractions right.
> >>
> >> i915 uses the quite common model for struct inheritance:
> >>
> >>      struct intel_connector {
> >>              struct drm_connector base;
> >>              /* ... */
> >>      }
> >>
> >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> >> radeon, tilcdc, and vboxvideo.
> >>
> >> We could argue about the relative merits of that abstraction, but I
> >> think the bottom line is that it's popular and the drivers using it are
> >> not going to be persuaded to move away from it.
> >
> > Nobody said inheritance is bad.
> >
> >> It's no coincidence that the drivers who've implemented writeback so far
> >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> >> because the drm_writeback_connector midlayer does, forcing the issue.
> >
> > Are you sure it's not a coincidence ? :-)
> >
> > The encoder and especially connector created by drm_writeback_connector
> > are there only because KMS requires a drm_encoder and a drm_connector to
> > be exposed to userspace (and I could argue that using a connector for
> > writeback is a hack, but that won't change). The connector is "virtual",
> > I still fail to see why i915 or any other driver would need to wrap it
> > into something else. The whole point of the drm_writeback_connector
> > abstraction is that drivers do not have to manage the writeback
> > drm_connector manually, they shouldn't touch it at all.
>
> The thing is, drm_writeback_connector_init() calling
> drm_connector_init() on the drm_connector embedded in
> drm_writeback_connector leads to that connector being added to the
> drm_device's list of connectors. Ditto for the encoder.
>
> All the driver code that handles drm_connectors would need to take into
> account they might not be embedded in intel_connector. Throughout the
> driver. Ditto for the encoders.

The assumption that a connector is embedded in intel_connector doesn't
really play that well with how bridge and panel connectors work.. so
in general this seems like a good thing to unwind.

But as a point of practicality, i915 is a large driver covering a lot
of generations of hw with a lot of users.  So I can understand
changing this design isn't something that can happen quickly or
easily.  IMO we should allow i915 to create it's own connector for
writeback, and just document clearly that this isn't the approach new
drivers should take.  I mean, I understand idealism, but sometimes a
dose of pragmatism is needed. :-)

BR,
-R

> The point is, you can't initialize a connector or an encoder for a
> drm_device in isolation of the rest of the driver, even if it were
> supposed to be hidden away.
>
> BR,
> Jani.
>

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Rob,

On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > >> >> Changing rcar_du driver to accomadate the change of
> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> > >> >> to a pointer the reason for which is explained in the
> > >> >> Patch(drm: add writeback pointers to drm_connector).
> > >> >>
> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > >> >> ---
> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> > >> >>
> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> index 66e8839db708..68f387a04502 100644
> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > >> >>   const char *const *sources;
> > >> >>   unsigned int sources_count;
> > >> >>
> > >> >> + struct drm_connector connector;
> > >> >> + struct drm_encoder encoder;
> > >> >
> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > >> > this driver or in other drivers using drm_writeback_connector to create
> > >> > an encoder or connector manually. Let's not polute all drivers because
> > >> > i915 doesn't have its abstractions right.
> > >>
> > >> i915 uses the quite common model for struct inheritance:
> > >>
> > >>      struct intel_connector {
> > >>              struct drm_connector base;
> > >>              /* ... */
> > >>      }
> > >>
> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > >> radeon, tilcdc, and vboxvideo.
> > >>
> > >> We could argue about the relative merits of that abstraction, but I
> > >> think the bottom line is that it's popular and the drivers using it are
> > >> not going to be persuaded to move away from it.
> > >
> > > Nobody said inheritance is bad.
> > >
> > >> It's no coincidence that the drivers who've implemented writeback so far
> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > >> because the drm_writeback_connector midlayer does, forcing the issue.
> > >
> > > Are you sure it's not a coincidence ? :-)
> > >
> > > The encoder and especially connector created by drm_writeback_connector
> > > are there only because KMS requires a drm_encoder and a drm_connector to
> > > be exposed to userspace (and I could argue that using a connector for
> > > writeback is a hack, but that won't change). The connector is "virtual",
> > > I still fail to see why i915 or any other driver would need to wrap it
> > > into something else. The whole point of the drm_writeback_connector
> > > abstraction is that drivers do not have to manage the writeback
> > > drm_connector manually, they shouldn't touch it at all.
> >
> > The thing is, drm_writeback_connector_init() calling
> > drm_connector_init() on the drm_connector embedded in
> > drm_writeback_connector leads to that connector being added to the
> > drm_device's list of connectors. Ditto for the encoder.
> >
> > All the driver code that handles drm_connectors would need to take into
> > account they might not be embedded in intel_connector. Throughout the
> > driver. Ditto for the encoders.
> 
> The assumption that a connector is embedded in intel_connector doesn't
> really play that well with how bridge and panel connectors work.. so
> in general this seems like a good thing to unwind.
> 
> But as a point of practicality, i915 is a large driver covering a lot
> of generations of hw with a lot of users.  So I can understand
> changing this design isn't something that can happen quickly or
> easily.  IMO we should allow i915 to create it's own connector for
> writeback, and just document clearly that this isn't the approach new
> drivers should take.  I mean, I understand idealism, but sometimes a
> dose of pragmatism is needed. :-)

i915 is big, but so is Intel. It's not fair to treat everybody else as a
second class citizen and let Intel get away without doing its homework.
I want to see this refactoring effort moving forward in i915 (and moving
to drm_bridge would then be a good idea too). If writeback support in
i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
but not without a real effort to fix the core issue.

> > The point is, you can't initialize a connector or an encoder for a
> > drm_device in isolation of the rest of the driver, even if it were
> > supposed to be hidden away.

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Jani Nikula]

On Mon, 28 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Rob,
>
> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
>> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>> > >> >> Changing rcar_du driver to accomadate the change of
>> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
>> > >> >> to a pointer the reason for which is explained in the
>> > >> >> Patch(drm: add writeback pointers to drm_connector).
>> > >> >>
>> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> > >> >> ---
>> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>> > >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
>> > >> >>
>> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> > >> >> index 66e8839db708..68f387a04502 100644
>> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>> > >> >>   const char *const *sources;
>> > >> >>   unsigned int sources_count;
>> > >> >>
>> > >> >> + struct drm_connector connector;
>> > >> >> + struct drm_encoder encoder;
>> > >> >
>> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in
>> > >> > this driver or in other drivers using drm_writeback_connector to create
>> > >> > an encoder or connector manually. Let's not polute all drivers because
>> > >> > i915 doesn't have its abstractions right.
>> > >>
>> > >> i915 uses the quite common model for struct inheritance:
>> > >>
>> > >>      struct intel_connector {
>> > >>              struct drm_connector base;
>> > >>              /* ... */
>> > >>      }
>> > >>
>> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>> > >> radeon, tilcdc, and vboxvideo.
>> > >>
>> > >> We could argue about the relative merits of that abstraction, but I
>> > >> think the bottom line is that it's popular and the drivers using it are
>> > >> not going to be persuaded to move away from it.
>> > >
>> > > Nobody said inheritance is bad.
>> > >
>> > >> It's no coincidence that the drivers who've implemented writeback so far
>> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>> > >> because the drm_writeback_connector midlayer does, forcing the issue.
>> > >
>> > > Are you sure it's not a coincidence ? :-)
>> > >
>> > > The encoder and especially connector created by drm_writeback_connector
>> > > are there only because KMS requires a drm_encoder and a drm_connector to
>> > > be exposed to userspace (and I could argue that using a connector for
>> > > writeback is a hack, but that won't change). The connector is "virtual",
>> > > I still fail to see why i915 or any other driver would need to wrap it
>> > > into something else. The whole point of the drm_writeback_connector
>> > > abstraction is that drivers do not have to manage the writeback
>> > > drm_connector manually, they shouldn't touch it at all.
>> >
>> > The thing is, drm_writeback_connector_init() calling
>> > drm_connector_init() on the drm_connector embedded in
>> > drm_writeback_connector leads to that connector being added to the
>> > drm_device's list of connectors. Ditto for the encoder.
>> >
>> > All the driver code that handles drm_connectors would need to take into
>> > account they might not be embedded in intel_connector. Throughout the
>> > driver. Ditto for the encoders.
>> 
>> The assumption that a connector is embedded in intel_connector doesn't
>> really play that well with how bridge and panel connectors work.. so
>> in general this seems like a good thing to unwind.
>> 
>> But as a point of practicality, i915 is a large driver covering a lot
>> of generations of hw with a lot of users.  So I can understand
>> changing this design isn't something that can happen quickly or
>> easily.  IMO we should allow i915 to create it's own connector for
>> writeback, and just document clearly that this isn't the approach new
>> drivers should take.  I mean, I understand idealism, but sometimes a
>> dose of pragmatism is needed. :-)
>
> i915 is big, but so is Intel. It's not fair to treat everybody else as a
> second class citizen and let Intel get away without doing its homework.

Laurent, as you accuse us of not doing our homework, I'll point out that
we've been embedding drm crtc, encoder and connector ever since
modesetting support was added to i915 in 2008, since before *any* of the
things you now use as a rationale for asking us to do a massive rewrite
of the driver existed.

It's been ok to embed those structures for well over ten years. It's a
common pattern, basically throughout the kernel. Other drivers do it
too, not just i915. There hasn't been the slightest hint this should not
be done until this very conversation.

> I want to see this refactoring effort moving forward in i915 (and moving
> to drm_bridge would then be a good idea too). If writeback support in
> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
> but not without a real effort to fix the core issue.

I think the onus is on you to first convince everyone that embedding the
drm core kms structures is an antipattern that all drivers, not just
i915, should stop using. In OO terms, you're saying they are classes
that should be final and not extended.

And even then, to be totally honest, refactoring the structures is not
going to be anywhere near the top of our list of things to do, for a
very long time.


BR,
Jani.

>
>> > The point is, you can't initialize a connector or an encoder for a
>> > drm_device in isolation of the rest of the driver, even if it were
>> > supposed to be hidden away.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Jani,
On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
> On Mon, 28 Feb 2022, Laurent Pinchart wrote:
> > On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
> >> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
> >> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> >> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >> > >> >> Changing rcar_du driver to accomadate the change of
> >> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> >> > >> >> to a pointer the reason for which is explained in the
> >> > >> >> Patch(drm: add writeback pointers to drm_connector).
> >> > >> >>
> >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> > >> >> ---
> >> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >> > >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >> > >> >>
> >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> > >> >> index 66e8839db708..68f387a04502 100644
> >> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >> > >> >>   const char *const *sources;
> >> > >> >>   unsigned int sources_count;
> >> > >> >>
> >> > >> >> + struct drm_connector connector;
> >> > >> >> + struct drm_encoder encoder;
> >> > >> >
> >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in
> >> > >> > this driver or in other drivers using drm_writeback_connector to create
> >> > >> > an encoder or connector manually. Let's not polute all drivers because
> >> > >> > i915 doesn't have its abstractions right.
> >> > >>
> >> > >> i915 uses the quite common model for struct inheritance:
> >> > >>
> >> > >>      struct intel_connector {
> >> > >>              struct drm_connector base;
> >> > >>              /* ... */
> >> > >>      }
> >> > >>
> >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> >> > >> radeon, tilcdc, and vboxvideo.
> >> > >>
> >> > >> We could argue about the relative merits of that abstraction, but I
> >> > >> think the bottom line is that it's popular and the drivers using it are
> >> > >> not going to be persuaded to move away from it.
> >> > >
> >> > > Nobody said inheritance is bad.
> >> > >
> >> > >> It's no coincidence that the drivers who've implemented writeback so far
> >> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> >> > >> because the drm_writeback_connector midlayer does, forcing the issue.
> >> > >
> >> > > Are you sure it's not a coincidence ? :-)
> >> > >
> >> > > The encoder and especially connector created by drm_writeback_connector
> >> > > are there only because KMS requires a drm_encoder and a drm_connector to
> >> > > be exposed to userspace (and I could argue that using a connector for
> >> > > writeback is a hack, but that won't change). The connector is "virtual",
> >> > > I still fail to see why i915 or any other driver would need to wrap it
> >> > > into something else. The whole point of the drm_writeback_connector
> >> > > abstraction is that drivers do not have to manage the writeback
> >> > > drm_connector manually, they shouldn't touch it at all.
> >> >
> >> > The thing is, drm_writeback_connector_init() calling
> >> > drm_connector_init() on the drm_connector embedded in
> >> > drm_writeback_connector leads to that connector being added to the
> >> > drm_device's list of connectors. Ditto for the encoder.
> >> >
> >> > All the driver code that handles drm_connectors would need to take into
> >> > account they might not be embedded in intel_connector. Throughout the
> >> > driver. Ditto for the encoders.
> >> 
> >> The assumption that a connector is embedded in intel_connector doesn't
> >> really play that well with how bridge and panel connectors work.. so
> >> in general this seems like a good thing to unwind.
> >> 
> >> But as a point of practicality, i915 is a large driver covering a lot
> >> of generations of hw with a lot of users.  So I can understand
> >> changing this design isn't something that can happen quickly or
> >> easily.  IMO we should allow i915 to create it's own connector for
> >> writeback, and just document clearly that this isn't the approach new
> >> drivers should take.  I mean, I understand idealism, but sometimes a
> >> dose of pragmatism is needed. :-)
> >
> > i915 is big, but so is Intel. It's not fair to treat everybody else as a
> > second class citizen and let Intel get away without doing its homework.
> 
> Laurent, as you accuse us of not doing our homework, I'll point out that
> we've been embedding drm crtc, encoder and connector ever since
> modesetting support was added to i915 in 2008, since before *any* of the
> things you now use as a rationale for asking us to do a massive rewrite
> of the driver existed.
> 
> It's been ok to embed those structures for well over ten years. It's a
> common pattern, basically throughout the kernel. Other drivers do it
> too, not just i915. There hasn't been the slightest hint this should not
> be done until this very conversation.
> 
> > I want to see this refactoring effort moving forward in i915 (and moving
> > to drm_bridge would then be a good idea too). If writeback support in
> > i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
> > but not without a real effort to fix the core issue.
> 
> I think the onus is on you to first convince everyone that embedding the
> drm core kms structures is an antipattern that all drivers, not just
> i915, should stop using. In OO terms, you're saying they are classes
> that should be final and not extended.
> 
> And even then, to be totally honest, refactoring the structures is not
> going to be anywhere near the top of our list of things to do, for a
> very long time.

I may have not expressed myself correctly. There's nothing wrong as such
in embedded those structures in driver-specific structures (a.k.a. C
inheritance). That doesn't need to change (albeit for drm_encoder I
think we should move away from that pattern, but that's an entirely
different issue, and nothing that needs to be addressed soonà.

The issue here is assuming that every drm_connector instance can be
up-casted to an i915-specific structure.

> >> > The point is, you can't initialize a connector or an encoder for a
> >> > drm_device in isolation of the rest of the driver, even if it were
> >> > supposed to be hidden away.

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hello,

On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
> > On Mon, 28 Feb 2022, Laurent Pinchart wrote:
> > > On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
> > >> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
> > >> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > >> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > >> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > >> > >> >> Changing rcar_du driver to accomadate the change of
> > >> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> > >> > >> >> to a pointer the reason for which is explained in the
> > >> > >> >> Patch(drm: add writeback pointers to drm_connector).
> > >> > >> >>
> > >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > >> > >> >> ---
> > >> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > >> > >> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > >> > >> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> > >> > >> >>
> > >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> > >> >> index 66e8839db708..68f387a04502 100644
> > >> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > >> > >> >>   const char *const *sources;
> > >> > >> >>   unsigned int sources_count;
> > >> > >> >>
> > >> > >> >> + struct drm_connector connector;
> > >> > >> >> + struct drm_encoder encoder;
> > >> > >> >
> > >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > >> > >> > this driver or in other drivers using drm_writeback_connector to create
> > >> > >> > an encoder or connector manually. Let's not polute all drivers because
> > >> > >> > i915 doesn't have its abstractions right.
> > >> > >>
> > >> > >> i915 uses the quite common model for struct inheritance:
> > >> > >>
> > >> > >>      struct intel_connector {
> > >> > >>              struct drm_connector base;
> > >> > >>              /* ... */
> > >> > >>      }
> > >> > >>
> > >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > >> > >> radeon, tilcdc, and vboxvideo.
> > >> > >>
> > >> > >> We could argue about the relative merits of that abstraction, but I
> > >> > >> think the bottom line is that it's popular and the drivers using it are
> > >> > >> not going to be persuaded to move away from it.
> > >> > >
> > >> > > Nobody said inheritance is bad.
> > >> > >
> > >> > >> It's no coincidence that the drivers who've implemented writeback so far
> > >> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > >> > >> because the drm_writeback_connector midlayer does, forcing the issue.
> > >> > >
> > >> > > Are you sure it's not a coincidence ? :-)
> > >> > >
> > >> > > The encoder and especially connector created by drm_writeback_connector
> > >> > > are there only because KMS requires a drm_encoder and a drm_connector to
> > >> > > be exposed to userspace (and I could argue that using a connector for
> > >> > > writeback is a hack, but that won't change). The connector is "virtual",
> > >> > > I still fail to see why i915 or any other driver would need to wrap it
> > >> > > into something else. The whole point of the drm_writeback_connector
> > >> > > abstraction is that drivers do not have to manage the writeback
> > >> > > drm_connector manually, they shouldn't touch it at all.
> > >> >
> > >> > The thing is, drm_writeback_connector_init() calling
> > >> > drm_connector_init() on the drm_connector embedded in
> > >> > drm_writeback_connector leads to that connector being added to the
> > >> > drm_device's list of connectors. Ditto for the encoder.
> > >> >
> > >> > All the driver code that handles drm_connectors would need to take into
> > >> > account they might not be embedded in intel_connector. Throughout the
> > >> > driver. Ditto for the encoders.
> > >> 
> > >> The assumption that a connector is embedded in intel_connector doesn't
> > >> really play that well with how bridge and panel connectors work.. so
> > >> in general this seems like a good thing to unwind.
> > >> 
> > >> But as a point of practicality, i915 is a large driver covering a lot
> > >> of generations of hw with a lot of users.  So I can understand
> > >> changing this design isn't something that can happen quickly or
> > >> easily.  IMO we should allow i915 to create it's own connector for
> > >> writeback, and just document clearly that this isn't the approach new
> > >> drivers should take.  I mean, I understand idealism, but sometimes a
> > >> dose of pragmatism is needed. :-)
> > >
> > > i915 is big, but so is Intel. It's not fair to treat everybody else as a
> > > second class citizen and let Intel get away without doing its homework.
> > 
> > Laurent, as you accuse us of not doing our homework, I'll point out that
> > we've been embedding drm crtc, encoder and connector ever since
> > modesetting support was added to i915 in 2008, since before *any* of the
> > things you now use as a rationale for asking us to do a massive rewrite
> > of the driver existed.
> > 
> > It's been ok to embed those structures for well over ten years. It's a
> > common pattern, basically throughout the kernel. Other drivers do it
> > too, not just i915. There hasn't been the slightest hint this should not
> > be done until this very conversation.
> > 
> > > I want to see this refactoring effort moving forward in i915 (and moving
> > > to drm_bridge would then be a good idea too). If writeback support in
> > > i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
> > > but not without a real effort to fix the core issue.
> > 
> > I think the onus is on you to first convince everyone that embedding the
> > drm core kms structures is an antipattern that all drivers, not just
> > i915, should stop using. In OO terms, you're saying they are classes
> > that should be final and not extended.
> > 
> > And even then, to be totally honest, refactoring the structures is not
> > going to be anywhere near the top of our list of things to do, for a
> > very long time.
> 
> I may have not expressed myself correctly. There's nothing wrong as such
> in embedded those structures in driver-specific structures (a.k.a. C
> inheritance). That doesn't need to change (albeit for drm_encoder I
> think we should move away from that pattern, but that's an entirely
> different issue, and nothing that needs to be addressed soonà.
> 
> The issue here is assuming that every drm_connector instance can be
> up-casted to an i915-specific structure.

Thinking some more about this, I wonder a way forward could be to drop
the writeback connectors from the connectors list, or at least make them
invisible to drivers. The connectors list is used extensively for two
different purposes: tracking all drm_connector instances, and tracking
all real connectors. The former is mostly needed by the DRM core for
bookkeeping purposes and to expose all drm_connector instances to
userspace, while the latter is also used by drivers, in many cases in
locations that don't expect writeback connectors. Using a drm_connector
to implement writeback isn't something we can revisit, but we could
avoid exposing that to drivers by considering "real" connectors and
writeback connectors two different types of entities in the APIs the DRM
core exposes to drivers. What do you think, would it help for i915 ?

> > >> > The point is, you can't initialize a connector or an encoder for a
> > >> > drm_device in isolation of the rest of the driver, even if it were
> > >> > supposed to be hidden away.

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
> Hello,
> 
> On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
>> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
>>> On Mon, 28 Feb 2022, Laurent Pinchart wrote:
>>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
>>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>>>>>>>>>> Changing rcar_du driver to accomadate the change of
>>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
>>>>>>>>>> to a pointer the reason for which is explained in the
>>>>>>>>>> Patch(drm: add writeback pointers to drm_connector).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>>>>>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> index 66e8839db708..68f387a04502 100644
>>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>>>>>>>>>    const char *const *sources;
>>>>>>>>>>    unsigned int sources_count;
>>>>>>>>>>
>>>>>>>>>> + struct drm_connector connector;
>>>>>>>>>> + struct drm_encoder encoder;
>>>>>>>>>
>>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
>>>>>>>>> this driver or in other drivers using drm_writeback_connector to create
>>>>>>>>> an encoder or connector manually. Let's not polute all drivers because
>>>>>>>>> i915 doesn't have its abstractions right.
>>>>>>>>
>>>>>>>> i915 uses the quite common model for struct inheritance:
>>>>>>>>
>>>>>>>>       struct intel_connector {
>>>>>>>>               struct drm_connector base;
>>>>>>>>               /* ... */
>>>>>>>>       }
>>>>>>>>
>>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>>>>>>>> radeon, tilcdc, and vboxvideo.
>>>>>>>>
>>>>>>>> We could argue about the relative merits of that abstraction, but I
>>>>>>>> think the bottom line is that it's popular and the drivers using it are
>>>>>>>> not going to be persuaded to move away from it.
>>>>>>>
>>>>>>> Nobody said inheritance is bad.
>>>>>>>
>>>>>>>> It's no coincidence that the drivers who've implemented writeback so far
>>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue.
>>>>>>>
>>>>>>> Are you sure it's not a coincidence ? :-)
>>>>>>>
>>>>>>> The encoder and especially connector created by drm_writeback_connector
>>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to
>>>>>>> be exposed to userspace (and I could argue that using a connector for
>>>>>>> writeback is a hack, but that won't change). The connector is "virtual",
>>>>>>> I still fail to see why i915 or any other driver would need to wrap it
>>>>>>> into something else. The whole point of the drm_writeback_connector
>>>>>>> abstraction is that drivers do not have to manage the writeback
>>>>>>> drm_connector manually, they shouldn't touch it at all.
>>>>>>
>>>>>> The thing is, drm_writeback_connector_init() calling
>>>>>> drm_connector_init() on the drm_connector embedded in
>>>>>> drm_writeback_connector leads to that connector being added to the
>>>>>> drm_device's list of connectors. Ditto for the encoder.
>>>>>>
>>>>>> All the driver code that handles drm_connectors would need to take into
>>>>>> account they might not be embedded in intel_connector. Throughout the
>>>>>> driver. Ditto for the encoders.
>>>>>
>>>>> The assumption that a connector is embedded in intel_connector doesn't
>>>>> really play that well with how bridge and panel connectors work.. so
>>>>> in general this seems like a good thing to unwind.
>>>>>
>>>>> But as a point of practicality, i915 is a large driver covering a lot
>>>>> of generations of hw with a lot of users.  So I can understand
>>>>> changing this design isn't something that can happen quickly or
>>>>> easily.  IMO we should allow i915 to create it's own connector for
>>>>> writeback, and just document clearly that this isn't the approach new
>>>>> drivers should take.  I mean, I understand idealism, but sometimes a
>>>>> dose of pragmatism is needed. :-)
>>>>
>>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a
>>>> second class citizen and let Intel get away without doing its homework.
>>>
>>> Laurent, as you accuse us of not doing our homework, I'll point out that
>>> we've been embedding drm crtc, encoder and connector ever since
>>> modesetting support was added to i915 in 2008, since before *any* of the
>>> things you now use as a rationale for asking us to do a massive rewrite
>>> of the driver existed.
>>>
>>> It's been ok to embed those structures for well over ten years. It's a
>>> common pattern, basically throughout the kernel. Other drivers do it
>>> too, not just i915. There hasn't been the slightest hint this should not
>>> be done until this very conversation.
>>>
>>>> I want to see this refactoring effort moving forward in i915 (and moving
>>>> to drm_bridge would then be a good idea too). If writeback support in
>>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
>>>> but not without a real effort to fix the core issue.
>>>
>>> I think the onus is on you to first convince everyone that embedding the
>>> drm core kms structures is an antipattern that all drivers, not just
>>> i915, should stop using. In OO terms, you're saying they are classes
>>> that should be final and not extended.
>>>
>>> And even then, to be totally honest, refactoring the structures is not
>>> going to be anywhere near the top of our list of things to do, for a
>>> very long time.
>>
>> I may have not expressed myself correctly. There's nothing wrong as such
>> in embedded those structures in driver-specific structures (a.k.a. C
>> inheritance). That doesn't need to change (albeit for drm_encoder I
>> think we should move away from that pattern, but that's an entirely
>> different issue, and nothing that needs to be addressed soonà.
>>
>> The issue here is assuming that every drm_connector instance can be
>> up-casted to an i915-specific structure.
> 
> Thinking some more about this, I wonder a way forward could be to drop
> the writeback connectors from the connectors list, or at least make them
> invisible to drivers. The connectors list is used extensively for two
> different purposes: tracking all drm_connector instances, and tracking
> all real connectors. The former is mostly needed by the DRM core for
> bookkeeping purposes and to expose all drm_connector instances to
> userspace, while the latter is also used by drivers, in many cases in
> locations that don't expect writeback connectors. Using a drm_connector
> to implement writeback isn't something we can revisit, but we could
> avoid exposing that to drivers by considering "real" connectors and
> writeback connectors two different types of entities in the APIs the DRM
> core exposes to drivers. What do you think, would it help for i915 ?
> 

Hi Jani and Suraj

Since atleast there is agreement on changing the drm_encoder to a 
pointer in the drm_writeback_connector, can we re-arrange the series OR 
split it into encoder first and then connector so that atleast those 
bits can go in first? It will benefit both our (i915 & MSM ) 
implementations.

Hi Laurent

For the connector part, can you please post a RFC for your proposal?
Perhaps myself and Suraj can evaluate our implementations on top of that 
and the encoder change.

Thanks

Abhinav

>>>>>> The point is, you can't initialize a connector or an encoder for a
>>>>>> drm_device in isolation of the rest of the driver, even if it were
>>>>>> supposed to be hidden away.
> 

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Abhinav,

On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote:
> On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
> > On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
> >> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
> >>> On Mon, 28 Feb 2022, Laurent Pinchart wrote:
> >>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
> >>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
> >>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> >>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >>>>>>>>>> Changing rcar_du driver to accomadate the change of
> >>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
> >>>>>>>>>> to a pointer the reason for which is explained in the
> >>>>>>>>>> Patch(drm: add writeback pointers to drm_connector).
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >>>>>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >>>>>>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>>>>>> index 66e8839db708..68f387a04502 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >>>>>>>>>>    const char *const *sources;
> >>>>>>>>>>    unsigned int sources_count;
> >>>>>>>>>>
> >>>>>>>>>> + struct drm_connector connector;
> >>>>>>>>>> + struct drm_encoder encoder;
> >>>>>>>>>
> >>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
> >>>>>>>>> this driver or in other drivers using drm_writeback_connector to create
> >>>>>>>>> an encoder or connector manually. Let's not polute all drivers because
> >>>>>>>>> i915 doesn't have its abstractions right.
> >>>>>>>>
> >>>>>>>> i915 uses the quite common model for struct inheritance:
> >>>>>>>>
> >>>>>>>>       struct intel_connector {
> >>>>>>>>               struct drm_connector base;
> >>>>>>>>               /* ... */
> >>>>>>>>       }
> >>>>>>>>
> >>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> >>>>>>>> radeon, tilcdc, and vboxvideo.
> >>>>>>>>
> >>>>>>>> We could argue about the relative merits of that abstraction, but I
> >>>>>>>> think the bottom line is that it's popular and the drivers using it are
> >>>>>>>> not going to be persuaded to move away from it.
> >>>>>>>
> >>>>>>> Nobody said inheritance is bad.
> >>>>>>>
> >>>>>>>> It's no coincidence that the drivers who've implemented writeback so far
> >>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> >>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue.
> >>>>>>>
> >>>>>>> Are you sure it's not a coincidence ? :-)
> >>>>>>>
> >>>>>>> The encoder and especially connector created by drm_writeback_connector
> >>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to
> >>>>>>> be exposed to userspace (and I could argue that using a connector for
> >>>>>>> writeback is a hack, but that won't change). The connector is "virtual",
> >>>>>>> I still fail to see why i915 or any other driver would need to wrap it
> >>>>>>> into something else. The whole point of the drm_writeback_connector
> >>>>>>> abstraction is that drivers do not have to manage the writeback
> >>>>>>> drm_connector manually, they shouldn't touch it at all.
> >>>>>>
> >>>>>> The thing is, drm_writeback_connector_init() calling
> >>>>>> drm_connector_init() on the drm_connector embedded in
> >>>>>> drm_writeback_connector leads to that connector being added to the
> >>>>>> drm_device's list of connectors. Ditto for the encoder.
> >>>>>>
> >>>>>> All the driver code that handles drm_connectors would need to take into
> >>>>>> account they might not be embedded in intel_connector. Throughout the
> >>>>>> driver. Ditto for the encoders.
> >>>>>
> >>>>> The assumption that a connector is embedded in intel_connector doesn't
> >>>>> really play that well with how bridge and panel connectors work.. so
> >>>>> in general this seems like a good thing to unwind.
> >>>>>
> >>>>> But as a point of practicality, i915 is a large driver covering a lot
> >>>>> of generations of hw with a lot of users.  So I can understand
> >>>>> changing this design isn't something that can happen quickly or
> >>>>> easily.  IMO we should allow i915 to create it's own connector for
> >>>>> writeback, and just document clearly that this isn't the approach new
> >>>>> drivers should take.  I mean, I understand idealism, but sometimes a
> >>>>> dose of pragmatism is needed. :-)
> >>>>
> >>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a
> >>>> second class citizen and let Intel get away without doing its homework.
> >>>
> >>> Laurent, as you accuse us of not doing our homework, I'll point out that
> >>> we've been embedding drm crtc, encoder and connector ever since
> >>> modesetting support was added to i915 in 2008, since before *any* of the
> >>> things you now use as a rationale for asking us to do a massive rewrite
> >>> of the driver existed.
> >>>
> >>> It's been ok to embed those structures for well over ten years. It's a
> >>> common pattern, basically throughout the kernel. Other drivers do it
> >>> too, not just i915. There hasn't been the slightest hint this should not
> >>> be done until this very conversation.
> >>>
> >>>> I want to see this refactoring effort moving forward in i915 (and moving
> >>>> to drm_bridge would then be a good idea too). If writeback support in
> >>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
> >>>> but not without a real effort to fix the core issue.
> >>>
> >>> I think the onus is on you to first convince everyone that embedding the
> >>> drm core kms structures is an antipattern that all drivers, not just
> >>> i915, should stop using. In OO terms, you're saying they are classes
> >>> that should be final and not extended.
> >>>
> >>> And even then, to be totally honest, refactoring the structures is not
> >>> going to be anywhere near the top of our list of things to do, for a
> >>> very long time.
> >>
> >> I may have not expressed myself correctly. There's nothing wrong as such
> >> in embedded those structures in driver-specific structures (a.k.a. C
> >> inheritance). That doesn't need to change (albeit for drm_encoder I
> >> think we should move away from that pattern, but that's an entirely
> >> different issue, and nothing that needs to be addressed soonà.
> >>
> >> The issue here is assuming that every drm_connector instance can be
> >> up-casted to an i915-specific structure.
> > 
> > Thinking some more about this, I wonder a way forward could be to drop
> > the writeback connectors from the connectors list, or at least make them
> > invisible to drivers. The connectors list is used extensively for two
> > different purposes: tracking all drm_connector instances, and tracking
> > all real connectors. The former is mostly needed by the DRM core for
> > bookkeeping purposes and to expose all drm_connector instances to
> > userspace, while the latter is also used by drivers, in many cases in
> > locations that don't expect writeback connectors. Using a drm_connector
> > to implement writeback isn't something we can revisit, but we could
> > avoid exposing that to drivers by considering "real" connectors and
> > writeback connectors two different types of entities in the APIs the DRM
> > core exposes to drivers. What do you think, would it help for i915 ?
> 
> Hi Jani and Suraj
> 
> Since atleast there is agreement on changing the drm_encoder to a 
> pointer in the drm_writeback_connector, can we re-arrange the series OR 
> split it into encoder first and then connector so that atleast those 
> bits can go in first? It will benefit both our (i915 & MSM ) 
> implementations.
> 
> Hi Laurent
> 
> For the connector part, can you please post a RFC for your proposal?
> Perhaps myself and Suraj can evaluate our implementations on top of that 
> and the encoder change.

I'm afraid I won't have time to work on this personally for at least
several weeks, if not more.

> >>>>>> The point is, you can't initialize a connector or an encoder for a
> >>>>>> drm_device in isolation of the rest of the driver, even if it were
> >>>>>> supposed to be hidden away.

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

On 3/2/2022 10:31 AM, Laurent Pinchart wrote:
> Hi Abhinav,
> 
> On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote:
>> On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
>>> On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
>>>> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
>>>>> On Mon, 28 Feb 2022, Laurent Pinchart wrote:
>>>>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
>>>>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
>>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>>>>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>>>>>>>>>>>> Changing rcar_du driver to accomadate the change of
>>>>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
>>>>>>>>>>>> to a pointer the reason for which is explained in the
>>>>>>>>>>>> Patch(drm: add writeback pointers to drm_connector).
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>>>>>>>>>>>    drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>>>>>>>>>>>    2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>>>> index 66e8839db708..68f387a04502 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>>>>>>>>>>>     const char *const *sources;
>>>>>>>>>>>>     unsigned int sources_count;
>>>>>>>>>>>>
>>>>>>>>>>>> + struct drm_connector connector;
>>>>>>>>>>>> + struct drm_encoder encoder;
>>>>>>>>>>>
>>>>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
>>>>>>>>>>> this driver or in other drivers using drm_writeback_connector to create
>>>>>>>>>>> an encoder or connector manually. Let's not polute all drivers because
>>>>>>>>>>> i915 doesn't have its abstractions right.
>>>>>>>>>>
>>>>>>>>>> i915 uses the quite common model for struct inheritance:
>>>>>>>>>>
>>>>>>>>>>        struct intel_connector {
>>>>>>>>>>                struct drm_connector base;
>>>>>>>>>>                /* ... */
>>>>>>>>>>        }
>>>>>>>>>>
>>>>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>>>>>>>>>> radeon, tilcdc, and vboxvideo.
>>>>>>>>>>
>>>>>>>>>> We could argue about the relative merits of that abstraction, but I
>>>>>>>>>> think the bottom line is that it's popular and the drivers using it are
>>>>>>>>>> not going to be persuaded to move away from it.
>>>>>>>>>
>>>>>>>>> Nobody said inheritance is bad.
>>>>>>>>>
>>>>>>>>>> It's no coincidence that the drivers who've implemented writeback so far
>>>>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>>>>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue.
>>>>>>>>>
>>>>>>>>> Are you sure it's not a coincidence ? :-)
>>>>>>>>>
>>>>>>>>> The encoder and especially connector created by drm_writeback_connector
>>>>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to
>>>>>>>>> be exposed to userspace (and I could argue that using a connector for
>>>>>>>>> writeback is a hack, but that won't change). The connector is "virtual",
>>>>>>>>> I still fail to see why i915 or any other driver would need to wrap it
>>>>>>>>> into something else. The whole point of the drm_writeback_connector
>>>>>>>>> abstraction is that drivers do not have to manage the writeback
>>>>>>>>> drm_connector manually, they shouldn't touch it at all.
>>>>>>>>
>>>>>>>> The thing is, drm_writeback_connector_init() calling
>>>>>>>> drm_connector_init() on the drm_connector embedded in
>>>>>>>> drm_writeback_connector leads to that connector being added to the
>>>>>>>> drm_device's list of connectors. Ditto for the encoder.
>>>>>>>>
>>>>>>>> All the driver code that handles drm_connectors would need to take into
>>>>>>>> account they might not be embedded in intel_connector. Throughout the
>>>>>>>> driver. Ditto for the encoders.
>>>>>>>
>>>>>>> The assumption that a connector is embedded in intel_connector doesn't
>>>>>>> really play that well with how bridge and panel connectors work.. so
>>>>>>> in general this seems like a good thing to unwind.
>>>>>>>
>>>>>>> But as a point of practicality, i915 is a large driver covering a lot
>>>>>>> of generations of hw with a lot of users.  So I can understand
>>>>>>> changing this design isn't something that can happen quickly or
>>>>>>> easily.  IMO we should allow i915 to create it's own connector for
>>>>>>> writeback, and just document clearly that this isn't the approach new
>>>>>>> drivers should take.  I mean, I understand idealism, but sometimes a
>>>>>>> dose of pragmatism is needed. :-)
>>>>>>
>>>>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a
>>>>>> second class citizen and let Intel get away without doing its homework.
>>>>>
>>>>> Laurent, as you accuse us of not doing our homework, I'll point out that
>>>>> we've been embedding drm crtc, encoder and connector ever since
>>>>> modesetting support was added to i915 in 2008, since before *any* of the
>>>>> things you now use as a rationale for asking us to do a massive rewrite
>>>>> of the driver existed.
>>>>>
>>>>> It's been ok to embed those structures for well over ten years. It's a
>>>>> common pattern, basically throughout the kernel. Other drivers do it
>>>>> too, not just i915. There hasn't been the slightest hint this should not
>>>>> be done until this very conversation.
>>>>>
>>>>>> I want to see this refactoring effort moving forward in i915 (and moving
>>>>>> to drm_bridge would then be a good idea too). If writeback support in
>>>>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
>>>>>> but not without a real effort to fix the core issue.
>>>>>
>>>>> I think the onus is on you to first convince everyone that embedding the
>>>>> drm core kms structures is an antipattern that all drivers, not just
>>>>> i915, should stop using. In OO terms, you're saying they are classes
>>>>> that should be final and not extended.
>>>>>
>>>>> And even then, to be totally honest, refactoring the structures is not
>>>>> going to be anywhere near the top of our list of things to do, for a
>>>>> very long time.
>>>>
>>>> I may have not expressed myself correctly. There's nothing wrong as such
>>>> in embedded those structures in driver-specific structures (a.k.a. C
>>>> inheritance). That doesn't need to change (albeit for drm_encoder I
>>>> think we should move away from that pattern, but that's an entirely
>>>> different issue, and nothing that needs to be addressed soonà.
>>>>
>>>> The issue here is assuming that every drm_connector instance can be
>>>> up-casted to an i915-specific structure.
>>>
>>> Thinking some more about this, I wonder a way forward could be to drop
>>> the writeback connectors from the connectors list, or at least make them
>>> invisible to drivers. The connectors list is used extensively for two
>>> different purposes: tracking all drm_connector instances, and tracking
>>> all real connectors. The former is mostly needed by the DRM core for
>>> bookkeeping purposes and to expose all drm_connector instances to
>>> userspace, while the latter is also used by drivers, in many cases in
>>> locations that don't expect writeback connectors. Using a drm_connector
>>> to implement writeback isn't something we can revisit, but we could
>>> avoid exposing that to drivers by considering "real" connectors and
>>> writeback connectors two different types of entities in the APIs the DRM
>>> core exposes to drivers. What do you think, would it help for i915 ?
>>
>> Hi Jani and Suraj
>>
>> Since atleast there is agreement on changing the drm_encoder to a
>> pointer in the drm_writeback_connector, can we re-arrange the series OR
>> split it into encoder first and then connector so that atleast those
>> bits can go in first? It will benefit both our (i915 & MSM )
>> implementations.
>>
>> Hi Laurent
>>
>> For the connector part, can you please post a RFC for your proposal?
>> Perhaps myself and Suraj can evaluate our implementations on top of that
>> and the encoder change.
> 
> I'm afraid I won't have time to work on this personally for at least
> several weeks, if not more.
> 
Hi Laurent

Ok sure, I can take this up but I need to understand the proposal a 
little bit more before proceeding on this. So we will discuss this in 
another email where we specifically talk about the connector changes.

Also, I am willing to take this up once the encoder part is done and 
merged so that atleast we keep moving on that as MSM WB implementation 
can proceed with that first.

Hi Jani and Suraj

Any concerns with splitting up the series into encoder and connector OR 
re-arranging them?

Let me know if you can do this OR I can also split this up keeping 
Suraj's name in the Co-developed by tag.

Thanks

Abhinav
>>>>>>>> The point is, you can't initialize a connector or an encoder for a
>>>>>>>> drm_device in isolation of the rest of the driver, even if it were
>>>>>>>> supposed to be hidden away.
> 

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hi Abhinav,
> Hi Laurent
> 
> Ok sure, I can take this up but I need to understand the proposal a little bit
> more before proceeding on this. So we will discuss this in another email
> where we specifically talk about the connector changes.
> 
> Also, I am willing to take this up once the encoder part is done and merged
> so that atleast we keep moving on that as MSM WB implementation can
> proceed with that first.
> 
> Hi Jani and Suraj
> 
> Any concerns with splitting up the series into encoder and connector OR re-
> arranging them?
> 
> Let me know if you can do this OR I can also split this up keeping Suraj's
> name in the Co-developed by tag.
I was actually thinking of floating a new patch series with both encoder and 
connector pointer changes but with a change in initialization functions wherein
we allocate a connector and encoder incase the driver doesn’t have its own this
should minimize the effect on other drivers already using current drm writeback 
framework and also allow i915 to create its own connector.
We can work on Laurent's suggestion but that would first require the initial RFC
patches and then a rework from both of our drivers side to see if they gel well 
with our current code which will take a considerable amount of time. We can for
now however float the patch series up which minimally affects the current drivers
and also allows i915 and msm to move forward with its writeback implementation
off course with a proper documentation stating new drivers shouldn't try to make
their own connectors and encoders and that drm_writeback will do that for them.
Once that's done and merged we can work with Laurent on his proposal to improve 
the drm writeback framework so that this issue can be side stepped entirely in future.
For now I would like to keep the encoder and connector pointer changes together.

Best Regards,
Suraj Kandpal

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

Hi,

On Fri, 4 Mar 2022 at 12:56, Kandpal, Suraj <suraj.kandpal@intel.com> wrote:
>
> Hi Abhinav,
> > Hi Laurent
> >
> > Ok sure, I can take this up but I need to understand the proposal a little bit
> > more before proceeding on this. So we will discuss this in another email
> > where we specifically talk about the connector changes.
> >
> > Also, I am willing to take this up once the encoder part is done and merged
> > so that atleast we keep moving on that as MSM WB implementation can
> > proceed with that first.
> >
> > Hi Jani and Suraj
> >
> > Any concerns with splitting up the series into encoder and connector OR re-
> > arranging them?
> >
> > Let me know if you can do this OR I can also split this up keeping Suraj's
> > name in the Co-developed by tag.
> I was actually thinking of floating a new patch series with both encoder and
> connector pointer changes but with a change in initialization functions wherein
> we allocate a connector and encoder incase the driver doesn’t have its own this
> should minimize the effect on other drivers already using current drm writeback
> framework and also allow i915 to create its own connector.

I thought that Laurent was quite strict about the connector. So I'd
suggest targeting drm_writeback_connector with an optionally created
encoder and the builtin connector

> We can work on Laurent's suggestion but that would first require the initial RFC
> patches and then a rework from both of our drivers side to see if they gel well
> with our current code which will take a considerable amount of time. We can for
> now however float the patch series up which minimally affects the current drivers
> and also allows i915 and msm to move forward with its writeback implementation
> off course with a proper documentation stating new drivers shouldn't try to make
> their own connectors and encoders and that drm_writeback will do that for them.
> Once that's done and merged we can work with Laurent on his proposal to improve
> the drm writeback framework so that this issue can be side stepped entirely in future.
> For now I would like to keep the encoder and connector pointer changes together.


-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hi,
> > Hi Abhinav,
> > > Hi Laurent
> > >
> > > Ok sure, I can take this up but I need to understand the proposal a
> > > little bit more before proceeding on this. So we will discuss this
> > > in another email where we specifically talk about the connector changes.
> > >
> > > Also, I am willing to take this up once the encoder part is done and
> > > merged so that atleast we keep moving on that as MSM WB
> > > implementation can proceed with that first.
> > >
> > > Hi Jani and Suraj
> > >
> > > Any concerns with splitting up the series into encoder and connector
> > > OR re- arranging them?
> > >
> > > Let me know if you can do this OR I can also split this up keeping
> > > Suraj's name in the Co-developed by tag.
> > I was actually thinking of floating a new patch series with both
> > encoder and connector pointer changes but with a change in
> > initialization functions wherein we allocate a connector and encoder
> > incase the driver doesn’t have its own this should minimize the effect
> > on other drivers already using current drm writeback framework and also
> allow i915 to create its own connector.
> 
> I thought that Laurent was quite strict about the connector. So I'd suggest
> targeting drm_writeback_connector with an optionally created encoder and
> the builtin connector
In that case even if we target that i915 will not be able to move forward with its
implementation of writeback as builtin connector does not work for us right now
as explained earlier, optionally creating encoder and connector will help everyone
move forward and once done we can actually sit and work on how to side step this 
issue using Laurent's suggestion
> 
> > We can work on Laurent's suggestion but that would first require the
> > initial RFC patches and then a rework from both of our drivers side to
> > see if they gel well with our current code which will take a
> > considerable amount of time. We can for now however float the patch
> > series up which minimally affects the current drivers and also allows
> > i915 and msm to move forward with its writeback implementation off
> > course with a proper documentation stating new drivers shouldn't try to
> make their own connectors and encoders and that drm_writeback will do
> that for them.
> > Once that's done and merged we can work with Laurent on his proposal
> > to improve the drm writeback framework so that this issue can be side
> stepped entirely in future.
> > For now I would like to keep the encoder and connector pointer changes
> together.

Best Regards,
Suraj Kandpal

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

On Fri, 4 Mar 2022 at 13:47, Kandpal, Suraj <suraj.kandpal@intel.com> wrote:
>
> Hi,
> > > Hi Abhinav,
> > > > Hi Laurent
> > > >
> > > > Ok sure, I can take this up but I need to understand the proposal a
> > > > little bit more before proceeding on this. So we will discuss this
> > > > in another email where we specifically talk about the connector changes.
> > > >
> > > > Also, I am willing to take this up once the encoder part is done and
> > > > merged so that atleast we keep moving on that as MSM WB
> > > > implementation can proceed with that first.
> > > >
> > > > Hi Jani and Suraj
> > > >
> > > > Any concerns with splitting up the series into encoder and connector
> > > > OR re- arranging them?
> > > >
> > > > Let me know if you can do this OR I can also split this up keeping
> > > > Suraj's name in the Co-developed by tag.
> > > I was actually thinking of floating a new patch series with both
> > > encoder and connector pointer changes but with a change in
> > > initialization functions wherein we allocate a connector and encoder
> > > incase the driver doesn’t have its own this should minimize the effect
> > > on other drivers already using current drm writeback framework and also
> > allow i915 to create its own connector.
> >
> > I thought that Laurent was quite strict about the connector. So I'd suggest
> > targeting drm_writeback_connector with an optionally created encoder and
> > the builtin connector
> In that case even if we target that i915 will not be able to move forward with its
> implementation of writeback as builtin connector does not work for us right now
> as explained earlier, optionally creating encoder and connector will help everyone
> move forward and once done we can actually sit and work on how to side step this
> issue using Laurent's suggestion

This is up to Laurent & other DRM maintainers to decide whether this
approach would be accepted or not.
So far unfortunately I have mostly seen the pushback and a strong
suggestion to stop treating each drm_connector as i915_connector.
I haven't checked the exact details, but IMO adding a branch if the
connector's type is DRM_CONNECTOR_VIRTUAL should be doable.

> >
> > > We can work on Laurent's suggestion but that would first require the
> > > initial RFC patches and then a rework from both of our drivers side to
> > > see if they gel well with our current code which will take a
> > > considerable amount of time. We can for now however float the patch
> > > series up which minimally affects the current drivers and also allows
> > > i915 and msm to move forward with its writeback implementation off
> > > course with a proper documentation stating new drivers shouldn't try to
> > make their own connectors and encoders and that drm_writeback will do
> > that for them.
> > > Once that's done and merged we can work with Laurent on his proposal
> > > to improve the drm writeback framework so that this issue can be side
> > stepped entirely in future.
> > > For now I would like to keep the encoder and connector pointer changes
> > together.
>
> Best Regards,
> Suraj Kandpal



-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hi, 
> > Hi,
> > > > Hi Abhinav,
> > > > > Hi Laurent
> > > > >
> > > > > Ok sure, I can take this up but I need to understand the
> > > > > proposal a little bit more before proceeding on this. So we will
> > > > > discuss this in another email where we specifically talk about the
> connector changes.
> > > > >
> > > > > Also, I am willing to take this up once the encoder part is done
> > > > > and merged so that atleast we keep moving on that as MSM WB
> > > > > implementation can proceed with that first.
> > > > >
> > > > > Hi Jani and Suraj
> > > > >
> > > > > Any concerns with splitting up the series into encoder and
> > > > > connector OR re- arranging them?
> > > > >
> > > > > Let me know if you can do this OR I can also split this up
> > > > > keeping Suraj's name in the Co-developed by tag.
> > > > I was actually thinking of floating a new patch series with both
> > > > encoder and connector pointer changes but with a change in
> > > > initialization functions wherein we allocate a connector and
> > > > encoder incase the driver doesn’t have its own this should
> > > > minimize the effect on other drivers already using current drm
> > > > writeback framework and also
> > > allow i915 to create its own connector.
> > >
> > > I thought that Laurent was quite strict about the connector. So I'd
> > > suggest targeting drm_writeback_connector with an optionally created
> > > encoder and the builtin connector
> > In that case even if we target that i915 will not be able to move
> > forward with its implementation of writeback as builtin connector does
> > not work for us right now as explained earlier, optionally creating
> > encoder and connector will help everyone move forward and once done
> we
> > can actually sit and work on how to side step this issue using
> > Laurent's suggestion
> 
> This is up to Laurent & other DRM maintainers to decide whether this
> approach would be accepted or not.
> So far unfortunately I have mostly seen the pushback and a strong
> suggestion to stop treating each drm_connector as i915_connector.
> I haven't checked the exact details, but IMO adding a branch if the
> connector's type is DRM_CONNECTOR_VIRTUAL should be doable.
Bringing in the change where we don’t treat each drm_connector as
an intel_connector or even adding a branch which checks if
drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it
to intel_connector is quite a lot of work for i915.
That's why I was suggesting if for now we could move forward with optionally
creating both encoder and connector minimizing affects on other drivers as
well as allowing us to move forward.
What do you think, Launrent?

> 
> > >
> > > > We can work on Laurent's suggestion but that would first require
> > > > the initial RFC patches and then a rework from both of our drivers
> > > > side to see if they gel well with our current code which will take
> > > > a considerable amount of time. We can for now however float the
> > > > patch series up which minimally affects the current drivers and
> > > > also allows
> > > > i915 and msm to move forward with its writeback implementation off
> > > > course with a proper documentation stating new drivers shouldn't
> > > > try to
> > > make their own connectors and encoders and that drm_writeback will
> > > do that for them.
> > > > Once that's done and merged we can work with Laurent on his
> > > > proposal to improve the drm writeback framework so that this issue
> > > > can be side
> > > stepped entirely in future.
> > > > For now I would like to keep the encoder and connector pointer
> > > > changes
> > > together.
> >
> 
> 
> 
> --
> With best wishes
> Dmitry

Best Regards,
Suraj Kandpal

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Suraj

On 3/4/2022 6:16 AM, Kandpal, Suraj wrote:
> Hi,
>>> Hi,
>>>>> Hi Abhinav,
>>>>>> Hi Laurent
>>>>>>
>>>>>> Ok sure, I can take this up but I need to understand the
>>>>>> proposal a little bit more before proceeding on this. So we will
>>>>>> discuss this in another email where we specifically talk about the
>> connector changes.
>>>>>>
>>>>>> Also, I am willing to take this up once the encoder part is done
>>>>>> and merged so that atleast we keep moving on that as MSM WB
>>>>>> implementation can proceed with that first.
>>>>>>
>>>>>> Hi Jani and Suraj
>>>>>>
>>>>>> Any concerns with splitting up the series into encoder and
>>>>>> connector OR re- arranging them?
>>>>>>
>>>>>> Let me know if you can do this OR I can also split this up
>>>>>> keeping Suraj's name in the Co-developed by tag.
>>>>> I was actually thinking of floating a new patch series with both
>>>>> encoder and connector pointer changes but with a change in
>>>>> initialization functions wherein we allocate a connector and
>>>>> encoder incase the driver doesn’t have its own this should
>>>>> minimize the effect on other drivers already using current drm
>>>>> writeback framework and also
>>>> allow i915 to create its own connector.
>>>>

I was proposing to split up the encoder and connector because the 
comments from Laurent meant we were in agreement about the encoder but 
not about the connector.

I am afraid another attempt with the similar idea is only going to stall 
the series again and hence i gave this option.

Eventually its upto Laurent and the other maintainers to guide us 
forward on this as this series has been stalled for weeks now.

>>>> I thought that Laurent was quite strict about the connector. So I'd
>>>> suggest targeting drm_writeback_connector with an optionally created
>>>> encoder and the builtin connector
>>> In that case even if we target that i915 will not be able to move
>>> forward with its implementation of writeback as builtin connector does
>>> not work for us right now as explained earlier, optionally creating
>>> encoder and connector will help everyone move forward and once done
>> we
>>> can actually sit and work on how to side step this issue using
>>> Laurent's suggestion
>>
>> This is up to Laurent & other DRM maintainers to decide whether this
>> approach would be accepted or not.
>> So far unfortunately I have mostly seen the pushback and a strong
>> suggestion to stop treating each drm_connector as i915_connector.
>> I haven't checked the exact details, but IMO adding a branch if the
>> connector's type is DRM_CONNECTOR_VIRTUAL should be doable.
> Bringing in the change where we don’t treat each drm_connector as
> an intel_connector or even adding a branch which checks if
> drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it
> to intel_connector is quite a lot of work for i915.
> That's why I was suggesting if for now we could move forward with optionally
> creating both encoder and connector minimizing affects on other drivers as
> well as allowing us to move forward.
> What do you think, Launrent?
> 
>>
>>>>
>>>>> We can work on Laurent's suggestion but that would first require
>>>>> the initial RFC patches and then a rework from both of our drivers
>>>>> side to see if they gel well with our current code which will take
>>>>> a considerable amount of time. We can for now however float the
>>>>> patch series up which minimally affects the current drivers and
>>>>> also allows
>>>>> i915 and msm to move forward with its writeback implementation off
>>>>> course with a proper documentation stating new drivers shouldn't
>>>>> try to
>>>> make their own connectors and encoders and that drm_writeback will
>>>> do that for them.
>>>>> Once that's done and merged we can work with Laurent on his
>>>>> proposal to improve the drm writeback framework so that this issue
>>>>> can be side
>>>> stepped entirely in future.
>>>>> For now I would like to keep the encoder and connector pointer
>>>>> changes
>>>> together.
>>>
>>
>>
>>
>> --
>> With best wishes
>> Dmitry
> 
> Best Regards,
> Suraj Kandpal

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hi Abhinav,
> > Hi,
> >>> Hi,
> >>>>> Hi Abhinav,
> >>>>>> Hi Laurent
> >>>>>>
> >>>>>> Ok sure, I can take this up but I need to understand the proposal
> >>>>>> a little bit more before proceeding on this. So we will discuss
> >>>>>> this in another email where we specifically talk about the
> >> connector changes.
> >>>>>>
> >>>>>> Also, I am willing to take this up once the encoder part is done
> >>>>>> and merged so that atleast we keep moving on that as MSM WB
> >>>>>> implementation can proceed with that first.
> >>>>>>
> >>>>>> Hi Jani and Suraj
> >>>>>>
> >>>>>> Any concerns with splitting up the series into encoder and
> >>>>>> connector OR re- arranging them?
> >>>>>>
> >>>>>> Let me know if you can do this OR I can also split this up
> >>>>>> keeping Suraj's name in the Co-developed by tag.
> >>>>> I was actually thinking of floating a new patch series with both
> >>>>> encoder and connector pointer changes but with a change in
> >>>>> initialization functions wherein we allocate a connector and
> >>>>> encoder incase the driver doesn’t have its own this should
> >>>>> minimize the effect on other drivers already using current drm
> >>>>> writeback framework and also
> >>>> allow i915 to create its own connector.
> >>>>
> 
> I was proposing to split up the encoder and connector because the
> comments from Laurent meant we were in agreement about the encoder
> but not about the connector.
> 
> I am afraid another attempt with the similar idea is only going to stall the
> series again and hence i gave this option.

Seems like this patch series currently won't be able to move forward with the
connector pointer change so I guess you can split the series to encoder pointer
change where we optionally create encoder if required ,keeping my name in 
co-developed tag so that msm can move forward with its implementation and
then we can work to see how the connector issue can be bypassed.

Best Regards,
Suraj Kandpal
> Eventually its upto Laurent and the other maintainers to guide us forward on
> this as this series has been stalled for weeks now.
> 
> >>>> I thought that Laurent was quite strict about the connector. So I'd
> >>>> suggest targeting drm_writeback_connector with an optionally
> >>>> created encoder and the builtin connector
> >>> In that case even if we target that i915 will not be able to move
> >>> forward with its implementation of writeback as builtin connector
> >>> does not work for us right now as explained earlier, optionally
> >>> creating encoder and connector will help everyone move forward and
> >>> once done
> >> we
> >>> can actually sit and work on how to side step this issue using
> >>> Laurent's suggestion
> >>
> >> This is up to Laurent & other DRM maintainers to decide whether this
> >> approach would be accepted or not.
> >> So far unfortunately I have mostly seen the pushback and a strong
> >> suggestion to stop treating each drm_connector as i915_connector.
> >> I haven't checked the exact details, but IMO adding a branch if the
> >> connector's type is DRM_CONNECTOR_VIRTUAL should be doable.
> > Bringing in the change where we don’t treat each drm_connector as an
> > intel_connector or even adding a branch which checks if drm_connector
> > is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector
> > is quite a lot of work for i915.
> > That's why I was suggesting if for now we could move forward with
> > optionally creating both encoder and connector minimizing affects on
> > other drivers as well as allowing us to move forward.
> > What do you think, Launrent?
> >
> >>
> >>>>
> >>>>> We can work on Laurent's suggestion but that would first require
> >>>>> the initial RFC patches and then a rework from both of our drivers
> >>>>> side to see if they gel well with our current code which will take
> >>>>> a considerable amount of time. We can for now however float the
> >>>>> patch series up which minimally affects the current drivers and
> >>>>> also allows
> >>>>> i915 and msm to move forward with its writeback implementation off
> >>>>> course with a proper documentation stating new drivers shouldn't
> >>>>> try to
> >>>> make their own connectors and encoders and that drm_writeback will
> >>>> do that for them.
> >>>>> Once that's done and merged we can work with Laurent on his
> >>>>> proposal to improve the drm writeback framework so that this issue
> >>>>> can be side
> >>>> stepped entirely in future.
> >>>>> For now I would like to keep the encoder and connector pointer
> >>>>> changes
> >>>> together.
> >>>
> >>

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Suraj

On 3/8/2022 6:30 AM, Kandpal, Suraj wrote:
> Hi Abhinav,
>>> Hi,
>>>>> Hi,
>>>>>>> Hi Abhinav,
>>>>>>>> Hi Laurent
>>>>>>>>
>>>>>>>> Ok sure, I can take this up but I need to understand the proposal
>>>>>>>> a little bit more before proceeding on this. So we will discuss
>>>>>>>> this in another email where we specifically talk about the
>>>> connector changes.
>>>>>>>>
>>>>>>>> Also, I am willing to take this up once the encoder part is done
>>>>>>>> and merged so that atleast we keep moving on that as MSM WB
>>>>>>>> implementation can proceed with that first.
>>>>>>>>
>>>>>>>> Hi Jani and Suraj
>>>>>>>>
>>>>>>>> Any concerns with splitting up the series into encoder and
>>>>>>>> connector OR re- arranging them?
>>>>>>>>
>>>>>>>> Let me know if you can do this OR I can also split this up
>>>>>>>> keeping Suraj's name in the Co-developed by tag.
>>>>>>> I was actually thinking of floating a new patch series with both
>>>>>>> encoder and connector pointer changes but with a change in
>>>>>>> initialization functions wherein we allocate a connector and
>>>>>>> encoder incase the driver doesn’t have its own this should
>>>>>>> minimize the effect on other drivers already using current drm
>>>>>>> writeback framework and also
>>>>>> allow i915 to create its own connector.
>>>>>>
>>
>> I was proposing to split up the encoder and connector because the
>> comments from Laurent meant we were in agreement about the encoder
>> but not about the connector.
>>
>> I am afraid another attempt with the similar idea is only going to stall the
>> series again and hence i gave this option.
> 
> Seems like this patch series currently won't be able to move forward with the
> connector pointer change so I guess you can split the series to encoder pointer
> change where we optionally create encoder if required ,keeping my name in
> co-developed tag so that msm can move forward with its implementation and
> then we can work to see how the connector issue can be bypassed.
> 
> Best Regards,
> Suraj Kandpal

Thanks, let me re-spin this and will keep your name as co-developer.
Should be able to get it on the list in a week.

Thanks

Abhinav
>> Eventually its upto Laurent and the other maintainers to guide us forward on
>> this as this series has been stalled for weeks now.
>>
>>>>>> I thought that Laurent was quite strict about the connector. So I'd
>>>>>> suggest targeting drm_writeback_connector with an optionally
>>>>>> created encoder and the builtin connector
>>>>> In that case even if we target that i915 will not be able to move
>>>>> forward with its implementation of writeback as builtin connector
>>>>> does not work for us right now as explained earlier, optionally
>>>>> creating encoder and connector will help everyone move forward and
>>>>> once done
>>>> we
>>>>> can actually sit and work on how to side step this issue using
>>>>> Laurent's suggestion
>>>>
>>>> This is up to Laurent & other DRM maintainers to decide whether this
>>>> approach would be accepted or not.
>>>> So far unfortunately I have mostly seen the pushback and a strong
>>>> suggestion to stop treating each drm_connector as i915_connector.
>>>> I haven't checked the exact details, but IMO adding a branch if the
>>>> connector's type is DRM_CONNECTOR_VIRTUAL should be doable.
>>> Bringing in the change where we don’t treat each drm_connector as an
>>> intel_connector or even adding a branch which checks if drm_connector
>>> is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector
>>> is quite a lot of work for i915.
>>> That's why I was suggesting if for now we could move forward with
>>> optionally creating both encoder and connector minimizing affects on
>>> other drivers as well as allowing us to move forward.
>>> What do you think, Launrent?
>>>
>>>>
>>>>>>
>>>>>>> We can work on Laurent's suggestion but that would first require
>>>>>>> the initial RFC patches and then a rework from both of our drivers
>>>>>>> side to see if they gel well with our current code which will take
>>>>>>> a considerable amount of time. We can for now however float the
>>>>>>> patch series up which minimally affects the current drivers and
>>>>>>> also allows
>>>>>>> i915 and msm to move forward with its writeback implementation off
>>>>>>> course with a proper documentation stating new drivers shouldn't
>>>>>>> try to
>>>>>> make their own connectors and encoders and that drm_writeback will
>>>>>> do that for them.
>>>>>>> Once that's done and merged we can work with Laurent on his
>>>>>>> proposal to improve the drm writeback framework so that this issue
>>>>>>> can be side
>>>>>> stepped entirely in future.
>>>>>>> For now I would like to keep the encoder and connector pointer
>>>>>>> changes
>>>>>> together.
>>>>>
>>>>

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jani,
>
> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > >> Changing rcar_du driver to accomadate the change of
> > >> drm_writeback_connector.base and drm_writeback_connector.encoder
> > >> to a pointer the reason for which is explained in the
> > >> Patch(drm: add writeback pointers to drm_connector).
> > >>
> > >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > >>  2 files changed, 7 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> index 66e8839db708..68f387a04502 100644
> > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > >>    const char *const *sources;
> > >>    unsigned int sources_count;
> > >>
> > >> +  struct drm_connector connector;
> > >> +  struct drm_encoder encoder;
> > >
> > > Those fields are, at best, poorly named. Furthermore, there's no need in
> > > this driver or in other drivers using drm_writeback_connector to create
> > > an encoder or connector manually. Let's not polute all drivers because
> > > i915 doesn't have its abstractions right.
> >
> > i915 uses the quite common model for struct inheritance:
> >
> >       struct intel_connector {
> >               struct drm_connector base;
> >               /* ... */
> >       }
> >
> > Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > radeon, tilcdc, and vboxvideo.
> >
> > We could argue about the relative merits of that abstraction, but I
> > think the bottom line is that it's popular and the drivers using it are
> > not going to be persuaded to move away from it.
>
> Nobody said inheritance is bad.
>
> > It's no coincidence that the drivers who've implemented writeback so far
> > (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > because the drm_writeback_connector midlayer does, forcing the issue.
>
> Are you sure it's not a coincidence ? :-)
>
> The encoder and especially connector created by drm_writeback_connector
> are there only because KMS requires a drm_encoder and a drm_connector to
> be exposed to userspace (and I could argue that using a connector for
> writeback is a hack, but that won't change). The connector is "virtual",
> I still fail to see why i915 or any other driver would need to wrap it
> into something else. The whole point of the drm_writeback_connector
> abstraction is that drivers do not have to manage the writeback
> drm_connector manually, they shouldn't touch it at all.

Laurent, I wanted to shift a bit from the question of drm_connector to
the question of drm_encoder being embedded in the
drm_writeback_connector.
In case of the msm driver the drm_encoder is not a lightweight entity,
but a full-featured driver part. Significant part of it can be shared
with the writeback implementation, if we allow using a pointer to the
external drm_encoder with the drm_writeback_connector.
Does the following patch set stand a chance to receive your ack?
 - Switch drm_writeback_connector to point to drm_encoder rather than
embedding it?
 - Create drm_encoder for the drm_writeback_connector when one is not
specified, so the current drivers can be left unchanged.

>
> > So I think drm_writeback_connector should *not* use the inheritance
> > abstraction because it's a midlayer that should leave that option to the
> > drivers. I think drm_writeback_connector needs to be changed to
> > accommodate that, and, unfortunately, it means current writeback users
> > need to be changed as well.
> >
> > I am not sure, however, if the series at hand is the right
> > approach. Perhaps writeback can be modified to allocate the stuff for
> > you if you prefer it that way, as long as the drm_connector is not
> > embedded in struct drm_writeback_connector.
> >
> > > Nack.
> > >
> > >>    struct drm_writeback_connector writeback;
> > >>  };
> > >>
> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >> index c79d1259e49b..5b1e83380c47 100644
> > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >> @@ -200,8 +200,10 @@ 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);
> > >> -  drm_connector_helper_add(&wb_conn->base,
> > >> +  wb_conn->base = &rcrtc->connector;
> > >> +  wb_conn->encoder = &rcrtc->encoder;
> > >> +  wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> > >> +  drm_connector_helper_add(wb_conn->base,
> > >>                             &rcar_du_wb_conn_helper_funcs);
> > >>
> > >>    return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> > >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> > >>    struct drm_framebuffer *fb;
> > >>    unsigned int i;
> > >>
> > >> -  state = rcrtc->writeback.base.state;
> > >> +  state = rcrtc->writeback.base->state;
> > >>    if (!state || !state->writeback_job)
> > >>            return;
> > >>
>
> --
> Regards,
>
> Laurent Pinchart



-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Laurent

On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Jani,
>>
>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>>>>> Changing rcar_du driver to accomadate the change of
>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
>>>>> to a pointer the reason for which is explained in the
>>>>> Patch(drm: add writeback pointers to drm_connector).
>>>>>
>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>> index 66e8839db708..68f387a04502 100644
>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>>>>     const char *const *sources;
>>>>>     unsigned int sources_count;
>>>>>
>>>>> +  struct drm_connector connector;
>>>>> +  struct drm_encoder encoder;
>>>>
>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
>>>> this driver or in other drivers using drm_writeback_connector to create
>>>> an encoder or connector manually. Let's not polute all drivers because
>>>> i915 doesn't have its abstractions right.
>>>
>>> i915 uses the quite common model for struct inheritance:
>>>
>>>        struct intel_connector {
>>>                struct drm_connector base;
>>>                /* ... */
>>>        }
>>>
>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>>> radeon, tilcdc, and vboxvideo.
>>>
>>> We could argue about the relative merits of that abstraction, but I
>>> think the bottom line is that it's popular and the drivers using it are
>>> not going to be persuaded to move away from it.
>>
>> Nobody said inheritance is bad.
>>
>>> It's no coincidence that the drivers who've implemented writeback so far
>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>>> because the drm_writeback_connector midlayer does, forcing the issue.
>>
>> Are you sure it's not a coincidence ? :-)
>>
>> The encoder and especially connector created by drm_writeback_connector
>> are there only because KMS requires a drm_encoder and a drm_connector to
>> be exposed to userspace (and I could argue that using a connector for
>> writeback is a hack, but that won't change). The connector is "virtual",
>> I still fail to see why i915 or any other driver would need to wrap it
>> into something else. The whole point of the drm_writeback_connector
>> abstraction is that drivers do not have to manage the writeback
>> drm_connector manually, they shouldn't touch it at all.
> 
> Laurent, I wanted to shift a bit from the question of drm_connector to
> the question of drm_encoder being embedded in the
> drm_writeback_connector.
> In case of the msm driver the drm_encoder is not a lightweight entity,
> but a full-featured driver part. Significant part of it can be shared
> with the writeback implementation, if we allow using a pointer to the
> external drm_encoder with the drm_writeback_connector.
> Does the following patch set stand a chance to receive your ack?
>   - Switch drm_writeback_connector to point to drm_encoder rather than
> embedding it?
>   - Create drm_encoder for the drm_writeback_connector when one is not
> specified, so the current drivers can be left unchanged.
> 

I second Dmitry's request here. For the reasons he has mentioned along 
with the possibility of the writeback encoder being shared across 
display pipelines, strengthens our request of the drm encoder being a 
pointer inside the drm_writeback_connector instead of embedding it.

Like I had shown in my RFC, in case the other drivers dont specify one,
we can allocate one:

https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/

We think this should be a reasonable accomodation to the existing
drm_writeback driver.

Thanks

Abhinav

>>
>>> So I think drm_writeback_connector should *not* use the inheritance
>>> abstraction because it's a midlayer that should leave that option to the
>>> drivers. I think drm_writeback_connector needs to be changed to
>>> accommodate that, and, unfortunately, it means current writeback users
>>> need to be changed as well.
>>>
>>> I am not sure, however, if the series at hand is the right
>>> approach. Perhaps writeback can be modified to allocate the stuff for
>>> you if you prefer it that way, as long as the drm_connector is not
>>> embedded in struct drm_writeback_connector.
>>>
>>>> Nack.
>>>>
>>>>>     struct drm_writeback_connector writeback;
>>>>>   };
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>> index c79d1259e49b..5b1e83380c47 100644
>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>> @@ -200,8 +200,10 @@ 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);
>>>>> -  drm_connector_helper_add(&wb_conn->base,
>>>>> +  wb_conn->base = &rcrtc->connector;
>>>>> +  wb_conn->encoder = &rcrtc->encoder;
>>>>> +  wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>>> +  drm_connector_helper_add(wb_conn->base,
>>>>>                              &rcar_du_wb_conn_helper_funcs);
>>>>>
>>>>>     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>>>>>     struct drm_framebuffer *fb;
>>>>>     unsigned int i;
>>>>>
>>>>> -  state = rcrtc->writeback.base.state;
>>>>> +  state = rcrtc->writeback.base->state;
>>>>>     if (!state || !state->writeback_job)
>>>>>             return;
>>>>>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
> 

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Laurent

Gentle reminder on this.

Thanks

Abhinav

On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
> Hi Laurent
> 
> On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
>> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>>
>>> Hi Jani,
>>>
>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>>>>>> Changing rcar_du driver to accomadate the change of
>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
>>>>>> to a pointer the reason for which is explained in the
>>>>>> Patch(drm: add writeback pointers to drm_connector).
>>>>>>
>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h 
>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>> index 66e8839db708..68f387a04502 100644
>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>>>>>     const char *const *sources;
>>>>>>     unsigned int sources_count;
>>>>>>
>>>>>> +  struct drm_connector connector;
>>>>>> +  struct drm_encoder encoder;
>>>>>
>>>>> Those fields are, at best, poorly named. Furthermore, there's no 
>>>>> need in
>>>>> this driver or in other drivers using drm_writeback_connector to 
>>>>> create
>>>>> an encoder or connector manually. Let's not polute all drivers because
>>>>> i915 doesn't have its abstractions right.
>>>>
>>>> i915 uses the quite common model for struct inheritance:
>>>>
>>>>        struct intel_connector {
>>>>                struct drm_connector base;
>>>>                /* ... */
>>>>        }
>>>>
>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>>>> radeon, tilcdc, and vboxvideo.
>>>>
>>>> We could argue about the relative merits of that abstraction, but I
>>>> think the bottom line is that it's popular and the drivers using it are
>>>> not going to be persuaded to move away from it.
>>>
>>> Nobody said inheritance is bad.
>>>
>>>> It's no coincidence that the drivers who've implemented writeback so 
>>>> far
>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>>>> because the drm_writeback_connector midlayer does, forcing the issue.
>>>
>>> Are you sure it's not a coincidence ? :-)
>>>
>>> The encoder and especially connector created by drm_writeback_connector
>>> are there only because KMS requires a drm_encoder and a drm_connector to
>>> be exposed to userspace (and I could argue that using a connector for
>>> writeback is a hack, but that won't change). The connector is "virtual",
>>> I still fail to see why i915 or any other driver would need to wrap it
>>> into something else. The whole point of the drm_writeback_connector
>>> abstraction is that drivers do not have to manage the writeback
>>> drm_connector manually, they shouldn't touch it at all.
>>
>> Laurent, I wanted to shift a bit from the question of drm_connector to
>> the question of drm_encoder being embedded in the
>> drm_writeback_connector.
>> In case of the msm driver the drm_encoder is not a lightweight entity,
>> but a full-featured driver part. Significant part of it can be shared
>> with the writeback implementation, if we allow using a pointer to the
>> external drm_encoder with the drm_writeback_connector.
>> Does the following patch set stand a chance to receive your ack?
>>   - Switch drm_writeback_connector to point to drm_encoder rather than
>> embedding it?
>>   - Create drm_encoder for the drm_writeback_connector when one is not
>> specified, so the current drivers can be left unchanged.
>>
> 
> I second Dmitry's request here. For the reasons he has mentioned along 
> with the possibility of the writeback encoder being shared across 
> display pipelines, strengthens our request of the drm encoder being a 
> pointer inside the drm_writeback_connector instead of embedding it.
> 
> Like I had shown in my RFC, in case the other drivers dont specify one,
> we can allocate one:
> 
> https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ 
> 
> 
> We think this should be a reasonable accomodation to the existing
> drm_writeback driver.
> 
> Thanks
> 
> Abhinav
> 
>>>
>>>> So I think drm_writeback_connector should *not* use the inheritance
>>>> abstraction because it's a midlayer that should leave that option to 
>>>> the
>>>> drivers. I think drm_writeback_connector needs to be changed to
>>>> accommodate that, and, unfortunately, it means current writeback users
>>>> need to be changed as well.
>>>>
>>>> I am not sure, however, if the series at hand is the right
>>>> approach. Perhaps writeback can be modified to allocate the stuff for
>>>> you if you prefer it that way, as long as the drm_connector is not
>>>> embedded in struct drm_writeback_connector.
>>>>
>>>>> Nack.
>>>>>
>>>>>>     struct drm_writeback_connector writeback;
>>>>>>   };
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c 
>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>> index c79d1259e49b..5b1e83380c47 100644
>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>> @@ -200,8 +200,10 @@ 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);
>>>>>> -  drm_connector_helper_add(&wb_conn->base,
>>>>>> +  wb_conn->base = &rcrtc->connector;
>>>>>> +  wb_conn->encoder = &rcrtc->encoder;
>>>>>> +  wb_conn->encoder->possible_crtcs = 1 << 
>>>>>> drm_crtc_index(&rcrtc->crtc);
>>>>>> +  drm_connector_helper_add(wb_conn->base,
>>>>>>                              &rcar_du_wb_conn_helper_funcs);
>>>>>>
>>>>>>     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct 
>>>>>> rcar_du_crtc *rcrtc,
>>>>>>     struct drm_framebuffer *fb;
>>>>>>     unsigned int i;
>>>>>>
>>>>>> -  state = rcrtc->writeback.base.state;
>>>>>> +  state = rcrtc->writeback.base->state;
>>>>>>     if (!state || !state->writeback_job)
>>>>>>             return;
>>>>>>
>>>
>>> -- 
>>> Regards,
>>>
>>> Laurent Pinchart
>>
>>
>>

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Abhinav,

On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
> Hi Laurent
> 
> Gentle reminder on this.

I won't have time before next week I'm afraid.

> On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
> > Hi Laurent
> > 
> > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
> >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
> >> <laurent.pinchart@ideasonboard.com> wrote:
> >>>
> >>> Hi Jani,
> >>>
> >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >>>>>> Changing rcar_du driver to accomadate the change of
> >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
> >>>>>> to a pointer the reason for which is explained in the
> >>>>>> Patch(drm: add writeback pointers to drm_connector).
> >>>>>>
> >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h 
> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>> index 66e8839db708..68f387a04502 100644
> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >>>>>>     const char *const *sources;
> >>>>>>     unsigned int sources_count;
> >>>>>>
> >>>>>> +  struct drm_connector connector;
> >>>>>> +  struct drm_encoder encoder;
> >>>>>
> >>>>> Those fields are, at best, poorly named. Furthermore, there's no 
> >>>>> need in
> >>>>> this driver or in other drivers using drm_writeback_connector to 
> >>>>> create
> >>>>> an encoder or connector manually. Let's not polute all drivers because
> >>>>> i915 doesn't have its abstractions right.
> >>>>
> >>>> i915 uses the quite common model for struct inheritance:
> >>>>
> >>>>        struct intel_connector {
> >>>>                struct drm_connector base;
> >>>>                /* ... */
> >>>>        }
> >>>>
> >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> >>>> radeon, tilcdc, and vboxvideo.
> >>>>
> >>>> We could argue about the relative merits of that abstraction, but I
> >>>> think the bottom line is that it's popular and the drivers using it are
> >>>> not going to be persuaded to move away from it.
> >>>
> >>> Nobody said inheritance is bad.
> >>>
> >>>> It's no coincidence that the drivers who've implemented writeback so 
> >>>> far
> >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> >>>> because the drm_writeback_connector midlayer does, forcing the issue.
> >>>
> >>> Are you sure it's not a coincidence ? :-)
> >>>
> >>> The encoder and especially connector created by drm_writeback_connector
> >>> are there only because KMS requires a drm_encoder and a drm_connector to
> >>> be exposed to userspace (and I could argue that using a connector for
> >>> writeback is a hack, but that won't change). The connector is "virtual",
> >>> I still fail to see why i915 or any other driver would need to wrap it
> >>> into something else. The whole point of the drm_writeback_connector
> >>> abstraction is that drivers do not have to manage the writeback
> >>> drm_connector manually, they shouldn't touch it at all.
> >>
> >> Laurent, I wanted to shift a bit from the question of drm_connector to
> >> the question of drm_encoder being embedded in the
> >> drm_writeback_connector.
> >> In case of the msm driver the drm_encoder is not a lightweight entity,
> >> but a full-featured driver part. Significant part of it can be shared
> >> with the writeback implementation, if we allow using a pointer to the
> >> external drm_encoder with the drm_writeback_connector.
> >> Does the following patch set stand a chance to receive your ack?
> >>   - Switch drm_writeback_connector to point to drm_encoder rather than
> >> embedding it?
> >>   - Create drm_encoder for the drm_writeback_connector when one is not
> >> specified, so the current drivers can be left unchanged.
> >>
> > 
> > I second Dmitry's request here. For the reasons he has mentioned along 
> > with the possibility of the writeback encoder being shared across 
> > display pipelines, strengthens our request of the drm encoder being a 
> > pointer inside the drm_writeback_connector instead of embedding it.
> > 
> > Like I had shown in my RFC, in case the other drivers dont specify one,
> > we can allocate one:
> > 
> > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ 
> > 
> > 
> > We think this should be a reasonable accomodation to the existing
> > drm_writeback driver.
> > 
> > Thanks
> > 
> > Abhinav
> > 
> >>>
> >>>> So I think drm_writeback_connector should *not* use the inheritance
> >>>> abstraction because it's a midlayer that should leave that option to 
> >>>> the
> >>>> drivers. I think drm_writeback_connector needs to be changed to
> >>>> accommodate that, and, unfortunately, it means current writeback users
> >>>> need to be changed as well.
> >>>>
> >>>> I am not sure, however, if the series at hand is the right
> >>>> approach. Perhaps writeback can be modified to allocate the stuff for
> >>>> you if you prefer it that way, as long as the drm_connector is not
> >>>> embedded in struct drm_writeback_connector.
> >>>>
> >>>>> Nack.
> >>>>>
> >>>>>>     struct drm_writeback_connector writeback;
> >>>>>>   };
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c 
> >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >>>>>> index c79d1259e49b..5b1e83380c47 100644
> >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >>>>>> @@ -200,8 +200,10 @@ 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);
> >>>>>> -  drm_connector_helper_add(&wb_conn->base,
> >>>>>> +  wb_conn->base = &rcrtc->connector;
> >>>>>> +  wb_conn->encoder = &rcrtc->encoder;
> >>>>>> +  wb_conn->encoder->possible_crtcs = 1 << 
> >>>>>> drm_crtc_index(&rcrtc->crtc);
> >>>>>> +  drm_connector_helper_add(wb_conn->base,
> >>>>>>                              &rcar_du_wb_conn_helper_funcs);
> >>>>>>
> >>>>>>     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct 
> >>>>>> rcar_du_crtc *rcrtc,
> >>>>>>     struct drm_framebuffer *fb;
> >>>>>>     unsigned int i;
> >>>>>>
> >>>>>> -  state = rcrtc->writeback.base.state;
> >>>>>> +  state = rcrtc->writeback.base->state;
> >>>>>>     if (!state || !state->writeback_job)
> >>>>>>             return;
> >>>>>>

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Abhinav,
>
> On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
> > Hi Laurent
> >
> > Gentle reminder on this.
>
> I won't have time before next week I'm afraid.

Laurent, another gentle ping.

>
> > On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
> > > Hi Laurent
> > >
> > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
> > >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
> > >> <laurent.pinchart@ideasonboard.com> wrote:
> > >>>
> > >>> Hi Jani,
> > >>>
> > >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > >>>>>> Changing rcar_du driver to accomadate the change of
> > >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
> > >>>>>> to a pointer the reason for which is explained in the
> > >>>>>> Patch(drm: add writeback pointers to drm_connector).
> > >>>>>>
> > >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > >>>>>> ---
> > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > >>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >>>>>> index 66e8839db708..68f387a04502 100644
> > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > >>>>>>     const char *const *sources;
> > >>>>>>     unsigned int sources_count;
> > >>>>>>
> > >>>>>> +  struct drm_connector connector;
> > >>>>>> +  struct drm_encoder encoder;
> > >>>>>
> > >>>>> Those fields are, at best, poorly named. Furthermore, there's no
> > >>>>> need in
> > >>>>> this driver or in other drivers using drm_writeback_connector to
> > >>>>> create
> > >>>>> an encoder or connector manually. Let's not polute all drivers because
> > >>>>> i915 doesn't have its abstractions right.
> > >>>>
> > >>>> i915 uses the quite common model for struct inheritance:
> > >>>>
> > >>>>        struct intel_connector {
> > >>>>                struct drm_connector base;
> > >>>>                /* ... */
> > >>>>        }
> > >>>>
> > >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > >>>> radeon, tilcdc, and vboxvideo.
> > >>>>
> > >>>> We could argue about the relative merits of that abstraction, but I
> > >>>> think the bottom line is that it's popular and the drivers using it are
> > >>>> not going to be persuaded to move away from it.
> > >>>
> > >>> Nobody said inheritance is bad.
> > >>>
> > >>>> It's no coincidence that the drivers who've implemented writeback so
> > >>>> far
> > >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > >>>> because the drm_writeback_connector midlayer does, forcing the issue.
> > >>>
> > >>> Are you sure it's not a coincidence ? :-)
> > >>>
> > >>> The encoder and especially connector created by drm_writeback_connector
> > >>> are there only because KMS requires a drm_encoder and a drm_connector to
> > >>> be exposed to userspace (and I could argue that using a connector for
> > >>> writeback is a hack, but that won't change). The connector is "virtual",
> > >>> I still fail to see why i915 or any other driver would need to wrap it
> > >>> into something else. The whole point of the drm_writeback_connector
> > >>> abstraction is that drivers do not have to manage the writeback
> > >>> drm_connector manually, they shouldn't touch it at all.
> > >>
> > >> Laurent, I wanted to shift a bit from the question of drm_connector to
> > >> the question of drm_encoder being embedded in the
> > >> drm_writeback_connector.
> > >> In case of the msm driver the drm_encoder is not a lightweight entity,
> > >> but a full-featured driver part. Significant part of it can be shared
> > >> with the writeback implementation, if we allow using a pointer to the
> > >> external drm_encoder with the drm_writeback_connector.
> > >> Does the following patch set stand a chance to receive your ack?
> > >>   - Switch drm_writeback_connector to point to drm_encoder rather than
> > >> embedding it?
> > >>   - Create drm_encoder for the drm_writeback_connector when one is not
> > >> specified, so the current drivers can be left unchanged.
> > >>
> > >
> > > I second Dmitry's request here. For the reasons he has mentioned along
> > > with the possibility of the writeback encoder being shared across
> > > display pipelines, strengthens our request of the drm encoder being a
> > > pointer inside the drm_writeback_connector instead of embedding it.
> > >
> > > Like I had shown in my RFC, in case the other drivers dont specify one,
> > > we can allocate one:
> > >
> > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/
> > >
> > >
> > > We think this should be a reasonable accomodation to the existing
> > > drm_writeback driver.
> > >
> > > Thanks
> > >
> > > Abhinav
> > >
> > >>>
> > >>>> So I think drm_writeback_connector should *not* use the inheritance
> > >>>> abstraction because it's a midlayer that should leave that option to
> > >>>> the
> > >>>> drivers. I think drm_writeback_connector needs to be changed to
> > >>>> accommodate that, and, unfortunately, it means current writeback users
> > >>>> need to be changed as well.
> > >>>>
> > >>>> I am not sure, however, if the series at hand is the right
> > >>>> approach. Perhaps writeback can be modified to allocate the stuff for
> > >>>> you if you prefer it that way, as long as the drm_connector is not
> > >>>> embedded in struct drm_writeback_connector.
> > >>>>
> > >>>>> Nack.
> > >>>>>
> > >>>>>>     struct drm_writeback_connector writeback;
> > >>>>>>   };
> > >>>>>>
> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >>>>>> index c79d1259e49b..5b1e83380c47 100644
> > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > >>>>>> @@ -200,8 +200,10 @@ 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);
> > >>>>>> -  drm_connector_helper_add(&wb_conn->base,
> > >>>>>> +  wb_conn->base = &rcrtc->connector;
> > >>>>>> +  wb_conn->encoder = &rcrtc->encoder;
> > >>>>>> +  wb_conn->encoder->possible_crtcs = 1 <<
> > >>>>>> drm_crtc_index(&rcrtc->crtc);
> > >>>>>> +  drm_connector_helper_add(wb_conn->base,
> > >>>>>>                              &rcar_du_wb_conn_helper_funcs);
> > >>>>>>
> > >>>>>>     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> > >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct
> > >>>>>> rcar_du_crtc *rcrtc,
> > >>>>>>     struct drm_framebuffer *fb;
> > >>>>>>     unsigned int i;
> > >>>>>>
> > >>>>>> -  state = rcrtc->writeback.base.state;
> > >>>>>> +  state = rcrtc->writeback.base->state;
> > >>>>>>     if (!state || !state->writeback_job)
> > >>>>>>             return;
> > >>>>>>
>
> --
> Regards,
>
> Laurent Pinchart



-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Dmitry,

On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote:
> On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote:
> > On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
> > > Hi Laurent
> > >
> > > Gentle reminder on this.
> >
> > I won't have time before next week I'm afraid.
> 
> Laurent, another gentle ping.

I'm really late on this so I probably deserve a bit of a rougher ping,
but thanks for being gentle :-)

> > > On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
> > > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
> > > >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote:
> > > >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> > > >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
> > > >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> > > >>>>>> Changing rcar_du driver to accomadate the change of
> > > >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
> > > >>>>>> to a pointer the reason for which is explained in the
> > > >>>>>> Patch(drm: add writeback pointers to drm_connector).
> > > >>>>>>
> > > >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > > >>>>>> ---
> > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> > > >>>>>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> > > >>>>>>   2 files changed, 7 insertions(+), 3 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > > >>>>>> index 66e8839db708..68f387a04502 100644
> > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> > > >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> > > >>>>>>     const char *const *sources;
> > > >>>>>>     unsigned int sources_count;
> > > >>>>>>
> > > >>>>>> +  struct drm_connector connector;
> > > >>>>>> +  struct drm_encoder encoder;
> > > >>>>>
> > > >>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
> > > >>>>> this driver or in other drivers using drm_writeback_connector to create
> > > >>>>> an encoder or connector manually. Let's not polute all drivers because
> > > >>>>> i915 doesn't have its abstractions right.
> > > >>>>
> > > >>>> i915 uses the quite common model for struct inheritance:
> > > >>>>
> > > >>>>        struct intel_connector {
> > > >>>>                struct drm_connector base;
> > > >>>>                /* ... */
> > > >>>>        }
> > > >>>>
> > > >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> > > >>>> radeon, tilcdc, and vboxvideo.
> > > >>>>
> > > >>>> We could argue about the relative merits of that abstraction, but I
> > > >>>> think the bottom line is that it's popular and the drivers using it are
> > > >>>> not going to be persuaded to move away from it.
> > > >>>
> > > >>> Nobody said inheritance is bad.
> > > >>>
> > > >>>> It's no coincidence that the drivers who've implemented writeback so far
> > > >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> > > >>>> because the drm_writeback_connector midlayer does, forcing the issue.
> > > >>>
> > > >>> Are you sure it's not a coincidence ? :-)
> > > >>>
> > > >>> The encoder and especially connector created by drm_writeback_connector
> > > >>> are there only because KMS requires a drm_encoder and a drm_connector to
> > > >>> be exposed to userspace (and I could argue that using a connector for
> > > >>> writeback is a hack, but that won't change). The connector is "virtual",
> > > >>> I still fail to see why i915 or any other driver would need to wrap it
> > > >>> into something else. The whole point of the drm_writeback_connector
> > > >>> abstraction is that drivers do not have to manage the writeback
> > > >>> drm_connector manually, they shouldn't touch it at all.
> > > >>
> > > >> Laurent, I wanted to shift a bit from the question of drm_connector to
> > > >> the question of drm_encoder being embedded in the drm_writeback_connector.
> > > >> In case of the msm driver the drm_encoder is not a lightweight entity,
> > > >> but a full-featured driver part. Significant part of it can be shared
> > > >> with the writeback implementation, if we allow using a pointer to the
> > > >> external drm_encoder with the drm_writeback_connector.
> > > >> Does the following patch set stand a chance to receive your ack?
> > > >>   - Switch drm_writeback_connector to point to drm_encoder rather than
> > > >> embedding it?
> > > >>   - Create drm_encoder for the drm_writeback_connector when one is not
> > > >> specified, so the current drivers can be left unchanged.

The situation is a bit different for the encoder indeed.

The encoder concept is loosely defined nowadays, with more and more of
the "real" encoders being implemented as a drm_bridge. That's what I
usually recommend when reviewing new drivers. drm_encoder is slowly
becoming an empty shell (see for instance [1]), although that transition
is not enforced globally and will thus take a long time to complete (if
ever).

This being said, lots of drivers have "featureful" encoder
implementations, and that won't go away any time soon. In those cases, I
could be OK with drivers optionally passing an encoder fo the writeback
helper if the hardware really shares resources between writeback and a
real encoder. I would however be careful there, as in many cases I would
expect the need to pass a custom encoder to originate from an old
software design decision rather than from the hardware architecture. In
those cases it would be best, I think, to move towards cleaning up the
software architecture, but that can be done step by step and I won't
consider that a requirement to implement writeback support.

In the MSM case in particular, can you explain what resources are shared
between writeback and hardware encoder(s) ?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rcar-du/rcar_du_encoder.c

> > > > I second Dmitry's request here. For the reasons he has mentioned along
> > > > with the possibility of the writeback encoder being shared across
> > > > display pipelines, strengthens our request of the drm encoder being a
> > > > pointer inside the drm_writeback_connector instead of embedding it.
> > > >
> > > > Like I had shown in my RFC, in case the other drivers dont specify one,
> > > > we can allocate one:
> > > >
> > > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/
> > > >
> > > > We think this should be a reasonable accomodation to the existing
> > > > drm_writeback driver.
> > > >
> > > >>>> So I think drm_writeback_connector should *not* use the inheritance
> > > >>>> abstraction because it's a midlayer that should leave that option tothe
> > > >>>> drivers. I think drm_writeback_connector needs to be changed to
> > > >>>> accommodate that, and, unfortunately, it means current writeback users
> > > >>>> need to be changed as well.
> > > >>>>
> > > >>>> I am not sure, however, if the series at hand is the right
> > > >>>> approach. Perhaps writeback can be modified to allocate the stuff for
> > > >>>> you if you prefer it that way, as long as the drm_connector is not
> > > >>>> embedded in struct drm_writeback_connector.
> > > >>>>
> > > >>>>> Nack.
> > > >>>>>
> > > >>>>>>     struct drm_writeback_connector writeback;
> > > >>>>>>   };
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > >>>>>> index c79d1259e49b..5b1e83380c47 100644
> > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > >>>>>> @@ -200,8 +200,10 @@ 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);
> > > >>>>>> -  drm_connector_helper_add(&wb_conn->base,
> > > >>>>>> +  wb_conn->base = &rcrtc->connector;
> > > >>>>>> +  wb_conn->encoder = &rcrtc->encoder;
> > > >>>>>> +  wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> > > >>>>>> +  drm_connector_helper_add(wb_conn->base,
> > > >>>>>>                              &rcar_du_wb_conn_helper_funcs);
> > > >>>>>>
> > > >>>>>>     return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> > > >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> > > >>>>>>     struct drm_framebuffer *fb;
> > > >>>>>>     unsigned int i;
> > > >>>>>>
> > > >>>>>> -  state = rcrtc->writeback.base.state;
> > > >>>>>> +  state = rcrtc->writeback.base->state;
> > > >>>>>>     if (!state || !state->writeback_job)
> > > >>>>>>             return;
> > > >>>>>>

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Laurent

Thanks for responding.

On 2/21/2022 11:34 PM, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote:
>> On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote:
>>> On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
>>>> Hi Laurent
>>>>
>>>> Gentle reminder on this.
>>>
>>> I won't have time before next week I'm afraid.
>>
>> Laurent, another gentle ping.
> 
> I'm really late on this so I probably deserve a bit of a rougher ping,
> but thanks for being gentle :-)
> 
>>>> On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
>>>>> On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote:
>>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
>>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote:
>>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>>>>>>>>>> Changing rcar_du driver to accomadate the change of
>>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder
>>>>>>>>>> to a pointer the reason for which is explained in the
>>>>>>>>>> Patch(drm: add writeback pointers to drm_connector).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>    drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>>>>>>>>>>    drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>>>>>>>>>>    2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> index 66e8839db708..68f387a04502 100644
>>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>>>>>>>>>>      const char *const *sources;
>>>>>>>>>>      unsigned int sources_count;
>>>>>>>>>>
>>>>>>>>>> +  struct drm_connector connector;
>>>>>>>>>> +  struct drm_encoder encoder;
>>>>>>>>>
>>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in
>>>>>>>>> this driver or in other drivers using drm_writeback_connector to create
>>>>>>>>> an encoder or connector manually. Let's not polute all drivers because
>>>>>>>>> i915 doesn't have its abstractions right.
>>>>>>>>
>>>>>>>> i915 uses the quite common model for struct inheritance:
>>>>>>>>
>>>>>>>>         struct intel_connector {
>>>>>>>>                 struct drm_connector base;
>>>>>>>>                 /* ... */
>>>>>>>>         }
>>>>>>>>
>>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>>>>>>>> radeon, tilcdc, and vboxvideo.
>>>>>>>>
>>>>>>>> We could argue about the relative merits of that abstraction, but I
>>>>>>>> think the bottom line is that it's popular and the drivers using it are
>>>>>>>> not going to be persuaded to move away from it.
>>>>>>>
>>>>>>> Nobody said inheritance is bad.
>>>>>>>
>>>>>>>> It's no coincidence that the drivers who've implemented writeback so far
>>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue.
>>>>>>>
>>>>>>> Are you sure it's not a coincidence ? :-)
>>>>>>>
>>>>>>> The encoder and especially connector created by drm_writeback_connector
>>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to
>>>>>>> be exposed to userspace (and I could argue that using a connector for
>>>>>>> writeback is a hack, but that won't change). The connector is "virtual",
>>>>>>> I still fail to see why i915 or any other driver would need to wrap it
>>>>>>> into something else. The whole point of the drm_writeback_connector
>>>>>>> abstraction is that drivers do not have to manage the writeback
>>>>>>> drm_connector manually, they shouldn't touch it at all.
>>>>>>
>>>>>> Laurent, I wanted to shift a bit from the question of drm_connector to
>>>>>> the question of drm_encoder being embedded in the drm_writeback_connector.
>>>>>> In case of the msm driver the drm_encoder is not a lightweight entity,
>>>>>> but a full-featured driver part. Significant part of it can be shared
>>>>>> with the writeback implementation, if we allow using a pointer to the
>>>>>> external drm_encoder with the drm_writeback_connector.
>>>>>> Does the following patch set stand a chance to receive your ack?
>>>>>>    - Switch drm_writeback_connector to point to drm_encoder rather than
>>>>>> embedding it?
>>>>>>    - Create drm_encoder for the drm_writeback_connector when one is not
>>>>>> specified, so the current drivers can be left unchanged.
> 
> The situation is a bit different for the encoder indeed.
> 
> The encoder concept is loosely defined nowadays, with more and more of
> the "real" encoders being implemented as a drm_bridge. That's what I
> usually recommend when reviewing new drivers. drm_encoder is slowly
> becoming an empty shell (see for instance [1]), although that transition
> is not enforced globally and will thus take a long time to complete (if
> ever).
> 
> This being said, lots of drivers have "featureful" encoder
> implementations, and that won't go away any time soon. In those cases, I
> could be OK with drivers optionally passing an encoder fo the writeback
> helper if the hardware really shares resources between writeback and a
> real encoder. I would however be careful there, as in many cases I would
> expect the need to pass a custom encoder to originate from an old
> software design decision rather than from the hardware architecture. In
> those cases it would be best, I think, to move towards cleaning up the
> software architecture, but that can be done step by step and I won't
> consider that a requirement to implement writeback support.
> 
> In the MSM case in particular, can you explain what resources are shared
> between writeback and hardware encoder(s) ?
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> 

Yes, we indeed have a lot of functionality in our encoder. It shares 
both interrupt and clock control for all interfaces including writeback.

Moreover, like I was mentioning earlier, on some of the chipsets where 
display hardware is limited, the hardware components mapped to a drm 
encoder can be shared between the panel and writeback paths.

For your reference, please check [1]

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#n174

Hence we are requesting that drm_writeback not embed an encoder but 
acommodate a pointer to drm_encoder instead.

>>>>> I second Dmitry's request here. For the reasons he has mentioned along
>>>>> with the possibility of the writeback encoder being shared across
>>>>> display pipelines, strengthens our request of the drm encoder being a
>>>>> pointer inside the drm_writeback_connector instead of embedding it.
>>>>>
>>>>> Like I had shown in my RFC, in case the other drivers dont specify one,
>>>>> we can allocate one:
>>>>>
>>>>> https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/
>>>>>
>>>>> We think this should be a reasonable accomodation to the existing
>>>>> drm_writeback driver.
>>>>>
>>>>>>>> So I think drm_writeback_connector should *not* use the inheritance
>>>>>>>> abstraction because it's a midlayer that should leave that option tothe
>>>>>>>> drivers. I think drm_writeback_connector needs to be changed to
>>>>>>>> accommodate that, and, unfortunately, it means current writeback users
>>>>>>>> need to be changed as well.
>>>>>>>>
>>>>>>>> I am not sure, however, if the series at hand is the right
>>>>>>>> approach. Perhaps writeback can be modified to allocate the stuff for
>>>>>>>> you if you prefer it that way, as long as the drm_connector is not
>>>>>>>> embedded in struct drm_writeback_connector.
>>>>>>>>
>>>>>>>>> Nack.
>>>>>>>>>
>>>>>>>>>>      struct drm_writeback_connector writeback;
>>>>>>>>>>    };
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>>>>>> index c79d1259e49b..5b1e83380c47 100644
>>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>>>>>>>> @@ -200,8 +200,10 @@ 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);
>>>>>>>>>> -  drm_connector_helper_add(&wb_conn->base,
>>>>>>>>>> +  wb_conn->base = &rcrtc->connector;
>>>>>>>>>> +  wb_conn->encoder = &rcrtc->encoder;
>>>>>>>>>> +  wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>>>>>>>> +  drm_connector_helper_add(wb_conn->base,
>>>>>>>>>>                               &rcar_du_wb_conn_helper_funcs);
>>>>>>>>>>
>>>>>>>>>>      return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
>>>>>>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>>>>>>>>>>      struct drm_framebuffer *fb;
>>>>>>>>>>      unsigned int i;
>>>>>>>>>>
>>>>>>>>>> -  state = rcrtc->writeback.base.state;
>>>>>>>>>> +  state = rcrtc->writeback.base->state;
>>>>>>>>>>      if (!state || !state->writeback_job)
>>>>>>>>>>              return;
>>>>>>>>>>
> 

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Ville Syrjälä]

On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
> On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Kandpal,
> >
> > Thank you for the patch.
> >
> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >> Changing rcar_du driver to accomadate the change of
> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> >> to a pointer the reason for which is explained in the
> >> Patch(drm: add writeback pointers to drm_connector).
> >> 
> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> index 66e8839db708..68f387a04502 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >>  	const char *const *sources;
> >>  	unsigned int sources_count;
> >>  
> >> +	struct drm_connector connector;
> >> +	struct drm_encoder encoder;
> >
> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > this driver or in other drivers using drm_writeback_connector to create
> > an encoder or connector manually. Let's not polute all drivers because
> > i915 doesn't have its abstractions right.
> 
> i915 uses the quite common model for struct inheritance:
> 
> 	struct intel_connector {
> 		struct drm_connector base;
> 		/* ... */
> 	}
> 
> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> radeon, tilcdc, and vboxvideo.
> 
> We could argue about the relative merits of that abstraction, but I
> think the bottom line is that it's popular and the drivers using it are
> not going to be persuaded to move away from it.
> 
> It's no coincidence that the drivers who've implemented writeback so far
> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> because the drm_writeback_connector midlayer does, forcing the issue.
> 
> So I think drm_writeback_connector should *not* use the inheritance
> abstraction because it's a midlayer that should leave that option to the
> drivers. I think drm_writeback_connector needs to be changed to
> accommodate that, and, unfortunately, it means current writeback users
> need to be changed as well.
> 
> I am not sure, however, if the series at hand is the right
> approach. Perhaps writeback can be modified to allocate the stuff for
> you if you prefer it that way, as long as the drm_connector is not
> embedded in struct drm_writeback_connector.

Maybe it's possible to split out the actual writeback functionality
into a separate lighter struct that then gets embedded into
the current drm_writeback_connector (which would therefore remain
as a midlayer for the drivers that want one). And other drivers
can embed the core writeback struct directly into their own things.

Something like that should perhaps minimize the driver changes for
the current users and we just need to adjust a bunch of things in
drm_writeback.c/etc. to not depend on the midlayer stuff.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

On Wed, 2 Feb 2022 at 16:15, Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Kandpal,
> >
> > Thank you for the patch.
> >
> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
> >> Changing rcar_du driver to accomadate the change of
> >> drm_writeback_connector.base and drm_writeback_connector.encoder
> >> to a pointer the reason for which is explained in the
> >> Patch(drm: add writeback pointers to drm_connector).
> >>
> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> index 66e8839db708..68f387a04502 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
> >>      const char *const *sources;
> >>      unsigned int sources_count;
> >>
> >> +    struct drm_connector connector;
> >> +    struct drm_encoder encoder;
> >
> > Those fields are, at best, poorly named. Furthermore, there's no need in
> > this driver or in other drivers using drm_writeback_connector to create
> > an encoder or connector manually. Let's not polute all drivers because
> > i915 doesn't have its abstractions right.
>
> i915 uses the quite common model for struct inheritance:
>
>         struct intel_connector {
>                 struct drm_connector base;
>                 /* ... */
>         }
>
> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
> radeon, tilcdc, and vboxvideo.

For the reference. msm does not wrap drm_connector into any _common_
structure, which is used internally.

> We could argue about the relative merits of that abstraction, but I
> think the bottom line is that it's popular and the drivers using it are
> not going to be persuaded to move away from it.

As I wrote earlier, I am not sure if these drivers would try using
their drm_connector subclass for writeback.
ast: ast_connector = drm_connector + respective i2c adapter for EDID,
not needed for WB
fsl-dcu: fsl_dcu_drm_connector = drm_connector + drm_encoder pointer +
drm_panel. Not needed for WB
hisilicon, mgag200: same as ast
tilcdc: same as fsl-dcu
vboxdrv: the only driver that may possibly benefit from using
vbox_connector in the writeback support, as the connector is bare
drm_connector + crtc pointer + hints (width, height, disconnected).

I have left amd, nouveau and radeon out of this list, too complex to
analyze in several minutes.

I'd second the proposal of supporting optional drm_encoder for
drm_writeback_connector (as the crtc/encoder split can be vague), but
I do not see the benefit for the drivers to use their own
drm_connector subclass for drm_writeback.
It well might be that we all misunderstand your design. Do you have a
complete intel drm_writeback implementation based on this patchset? It
would be easier to judge if the approach is correct seeing your
target.

>
> It's no coincidence that the drivers who've implemented writeback so far
> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
> because the drm_writeback_connector midlayer does, forcing the issue.
>
> So I think drm_writeback_connector should *not* use the inheritance
> abstraction because it's a midlayer that should leave that option to the
> drivers. I think drm_writeback_connector needs to be changed to
> accommodate that, and, unfortunately, it means current writeback users
> need to be changed as well.
>
> I am not sure, however, if the series at hand is the right
> approach. Perhaps writeback can be modified to allocate the stuff for
> you if you prefer it that way, as long as the drm_connector is not
> embedded in struct drm_writeback_connector.

-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Jani Nikula]

On Wed, 02 Feb 2022, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Wed, 2 Feb 2022 at 16:15, Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>> > Hi Kandpal,
>> >
>> > Thank you for the patch.
>> >
>> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
>> >> Changing rcar_du driver to accomadate the change of
>> >> drm_writeback_connector.base and drm_writeback_connector.encoder
>> >> to a pointer the reason for which is explained in the
>> >> Patch(drm: add writeback pointers to drm_connector).
>> >>
>> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h      | 2 ++
>> >>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++---
>> >>  2 files changed, 7 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> index 66e8839db708..68f387a04502 100644
>> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc {
>> >>      const char *const *sources;
>> >>      unsigned int sources_count;
>> >>
>> >> +    struct drm_connector connector;
>> >> +    struct drm_encoder encoder;
>> >
>> > Those fields are, at best, poorly named. Furthermore, there's no need in
>> > this driver or in other drivers using drm_writeback_connector to create
>> > an encoder or connector manually. Let's not polute all drivers because
>> > i915 doesn't have its abstractions right.
>>
>> i915 uses the quite common model for struct inheritance:
>>
>>         struct intel_connector {
>>                 struct drm_connector base;
>>                 /* ... */
>>         }
>>
>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
>> radeon, tilcdc, and vboxvideo.
>
> For the reference. msm does not wrap drm_connector into any _common_
> structure, which is used internally.
>
>> We could argue about the relative merits of that abstraction, but I
>> think the bottom line is that it's popular and the drivers using it are
>> not going to be persuaded to move away from it.
>
> As I wrote earlier, I am not sure if these drivers would try using
> their drm_connector subclass for writeback.
> ast: ast_connector = drm_connector + respective i2c adapter for EDID,
> not needed for WB
> fsl-dcu: fsl_dcu_drm_connector = drm_connector + drm_encoder pointer +
> drm_panel. Not needed for WB
> hisilicon, mgag200: same as ast
> tilcdc: same as fsl-dcu
> vboxdrv: the only driver that may possibly benefit from using
> vbox_connector in the writeback support, as the connector is bare
> drm_connector + crtc pointer + hints (width, height, disconnected).
>
> I have left amd, nouveau and radeon out of this list, too complex to
> analyze in several minutes.
>
> I'd second the proposal of supporting optional drm_encoder for
> drm_writeback_connector (as the crtc/encoder split can be vague), but
> I do not see the benefit for the drivers to use their own
> drm_connector subclass for drm_writeback.

If a driver uses inheritance throughout the driver, and a *different*
subclass gets introduced into the mix, you need to add a ton of checks
all over the place when you cast the superclass pointer to the subclass.

The connector/encoder funcs you do have to pass to
drm_writeback_connector_init() can't use any of the shared driver
infrastructure that assume a certain inheritance.

See also my reply to Laurent [1].

> It well might be that we all misunderstand your design. Do you have a
> complete intel drm_writeback implementation based on this patchset? It
> would be easier to judge if the approach is correct seeing your
> target.

That would be up to Suraj Kandpal.

BR,
Jani.


[1] https://lore.kernel.org/r/87v8xxs2hz.fsf@intel.com


>
>>
>> It's no coincidence that the drivers who've implemented writeback so far
>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
>> because the drm_writeback_connector midlayer does, forcing the issue.
>>
>> So I think drm_writeback_connector should *not* use the inheritance
>> abstraction because it's a midlayer that should leave that option to the
>> drivers. I think drm_writeback_connector needs to be changed to
>> accommodate that, and, unfortunately, it means current writeback users
>> need to be changed as well.
>>
>> I am not sure, however, if the series at hand is the right
>> approach. Perhaps writeback can be modified to allocate the stuff for
>> you if you prefer it that way, as long as the drm_connector is not
>> embedded in struct drm_writeback_connector.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hey,

> The connector/encoder funcs you do have to pass to
> drm_writeback_connector_init() can't use any of the shared driver
> infrastructure that assume a certain inheritance.
> 
> See also my reply to Laurent [1].
> 
> > It well might be that we all misunderstand your design. Do you have a
> > complete intel drm_writeback implementation based on this patchset? It
> > would be easier to judge if the approach is correct seeing your
> > target.
> 
> That would be up to Suraj Kandpal.
I have floated the intel drm_writeback implementation.
Please refer to [1] to get a better understanding of how we are implementing
writeback functionality.

[1] https://patchwork.freedesktop.org/series/100617/

Thanks,
Suraj Kandpal 

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Abhinav Kumar]

Hi Suraj

On 2/22/2022 10:17 PM, Kandpal, Suraj wrote:
> Hey,
> 
>> The connector/encoder funcs you do have to pass to
>> drm_writeback_connector_init() can't use any of the shared driver
>> infrastructure that assume a certain inheritance.
>>
>> See also my reply to Laurent [1].
>>
>>> It well might be that we all misunderstand your design. Do you have a
>>> complete intel drm_writeback implementation based on this patchset? It
>>> would be easier to judge if the approach is correct seeing your
>>> target.
>>
>> That would be up to Suraj Kandpal.
> I have floated the intel drm_writeback implementation.
> Please refer to [1] to get a better understanding of how we are implementing
> writeback functionality.
> 
> [1] https://patchwork.freedesktop.org/series/100617/
> 
> Thanks,
> Suraj Kandpal

Based on the discussion in this thread [1] , it seems like having 
drm_encoder as a pointer seems to have merits for both of us and also in 
agreement with the folks on this thread and has a better chance of an ack.

However drm_connector is not.

I had a brief look at your implementation. Any reason you need to go 
through intel_connector here and not drm_writeback_connector directly?

The reason I ask is that I see you are not using prepare_writeback_job 
hook. If you use that, you can use the drm_writeback_connector passed on 
from there instead of looping through your list like you are doing in 
intel_find_writeback_connector.

Also, none of the other entries of struct intel_connector are being used 
for the writeback implementation. So does the drm_writeback_connector in 
your implementation need to be an intel_connector when its anyway not 
using other fields? Why cant it be just stored as a 
drm_writeback_connector itself in your struct intel_wd.

@@ -539,6 +540,8 @@  struct intel_connector {
  	struct work_struct modeset_retry_work;

  	struct intel_hdcp hdcp;
+
+	struct drm_writeback_connector wb_conn;
  };

[1] 
https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/#24747889

If you are in agreement with this, do you think you can re-spin the 
series only with the drm_encoder as a pointer without the drm_connector 
part.

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Kandpal, Suraj]

Hi Abhinav,

> Based on the discussion in this thread [1] , it seems like having drm_encoder
> as a pointer seems to have merits for both of us and also in agreement with
> the folks on this thread and has a better chance of an ack.
> 
> However drm_connector is not.
> 
> I had a brief look at your implementation. Any reason you need to go
> through intel_connector here and not drm_writeback_connector directly?
> 
> The reason I ask is that I see you are not using prepare_writeback_job hook.
> If you use that, you can use the drm_writeback_connector passed on from
> there instead of looping through your list like you are doing in
> intel_find_writeback_connector.
> 
> Also, none of the other entries of struct intel_connector are being used for
> the writeback implementation. So does the drm_writeback_connector in
> your implementation need to be an intel_connector when its anyway not
> using other fields? Why cant it be just stored as a drm_writeback_connector
> itself in your struct intel_wd.

The reason we can't do that is i915 driver always assumes that all connectors
present in device list is an intel connector and since drm_writeback_connector
even though a faux connector in it's initialization calls drm_connector_init()
and gets added to the drm device list and hence the i915 driver also expects 
a corresponding intel connector to go with it. In case I do try to make writeback
connector standalone a lot of checks, a lot will have to be added all around the 
driver as there could be a chance that one of the drm_connector in the drm device
list may not be an intel_connector.
Yes right now not all fields of intel_connector are being used but we will be working
on filling them as we add more functionality to writeback for example introducing
content protection. 
So even if I do float the patch series with just drm_encoder as pointer it won't help
us with our implementation if drm_connector isn't a pointer too.
 
Regards,
Suraj Kandpal

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Suraj,

On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote:
> Hi Abhinav,
> 
> > Based on the discussion in this thread [1] , it seems like having drm_encoder
> > as a pointer seems to have merits for both of us and also in agreement with
> > the folks on this thread and has a better chance of an ack.
> > 
> > However drm_connector is not.
> > 
> > I had a brief look at your implementation. Any reason you need to go
> > through intel_connector here and not drm_writeback_connector directly?
> > 
> > The reason I ask is that I see you are not using prepare_writeback_job hook.
> > If you use that, you can use the drm_writeback_connector passed on from
> > there instead of looping through your list like you are doing in
> > intel_find_writeback_connector.
> > 
> > Also, none of the other entries of struct intel_connector are being used for
> > the writeback implementation. So does the drm_writeback_connector in
> > your implementation need to be an intel_connector when its anyway not
> > using other fields? Why cant it be just stored as a drm_writeback_connector
> > itself in your struct intel_wd.
> 
> The reason we can't do that is i915 driver always assumes that all connectors
> present in device list is an intel connector and since drm_writeback_connector
> even though a faux connector in it's initialization calls drm_connector_init()
> and gets added to the drm device list and hence the i915 driver also expects 
> a corresponding intel connector to go with it. In case I do try to make writeback
> connector standalone a lot of checks, a lot will have to be added all around the 
> driver as there could be a chance that one of the drm_connector in the drm device
> list may not be an intel_connector.
> Yes right now not all fields of intel_connector are being used but we will be working
> on filling them as we add more functionality to writeback for example introducing
> content protection. 
> So even if I do float the patch series with just drm_encoder as pointer it won't help
> us with our implementation if drm_connector isn't a pointer too.

This is a direct consequence of the decision to use connectors for
writeback in the userspace API. This disrupts any code that assumes that
a connector is a connector. The problem isn't limited to kernelspace,
userspace has the same exact problem, which resulted in a hack to avoid
breaking everything. Userspace software that needs to deal with
writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
capability to get the writeback connectors exposed by the kernel, but
more than that, a large refactoring is then often needed to chase all
code paths that assume a connector is a connector.

I'm afraid the same applies to the kernel. Drivers that don't use
writeback are largely unaffected. Drievrs that want to use writeback
need to be refactored to properly handle the fact that writeback
connectors are special. i915 will need to go that way.

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Dmitry Baryshkov]

On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Suraj,
>
> On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote:
> > Hi Abhinav,
> >
> > > Based on the discussion in this thread [1] , it seems like having drm_encoder
> > > as a pointer seems to have merits for both of us and also in agreement with
> > > the folks on this thread and has a better chance of an ack.
> > >
> > > However drm_connector is not.
> > >
> > > I had a brief look at your implementation. Any reason you need to go
> > > through intel_connector here and not drm_writeback_connector directly?
> > >
> > > The reason I ask is that I see you are not using prepare_writeback_job hook.
> > > If you use that, you can use the drm_writeback_connector passed on from
> > > there instead of looping through your list like you are doing in
> > > intel_find_writeback_connector.
> > >
> > > Also, none of the other entries of struct intel_connector are being used for
> > > the writeback implementation. So does the drm_writeback_connector in
> > > your implementation need to be an intel_connector when its anyway not
> > > using other fields? Why cant it be just stored as a drm_writeback_connector
> > > itself in your struct intel_wd.
> >
> > The reason we can't do that is i915 driver always assumes that all connectors
> > present in device list is an intel connector and since drm_writeback_connector
> > even though a faux connector in it's initialization calls drm_connector_init()
> > and gets added to the drm device list and hence the i915 driver also expects
> > a corresponding intel connector to go with it. In case I do try to make writeback
> > connector standalone a lot of checks, a lot will have to be added all around the
> > driver as there could be a chance that one of the drm_connector in the drm device
> > list may not be an intel_connector.
> > Yes right now not all fields of intel_connector are being used but we will be working
> > on filling them as we add more functionality to writeback for example introducing
> > content protection.
> > So even if I do float the patch series with just drm_encoder as pointer it won't help
> > us with our implementation if drm_connector isn't a pointer too.
>
> This is a direct consequence of the decision to use connectors for
> writeback in the userspace API. This disrupts any code that assumes that
> a connector is a connector. The problem isn't limited to kernelspace,
> userspace has the same exact problem, which resulted in a hack to avoid
> breaking everything. Userspace software that needs to deal with
> writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> capability to get the writeback connectors exposed by the kernel, but
> more than that, a large refactoring is then often needed to chase all
> code paths that assume a connector is a connector.
>
> I'm afraid the same applies to the kernel. Drivers that don't use
> writeback are largely unaffected. Drievrs that want to use writeback
> need to be refactored to properly handle the fact that writeback
> connectors are special. i915 will need to go that way.

Laurent, you have frown upon the idea of separating the connector from
the drm_writeback_connector struct. What about the encoder?
The msm code in question can be found at the patchwork:
https://patchwork.freedesktop.org/series/99724/. This series depends
on Intel's patch, but should give you the overall feeling of the code
being shared between to-the-display and writeback pipelines.

-- 
With best wishes
Dmitry

Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes

[Laurent Pinchart]

Hi Dmitry,

On Mon, Feb 28, 2022 at 11:07:41AM +0300, Dmitry Baryshkov wrote:
> On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart wrote:
> > On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote:
> > > Hi Abhinav,
> > >
> > > > Based on the discussion in this thread [1] , it seems like having drm_encoder
> > > > as a pointer seems to have merits for both of us and also in agreement with
> > > > the folks on this thread and has a better chance of an ack.
> > > >
> > > > However drm_connector is not.
> > > >
> > > > I had a brief look at your implementation. Any reason you need to go
> > > > through intel_connector here and not drm_writeback_connector directly?
> > > >
> > > > The reason I ask is that I see you are not using prepare_writeback_job hook.
> > > > If you use that, you can use the drm_writeback_connector passed on from
> > > > there instead of looping through your list like you are doing in
> > > > intel_find_writeback_connector.
> > > >
> > > > Also, none of the other entries of struct intel_connector are being used for
> > > > the writeback implementation. So does the drm_writeback_connector in
> > > > your implementation need to be an intel_connector when its anyway not
> > > > using other fields? Why cant it be just stored as a drm_writeback_connector
> > > > itself in your struct intel_wd.
> > >
> > > The reason we can't do that is i915 driver always assumes that all connectors
> > > present in device list is an intel connector and since drm_writeback_connector
> > > even though a faux connector in it's initialization calls drm_connector_init()
> > > and gets added to the drm device list and hence the i915 driver also expects
> > > a corresponding intel connector to go with it. In case I do try to make writeback
> > > connector standalone a lot of checks, a lot will have to be added all around the
> > > driver as there could be a chance that one of the drm_connector in the drm device
> > > list may not be an intel_connector.
> > > Yes right now not all fields of intel_connector are being used but we will be working
> > > on filling them as we add more functionality to writeback for example introducing
> > > content protection.
> > > So even if I do float the patch series with just drm_encoder as pointer it won't help
> > > us with our implementation if drm_connector isn't a pointer too.
> >
> > This is a direct consequence of the decision to use connectors for
> > writeback in the userspace API. This disrupts any code that assumes that
> > a connector is a connector. The problem isn't limited to kernelspace,
> > userspace has the same exact problem, which resulted in a hack to avoid
> > breaking everything. Userspace software that needs to deal with
> > writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
> > capability to get the writeback connectors exposed by the kernel, but
> > more than that, a large refactoring is then often needed to chase all
> > code paths that assume a connector is a connector.
> >
> > I'm afraid the same applies to the kernel. Drivers that don't use
> > writeback are largely unaffected. Drievrs that want to use writeback
> > need to be refactored to properly handle the fact that writeback
> > connectors are special. i915 will need to go that way.
> 
> Laurent, you have frown upon the idea of separating the connector from
> the drm_writeback_connector struct. What about the encoder?
> The msm code in question can be found at the patchwork:
> https://patchwork.freedesktop.org/series/99724/. This series depends
> on Intel's patch, but should give you the overall feeling of the code
> being shared between to-the-display and writeback pipelines.

I'm not too fond of separating the encoder either as explained
separately in this mail thread, but I won't block that as it's even more
difficult to avoid today. drm_encoder is a bit of a historical mistake
that we need to keep around because it's exposed to userspace. With
drivers more and more reliant on drm_bridge, drm_encoder is less
meaningful than it used to be. I would like to see the subsystem
continuing in that direction, with drm_encoder becoming an empty shell.
Drivers should decouple the CRTC outputs from the drm_encoder object,
likely creating driver-specific structures to model a CRTC output (which
is largely what the driver-specific subclasses of drm_encoder do today),
and create drm_encoder instances only for the purpose of exposing the
display topology to userspace.

Longer term I can even imagine having a different way to expose the
display topology to userspace, without drm_encoder but with objects that
will be allowed to support more complex topologies that the CRTC +
encoder + connector abstraction can't model. Later :-)

-- 
Regards,

Laurent Pinchart

[Intel-gfx] [PATCH 6/6] drm/arm: changes to malidp driver resulting from drm_writeback_connector structure changes

[Kandpal Suraj]

Changing malidp driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).

Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/arm/malidp_crtc.c |  2 +-
 drivers/gpu/drm/arm/malidp_drv.h  |  2 ++
 drivers/gpu/drm/arm/malidp_mw.c   | 12 ++++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 494075ddbef6..294aacd4beef 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -424,7 +424,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
 		u32 new_mask = crtc_state->connector_mask;
 
 		if ((old_mask ^ new_mask) ==
-		    (1 << drm_connector_index(&malidp->mw_connector.base)))
+		    (1 << drm_connector_index(malidp->mw_connector.base)))
 			crtc_state->connectors_changed = false;
 	}
 
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index cdfddfabf2d1..971810a685f1 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -31,6 +31,8 @@ struct malidp_error_stats {
 struct malidp_drm {
 	struct malidp_hw_device *dev;
 	struct drm_crtc crtc;
+	struct drm_connector connector;
+	struct drm_encoder encoder;
 	struct drm_writeback_connector mw_connector;
 	wait_queue_head_t wq;
 	struct drm_pending_vblank_event *event;
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index f5847a79dd7e..9bd2e400cd3d 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -206,21 +206,25 @@ static u32 *get_writeback_formats(struct malidp_drm *malidp, int *n_formats)
 int malidp_mw_connector_init(struct drm_device *drm)
 {
 	struct malidp_drm *malidp = drm->dev_private;
+	struct drm_writeback_connector *wb_conn;
 	u32 *formats;
 	int ret, n_formats;
 
 	if (!malidp->dev->hw->enable_memwrite)
 		return 0;
 
-	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
-	drm_connector_helper_add(&malidp->mw_connector.base,
+	wb_conn = &malidp->mw_connector;
+	wb_conn->base = &malidp->connector;
+	wb_conn->encoder = &malidp->encoder;
+	malidp->mw_connector.encoder->possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+	drm_connector_helper_add(wb_conn->base,
 				 &malidp_mw_connector_helper_funcs);
 
 	formats = get_writeback_formats(malidp, &n_formats);
 	if (!formats)
 		return -ENOMEM;
 
-	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
+	ret = drm_writeback_connector_init(drm, wb_conn,
 					   &malidp_mw_connector_funcs,
 					   &malidp_mw_encoder_helper_funcs,
 					   formats, n_formats);
@@ -236,7 +240,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
 {
 	struct malidp_drm *malidp = drm->dev_private;
 	struct drm_writeback_connector *mw_conn = &malidp->mw_connector;
-	struct drm_connector_state *conn_state = mw_conn->base.state;
+	struct drm_connector_state *conn_state = mw_conn->base->state;
 	struct malidp_hw_device *hwdev = malidp->dev;
 	struct malidp_mw_connector_state *mw_state;
 
-- 
2.17.1

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm writeback connector changes

[Patchwork]

== Series Details ==

Series: drm writeback connector changes
URL   : https://patchwork.freedesktop.org/series/99610/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm writeback connector changes

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4658 bytes --]

== Series Details ==

Series: drm writeback connector changes
URL   : https://patchwork.freedesktop.org/series/99610/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11177 -> Patchwork_22152
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/index.html

Participating hosts (45 -> 43)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (3): fi-bsw-cyan fi-icl-u2 fi-bdw-samus 

Known issues
------------

  Here are the changes found in Patchwork_22152 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-snb-2600:        NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-snb-2600/igt@amdgpu/amd_cs_nop@sync-fork-compute0.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-bdw-5557u:       [PASS][2] -> [INCOMPLETE][3] ([i915#146])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@kms_psr@primary_page_flip:
    - fi-skl-6600u:       [PASS][4] -> [FAIL][5] ([i915#4547])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/fi-skl-6600u/igt@kms_psr@primary_page_flip.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-skl-6600u/igt@kms_psr@primary_page_flip.html

  * igt@prime_vgem@basic-userptr:
    - fi-pnv-d510:        NOTRUN -> [SKIP][6] ([fdo#109271]) +57 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-pnv-d510/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-skl-6600u:       NOTRUN -> [FAIL][7] ([i915#4312])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-skl-6600u/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [INCOMPLETE][8] ([i915#3921]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-cfl-8109u:       [DMESG-FAIL][10] ([i915#295]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b:
    - fi-cfl-8109u:       [DMESG-WARN][12] ([i915#295]) -> [PASS][13] +10 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#295]: https://gitlab.freedesktop.org/drm/intel/issues/295
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547


Build changes
-------------

  * Linux: CI_DRM_11177 -> Patchwork_22152

  CI-20190529: 20190529
  CI_DRM_11177: 22db355d36b6c879c506e293b0255bf951df1dff @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6337: 7c9c034619ef9dbfbfe041fbf3973a1cf1ac7a22 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22152: 1a9c28f7f63392f2af9f676132e80cf0ad5df75b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1a9c28f7f633 drm/arm: changes to malidp driver resulting from drm_writeback_connector structure changes
3e0f8d61446a drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
735e7a771cba drm/vc4: vc4 driver changes to accommodate changes done in drm_writeback_connector structure
4a3bcb073fe2 drm/vkms: change vkms driver to use drm_writeback_connector.base pointer
57ef80524dde drm/arm/komeda : change driver to use drm_writeback_connector.base pointer
bb318d5a51f7 drm: add writeback pointers to drm_connector

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/index.html

[-- Attachment #2: Type: text/html, Size: 5629 bytes --]

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm writeback connector changes

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 30254 bytes --]

== Series Details ==

Series: drm writeback connector changes
URL   : https://patchwork.freedesktop.org/series/99610/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11177_full -> Patchwork_22152_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_22152_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_22152_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (13 -> 13)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_22152_full:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@evict:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb8/igt@i915_selftest@live@evict.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb4/igt@i915_selftest@live@evict.html

  
Known issues
------------

  Here are the changes found in Patchwork_22152_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_balancer@parallel-keep-submit-fence:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([i915#4525])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb1/igt@gem_exec_balancer@parallel-keep-submit-fence.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb3/igt@gem_exec_balancer@parallel-keep-submit-fence.html

  * igt@gem_exec_fair@basic-none@vecs0:
    - shard-kbl:          NOTRUN -> [FAIL][5] ([i915#2842])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@gem_exec_fair@basic-none@vecs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][6] -> [FAIL][7] ([i915#2842])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-glk6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@vcs1:
    - shard-kbl:          [PASS][8] -> [FAIL][9] ([i915#2842]) +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl4/igt@gem_exec_fair@basic-pace@vcs1.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl6/igt@gem_exec_fair@basic-pace@vcs1.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - shard-skl:          NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#4613])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@gem_lmem_swapping@parallel-random-engines.html
    - shard-kbl:          NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#4613])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl3/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_userptr_blits@dmabuf-sync:
    - shard-kbl:          NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#3323])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl3/igt@gem_userptr_blits@dmabuf-sync.html
    - shard-skl:          NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#3323])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@gem_userptr_blits@dmabuf-sync.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-skl:          [PASS][14] -> [DMESG-WARN][15] ([i915#1982])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl4/igt@gem_userptr_blits@sync-unmap-cycles.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-apl:          [PASS][16] -> [DMESG-WARN][17] ([i915#1436] / [i915#716])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-apl4/igt@gen9_exec_parse@allowed-all.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl6/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-skl:          NOTRUN -> [SKIP][18] ([fdo#109271] / [i915#658])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@i915_pm_dc@dc3co-vpb-simulation.html
    - shard-kbl:          NOTRUN -> [SKIP][19] ([fdo#109271] / [i915#658])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl3/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [PASS][20] -> [DMESG-WARN][21] ([i915#180]) +2 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl3/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - shard-skl:          NOTRUN -> [SKIP][22] ([fdo#109271] / [i915#3777]) +2 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip:
    - shard-kbl:          NOTRUN -> [SKIP][23] ([fdo#109271] / [i915#3777]) +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip.html

  * igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs:
    - shard-apl:          NOTRUN -> [SKIP][24] ([fdo#109271] / [i915#3886])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs.html

  * igt@kms_ccs@pipe-a-crc-primary-basic-y_tiled_gen12_rc_ccs_cc:
    - shard-skl:          NOTRUN -> [SKIP][25] ([fdo#109271] / [i915#3886]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@kms_ccs@pipe-a-crc-primary-basic-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_mc_ccs:
    - shard-kbl:          NOTRUN -> [SKIP][26] ([fdo#109271] / [i915#3886]) +5 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_mc_ccs.html

  * igt@kms_chamelium@hdmi-hpd-storm:
    - shard-kbl:          NOTRUN -> [SKIP][27] ([fdo#109271] / [fdo#111827]) +12 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_chamelium@hdmi-hpd-storm.html

  * igt@kms_chamelium@hdmi-hpd-storm-disable:
    - shard-skl:          NOTRUN -> [SKIP][28] ([fdo#109271] / [fdo#111827]) +5 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@kms_chamelium@hdmi-hpd-storm-disable.html

  * igt@kms_color_chamelium@pipe-c-ctm-max:
    - shard-apl:          NOTRUN -> [SKIP][29] ([fdo#109271] / [fdo#111827]) +2 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@kms_color_chamelium@pipe-c-ctm-max.html

  * igt@kms_content_protection@legacy:
    - shard-kbl:          NOTRUN -> [TIMEOUT][30] ([i915#1319])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_content_protection@legacy.html

  * igt@kms_cursor_crc@pipe-d-cursor-suspend:
    - shard-kbl:          NOTRUN -> [SKIP][31] ([fdo#109271]) +134 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl3/igt@kms_cursor_crc@pipe-d-cursor-suspend.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [PASS][32] -> [INCOMPLETE][33] ([i915#180] / [i915#636])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl6/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank@b-edp1:
    - shard-skl:          [PASS][34] -> [FAIL][35] ([i915#79])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl7/igt@kms_flip@flip-vs-expired-vblank@b-edp1.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl2/igt@kms_flip@flip-vs-expired-vblank@b-edp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytileccs-to-64bpp-ytile-upscaling:
    - shard-glk:          [PASS][36] -> [FAIL][37] ([i915#4911])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-glk3/igt@kms_flip_scaled_crc@flip-32bpp-ytileccs-to-64bpp-ytile-upscaling.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-glk8/igt@kms_flip_scaled_crc@flip-32bpp-ytileccs-to-64bpp-ytile-upscaling.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          [PASS][38] -> [DMESG-WARN][39] ([i915#180]) +6 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-indfb-draw-blt:
    - shard-apl:          NOTRUN -> [SKIP][40] ([fdo#109271]) +26 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-indfb-draw-blt.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          NOTRUN -> [DMESG-WARN][41] ([i915#180])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          NOTRUN -> [FAIL][42] ([fdo#108145] / [i915#265]) +1 similar issue
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][43] -> [FAIL][44] ([fdo#108145] / [i915#265])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-apl:          NOTRUN -> [FAIL][45] ([fdo#108145] / [i915#265]) +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@kms_plane_alpha_blend@pipe-c-alpha-basic.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb:
    - shard-skl:          NOTRUN -> [FAIL][46] ([i915#265])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][47] -> [SKIP][48] ([fdo#109441]) +1 similar issue
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb7/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-glk:          [PASS][49] -> [FAIL][50] ([i915#31])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-glk6/igt@kms_setmode@basic.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-glk3/igt@kms_setmode@basic.html

  * igt@kms_writeback@writeback-check-output:
    - shard-kbl:          NOTRUN -> [SKIP][51] ([fdo#109271] / [i915#2437])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_writeback@writeback-check-output.html

  * igt@perf@gen12-mi-rpc:
    - shard-skl:          NOTRUN -> [SKIP][52] ([fdo#109271]) +55 similar issues
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl9/igt@perf@gen12-mi-rpc.html

  * igt@sysfs_clients@busy:
    - shard-skl:          NOTRUN -> [SKIP][53] ([fdo#109271] / [i915#2994])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl10/igt@sysfs_clients@busy.html
    - shard-apl:          NOTRUN -> [SKIP][54] ([fdo#109271] / [i915#2994])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@sysfs_clients@busy.html

  
#### Possible fixes ####

  * igt@fbdev@nullptr:
    - {shard-rkl}:        [SKIP][55] ([i915#2582]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@fbdev@nullptr.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@fbdev@nullptr.html

  * igt@gem_exec_balancer@parallel-contexts:
    - shard-iclb:         [SKIP][57] ([i915#4525]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb7/igt@gem_exec_balancer@parallel-contexts.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb2/igt@gem_exec_balancer@parallel-contexts.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-tglb:         [FAIL][59] ([i915#2842]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-tglb3/igt@gem_exec_fair@basic-none-share@rcs0.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-tglb1/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-kbl:          [FAIL][61] ([i915#2842]) -> [PASS][62] +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl4/igt@gem_exec_fair@basic-pace@vecs0.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl6/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@gem_exec_fair@basic-throttle@rcs0:
    - shard-glk:          [FAIL][63] ([i915#2842]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-glk2/igt@gem_exec_fair@basic-throttle@rcs0.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-glk5/igt@gem_exec_fair@basic-throttle@rcs0.html

  * igt@i915_pm_rpm@modeset-lpsp-stress:
    - {shard-dg1}:        [SKIP][65] ([i915#1397]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-dg1-18/igt@i915_pm_rpm@modeset-lpsp-stress.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-dg1-16/igt@i915_pm_rpm@modeset-lpsp-stress.html
    - {shard-rkl}:        [SKIP][67] ([i915#1397]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@i915_pm_rpm@modeset-lpsp-stress.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@i915_pm_rpm@modeset-lpsp-stress.html

  * igt@i915_pm_rpm@pm-tiling:
    - {shard-rkl}:        ([SKIP][69], [SKIP][70]) ([fdo#109308]) -> [PASS][71]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@i915_pm_rpm@pm-tiling.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@i915_pm_rpm@pm-tiling.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@i915_pm_rpm@pm-tiling.html

  * igt@i915_suspend@forcewake:
    - shard-apl:          [DMESG-WARN][72] ([i915#180]) -> [PASS][73]
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-apl8/igt@i915_suspend@forcewake.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-apl8/igt@i915_suspend@forcewake.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip:
    - {shard-rkl}:        ([SKIP][74], [SKIP][75]) ([i915#1845]) -> [PASS][76] +5 similar issues
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip-async-flip.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip:
    - {shard-rkl}:        [SKIP][77] ([i915#1845]) -> [PASS][78] +16 similar issues
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html

  * igt@kms_ccs@pipe-b-bad-pixel-format-y_tiled_gen12_rc_ccs_cc:
    - {shard-rkl}:        [SKIP][79] ([i915#1845] / [i915#4098]) -> [PASS][80] +4 similar issues
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_ccs@pipe-b-bad-pixel-format-y_tiled_gen12_rc_ccs_cc.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_ccs@pipe-b-bad-pixel-format-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_color@pipe-a-ctm-0-25:
    - {shard-rkl}:        [SKIP][81] ([i915#1149] / [i915#1849] / [i915#4070]) -> [PASS][82] +1 similar issue
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_color@pipe-a-ctm-0-25.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_color@pipe-a-ctm-0-25.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen:
    - {shard-rkl}:        [SKIP][83] ([fdo#112022]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding:
    - {shard-rkl}:        ([SKIP][85], [SKIP][86]) ([fdo#112022] / [i915#4070]) -> [PASS][87]
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html

  * igt@kms_cursor_crc@pipe-b-cursor-alpha-transparent:
    - {shard-rkl}:        [SKIP][88] ([fdo#112022] / [i915#4070]) -> [PASS][89] +3 similar issues
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_cursor_crc@pipe-b-cursor-alpha-transparent.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_crc@pipe-b-cursor-alpha-transparent.html

  * igt@kms_cursor_edge_walk@pipe-a-256x256-bottom-edge:
    - {shard-rkl}:        [SKIP][90] ([i915#1849] / [i915#4070]) -> [PASS][91] +1 similar issue
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_cursor_edge_walk@pipe-a-256x256-bottom-edge.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_edge_walk@pipe-a-256x256-bottom-edge.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          [FAIL][92] ([i915#2346] / [i915#533]) -> [PASS][93]
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_cursor_legacy@flip-vs-cursor-varying-size:
    - {shard-rkl}:        [SKIP][94] ([fdo#111825] / [i915#4070]) -> [PASS][95] +1 similar issue
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html

  * igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic:
    - {shard-rkl}:        ([SKIP][96], [SKIP][97]) ([fdo#111825] / [i915#4070]) -> [PASS][98]
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_cursor_legacy@long-nonblocking-modeset-vs-cursor-atomic.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-untiled:
    - {shard-rkl}:        [SKIP][99] ([fdo#111314]) -> [PASS][100] +2 similar issues
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-untiled.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-untiled.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
    - shard-skl:          [FAIL][101] ([i915#79]) -> [PASS][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl10/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl8/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@a-dp1:
    - shard-kbl:          [DMESG-WARN][103] ([i915#180]) -> [PASS][104] +7 similar issues
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl6/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl4/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html

  * igt@kms_flip@plain-flip-fb-recreate-interruptible@a-edp1:
    - shard-skl:          [FAIL][105] ([i915#2122]) -> [PASS][106]
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-skl2/igt@kms_flip@plain-flip-fb-recreate-interruptible@a-edp1.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-skl10/igt@kms_flip@plain-flip-fb-recreate-interruptible@a-edp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling:
    - shard-iclb:         [SKIP][107] ([i915#3701]) -> [PASS][108]
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb2/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling.html
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb1/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling:
    - shard-glk:          [FAIL][109] ([i915#4911]) -> [PASS][110]
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-glk8/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling.html
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-glk6/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling.html

  * igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile-upscaling:
    - {shard-rkl}:        [INCOMPLETE][111] ([i915#3701]) -> [PASS][112]
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile-upscaling.html
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile-upscaling.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-gtt:
    - {shard-rkl}:        [SKIP][113] ([i915#1849]) -> [PASS][114] +20 similar issues
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-gtt.html
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-gtt.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - {shard-rkl}:        ([SKIP][115], [SKIP][116]) ([i915#1849] / [i915#4098]) -> [PASS][117] +4 similar issues
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-gtt.html
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-gtt.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-mmap-gtt.html

  * igt@kms_invalid_mode@bad-hsync-start:
    - {shard-rkl}:        [SKIP][118] ([i915#4278]) -> [PASS][119] +1 similar issue
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_invalid_mode@bad-hsync-start.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_invalid_mode@bad-hsync-start.html

  * igt@kms_lease@lease_unleased_connector:
    - shard-snb:          [SKIP][120] ([fdo#109271]) -> [PASS][121]
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-snb5/igt@kms_lease@lease_unleased_connector.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-snb7/igt@kms_lease@lease_unleased_connector.html

  * igt@kms_lease@lease_unleased_crtc:
    - {shard-rkl}:        ([SKIP][122], [SKIP][123]) ([i915#1845] / [i915#4098]) -> [PASS][124]
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_lease@lease_unleased_crtc.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_lease@lease_unleased_crtc.html
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_lease@lease_unleased_crtc.html

  * igt@kms_plane@plane-position-hole@pipe-b-planes:
    - {shard-rkl}:        [SKIP][125] ([i915#3558]) -> [PASS][126] +3 similar issues
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_plane@plane-position-hole@pipe-b-planes.html
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_plane@plane-position-hole@pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - {shard-rkl}:        ([SKIP][127], [SKIP][128]) ([i915#1849] / [i915#4070] / [i915#4098]) -> [PASS][129] +1 similar issue
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-4/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-none:
    - {shard-rkl}:        [SKIP][130] ([i915#3558] / [i915#4070]) -> [PASS][131] +1 similar issue
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_plane_multiple@atomic-pipe-a-tiling-none.html
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_plane_multiple@atomic-pipe-a-tiling-none.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [SKIP][132] ([fdo#109441]) -> [PASS][133] +3 similar issues
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb5/igt@kms_psr@psr2_cursor_blt.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html

  * igt@kms_psr@sprite_mmap_cpu:
    - {shard-rkl}:        [SKIP][134] ([i915#1072]) -> [PASS][135] +1 similar issue
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_psr@sprite_mmap_cpu.html
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_psr@sprite_mmap_cpu.html

  * igt@kms_universal_plane@cursor-fb-leak-pipe-a:
    - {shard-rkl}:        [SKIP][136] ([i915#1845] / [i915#4070]) -> [PASS][137]
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-rkl-1/igt@kms_universal_plane@cursor-fb-leak-pipe-a.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-rkl-6/igt@kms_universal_plane@cursor-fb-leak-pipe-a.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [DMESG-WARN][138] ([i915#180] / [i915#295]) -> [PASS][139]
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-kbl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Warnings ####

  * igt@gem_exec_balancer@parallel-ordering:
    - shard-iclb:         [SKIP][140] ([i915#4525]) -> [FAIL][141] ([i915#4916])
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb7/igt@gem_exec_balancer@parallel-ordering.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb1/igt@gem_exec_balancer@parallel-ordering.html

  * igt@i915_pm_rc6_residency@rc6-fence:
    - shard-iclb:         [WARN][142] ([i915#2684]) -> [WARN][143] ([i915#1804] / [i915#2684])
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb5/igt@i915_pm_rc6_residency@rc6-fence.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb7/igt@i915_pm_rc6_residency@rc6-fence.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         [WARN][144] ([i915#1804] / [i915#2684]) -> [FAIL][145] ([i915#2680])
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11177/shard-iclb6/igt@i915_pm_rc6_residency@rc6-idle.html
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/shard-iclb5/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@k

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22152/index.html

[-- Attachment #2: Type: text/html, Size: 33024 bytes --]