[Intel-gfx] [PATCH v5 08/11] drm/fb-helper: Remove drm_fb_helper_connector
Sam Ravnborg
sam at ravnborg.org
Thu May 16 13:07:32 UTC 2019
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 <noralf at 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.
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_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..
> + drm_connector_list_iter_end(&conn_iter);
>
> switch (sw_rotations) {
> case DRM_MODE_ROTATE_0:
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 6cf48419f77f..8d94880bbe25 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -7,6 +7,7 @@
> #include <linux/mutex.h>
> #include <linux/types.h>
>
> +#include <drm/drm_connector.h>
> #include <drm/drm_crtc.h>
>
> struct drm_client_dev;
> @@ -169,6 +170,20 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
> for (({ lockdep_assert_held(&(client)->modeset_mutex); }), \
> modeset = (client)->modesets; modeset->crtc; modeset++)
>
> +/**
> + * drm_client_for_each_connector_iter - connector_list iterator macro
> + * @connector: &struct drm_connector pointer used as cursor
> + * @iter: &struct drm_connector_list_iter
> + *
> + * This iterates the connectors that are useable for internal clients (excludes
> + * writeback connectors).
> + *
> + * For more info see drm_for_each_connector_iter().
> + */
> +#define drm_client_for_each_connector_iter(connector, iter) \
> + drm_for_each_connector_iter(connector, iter) \
> + if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> +
> int drm_client_debugfs_init(struct drm_minor *minor);
>
> #endif
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6b334f4d8a22..e14d52ef9638 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -97,16 +97,10 @@ struct drm_fb_helper_funcs {
> struct drm_fb_helper_surface_size *sizes);
> };
>
> -struct drm_fb_helper_connector {
> - struct drm_connector *connector;
> -};
> -
> /**
> * struct drm_fb_helper - main structure to emulate fbdev on top of KMS
> * @fb: Scanout framebuffer object
> * @dev: DRM device
> - * @connector_count: number of connected connectors
> - * @connector_info_alloc_count: size of connector_info
> * @funcs: driver callbacks for fb helper
> * @fbdev: emulated fbdev device info struct
> * @pseudo_palette: fake palette of 16 colors
> @@ -138,15 +132,6 @@ struct drm_fb_helper {
>
> struct drm_framebuffer *fb;
> struct drm_device *dev;
> - int connector_count;
> - int connector_info_alloc_count;
> - /**
> - * @connector_info:
> - *
> - * Array of per-connector information. Do not iterate directly, but use
> - * drm_fb_helper_for_each_connector.
> - */
> - struct drm_fb_helper_connector **connector_info;
As we are removing connector_info the comment for the @lock member
should be adjusted not to mention connector_info.
> const struct drm_fb_helper_funcs *funcs;
> struct fb_info *fbdev;
> u32 pseudo_palette[17];
More information about the Intel-gfx
mailing list