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(-)
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,
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
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
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-connect... base: git://anongit.freedesktop.org/drm/drm drm-next config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20220202/202202021914.BKeUA6Fh-lkp@i...) 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/57ad56d769873f62f87fe432243030ceb167... 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
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-connect... 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@i...) 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/57ad56d769873f62f87fe432243030ceb167... 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
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;
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,
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; }
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;
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;
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;
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;
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;
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.
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.
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.
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.
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.
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.
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.
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.
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
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.
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
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
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
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
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.
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.
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
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-...
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
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-...
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
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-...
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; >
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-...
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
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/driv...
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-...
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; >>>
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/driv...
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/driv...
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-...
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; >>>>
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.
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.
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.
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
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-...
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.
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
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.
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.
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 :-)
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;
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Dmitry Baryshkov -
Jani Nikula -
Kandpal Suraj -
Kandpal, Suraj
-
kernel test robot -
Laurent Pinchart -
Rob Clark -
Ville Syrjälä