[Intel-gfx] [PATCH 6/7] drm: Don't walk fb list twice in getresources

[Daniel Vetter]

On Fri, Dec 09, 2016 at 09:57:45PM +0000, Chris Wilson wrote:
> On Fri, Dec 09, 2016 at 03:19:43PM +0100, Daniel Vetter wrote:
> > We can be more clever than that and merge the 2 list walking loops.
> > It's all under the one mutex critical section anyway, so nothing funny
> > can happen.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/drm_mode_config.c | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 64c2971478db..a6205a92dfee 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -84,7 +84,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  			  struct drm_file *file_priv)
> >  {
> >  	struct drm_mode_card_res *card_res = data;
> > -	struct list_head *lh;
> >  	struct drm_framebuffer *fb;
> >  	struct drm_connector *connector;
> >  	struct drm_crtc *crtc;
> > @@ -105,25 +104,20 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  
> >  
> >  	mutex_lock(&file_priv->fbs_lock);
> > -	/*
> > -	 * For the non-control nodes we need to limit the list of resources
> > -	 * by IDs in the group list for this node
> > -	 */
> > -	list_for_each(lh, &file_priv->fbs)
> > -		fb_count++;
> > -
> >  	/* handle this in 4 parts */
> >  	/* FBs */
> > -	if (card_res->count_fbs >= fb_count) {
> > -		copied = 0;
> > -		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
> > -		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> > -			if (put_user(fb->base.id, fb_id + copied)) {
> > -				mutex_unlock(&file_priv->fbs_lock);
> > -				return -EFAULT;
> > -			}
> > -			copied++;
> > +	copied = 0;
> > +	fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
> > +	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> > +		fb_count++;
> > +		if (fb_count > card_res->count_fbs)
> > +			continue;
> > +
> > +		if (put_user(fb->base.id, fb_id + copied)) {
> > +			mutex_unlock(&file_priv->fbs_lock);
> > +			return -EFAULT;
> >  		}
> > +		copied++;
> 
> if (fb_count < card_res->count_fbs &&
>     put_user(fb->base.id fb_id + fb_count) {
> }
> fb_count++; // no need for copied
> 
> I'd just like for one style :) Otoh, this seems reasonable to apply to
> crtc/encoders/connectors as well, and on the other we already have a
> counter for the others. Hmm, setting count = copied in the previous
> patch is thereby wrong (change in uABI).

It's only a change if the connector count changed between when we read it
and when we walked the list. But I guess you've convinced me that
simplifying this loops more is a good idea, and this way we can use the
same style for all of them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch