Re: [Intel-gfx] [PATCH 2/5] drm/fb_helper: add connector reference counting.

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 27, 2016 at 12:03:26PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This takes a reference count when fbdev adds the connector,
> and drops it when it removes the connector.
> 
> It also drops the now unneeded code to find connectors
> and remove the from the modeset as they are reference counted.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Ok, I screwed up part of my first review, and add_all_connectors calls
add_one_connector. That looks like we'll leak non-mst connectors when
unloading i915. I think we need to add a loop to drm_fb_helper_fini and
call drm_fb_helper_remove_one_connector on all the connectors still in the
helper list. I think otherwise it looks good.

Would be really good to give this is a spin with full debugging enabled
when reloading i915.ko. Just to make sure I didn't miss anything. Or cc
intel-gfx and CI should be able to pick it up and give it a spin.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 34 ++--------------------------------
>  1 file changed, 2 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..b2f58fb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
>  	if (!fb_helper_connector)
>  		return -ENOMEM;
>  
> +	drm_connector_reference(connector);
>  	fb_helper_connector->connector = connector;
>  	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>  
> -static void remove_from_modeset(struct drm_mode_set *set,
> -		struct drm_connector *connector)
> -{
> -	int i, j;
> -
> -	for (i = 0; i < set->num_connectors; i++) {
> -		if (set->connectors[i] == connector)
> -			break;
> -	}
> -
> -	if (i == set->num_connectors)
> -		return;
> -
> -	for (j = i + 1; j < set->num_connectors; j++) {
> -		set->connectors[j - 1] = set->connectors[j];
> -	}
> -	set->num_connectors--;
> -
> -	/*
> -	 * TODO maybe need to makes sure we set it back to !=NULL somewhere?
> -	 */
> -	if (set->num_connectors == 0) {
> -		set->fb = NULL;
> -		drm_mode_destroy(connector->dev, set->mode);
> -		set->mode = NULL;
> -	}
> -}
> -
>  int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  				       struct drm_connector *connector)
>  {
> @@ -206,6 +179,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	if (i == fb_helper->connector_count)
>  		return -EINVAL;
>  	fb_helper_connector = fb_helper->connector_info[i];
> +	drm_connector_unreference(fb_helper_connector->connector);
>  
>  	for (j = i + 1; j < fb_helper->connector_count; j++) {
>  		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
> @@ -213,10 +187,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	fb_helper->connector_count--;
>  	kfree(fb_helper_connector);
>  
> -	/* also cleanup dangling references to the connector: */
> -	for (i = 0; i < fb_helper->crtc_count; i++)
> -		remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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