diff mbox series

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

Message ID 20220202085429.22261-2-suraj.kandpal@intel.com (mailing list archive)
State New, archived
Headers show
Series drm writeback connector changes | expand

Commit Message

Kandpal, Suraj Feb. 2, 2022, 8:54 a.m. UTC 
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(-)

Comments

Dmitry Baryshkov Feb. 2, 2022, 10:28 a.m. UTC | #1 
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
>
kernel test robot Feb. 2, 2022, 11:17 a.m. UTC | #2 
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
kernel test robot Feb. 2, 2022, 8:07 p.m. UTC | #3 
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
Kandpal, Suraj Feb. 3, 2022, 8:46 a.m. UTC | #4 
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
diff mbox series

Patch

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,