Re: [PATCH] drm: Store the plane's index

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux