Re: [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector

["Noralf Trønnes" <noralf@tronnes.org>]

Den 16.05.2019 15.07, skrev Sam Ravnborg:
> Hi Noralf.
> 
> See few comments in the following.
> 
> 	Sam
> 
> On Mon, May 06, 2019 at 08:01:36PM +0200, Noralf Trønnes wrote:
>> All drivers add all their connectors so there's no need to keep around an
>> array of available connectors.

I could expand on this text a little:

All drivers add all their connectors so there's no need to keep around an
array of available connectors. Instead we just put the useable (not
writeback) connectors in a temporary array using
drm_client_for_each_connector_iter() everytime we probe the outputs.
Other places where it's necessary to look at the connectors, we just
iterate over them using the same iterator function.

>>
>> Rename functions which signature is changed since they will be moved to
>> drm_client in a later patch.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>> @@ -1645,8 +1461,9 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>  	struct drm_client_dev *client = &fb_helper->client;
>>  	int ret = 0;
>>  	int crtc_count = 0;
>> -	int i;
>> +	struct drm_connector_list_iter conn_iter;
>>  	struct drm_fb_helper_surface_size sizes;
>> +	struct drm_connector *connector;
>>  	struct drm_mode_set *mode_set;
>>  	int best_depth = 0;
>>  
>> @@ -1663,11 +1480,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>  	if (preferred_bpp != sizes.surface_bpp)
>>  		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>>  
>> -	drm_fb_helper_for_each_connector(fb_helper, i) {
>> -		struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
>> +	drm_connector_list_iter_begin(fb_helper->dev, &conn_iter);
>> +	drm_client_for_each_connector_iter(connector, &conn_iter) {
>>  		struct drm_cmdline_mode *cmdline_mode;
> 
> I fail to see when to use drm_client_for_each_connector_iter() and when
> to use a simple for loop.
> In this patch drm_fb_helper_for_each_connector() is here replaced by the
> iterator variant and in many other placed a simple for loop.
> 

When I use a for loop it's in the 'probe for outputs' path where we are
operating on a temporary connector array that is created in
drm_setup_crtcs().

> The old code, in this place, would go through all connectors, but this
> code will skip 	DRM_MODE_CONNECTOR_WRITEBACK conectors.
> So it does not like identical code.
> 

If you look at the removed drm_fb_helper_single_add_all_connectors()
you'll see that it skips writeback connectors.

>>  
>> -		cmdline_mode = &fb_helper_conn->connector->cmdline_mode;
>> +		cmdline_mode = &connector->cmdline_mode;
>>  
>>  		if (cmdline_mode->bpp_specified) {
>>  			switch (cmdline_mode->bpp) {
>> @@ -1692,6 +1509,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>  			break;
>>  		}
>>  	}
>> +	drm_connector_list_iter_end(&conn_iter);
>>  
>>  	/*
>>  	 * If we run into a situation where, for example, the primary plane
>> @@ -1876,26 +1694,12 @@ void drm_fb_helper_fill_info(struct fb_info *info,
>>  }
>>  EXPORT_SYMBOL(drm_fb_helper_fill_info);
>>  
>> +static void drm_client_connectors_enabled(struct drm_connector **connectors,
>> +					  unsigned int connector_count,
>> +					  bool *enabled)
>>  {
>>  	bool any_enabled = false;
>>  	struct drm_connector *connector;
>>  	int i = 0;
>>  
>> -	drm_fb_helper_for_each_connector(fb_helper, i) {
>> -		connector = fb_helper->connector_info[i]->connector;
>> +	for (i = 0; i < connector_count; i++) {
>> +		connector = connectors[i];
> This is for example a place where the drm_fb_helper_for_each_connector()
> is replaced by a simple for loop.

Yeah, this is downstream from drm_setup_crtcs() and we're operating on
the temporary 'connectors' array.

> The simple for loop is nice and readable - just a comment replated to
> the above comment.
> 
>>  		struct drm_display_mode *mode = modes[i];
>>  		struct drm_crtc *crtc = crtcs[i];
>>  		struct drm_fb_offset *offset = &offsets[i];
>>  
>>  		if (mode && crtc) {
>>  			struct drm_mode_set *modeset = drm_client_find_modeset(client, crtc);
>> -			struct drm_connector *connector =
>> -				fb_helper->connector_info[i]->connector;
>> +			struct drm_connector *connector = connectors[i];
>>  
>>  			DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n",
>>  				      mode->name, crtc->base.id, offset->x, offset->y);
>> @@ -2539,6 +2354,10 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>  	kfree(modes);
>>  	kfree(offsets);
>>  	kfree(enabled);
>> +free_connectors:
>> +	for (i = 0; i < connector_count; i++)
>> +		drm_connector_put(connectors[i]);
> This may call drm_connector_put(NULL) as not all connectors are init.
> 

If you look further up where the connectors array is created, you'll see
that connector_count is only incremented if we succeed in krealloc'ing
the array leading to getting a ref on the connector.

>> +	kfree(connectors);
>>  }
>>  
>>  /*
>> @@ -2551,10 +2370,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>  static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>>  {
>>  	struct drm_client_dev *client = &fb_helper->client;
>> +	struct drm_connector_list_iter conn_iter;
>>  	struct fb_info *info = fb_helper->fbdev;
>>  	unsigned int rotation, sw_rotations = 0;
>> +	struct drm_connector *connector;
>>  	struct drm_mode_set *modeset;
>> -	int i;
>>  
>>  	mutex_lock(&client->modeset_mutex);
>>  	drm_client_for_each_modeset(modeset, client) {
>> @@ -2571,10 +2391,8 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>>  	}
>>  	mutex_unlock(&client->modeset_mutex);
>>  
>> -	mutex_lock(&fb_helper->dev->mode_config.mutex);
> Why is it OK to drop this lock?
> In general I fail to see how the connector data is protected (locking
> rules?) This is likely just me missing the bigger picture here..
> 

The connector iterator uses some magic to avoid taking the mode_config
mutex. See drm_connector_list_iter_next().

Noralf.

>> +	drm_connector_list_iter_end(&conn_iter);
>>  
>>  	switch (sw_rotations) {
>>  	case DRM_MODE_ROTATE_0:
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx