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@xxxxxxxxxxx>
>> ---
>> @@ -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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
