* [PATCH] drm: Store the plane's index @ 2016-05-26 9:34 Chris Wilson 2016-05-26 10:02 ` ✗ Ro.CI.BAT: failure for " Patchwork 2016-05-26 11:27 ` [PATCH] " Ville Syrjälä 0 siblings, 2 replies; 9+ messages in thread From: Chris Wilson @ 2016-05-26 9:34 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx Currently the plane's index is determined by walking the list of all planes in the mode and finding the position of that plane in the list. A linear walk, especially a linear walk within a linear walk as frequently conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. The plane's index is constant for as long as no earlier planes are removed from the list. For most drivers, planes are static, determined at boot and then untouched until shutdown. Storing the index upon construction and then only walking the tail upon removal should be a major improvement for all. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_crtc.c | 38 +++++++++----------------------------- include/drm/drm_crtc.h | 6 +++++- 2 files changed, 14 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3a0384cce4a2..00ee01126b6f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1302,7 +1302,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->type = type; list_add_tail(&plane->head, &config->plane_list); - config->num_total_plane++; + plane->index = config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY) config->num_overlay_plane++; @@ -1369,6 +1369,7 @@ EXPORT_SYMBOL(drm_plane_init); void drm_plane_cleanup(struct drm_plane *plane) { struct drm_device *dev = plane->dev; + struct drm_plane *other; drm_modeset_lock_all(dev); kfree(plane->format_types); @@ -1376,6 +1377,10 @@ void drm_plane_cleanup(struct drm_plane *plane) BUG_ON(list_empty(&plane->head)); + other = list_next_entry(plane, head); + list_for_each_entry_from(other, &dev->mode_config.plane_list, head) + other->index--; + list_del(&plane->head); dev->mode_config.num_total_plane--; if (plane->type == DRM_PLANE_TYPE_OVERLAY) @@ -1393,29 +1398,6 @@ void drm_plane_cleanup(struct drm_plane *plane) EXPORT_SYMBOL(drm_plane_cleanup); /** - * drm_plane_index - find the index of a registered plane - * @plane: plane to find index for - * - * Given a registered plane, return the index of that CRTC within a DRM - * device's list of planes. - */ -unsigned int drm_plane_index(struct drm_plane *plane) -{ - unsigned int index = 0; - struct drm_plane *tmp; - - drm_for_each_plane(tmp, plane->dev) { - if (tmp == plane) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_plane_index); - -/** * drm_plane_from_index - find the registered plane at an index * @dev: DRM device * @idx: index of registered plane to find for @@ -1427,13 +1409,11 @@ struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx) { struct drm_plane *plane; - unsigned int i = 0; - drm_for_each_plane(plane, dev) { - if (i == idx) + drm_for_each_plane(plane, dev) + if (idx == plane->index) return plane; - i++; - } + return NULL; } EXPORT_SYMBOL(drm_plane_from_index); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 9771428e1ba8..eda3b1b3d3b4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1543,6 +1543,7 @@ struct drm_plane { struct drm_object_properties properties; enum drm_plane_type type; + unsigned index; const struct drm_plane_helper_funcs *helper_private; @@ -2316,7 +2317,10 @@ extern int drm_plane_init(struct drm_device *dev, const uint32_t *formats, unsigned int format_count, bool is_primary); extern void drm_plane_cleanup(struct drm_plane *plane); -extern unsigned int drm_plane_index(struct drm_plane *plane); +static inline unsigned int drm_plane_index(struct drm_plane *plane) +{ + return plane->index; +} extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); extern void drm_plane_force_disable(struct drm_plane *plane); extern int drm_plane_check_pixel_format(const struct drm_plane *plane, -- 2.8.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Ro.CI.BAT: failure for drm: Store the plane's index 2016-05-26 9:34 [PATCH] drm: Store the plane's index Chris Wilson @ 2016-05-26 10:02 ` Patchwork 2016-05-26 11:27 ` [PATCH] " Ville Syrjälä 1 sibling, 0 replies; 9+ messages in thread From: Patchwork @ 2016-05-26 10:02 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == Series Details == Series: drm: Store the plane's index URL : https://patchwork.freedesktop.org/series/7796/ State : failure == Summary == Series 7796v1 drm: Store the plane's index http://patchwork.freedesktop.org/api/1.0/series/7796/revisions/1/mbox Test drv_hangman: Subgroup error-state-basic: pass -> DMESG-WARN (ro-skl-i7-6700hq) Test gem_busy: Subgroup basic-blt: dmesg-warn -> PASS (ro-skl-i7-6700hq) Test gem_exec_flush: Subgroup basic-batch-kernel-default-cmd: pass -> FAIL (ro-byt-n2820) Test gem_exec_suspend: Subgroup basic-s3: pass -> INCOMPLETE (fi-hsw-i7-4770k) Test gem_flink_basic: Subgroup double-flink: pass -> DMESG-WARN (ro-skl-i7-6700hq) Test gem_mmap_gtt: Subgroup basic-read: dmesg-warn -> PASS (ro-skl-i7-6700hq) Test kms_flip: Subgroup basic-flip-vs-wf_vblank: fail -> PASS (ro-bdw-i7-5600u) skip -> PASS (fi-skl-i5-6260u) Test kms_frontbuffer_tracking: Subgroup basic: pass -> DMESG-WARN (ro-skl-i7-6700hq) fi-bdw-i7-5557u total:209 pass:197 dwarn:0 dfail:0 fail:0 skip:12 fi-bsw-n3050 total:209 pass:167 dwarn:0 dfail:0 fail:2 skip:40 fi-byt-n2820 total:209 pass:168 dwarn:0 dfail:0 fail:3 skip:38 fi-hsw-i7-4770k total:102 pass:86 dwarn:0 dfail:0 fail:0 skip:15 fi-hsw-i7-4770r total:209 pass:186 dwarn:0 dfail:0 fail:0 skip:23 fi-skl-i5-6260u total:209 pass:198 dwarn:0 dfail:0 fail:0 skip:11 fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 skip:25 fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 skip:39 ro-bdw-i5-5250u total:209 pass:172 dwarn:0 dfail:0 fail:0 skip:37 ro-bdw-i7-5557U total:209 pass:197 dwarn:0 dfail:0 fail:0 skip:12 ro-bdw-i7-5600u total:209 pass:181 dwarn:0 dfail:0 fail:0 skip:28 ro-byt-n2820 total:209 pass:169 dwarn:0 dfail:0 fail:3 skip:37 ro-hsw-i3-4010u total:209 pass:186 dwarn:0 dfail:0 fail:0 skip:23 ro-hsw-i7-4770r total:209 pass:186 dwarn:0 dfail:0 fail:0 skip:23 ro-ilk-i7-620lm total:209 pass:146 dwarn:0 dfail:0 fail:1 skip:62 ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 skip:57 ro-ivb-i7-3770 total:209 pass:177 dwarn:0 dfail:0 fail:0 skip:32 ro-ivb2-i7-3770 total:209 pass:181 dwarn:0 dfail:0 fail:0 skip:28 ro-skl-i7-6700hq total:204 pass:179 dwarn:4 dfail:0 fail:0 skip:21 ro-snb-i7-2620M total:209 pass:170 dwarn:0 dfail:0 fail:1 skip:38 ro-bsw-n3050 failed to connect after reboot Results at /archive/results/CI_IGT_test/RO_Patchwork_1019/ fc9d741 drm-intel-nightly: 2016y-05m-25d-07h-45m-48s UTC integration manifest a43c03e drm: Store the plane's index _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Store the plane's index 2016-05-26 9:34 [PATCH] drm: Store the plane's index Chris Wilson 2016-05-26 10:02 ` ✗ Ro.CI.BAT: failure for " Patchwork @ 2016-05-26 11:27 ` Ville Syrjälä 2016-05-26 12:17 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2016-05-26 11:27 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, dri-devel On Thu, May 26, 2016 at 10:34:57AM +0100, Chris Wilson wrote: > Currently the plane's index is determined by walking the list of all > planes in the mode and finding the position of that plane in the list. A > linear walk, especially a linear walk within a linear walk as frequently > conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. > > The plane's index is constant for as long as no earlier planes are > removed from the list. For most drivers, planes are static, determined > at boot and then untouched until shutdown. Storing the index upon > construction and then only walking the tail upon removal should > be a major improvement for all. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Matt Roper <matthew.d.roper@intel.com> I've been wondering about the cost of these silly walks myself. Patch looks sane to me Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Same for crtcs? > --- > drivers/gpu/drm/drm_crtc.c | 38 +++++++++----------------------------- > include/drm/drm_crtc.h | 6 +++++- > 2 files changed, 14 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 3a0384cce4a2..00ee01126b6f 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1302,7 +1302,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > plane->type = type; > > list_add_tail(&plane->head, &config->plane_list); > - config->num_total_plane++; > + plane->index = config->num_total_plane++; > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > config->num_overlay_plane++; > > @@ -1369,6 +1369,7 @@ EXPORT_SYMBOL(drm_plane_init); > void drm_plane_cleanup(struct drm_plane *plane) > { > struct drm_device *dev = plane->dev; > + struct drm_plane *other; > > drm_modeset_lock_all(dev); > kfree(plane->format_types); > @@ -1376,6 +1377,10 @@ void drm_plane_cleanup(struct drm_plane *plane) > > BUG_ON(list_empty(&plane->head)); > > + other = list_next_entry(plane, head); > + list_for_each_entry_from(other, &dev->mode_config.plane_list, head) > + other->index--; > + > list_del(&plane->head); > dev->mode_config.num_total_plane--; > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > @@ -1393,29 +1398,6 @@ void drm_plane_cleanup(struct drm_plane *plane) > EXPORT_SYMBOL(drm_plane_cleanup); > > /** > - * drm_plane_index - find the index of a registered plane > - * @plane: plane to find index for > - * > - * Given a registered plane, return the index of that CRTC within a DRM > - * device's list of planes. > - */ > -unsigned int drm_plane_index(struct drm_plane *plane) > -{ > - unsigned int index = 0; > - struct drm_plane *tmp; > - > - drm_for_each_plane(tmp, plane->dev) { > - if (tmp == plane) > - return index; > - > - index++; > - } > - > - BUG(); > -} > -EXPORT_SYMBOL(drm_plane_index); > - > -/** > * drm_plane_from_index - find the registered plane at an index > * @dev: DRM device > * @idx: index of registered plane to find for > @@ -1427,13 +1409,11 @@ struct drm_plane * > drm_plane_from_index(struct drm_device *dev, int idx) > { > struct drm_plane *plane; > - unsigned int i = 0; > > - drm_for_each_plane(plane, dev) { > - if (i == idx) > + drm_for_each_plane(plane, dev) > + if (idx == plane->index) > return plane; > - i++; > - } > + > return NULL; > } > EXPORT_SYMBOL(drm_plane_from_index); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 9771428e1ba8..eda3b1b3d3b4 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1543,6 +1543,7 @@ struct drm_plane { > struct drm_object_properties properties; > > enum drm_plane_type type; > + unsigned index; > > const struct drm_plane_helper_funcs *helper_private; > > @@ -2316,7 +2317,10 @@ extern int drm_plane_init(struct drm_device *dev, > const uint32_t *formats, unsigned int format_count, > bool is_primary); > extern void drm_plane_cleanup(struct drm_plane *plane); > -extern unsigned int drm_plane_index(struct drm_plane *plane); > +static inline unsigned int drm_plane_index(struct drm_plane *plane) > +{ > + return plane->index; > +} > extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); > extern void drm_plane_force_disable(struct drm_plane *plane); > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm: Store the plane's index 2016-05-26 11:27 ` [PATCH] " Ville Syrjälä @ 2016-05-26 12:17 ` Chris Wilson 2016-05-26 15:17 ` Matt Roper 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2016-05-26 12:17 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx Currently the plane's index is determined by walking the list of all planes in the mode and finding the position of that plane in the list. A linear walk, especially a linear walk within a linear walk as frequently conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. The plane's index is constant for as long as no earlier planes are removed from the list. For most drivers, planes are static, determined at boot and then untouched until shutdown. Storing the index upon construction and then only walking the tail upon removal should be a major improvement for all. v2: Convert drm_crtc_index() and drm_encoder_index() as well. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_crtc.c | 98 ++++++++++------------------------------------ include/drm/drm_crtc.h | 25 ++++++++++-- 2 files changed, 43 insertions(+), 80 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2a6d958ca76..4d978099aa17 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -692,7 +692,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, crtc->base.properties = &crtc->properties; list_add_tail(&crtc->head, &config->crtc_list); - config->num_crtc++; + crtc->index = config->num_crtc++; crtc->primary = primary; crtc->cursor = cursor; @@ -721,6 +721,11 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes); void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; + struct drm_crtc *other; + + other = list_next_entry(crtc, head); + list_for_each_entry_from(other, &dev->mode_config.crtc_list, head) + other->index--; kfree(crtc->gamma_store); crtc->gamma_store = NULL; @@ -741,29 +746,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_cleanup); -/** - * drm_crtc_index - find the index of a registered CRTC - * @crtc: CRTC to find index for - * - * Given a registered CRTC, return the index of that CRTC within a DRM - * device's list of CRTCs. - */ -unsigned int drm_crtc_index(struct drm_crtc *crtc) -{ - unsigned int index = 0; - struct drm_crtc *tmp; - - drm_for_each_crtc(tmp, crtc->dev) { - if (tmp == crtc) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_crtc_index); - /* * drm_mode_remove - remove and free a mode * @connector: connector list to modify @@ -1166,7 +1148,7 @@ int drm_encoder_init(struct drm_device *dev, } list_add_tail(&encoder->head, &dev->mode_config.encoder_list); - dev->mode_config.num_encoder++; + encoder->index = dev->mode_config.num_encoder++; out_put: if (ret) @@ -1180,29 +1162,6 @@ out_unlock: EXPORT_SYMBOL(drm_encoder_init); /** - * drm_encoder_index - find the index of a registered encoder - * @encoder: encoder to find index for - * - * Given a registered encoder, return the index of that encoder within a DRM - * device's list of encoders. - */ -unsigned int drm_encoder_index(struct drm_encoder *encoder) -{ - unsigned int index = 0; - struct drm_encoder *tmp; - - drm_for_each_encoder(tmp, encoder->dev) { - if (tmp == encoder) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_encoder_index); - -/** * drm_encoder_cleanup - cleans up an initialised encoder * @encoder: encoder to cleanup * @@ -1211,6 +1170,11 @@ EXPORT_SYMBOL(drm_encoder_index); void drm_encoder_cleanup(struct drm_encoder *encoder) { struct drm_device *dev = encoder->dev; + struct drm_encoder *other; + + other = list_next_entry(encoder, head); + list_for_each_entry_from(other, &dev->mode_config.encoder_list, head) + other->index--; drm_modeset_lock_all(dev); drm_mode_object_unregister(dev, &encoder->base); @@ -1300,7 +1264,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->type = type; list_add_tail(&plane->head, &config->plane_list); - config->num_total_plane++; + plane->index = config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY) config->num_overlay_plane++; @@ -1367,6 +1331,7 @@ EXPORT_SYMBOL(drm_plane_init); void drm_plane_cleanup(struct drm_plane *plane) { struct drm_device *dev = plane->dev; + struct drm_plane *other; drm_modeset_lock_all(dev); kfree(plane->format_types); @@ -1374,6 +1339,10 @@ void drm_plane_cleanup(struct drm_plane *plane) BUG_ON(list_empty(&plane->head)); + other = list_next_entry(plane, head); + list_for_each_entry_from(other, &dev->mode_config.plane_list, head) + other->index--; + list_del(&plane->head); dev->mode_config.num_total_plane--; if (plane->type == DRM_PLANE_TYPE_OVERLAY) @@ -1391,29 +1360,6 @@ void drm_plane_cleanup(struct drm_plane *plane) EXPORT_SYMBOL(drm_plane_cleanup); /** - * drm_plane_index - find the index of a registered plane - * @plane: plane to find index for - * - * Given a registered plane, return the index of that CRTC within a DRM - * device's list of planes. - */ -unsigned int drm_plane_index(struct drm_plane *plane) -{ - unsigned int index = 0; - struct drm_plane *tmp; - - drm_for_each_plane(tmp, plane->dev) { - if (tmp == plane) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_plane_index); - -/** * drm_plane_from_index - find the registered plane at an index * @dev: DRM device * @idx: index of registered plane to find for @@ -1425,13 +1371,11 @@ struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx) { struct drm_plane *plane; - unsigned int i = 0; - drm_for_each_plane(plane, dev) { - if (i == idx) + drm_for_each_plane(plane, dev) + if (idx == plane->index) return plane; - i++; - } + return NULL; } EXPORT_SYMBOL(drm_plane_from_index); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 150f7864c3ca..66b07c911e67 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -754,6 +754,9 @@ struct drm_crtc { struct drm_plane *primary; struct drm_plane *cursor; + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + /* position of cursor plane on crtc */ int cursor_x; int cursor_y; @@ -1098,6 +1101,10 @@ struct drm_encoder { struct drm_mode_object base; char *name; int encoder_type; + + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + uint32_t possible_crtcs; uint32_t possible_clones; @@ -1544,6 +1551,9 @@ struct drm_plane { enum drm_plane_type type; + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + const struct drm_plane_helper_funcs *helper_private; struct drm_plane_state *state; @@ -2231,7 +2241,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, const struct drm_crtc_funcs *funcs, const char *name, ...); extern void drm_crtc_cleanup(struct drm_crtc *crtc); -extern unsigned int drm_crtc_index(struct drm_crtc *crtc); +static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) +{ + return crtc->index; +} /** * drm_crtc_mask - find the mask of a registered CRTC @@ -2285,7 +2298,10 @@ int drm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...); -extern unsigned int drm_encoder_index(struct drm_encoder *encoder); +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder) +{ + return encoder->index; +} /** * drm_encoder_crtc_ok - can a given crtc drive a given encoder? @@ -2316,7 +2332,10 @@ extern int drm_plane_init(struct drm_device *dev, const uint32_t *formats, unsigned int format_count, bool is_primary); extern void drm_plane_cleanup(struct drm_plane *plane); -extern unsigned int drm_plane_index(struct drm_plane *plane); +static inline unsigned int drm_plane_index(struct drm_plane *plane) +{ + return plane->index; +} extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); extern void drm_plane_force_disable(struct drm_plane *plane); extern int drm_plane_check_pixel_format(const struct drm_plane *plane, -- 2.8.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Store the plane's index 2016-05-26 12:17 ` Chris Wilson @ 2016-05-26 15:17 ` Matt Roper 2016-05-27 6:44 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Matt Roper @ 2016-05-26 15:17 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, dri-devel On Thu, May 26, 2016 at 01:17:18PM +0100, Chris Wilson wrote: > Currently the plane's index is determined by walking the list of all > planes in the mode and finding the position of that plane in the list. A > linear walk, especially a linear walk within a linear walk as frequently > conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. > > The plane's index is constant for as long as no earlier planes are > removed from the list. For most drivers, planes are static, determined > at boot and then untouched until shutdown. Storing the index upon > construction and then only walking the tail upon removal should > be a major improvement for all. > > v2: Convert drm_crtc_index() and drm_encoder_index() as well. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Looks good to me. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Your commit message says "most drivers" have static planes/crtcs/encoders...are there any drivers today that this isn't true for? Wouldn't we need special locking for list iteration in general like we have for connectors if that was the case? Matt > --- > drivers/gpu/drm/drm_crtc.c | 98 ++++++++++------------------------------------ > include/drm/drm_crtc.h | 25 ++++++++++-- > 2 files changed, 43 insertions(+), 80 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index d2a6d958ca76..4d978099aa17 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -692,7 +692,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > crtc->base.properties = &crtc->properties; > > list_add_tail(&crtc->head, &config->crtc_list); > - config->num_crtc++; > + crtc->index = config->num_crtc++; > > crtc->primary = primary; > crtc->cursor = cursor; > @@ -721,6 +721,11 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes); > void drm_crtc_cleanup(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > + struct drm_crtc *other; > + > + other = list_next_entry(crtc, head); > + list_for_each_entry_from(other, &dev->mode_config.crtc_list, head) > + other->index--; > > kfree(crtc->gamma_store); > crtc->gamma_store = NULL; > @@ -741,29 +746,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > } > EXPORT_SYMBOL(drm_crtc_cleanup); > > -/** > - * drm_crtc_index - find the index of a registered CRTC > - * @crtc: CRTC to find index for > - * > - * Given a registered CRTC, return the index of that CRTC within a DRM > - * device's list of CRTCs. > - */ > -unsigned int drm_crtc_index(struct drm_crtc *crtc) > -{ > - unsigned int index = 0; > - struct drm_crtc *tmp; > - > - drm_for_each_crtc(tmp, crtc->dev) { > - if (tmp == crtc) > - return index; > - > - index++; > - } > - > - BUG(); > -} > -EXPORT_SYMBOL(drm_crtc_index); > - > /* > * drm_mode_remove - remove and free a mode > * @connector: connector list to modify > @@ -1166,7 +1148,7 @@ int drm_encoder_init(struct drm_device *dev, > } > > list_add_tail(&encoder->head, &dev->mode_config.encoder_list); > - dev->mode_config.num_encoder++; > + encoder->index = dev->mode_config.num_encoder++; > > out_put: > if (ret) > @@ -1180,29 +1162,6 @@ out_unlock: > EXPORT_SYMBOL(drm_encoder_init); > > /** > - * drm_encoder_index - find the index of a registered encoder > - * @encoder: encoder to find index for > - * > - * Given a registered encoder, return the index of that encoder within a DRM > - * device's list of encoders. > - */ > -unsigned int drm_encoder_index(struct drm_encoder *encoder) > -{ > - unsigned int index = 0; > - struct drm_encoder *tmp; > - > - drm_for_each_encoder(tmp, encoder->dev) { > - if (tmp == encoder) > - return index; > - > - index++; > - } > - > - BUG(); > -} > -EXPORT_SYMBOL(drm_encoder_index); > - > -/** > * drm_encoder_cleanup - cleans up an initialised encoder > * @encoder: encoder to cleanup > * > @@ -1211,6 +1170,11 @@ EXPORT_SYMBOL(drm_encoder_index); > void drm_encoder_cleanup(struct drm_encoder *encoder) > { > struct drm_device *dev = encoder->dev; > + struct drm_encoder *other; > + > + other = list_next_entry(encoder, head); > + list_for_each_entry_from(other, &dev->mode_config.encoder_list, head) > + other->index--; > > drm_modeset_lock_all(dev); > drm_mode_object_unregister(dev, &encoder->base); > @@ -1300,7 +1264,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > plane->type = type; > > list_add_tail(&plane->head, &config->plane_list); > - config->num_total_plane++; > + plane->index = config->num_total_plane++; > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > config->num_overlay_plane++; > > @@ -1367,6 +1331,7 @@ EXPORT_SYMBOL(drm_plane_init); > void drm_plane_cleanup(struct drm_plane *plane) > { > struct drm_device *dev = plane->dev; > + struct drm_plane *other; > > drm_modeset_lock_all(dev); > kfree(plane->format_types); > @@ -1374,6 +1339,10 @@ void drm_plane_cleanup(struct drm_plane *plane) > > BUG_ON(list_empty(&plane->head)); > > + other = list_next_entry(plane, head); > + list_for_each_entry_from(other, &dev->mode_config.plane_list, head) > + other->index--; > + > list_del(&plane->head); > dev->mode_config.num_total_plane--; > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > @@ -1391,29 +1360,6 @@ void drm_plane_cleanup(struct drm_plane *plane) > EXPORT_SYMBOL(drm_plane_cleanup); > > /** > - * drm_plane_index - find the index of a registered plane > - * @plane: plane to find index for > - * > - * Given a registered plane, return the index of that CRTC within a DRM > - * device's list of planes. > - */ > -unsigned int drm_plane_index(struct drm_plane *plane) > -{ > - unsigned int index = 0; > - struct drm_plane *tmp; > - > - drm_for_each_plane(tmp, plane->dev) { > - if (tmp == plane) > - return index; > - > - index++; > - } > - > - BUG(); > -} > -EXPORT_SYMBOL(drm_plane_index); > - > -/** > * drm_plane_from_index - find the registered plane at an index > * @dev: DRM device > * @idx: index of registered plane to find for > @@ -1425,13 +1371,11 @@ struct drm_plane * > drm_plane_from_index(struct drm_device *dev, int idx) > { > struct drm_plane *plane; > - unsigned int i = 0; > > - drm_for_each_plane(plane, dev) { > - if (i == idx) > + drm_for_each_plane(plane, dev) > + if (idx == plane->index) > return plane; > - i++; > - } > + > return NULL; > } > EXPORT_SYMBOL(drm_plane_from_index); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 150f7864c3ca..66b07c911e67 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -754,6 +754,9 @@ struct drm_crtc { > struct drm_plane *primary; > struct drm_plane *cursor; > > + /* position inside the mode_config.list, can be used as a [] idx */ > + unsigned index; > + > /* position of cursor plane on crtc */ > int cursor_x; > int cursor_y; > @@ -1098,6 +1101,10 @@ struct drm_encoder { > struct drm_mode_object base; > char *name; > int encoder_type; > + > + /* position inside the mode_config.list, can be used as a [] idx */ > + unsigned index; > + > uint32_t possible_crtcs; > uint32_t possible_clones; > > @@ -1544,6 +1551,9 @@ struct drm_plane { > > enum drm_plane_type type; > > + /* position inside the mode_config.list, can be used as a [] idx */ > + unsigned index; > + > const struct drm_plane_helper_funcs *helper_private; > > struct drm_plane_state *state; > @@ -2231,7 +2241,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > const struct drm_crtc_funcs *funcs, > const char *name, ...); > extern void drm_crtc_cleanup(struct drm_crtc *crtc); > -extern unsigned int drm_crtc_index(struct drm_crtc *crtc); > +static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) > +{ > + return crtc->index; > +} > > /** > * drm_crtc_mask - find the mask of a registered CRTC > @@ -2285,7 +2298,10 @@ int drm_encoder_init(struct drm_device *dev, > struct drm_encoder *encoder, > const struct drm_encoder_funcs *funcs, > int encoder_type, const char *name, ...); > -extern unsigned int drm_encoder_index(struct drm_encoder *encoder); > +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder) > +{ > + return encoder->index; > +} > > /** > * drm_encoder_crtc_ok - can a given crtc drive a given encoder? > @@ -2316,7 +2332,10 @@ extern int drm_plane_init(struct drm_device *dev, > const uint32_t *formats, unsigned int format_count, > bool is_primary); > extern void drm_plane_cleanup(struct drm_plane *plane); > -extern unsigned int drm_plane_index(struct drm_plane *plane); > +static inline unsigned int drm_plane_index(struct drm_plane *plane) > +{ > + return plane->index; > +} > extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); > extern void drm_plane_force_disable(struct drm_plane *plane); > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > -- > 2.8.1 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Store the plane's index 2016-05-26 15:17 ` Matt Roper @ 2016-05-27 6:44 ` Daniel Vetter 2016-05-27 19:05 ` [PATCH v3] " Chris Wilson 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2016-05-27 6:44 UTC (permalink / raw) To: Matt Roper; +Cc: Daniel Vetter, intel-gfx, dri-devel On Thu, May 26, 2016 at 08:17:40AM -0700, Matt Roper wrote: > On Thu, May 26, 2016 at 01:17:18PM +0100, Chris Wilson wrote: > > Currently the plane's index is determined by walking the list of all > > planes in the mode and finding the position of that plane in the list. A > > linear walk, especially a linear walk within a linear walk as frequently > > conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. > > > > The plane's index is constant for as long as no earlier planes are > > removed from the list. For most drivers, planes are static, determined > > at boot and then untouched until shutdown. Storing the index upon > > construction and then only walking the tail upon removal should > > be a major improvement for all. > > > > v2: Convert drm_crtc_index() and drm_encoder_index() as well. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Looks good to me. > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > Your commit message says "most drivers" have static > planes/crtcs/encoders...are there any drivers today that this isn't true > for? Wouldn't we need special locking for list iteration in general > like we have for connectors if that was the case? s/most/all/. DRM only allows hotplugging of connectors, nothing else. That also means we don't need any special code in drm_*_cleanup. Can you pls respin once more? More nitpicks below. > > > Matt > > > --- > > drivers/gpu/drm/drm_crtc.c | 98 ++++++++++------------------------------------ > > include/drm/drm_crtc.h | 25 ++++++++++-- > > 2 files changed, 43 insertions(+), 80 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index d2a6d958ca76..4d978099aa17 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -692,7 +692,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > > crtc->base.properties = &crtc->properties; > > > > list_add_tail(&crtc->head, &config->crtc_list); > > - config->num_crtc++; > > + crtc->index = config->num_crtc++; > > > > crtc->primary = primary; > > crtc->cursor = cursor; > > @@ -721,6 +721,11 @@ EXPORT_SYMBOL(drm_crtc_init_with_planes); > > void drm_crtc_cleanup(struct drm_crtc *crtc) > > { > > struct drm_device *dev = crtc->dev; > > + struct drm_crtc *other; > > + > > + other = list_next_entry(crtc, head); > > + list_for_each_entry_from(other, &dev->mode_config.crtc_list, head) > > + other->index--; > > > > kfree(crtc->gamma_store); > > crtc->gamma_store = NULL; > > @@ -741,29 +746,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) > > } > > EXPORT_SYMBOL(drm_crtc_cleanup); > > > > -/** > > - * drm_crtc_index - find the index of a registered CRTC > > - * @crtc: CRTC to find index for > > - * > > - * Given a registered CRTC, return the index of that CRTC within a DRM > > - * device's list of CRTCs. > > - */ Please move kerneldoc, too. Thanks, Daniel > > -unsigned int drm_crtc_index(struct drm_crtc *crtc) > > -{ > > - unsigned int index = 0; > > - struct drm_crtc *tmp; > > - > > - drm_for_each_crtc(tmp, crtc->dev) { > > - if (tmp == crtc) > > - return index; > > - > > - index++; > > - } > > - > > - BUG(); > > -} > > -EXPORT_SYMBOL(drm_crtc_index); > > - > > /* > > * drm_mode_remove - remove and free a mode > > * @connector: connector list to modify > > @@ -1166,7 +1148,7 @@ int drm_encoder_init(struct drm_device *dev, > > } > > > > list_add_tail(&encoder->head, &dev->mode_config.encoder_list); > > - dev->mode_config.num_encoder++; > > + encoder->index = dev->mode_config.num_encoder++; > > > > out_put: > > if (ret) > > @@ -1180,29 +1162,6 @@ out_unlock: > > EXPORT_SYMBOL(drm_encoder_init); > > > > /** > > - * drm_encoder_index - find the index of a registered encoder > > - * @encoder: encoder to find index for > > - * > > - * Given a registered encoder, return the index of that encoder within a DRM > > - * device's list of encoders. > > - */ > > -unsigned int drm_encoder_index(struct drm_encoder *encoder) > > -{ > > - unsigned int index = 0; > > - struct drm_encoder *tmp; > > - > > - drm_for_each_encoder(tmp, encoder->dev) { > > - if (tmp == encoder) > > - return index; > > - > > - index++; > > - } > > - > > - BUG(); > > -} > > -EXPORT_SYMBOL(drm_encoder_index); > > - > > -/** > > * drm_encoder_cleanup - cleans up an initialised encoder > > * @encoder: encoder to cleanup > > * > > @@ -1211,6 +1170,11 @@ EXPORT_SYMBOL(drm_encoder_index); > > void drm_encoder_cleanup(struct drm_encoder *encoder) > > { > > struct drm_device *dev = encoder->dev; > > + struct drm_encoder *other; > > + > > + other = list_next_entry(encoder, head); > > + list_for_each_entry_from(other, &dev->mode_config.encoder_list, head) > > + other->index--; > > > > drm_modeset_lock_all(dev); > > drm_mode_object_unregister(dev, &encoder->base); > > @@ -1300,7 +1264,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > > plane->type = type; > > > > list_add_tail(&plane->head, &config->plane_list); > > - config->num_total_plane++; > > + plane->index = config->num_total_plane++; > > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > > config->num_overlay_plane++; > > > > @@ -1367,6 +1331,7 @@ EXPORT_SYMBOL(drm_plane_init); > > void drm_plane_cleanup(struct drm_plane *plane) > > { > > struct drm_device *dev = plane->dev; > > + struct drm_plane *other; > > > > drm_modeset_lock_all(dev); > > kfree(plane->format_types); > > @@ -1374,6 +1339,10 @@ void drm_plane_cleanup(struct drm_plane *plane) > > > > BUG_ON(list_empty(&plane->head)); > > > > + other = list_next_entry(plane, head); > > + list_for_each_entry_from(other, &dev->mode_config.plane_list, head) > > + other->index--; > > + > > list_del(&plane->head); > > dev->mode_config.num_total_plane--; > > if (plane->type == DRM_PLANE_TYPE_OVERLAY) > > @@ -1391,29 +1360,6 @@ void drm_plane_cleanup(struct drm_plane *plane) > > EXPORT_SYMBOL(drm_plane_cleanup); > > > > /** > > - * drm_plane_index - find the index of a registered plane > > - * @plane: plane to find index for > > - * > > - * Given a registered plane, return the index of that CRTC within a DRM > > - * device's list of planes. > > - */ > > -unsigned int drm_plane_index(struct drm_plane *plane) > > -{ > > - unsigned int index = 0; > > - struct drm_plane *tmp; > > - > > - drm_for_each_plane(tmp, plane->dev) { > > - if (tmp == plane) > > - return index; > > - > > - index++; > > - } > > - > > - BUG(); > > -} > > -EXPORT_SYMBOL(drm_plane_index); > > - > > -/** > > * drm_plane_from_index - find the registered plane at an index > > * @dev: DRM device > > * @idx: index of registered plane to find for > > @@ -1425,13 +1371,11 @@ struct drm_plane * > > drm_plane_from_index(struct drm_device *dev, int idx) > > { > > struct drm_plane *plane; > > - unsigned int i = 0; > > > > - drm_for_each_plane(plane, dev) { > > - if (i == idx) > > + drm_for_each_plane(plane, dev) > > + if (idx == plane->index) > > return plane; > > - i++; > > - } > > + > > return NULL; > > } > > EXPORT_SYMBOL(drm_plane_from_index); > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 150f7864c3ca..66b07c911e67 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -754,6 +754,9 @@ struct drm_crtc { > > struct drm_plane *primary; > > struct drm_plane *cursor; > > > > + /* position inside the mode_config.list, can be used as a [] idx */ > > + unsigned index; > > + > > /* position of cursor plane on crtc */ > > int cursor_x; > > int cursor_y; > > @@ -1098,6 +1101,10 @@ struct drm_encoder { > > struct drm_mode_object base; > > char *name; > > int encoder_type; > > + > > + /* position inside the mode_config.list, can be used as a [] idx */ > > + unsigned index; > > + > > uint32_t possible_crtcs; > > uint32_t possible_clones; > > > > @@ -1544,6 +1551,9 @@ struct drm_plane { > > > > enum drm_plane_type type; > > > > + /* position inside the mode_config.list, can be used as a [] idx */ > > + unsigned index; > > + > > const struct drm_plane_helper_funcs *helper_private; > > > > struct drm_plane_state *state; > > @@ -2231,7 +2241,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > > const struct drm_crtc_funcs *funcs, > > const char *name, ...); > > extern void drm_crtc_cleanup(struct drm_crtc *crtc); > > -extern unsigned int drm_crtc_index(struct drm_crtc *crtc); > > +static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) > > +{ > > + return crtc->index; > > +} > > > > /** > > * drm_crtc_mask - find the mask of a registered CRTC > > @@ -2285,7 +2298,10 @@ int drm_encoder_init(struct drm_device *dev, > > struct drm_encoder *encoder, > > const struct drm_encoder_funcs *funcs, > > int encoder_type, const char *name, ...); > > -extern unsigned int drm_encoder_index(struct drm_encoder *encoder); > > +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder) > > +{ > > + return encoder->index; > > +} > > > > /** > > * drm_encoder_crtc_ok - can a given crtc drive a given encoder? > > @@ -2316,7 +2332,10 @@ extern int drm_plane_init(struct drm_device *dev, > > const uint32_t *formats, unsigned int format_count, > > bool is_primary); > > extern void drm_plane_cleanup(struct drm_plane *plane); > > -extern unsigned int drm_plane_index(struct drm_plane *plane); > > +static inline unsigned int drm_plane_index(struct drm_plane *plane) > > +{ > > + return plane->index; > > +} > > extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); > > extern void drm_plane_force_disable(struct drm_plane *plane); > > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > > -- > > 2.8.1 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] drm: Store the plane's index 2016-05-27 6:44 ` Daniel Vetter @ 2016-05-27 19:05 ` Chris Wilson 2016-05-27 20:43 ` Matt Roper 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2016-05-27 19:05 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx Currently the plane's index is determined by walking the list of all planes in the mode and finding the position of that plane in the list. A linear walk, especially a linear walk within a linear walk as frequently conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. The plane's index is constant for as long as no earlier planes are removed from the list. For all drivers, planes are static, determined at boot and then untouched until shutdown. In fact, there is no locking provided to allow for dynamic removal of planes/encoders/crtcs. v2: Convert drm_crtc_index() and drm_encoder_index() as well. v3: Stop adjusting the indices upon removal; consider the list construct-only. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_crtc.c | 98 ++++++++++------------------------------------ include/drm/drm_crtc.h | 49 +++++++++++++++++++++-- 2 files changed, 67 insertions(+), 80 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 81641544ac3e..0ce62e3bddd2 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -692,7 +692,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, crtc->base.properties = &crtc->properties; list_add_tail(&crtc->head, &config->crtc_list); - config->num_crtc++; + crtc->index = config->num_crtc++; crtc->primary = primary; crtc->cursor = cursor; @@ -722,6 +722,11 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; + /* Note that the crtc_list is considered to be static; should we + * remove the drm_crtc at runtime we would have to decrement all + * the indices on the drm_crtc after us in the crtc_list. + */ + kfree(crtc->gamma_store); crtc->gamma_store = NULL; @@ -741,29 +746,6 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_cleanup); -/** - * drm_crtc_index - find the index of a registered CRTC - * @crtc: CRTC to find index for - * - * Given a registered CRTC, return the index of that CRTC within a DRM - * device's list of CRTCs. - */ -unsigned int drm_crtc_index(struct drm_crtc *crtc) -{ - unsigned int index = 0; - struct drm_crtc *tmp; - - drm_for_each_crtc(tmp, crtc->dev) { - if (tmp == crtc) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_crtc_index); - /* * drm_mode_remove - remove and free a mode * @connector: connector list to modify @@ -1180,7 +1162,7 @@ int drm_encoder_init(struct drm_device *dev, } list_add_tail(&encoder->head, &dev->mode_config.encoder_list); - dev->mode_config.num_encoder++; + encoder->index = dev->mode_config.num_encoder++; out_put: if (ret) @@ -1194,29 +1176,6 @@ out_unlock: EXPORT_SYMBOL(drm_encoder_init); /** - * drm_encoder_index - find the index of a registered encoder - * @encoder: encoder to find index for - * - * Given a registered encoder, return the index of that encoder within a DRM - * device's list of encoders. - */ -unsigned int drm_encoder_index(struct drm_encoder *encoder) -{ - unsigned int index = 0; - struct drm_encoder *tmp; - - drm_for_each_encoder(tmp, encoder->dev) { - if (tmp == encoder) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_encoder_index); - -/** * drm_encoder_cleanup - cleans up an initialised encoder * @encoder: encoder to cleanup * @@ -1226,6 +1185,11 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) { struct drm_device *dev = encoder->dev; + /* Note that the encoder_list is considered to be static; should we + * remove the drm_encoder at runtime we would have to decrement all + * the indices on the drm_encoder after us in the encoder_list. + */ + drm_modeset_lock_all(dev); drm_mode_object_unregister(dev, &encoder->base); kfree(encoder->name); @@ -1314,7 +1278,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, plane->type = type; list_add_tail(&plane->head, &config->plane_list); - config->num_total_plane++; + plane->index = config->num_total_plane++; if (plane->type == DRM_PLANE_TYPE_OVERLAY) config->num_overlay_plane++; @@ -1388,6 +1352,11 @@ void drm_plane_cleanup(struct drm_plane *plane) BUG_ON(list_empty(&plane->head)); + /* Note that the plane_list is considered to be static; should we + * remove the drm_plane at runtime we would have to decrement all + * the indices on the drm_plane after us in the plane_list. + */ + list_del(&plane->head); dev->mode_config.num_total_plane--; if (plane->type == DRM_PLANE_TYPE_OVERLAY) @@ -1405,29 +1374,6 @@ void drm_plane_cleanup(struct drm_plane *plane) EXPORT_SYMBOL(drm_plane_cleanup); /** - * drm_plane_index - find the index of a registered plane - * @plane: plane to find index for - * - * Given a registered plane, return the index of that CRTC within a DRM - * device's list of planes. - */ -unsigned int drm_plane_index(struct drm_plane *plane) -{ - unsigned int index = 0; - struct drm_plane *tmp; - - drm_for_each_plane(tmp, plane->dev) { - if (tmp == plane) - return index; - - index++; - } - - BUG(); -} -EXPORT_SYMBOL(drm_plane_index); - -/** * drm_plane_from_index - find the registered plane at an index * @dev: DRM device * @idx: index of registered plane to find for @@ -1439,13 +1385,11 @@ struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx) { struct drm_plane *plane; - unsigned int i = 0; - drm_for_each_plane(plane, dev) { - if (i == idx) + drm_for_each_plane(plane, dev) + if (idx == plane->index) return plane; - i++; - } + return NULL; } EXPORT_SYMBOL(drm_plane_from_index); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d6fb1a763ea3..a2571c4e29a0 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -754,6 +754,9 @@ struct drm_crtc { struct drm_plane *primary; struct drm_plane *cursor; + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + /* position of cursor plane on crtc */ int cursor_x; int cursor_y; @@ -1119,6 +1122,10 @@ struct drm_encoder { struct drm_mode_object base; char *name; int encoder_type; + + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + uint32_t possible_crtcs; uint32_t possible_clones; @@ -1565,6 +1572,9 @@ struct drm_plane { enum drm_plane_type type; + /* position inside the mode_config.list, can be used as a [] idx */ + unsigned index; + const struct drm_plane_helper_funcs *helper_private; struct drm_plane_state *state; @@ -2252,7 +2262,18 @@ int drm_crtc_init_with_planes(struct drm_device *dev, const struct drm_crtc_funcs *funcs, const char *name, ...); extern void drm_crtc_cleanup(struct drm_crtc *crtc); -extern unsigned int drm_crtc_index(struct drm_crtc *crtc); + +/** + * drm_crtc_index - find the index of a registered CRTC + * @crtc: CRTC to find index for + * + * Given a registered CRTC, return the index of that CRTC within a DRM + * device's list of CRTCs. + */ +static inline unsigned int drm_crtc_index(struct drm_crtc *crtc) +{ + return crtc->index; +} /** * drm_crtc_mask - find the mask of a registered CRTC @@ -2306,7 +2327,18 @@ int drm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...); -extern unsigned int drm_encoder_index(struct drm_encoder *encoder); + +/** + * drm_encoder_index - find the index of a registered encoder + * @encoder: encoder to find index for + * + * Given a registered encoder, return the index of that encoder within a DRM + * device's list of encoders. + */ +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder) +{ + return encoder->index; +} /** * drm_encoder_crtc_ok - can a given crtc drive a given encoder? @@ -2337,7 +2369,18 @@ extern int drm_plane_init(struct drm_device *dev, const uint32_t *formats, unsigned int format_count, bool is_primary); extern void drm_plane_cleanup(struct drm_plane *plane); -extern unsigned int drm_plane_index(struct drm_plane *plane); + +/** + * drm_plane_index - find the index of a registered plane + * @plane: plane to find index for + * + * Given a registered plane, return the index of that CRTC within a DRM + * device's list of planes. + */ +static inline unsigned int drm_plane_index(struct drm_plane *plane) +{ + return plane->index; +} extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); extern void drm_plane_force_disable(struct drm_plane *plane); extern int drm_plane_check_pixel_format(const struct drm_plane *plane, -- 2.8.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drm: Store the plane's index 2016-05-27 19:05 ` [PATCH v3] " Chris Wilson @ 2016-05-27 20:43 ` Matt Roper 2016-06-02 22:02 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Matt Roper @ 2016-05-27 20:43 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, dri-devel On Fri, May 27, 2016 at 08:05:00PM +0100, Chris Wilson wrote: > Currently the plane's index is determined by walking the list of all > planes in the mode and finding the position of that plane in the list. A > linear walk, especially a linear walk within a linear walk as frequently > conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. > > The plane's index is constant for as long as no earlier planes are > removed from the list. For all drivers, planes are static, determined > at boot and then untouched until shutdown. In fact, there is no locking > provided to allow for dynamic removal of planes/encoders/crtcs. > > v2: Convert drm_crtc_index() and drm_encoder_index() as well. > v3: Stop adjusting the indices upon removal; consider the list > construct-only. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- ...snip... > @@ -2337,7 +2369,18 @@ extern int drm_plane_init(struct drm_device *dev, > const uint32_t *formats, unsigned int format_count, > bool is_primary); > extern void drm_plane_cleanup(struct drm_plane *plane); > -extern unsigned int drm_plane_index(struct drm_plane *plane); > + > +/** > + * drm_plane_index - find the index of a registered plane > + * @plane: plane to find index for > + * > + * Given a registered plane, return the index of that CRTC within a DRM "index of that plane" Otherwise, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > + * device's list of planes. > + */ > +static inline unsigned int drm_plane_index(struct drm_plane *plane) > +{ > + return plane->index; > +} > extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); > extern void drm_plane_force_disable(struct drm_plane *plane); > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > -- > 2.8.1 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drm: Store the plane's index 2016-05-27 20:43 ` Matt Roper @ 2016-06-02 22:02 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2016-06-02 22:02 UTC (permalink / raw) To: Matt Roper; +Cc: Daniel Vetter, intel-gfx, dri-devel On Fri, May 27, 2016 at 01:43:19PM -0700, Matt Roper wrote: > On Fri, May 27, 2016 at 08:05:00PM +0100, Chris Wilson wrote: > > Currently the plane's index is determined by walking the list of all > > planes in the mode and finding the position of that plane in the list. A > > linear walk, especially a linear walk within a linear walk as frequently > > conceived by i915.ko [O(N^2)] quickly comes to dominate profiles. > > > > The plane's index is constant for as long as no earlier planes are > > removed from the list. For all drivers, planes are static, determined > > at boot and then untouched until shutdown. In fact, there is no locking > > provided to allow for dynamic removal of planes/encoders/crtcs. > > > > v2: Convert drm_crtc_index() and drm_encoder_index() as well. > > v3: Stop adjusting the indices upon removal; consider the list > > construct-only. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > ...snip... > > @@ -2337,7 +2369,18 @@ extern int drm_plane_init(struct drm_device *dev, > > const uint32_t *formats, unsigned int format_count, > > bool is_primary); > > extern void drm_plane_cleanup(struct drm_plane *plane); > > -extern unsigned int drm_plane_index(struct drm_plane *plane); > > + > > +/** > > + * drm_plane_index - find the index of a registered plane > > + * @plane: plane to find index for > > + * > > + * Given a registered plane, return the index of that CRTC within a DRM > > "index of that plane" Fixed .. > > Otherwise, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> ... and applied to drm-misc. -Daniel > > > > Matt > > > + * device's list of planes. > > + */ > > +static inline unsigned int drm_plane_index(struct drm_plane *plane) > > +{ > > + return plane->index; > > +} > > extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx); > > extern void drm_plane_force_disable(struct drm_plane *plane); > > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > > -- > > 2.8.1 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-02 22:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-26 9:34 [PATCH] drm: Store the plane's index Chris Wilson 2016-05-26 10:02 ` ✗ Ro.CI.BAT: failure for " Patchwork 2016-05-26 11:27 ` [PATCH] " Ville Syrjälä 2016-05-26 12:17 ` Chris Wilson 2016-05-26 15:17 ` Matt Roper 2016-05-27 6:44 ` Daniel Vetter 2016-05-27 19:05 ` [PATCH v3] " Chris Wilson 2016-05-27 20:43 ` Matt Roper 2016-06-02 22:02 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox