From: Daniel Vetter <daniel@ffwll.ch>
To: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: daniel.vetter@ffwll.ch, sam@ravnborg.org, tzimmermann@suse.de,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/7] drm/gma500: Make use of the drm connector iterator
Date: Tue, 22 Mar 2022 15:39:07 +0100 [thread overview]
Message-ID: <Yjnfi1h8tKRgL0Kl@phenom.ffwll.local> (raw)
In-Reply-To: <20220322131742.11566-3-patrik.r.jakobsson@gmail.com>
On Tue, Mar 22, 2022 at 02:17:38PM +0100, Patrik Jakobsson wrote:
> This makes sure we're using proper locking when iterating the list of
> connectors.
>
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Note that this is only needed if your driver deals with hotpluggable
connectors. Since gma500 doesn't, not really a need to convert this over,
but it also doesn't hurt.
If the kerneldoc doesn't explain this, maybe we should add it? Care for a
patch.
Either way on the entire series:
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I'll leave it up to you whether you want to push this one too or not.
-Daniel
> ---
> drivers/gpu/drm/gma500/cdv_device.c | 10 ++++++--
> drivers/gpu/drm/gma500/cdv_intel_display.c | 9 +++++--
> drivers/gpu/drm/gma500/framebuffer.c | 6 +++--
> drivers/gpu/drm/gma500/gma_display.c | 16 ++++++++-----
> drivers/gpu/drm/gma500/oaktrail_crtc.c | 17 ++++++++-----
> drivers/gpu/drm/gma500/oaktrail_lvds.c | 15 ++++++------
> drivers/gpu/drm/gma500/psb_device.c | 28 +++++++++++++++-------
> drivers/gpu/drm/gma500/psb_drv.c | 10 ++++----
> drivers/gpu/drm/gma500/psb_intel_display.c | 15 ++++++++----
> 9 files changed, 84 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_device.c b/drivers/gpu/drm/gma500/cdv_device.c
> index f854f58bcbb3..dd32b484dd82 100644
> --- a/drivers/gpu/drm/gma500/cdv_device.c
> +++ b/drivers/gpu/drm/gma500/cdv_device.c
> @@ -262,6 +262,7 @@ static int cdv_save_display_registers(struct drm_device *dev)
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct psb_save_area *regs = &dev_priv->regs;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
>
> dev_dbg(dev->dev, "Saving GPU registers.\n");
> @@ -298,8 +299,10 @@ static int cdv_save_display_registers(struct drm_device *dev)
> regs->cdv.saveIER = REG_READ(PSB_INT_ENABLE_R);
> regs->cdv.saveIMR = REG_READ(PSB_INT_MASK_R);
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter)
> connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> + drm_connector_list_iter_end(&conn_iter);
>
> return 0;
> }
> @@ -317,6 +320,7 @@ static int cdv_restore_display_registers(struct drm_device *dev)
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct psb_save_area *regs = &dev_priv->regs;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> u32 temp;
>
> @@ -373,8 +377,10 @@ static int cdv_restore_display_registers(struct drm_device *dev)
>
> drm_mode_config_reset(dev);
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter)
> connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> + drm_connector_list_iter_end(&conn_iter);
>
> /* Resume the modeset for every activated CRTC */
> drm_helper_resume_force_mode(dev);
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c
> index 94ebc48a4349..0c3ddcdc29dc 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_display.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
> @@ -584,13 +584,14 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
> bool ok;
> bool is_lvds = false;
> bool is_dp = false;
> - struct drm_mode_config *mode_config = &dev->mode_config;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> const struct gma_limit_t *limit;
> u32 ddi_select = 0;
> bool is_edp = false;
>
> - list_for_each_entry(connector, &mode_config->connector_list, head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> struct gma_encoder *gma_encoder =
> gma_attached_encoder(connector);
>
> @@ -613,10 +614,14 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
> is_edp = true;
> break;
> default:
> + drm_connector_list_iter_end(&conn_iter);
> DRM_ERROR("invalid output type.\n");
> return 0;
> }
> +
> + break;
> }
> + drm_connector_list_iter_end(&conn_iter);
>
> if (dev_priv->dplla_96mhz)
> /* low-end sku, 96/100 mhz */
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 2b99c996fdc2..0ac6ea5fd3a1 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -451,6 +451,7 @@ static const struct drm_mode_config_funcs psb_mode_funcs = {
> static void psb_setup_outputs(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
>
> drm_mode_create_scaling_mode_property(dev);
> @@ -461,8 +462,8 @@ static void psb_setup_outputs(struct drm_device *dev)
> "backlight", 0, 100);
> dev_priv->ops->output_init(dev);
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list,
> - head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
> struct drm_encoder *encoder = &gma_encoder->base;
> int crtc_mask = 0, clone_mask = 0;
> @@ -505,6 +506,7 @@ static void psb_setup_outputs(struct drm_device *dev)
> encoder->possible_clones =
> gma_connector_clones(dev, clone_mask);
> }
> + drm_connector_list_iter_end(&conn_iter);
> }
>
> void psb_modeset_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 1d7964c339f4..e8157464d9eb 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -27,17 +27,21 @@
> bool gma_pipe_has_type(struct drm_crtc *crtc, int type)
> {
> struct drm_device *dev = crtc->dev;
> - struct drm_mode_config *mode_config = &dev->mode_config;
> - struct drm_connector *l_entry;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector;
>
> - list_for_each_entry(l_entry, &mode_config->connector_list, head) {
> - if (l_entry->encoder && l_entry->encoder->crtc == crtc) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + if (connector->encoder && connector->encoder->crtc == crtc) {
> struct gma_encoder *gma_encoder =
> - gma_attached_encoder(l_entry);
> - if (gma_encoder->type == type)
> + gma_attached_encoder(connector);
> + if (gma_encoder->type == type) {
> + drm_connector_list_iter_end(&conn_iter);
> return true;
> + }
> }
> }
> + drm_connector_list_iter_end(&conn_iter);
>
> return false;
> }
> diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index 36c7c2686c90..873c17cf8fb4 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -372,9 +372,9 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
> bool ok, is_sdvo = false;
> bool is_lvds = false;
> bool is_mipi = false;
> - struct drm_mode_config *mode_config = &dev->mode_config;
> struct gma_encoder *gma_encoder = NULL;
> uint64_t scalingType = DRM_MODE_SCALE_FULLSCREEN;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> int i;
> int need_aux = gma_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ? 1 : 0;
> @@ -392,7 +392,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
> adjusted_mode,
> sizeof(struct drm_display_mode));
>
> - list_for_each_entry(connector, &mode_config->connector_list, head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> if (!connector->encoder || connector->encoder->crtc != crtc)
> continue;
>
> @@ -409,8 +410,16 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
> is_mipi = true;
> break;
> }
> +
> + break;
> }
>
> + if (gma_encoder)
> + drm_object_property_get_value(&connector->base,
> + dev->mode_config.scaling_mode_property, &scalingType);
> +
> + drm_connector_list_iter_end(&conn_iter);
> +
> /* Disable the VGA plane that we never use */
> for (i = 0; i <= need_aux; i++)
> REG_WRITE_WITH_AUX(VGACNTRL, VGA_DISP_DISABLE, i);
> @@ -424,10 +433,6 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
> (mode->crtc_vdisplay - 1), i);
> }
>
> - if (gma_encoder)
> - drm_object_property_get_value(&connector->base,
> - dev->mode_config.scaling_mode_property, &scalingType);
> -
> if (scalingType == DRM_MODE_SCALE_NO_SCALE) {
> /* Moorestown doesn't have register support for centering so
> * we need to mess with the h/vblank and h/vsync start and
> diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> index 28b995ef2844..04852dbc7fb3 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c
> @@ -85,7 +85,7 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
> struct drm_device *dev = encoder->dev;
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> struct psb_intel_mode_device *mode_dev = &dev_priv->mode_dev;
> - struct drm_mode_config *mode_config = &dev->mode_config;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector = NULL;
> struct drm_crtc *crtc = encoder->crtc;
> u32 lvds_port;
> @@ -112,21 +112,22 @@ static void oaktrail_lvds_mode_set(struct drm_encoder *encoder,
> REG_WRITE(LVDS, lvds_port);
>
> /* Find the connector we're trying to set up */
> - list_for_each_entry(connector, &mode_config->connector_list, head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> if (connector->encoder && connector->encoder->crtc == crtc)
> break;
> }
>
> - if (list_entry_is_head(connector, &mode_config->connector_list, head)) {
> + if (!connector) {
> + drm_connector_list_iter_end(&conn_iter);
> DRM_ERROR("Couldn't find connector when setting mode");
> gma_power_end(dev);
> return;
> }
>
> - drm_object_property_get_value(
> - &connector->base,
> - dev->mode_config.scaling_mode_property,
> - &v);
> + drm_object_property_get_value( &connector->base,
> + dev->mode_config.scaling_mode_property, &v);
> + drm_connector_list_iter_end(&conn_iter);
>
> if (v == DRM_MODE_SCALE_NO_SCALE)
> REG_WRITE(PFIT_CONTROL, 0);
> diff --git a/drivers/gpu/drm/gma500/psb_device.c b/drivers/gpu/drm/gma500/psb_device.c
> index 59f325165667..71534f4ca834 100644
> --- a/drivers/gpu/drm/gma500/psb_device.c
> +++ b/drivers/gpu/drm/gma500/psb_device.c
> @@ -168,8 +168,10 @@ static void psb_init_pm(struct drm_device *dev)
> static int psb_save_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> + struct gma_connector *gma_connector;
> struct drm_crtc *crtc;
> - struct gma_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector;
> struct psb_state *regs = &dev_priv->regs.psb;
>
> /* Display arbitration control + watermarks */
> @@ -189,9 +191,13 @@ static int psb_save_display_registers(struct drm_device *dev)
> dev_priv->ops->save_crtc(crtc);
> }
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> - if (connector->save)
> - connector->save(&connector->base);
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + gma_connector = to_gma_connector(connector);
> + if (gma_connector->save)
> + gma_connector->save(connector);
> + }
> + drm_connector_list_iter_end(&conn_iter);
>
> drm_modeset_unlock_all(dev);
> return 0;
> @@ -206,8 +212,10 @@ static int psb_save_display_registers(struct drm_device *dev)
> static int psb_restore_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> + struct gma_connector *gma_connector;
> struct drm_crtc *crtc;
> - struct gma_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector;
> struct psb_state *regs = &dev_priv->regs.psb;
>
> /* Display arbitration + watermarks */
> @@ -228,9 +236,13 @@ static int psb_restore_display_registers(struct drm_device *dev)
> if (drm_helper_crtc_in_use(crtc))
> dev_priv->ops->restore_crtc(crtc);
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> - if (connector->restore)
> - connector->restore(&connector->base);
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + gma_connector = to_gma_connector(connector);
> + if (gma_connector->restore)
> + gma_connector->restore(connector);
> + }
> + drm_connector_list_iter_end(&conn_iter);
>
> drm_modeset_unlock_all(dev);
> return 0;
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index b231fddb8817..bb0e3288e35b 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -236,10 +236,11 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> unsigned long resource_start, resource_len;
> unsigned long irqflags;
> - int ret = -ENOMEM;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> struct gma_encoder *gma_encoder;
> struct psb_gtt *pg;
> + int ret = -ENOMEM;
>
> /* initializing driver private data */
>
> @@ -390,9 +391,9 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
> psb_fbdev_init(dev);
> drm_kms_helper_poll_init(dev);
>
> - /* Only add backlight support if we have LVDS output */
> - list_for_each_entry(connector, &dev->mode_config.connector_list,
> - head) {
> + /* Only add backlight support if we have LVDS or MIPI output */
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> gma_encoder = gma_attached_encoder(connector);
>
> switch (gma_encoder->type) {
> @@ -402,6 +403,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
> break;
> }
> }
> + drm_connector_list_iter_end(&conn_iter);
>
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index a99859b5b13a..fb8234f4d128 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -106,7 +106,7 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
> u32 dpll = 0, fp = 0, dspcntr, pipeconf;
> bool ok, is_sdvo = false;
> bool is_lvds = false, is_tv = false;
> - struct drm_mode_config *mode_config = &dev->mode_config;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> const struct gma_limit_t *limit;
>
> @@ -116,7 +116,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
> return 0;
> }
>
> - list_for_each_entry(connector, &mode_config->connector_list, head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>
> if (!connector->encoder
> @@ -135,6 +136,7 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
> break;
> }
> }
> + drm_connector_list_iter_end(&conn_iter);
>
> refclk = 96000;
>
> @@ -534,16 +536,19 @@ struct drm_crtc *psb_intel_get_crtc_from_pipe(struct drm_device *dev, int pipe)
>
> int gma_connector_clones(struct drm_device *dev, int type_mask)
> {
> - int index_mask = 0;
> + struct drm_connector_list_iter conn_iter;
> struct drm_connector *connector;
> + int index_mask = 0;
> int entry = 0;
>
> - list_for_each_entry(connector, &dev->mode_config.connector_list,
> - head) {
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
> if (type_mask & (1 << gma_encoder->type))
> index_mask |= (1 << entry);
> entry++;
> }
> + drm_connector_list_iter_end(&conn_iter);
> +
> return index_mask;
> }
> --
> 2.35.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
next prev parent reply other threads:[~2022-03-22 14:39 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-22 13:17 [PATCH 1/7] drm/gma500: Use managed drmm_mode_config_init() Patrik Jakobsson
2022-03-22 13:17 ` [PATCH 2/7] drm/gma500: Uninstall interrupts on driver removal Patrik Jakobsson
2022-03-22 13:17 ` [PATCH 3/7] drm/gma500: Make use of the drm connector iterator Patrik Jakobsson
2022-03-22 14:39 ` Daniel Vetter [this message]
2022-03-22 15:46 ` Patrik Jakobsson
2022-03-22 19:34 ` Thomas Zimmermann
2022-03-22 13:17 ` [PATCH 4/7] drm/gma500: gma500 don't register non-hotpluggable connectors Patrik Jakobsson
2022-03-22 13:17 ` [PATCH 5/7] drm/gma500: Free the correct connector allocation Patrik Jakobsson
2022-03-22 13:17 ` [PATCH 6/7] drm/gma500: Declare a few functions static Patrik Jakobsson
2022-03-22 13:17 ` [PATCH 7/7] drm/gma500: Add crtc prefix to vblank functions Patrik Jakobsson
2022-03-22 19:36 ` Thomas Zimmermann
2022-03-22 22:46 ` Patrik Jakobsson
2022-03-22 19:30 ` [PATCH 1/7] drm/gma500: Use managed drmm_mode_config_init() Thomas Zimmermann
2022-03-22 19:32 ` Thomas Zimmermann
2022-03-22 22:42 ` Patrik Jakobsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yjnfi1h8tKRgL0Kl@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=patrik.r.jakobsson@gmail.com \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.