Re: [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector
Hi Noralf. On Thu, May 16, 2019 at 03:53:07PM +0200, Noralf Trønnes wrote: > > > 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. Better, thanks. After you clarified things looks good to me and you can add my: Reviewed-by: Sam Ravnborg ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector
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
>> ---
>> @@ -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 skipDRM_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
Re: [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector
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.
>
> Rename functions which signature is changed since they will be moved to
> drm_client in a later patch.
>
> Signed-off-by: Noralf Trønnes
> ---
> @@ -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.
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.
>
> - 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.
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.
> + 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_l
[PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector
All drivers add all their connectors so there's no need to keep around an
array of available connectors.
Rename functions which signature is changed since they will be moved to
drm_client in a later patch.
Signed-off-by: Noralf Trønnes
---
Documentation/gpu/todo.rst | 4 +
drivers/gpu/drm/drm_fb_helper.c | 498 ++--
include/drm/drm_client.h| 15 +
include/drm/drm_fb_helper.h | 80 ++---
4 files changed, 193 insertions(+), 404 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 9d4038c50013..ab96ba0600a9 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -292,6 +292,10 @@ drm_fb_helper tasks
- The max connector argument for drm_fb_helper_init() and
drm_fb_helper_fbdev_setup() isn't used anymore and can be removed.
+- The helper doesn't keep an array of connectors anymore so these can be
+ removed: drm_fb_helper_single_add_all_connectors(),
+ drm_fb_helper_add_one_connector() and drm_fb_helper_remove_one_connector().
+
Core refactorings
=
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index afb540f56555..ee0af5534f7d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -90,12 +90,6 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
* Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
* down by calling drm_fb_helper_fbdev_teardown().
*
- * Drivers that need to handle connector hotplugging (e.g. dp mst) can't use
- * the setup helper and will need to do the whole four-step setup process with
- * drm_fb_helper_prepare(), drm_fb_helper_init(),
- * drm_fb_helper_single_add_all_connectors(), enable hotplugging and
- * drm_fb_helper_initial_config() to avoid a possible race window.
- *
* At runtime drivers should restore the fbdev console by using
* drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
* They should also notify the fb helper code from updates to the output
@@ -118,8 +112,7 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
* encoders and connectors. To finish up the fbdev helper initialization, the
* drm_fb_helper_init() function is called. To probe for all attached displays
* and set up an initial configuration using the detected hardware, drivers
- * should call drm_fb_helper_single_add_all_connectors() followed by
- * drm_fb_helper_initial_config().
+ * should call drm_fb_helper_initial_config().
*
* If &drm_framebuffer_funcs.dirty is set, the
* drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions will
@@ -132,165 +125,6 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
* deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
*/
-#define drm_fb_helper_for_each_connector(fbh, i__) \
- for (({ lockdep_assert_held(&(fbh)->lock); }), \
-i__ = 0; i__ < (fbh)->connector_count; i__++)
-
-static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
-struct drm_connector *connector)
-{
- struct drm_fb_helper_connector *fb_conn;
- struct drm_fb_helper_connector **temp;
- unsigned int count;
-
- if (!drm_fbdev_emulation)
- return 0;
-
- lockdep_assert_held(&fb_helper->lock);
-
- count = fb_helper->connector_count + 1;
-
- if (count > fb_helper->connector_info_alloc_count) {
- size_t size = count * sizeof(fb_conn);
-
- temp = krealloc(fb_helper->connector_info, size, GFP_KERNEL);
- if (!temp)
- return -ENOMEM;
-
- fb_helper->connector_info_alloc_count = count;
- fb_helper->connector_info = temp;
- }
-
- fb_conn = kzalloc(sizeof(*fb_conn), GFP_KERNEL);
- if (!fb_conn)
- return -ENOMEM;
-
- drm_connector_get(connector);
- fb_conn->connector = connector;
- fb_helper->connector_info[fb_helper->connector_count++] = fb_conn;
-
- return 0;
-}
-
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
- struct drm_connector *connector)
-{
- int err;
-
- if (!fb_helper)
- return 0;
-
- mutex_lock(&fb_helper->lock);
- err = __drm_fb_helper_add_one_connector(fb_helper, connector);
- mutex_unlock(&fb_helper->lock);
-
- return err;
-}
-EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
-
-/**
- * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
- *emulation helper
- * @fb_helper: fbdev initialized with drm_fb_helper_init, can be NULL
- *
- * This functions adds all the available connectors for use with the given
- * fb_helper. This is a separate step to allow drivers to freely assign
- * connectors to the fbdev, e.g. if some are reserved for special purposes or
- * not adequate to be used for the fbcon.
-
