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@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Looks good to me.
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
