From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/6] drm/i915/opregion: make struct intel_opregion opaque
Date: Thu, 11 Jan 2024 16:13:42 -0800 [thread overview]
Message-ID: <ZaCENhzNGDuZNF3H@invictus> (raw)
In-Reply-To: <3b68d7ff4b2930eaf15d9657618a738b9065f64b.1704992868.git.jani.nikula@intel.com>
On Thu, Jan 11, 2024 at 07:21:19PM +0200, Jani Nikula wrote:
> With the recent cleanups, only intel_opregion.c needs to know the
> definition of struct intel_opregion. Allocate it dynamically and make it
> opaque.
>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
> .../gpu/drm/i915/display/intel_display_core.h | 3 +-
> drivers/gpu/drm/i915/display/intel_opregion.c | 147 +++++++++++-------
> drivers/gpu/drm/i915/display/intel_opregion.h | 27 +---
> 3 files changed, 94 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 8853a05dc331..a90f1aa201be 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -49,6 +49,7 @@ struct intel_fbdev;
> struct intel_fdi_funcs;
> struct intel_hotplug_funcs;
> struct intel_initial_plane_config;
> +struct intel_opregion;
> struct intel_overlay;
>
> /* Amount of SAGV/QGV points, BSpec precisely defines this */
> @@ -526,7 +527,7 @@ struct intel_display {
> struct intel_fbc *fbc[I915_MAX_FBCS];
> struct intel_frontbuffer_tracking fb_tracking;
> struct intel_hotplug hotplug;
> - struct intel_opregion opregion;
> + struct intel_opregion *opregion;
> struct intel_overlay *overlay;
> struct intel_display_params params;
> struct intel_vbt_data vbt;
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 26aacb01f9ec..3f5a20f9153e 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -250,11 +250,37 @@ struct opregion_asle_ext {
>
> #define MAX_DSLP 1500
>
> +#define OPREGION_SIZE (8 * 1024)
> +
> +struct intel_opregion {
> + struct drm_i915_private *i915;
> +
> + struct opregion_header *header;
> + struct opregion_acpi *acpi;
> + struct opregion_swsci *swsci;
> + u32 swsci_gbda_sub_functions;
> + u32 swsci_sbcb_sub_functions;
> + struct opregion_asle *asle;
> + struct opregion_asle_ext *asle_ext;
> + void *rvda;
> + void *vbt_firmware;
> + const void *vbt;
> + u32 vbt_size;
> + u32 *lid_state;
> + struct work_struct asle_work;
> + struct notifier_block acpi_notifier;
> +};
> +
> static int check_swsci_function(struct drm_i915_private *i915, u32 function)
> {
> - struct opregion_swsci *swsci = i915->display.opregion.swsci;
> + struct intel_opregion *opregion = i915->display.opregion;
> + struct opregion_swsci *swsci;
> u32 main_function, sub_function;
>
> + if (!opregion)
> + return -ENODEV;
> +
> + swsci = opregion->swsci;
> if (!swsci)
> return -ENODEV;
>
> @@ -265,11 +291,11 @@ static int check_swsci_function(struct drm_i915_private *i915, u32 function)
>
> /* Check if we can call the function. See swsci_setup for details. */
> if (main_function == SWSCI_SBCB) {
> - if ((i915->display.opregion.swsci_sbcb_sub_functions &
> + if ((opregion->swsci_sbcb_sub_functions &
> (1 << sub_function)) == 0)
> return -EINVAL;
> } else if (main_function == SWSCI_GBDA) {
> - if ((i915->display.opregion.swsci_gbda_sub_functions &
> + if ((opregion->swsci_gbda_sub_functions &
> (1 << sub_function)) == 0)
> return -EINVAL;
> }
> @@ -280,7 +306,7 @@ static int check_swsci_function(struct drm_i915_private *i915, u32 function)
> static int swsci(struct drm_i915_private *dev_priv,
> u32 function, u32 parm, u32 *parm_out)
> {
> - struct opregion_swsci *swsci = dev_priv->display.opregion.swsci;
> + struct opregion_swsci *swsci;
> struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> u32 scic, dslp;
> u16 swsci_val;
> @@ -290,6 +316,8 @@ static int swsci(struct drm_i915_private *dev_priv,
> if (ret)
> return ret;
>
> + swsci = dev_priv->display.opregion->swsci;
> +
> /* Driver sleep timeout in ms. */
> dslp = swsci->dslp;
> if (!dslp) {
> @@ -462,7 +490,7 @@ static u32 asle_set_backlight(struct drm_i915_private *dev_priv, u32 bclp)
> {
> struct intel_connector *connector;
> struct drm_connector_list_iter conn_iter;
> - struct opregion_asle *asle = dev_priv->display.opregion.asle;
> + struct opregion_asle *asle = dev_priv->display.opregion->asle;
>
> drm_dbg(&dev_priv->drm, "bclp = 0x%08x\n", bclp);
>
> @@ -584,9 +612,8 @@ static void asle_work(struct work_struct *work)
> {
> struct intel_opregion *opregion =
> container_of(work, struct intel_opregion, asle_work);
> - struct drm_i915_private *dev_priv =
> - container_of(opregion, struct drm_i915_private, display.opregion);
> - struct opregion_asle *asle = dev_priv->display.opregion.asle;
> + struct drm_i915_private *dev_priv = opregion->i915;
> + struct opregion_asle *asle = opregion->asle;
> u32 aslc_stat = 0;
> u32 aslc_req;
>
> @@ -634,14 +661,15 @@ static void asle_work(struct work_struct *work)
>
> bool intel_opregion_asle_present(struct drm_i915_private *i915)
> {
> - return i915->display.opregion.asle;
> + return i915->display.opregion && i915->display.opregion->asle;
> }
>
> -void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
> +void intel_opregion_asle_intr(struct drm_i915_private *i915)
> {
> - if (dev_priv->display.opregion.asle)
> - queue_work(dev_priv->unordered_wq,
> - &dev_priv->display.opregion.asle_work);
> + struct intel_opregion *opregion = i915->display.opregion;
> +
> + if (opregion && opregion->asle)
> + queue_work(i915->unordered_wq, &opregion->asle_work);
> }
>
> #define ACPI_EV_DISPLAY_SWITCH (1<<0)
> @@ -697,7 +725,7 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
>
> static void intel_didl_outputs(struct drm_i915_private *dev_priv)
> {
> - struct intel_opregion *opregion = &dev_priv->display.opregion;
> + struct intel_opregion *opregion = dev_priv->display.opregion;
> struct intel_connector *connector;
> struct drm_connector_list_iter conn_iter;
> int i = 0, max_outputs;
> @@ -736,7 +764,7 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>
> static void intel_setup_cadls(struct drm_i915_private *dev_priv)
> {
> - struct intel_opregion *opregion = &dev_priv->display.opregion;
> + struct intel_opregion *opregion = dev_priv->display.opregion;
> struct intel_connector *connector;
> struct drm_connector_list_iter conn_iter;
> int i = 0;
> @@ -766,7 +794,7 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
>
> static void swsci_setup(struct drm_i915_private *dev_priv)
> {
> - struct intel_opregion *opregion = &dev_priv->display.opregion;
> + struct intel_opregion *opregion = dev_priv->display.opregion;
> bool requested_callbacks = false;
> u32 tmp;
>
> @@ -844,7 +872,7 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>
> static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> {
> - struct intel_opregion *opregion = &dev_priv->display.opregion;
> + struct intel_opregion *opregion = dev_priv->display.opregion;
> const struct firmware *fw = NULL;
> const char *name = dev_priv->display.params.vbt_firmware;
> int ret;
> @@ -884,7 +912,7 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
>
> int intel_opregion_setup(struct drm_i915_private *dev_priv)
> {
> - struct intel_opregion *opregion = &dev_priv->display.opregion;
> + struct intel_opregion *opregion;
> struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> u32 asls, mboxes;
> char buf[sizeof(OPREGION_SIGNATURE)];
> @@ -907,11 +935,20 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> return -ENOTSUPP;
> }
>
> + opregion = kzalloc(sizeof(*opregion), GFP_KERNEL);
> + if (!opregion)
> + return -ENOMEM;
> +
> + opregion->i915 = dev_priv;
> + dev_priv->display.opregion = opregion;
> +
> INIT_WORK(&opregion->asle_work, asle_work);
>
> base = memremap(asls, OPREGION_SIZE, MEMREMAP_WB);
> - if (!base)
> - return -ENOMEM;
> + if (!base) {
> + err = -ENOMEM;
> + goto err_memremap;
> + }
>
> memcpy(buf, base, sizeof(buf));
>
> @@ -1039,6 +1076,10 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>
> err_out:
> memunmap(base);
> +err_memremap:
> + kfree(opregion);
> + dev_priv->display.opregion = NULL;
> +
> return err;
> }
>
> @@ -1111,12 +1152,12 @@ const struct drm_edid *intel_opregion_get_edid(struct intel_connector *intel_con
> {
> struct drm_connector *connector = &intel_connector->base;
> struct drm_i915_private *i915 = to_i915(connector->dev);
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
> const struct drm_edid *drm_edid;
> const void *edid;
> int len;
>
> - if (!opregion->asle_ext)
> + if (!opregion || !opregion->asle_ext)
> return NULL;
>
> edid = opregion->asle_ext->bddc;
> @@ -1139,9 +1180,9 @@ const struct drm_edid *intel_opregion_get_edid(struct intel_connector *intel_con
>
> const void *intel_opregion_get_vbt(struct drm_i915_private *i915, size_t *size)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (!opregion->vbt)
> + if (!opregion || !opregion->vbt)
> return NULL;
>
> if (size)
> @@ -1152,8 +1193,13 @@ const void *intel_opregion_get_vbt(struct drm_i915_private *i915, size_t *size)
>
> bool intel_opregion_headless_sku(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> - struct opregion_header *header = opregion->header;
> + struct intel_opregion *opregion = i915->display.opregion;
> + struct opregion_header *header;
> +
> + if (!opregion)
> + return false;
> +
> + header = opregion->header;
>
> if (!header || header->over.major < 2 ||
> (header->over.major == 2 && header->over.minor < 3))
> @@ -1164,9 +1210,9 @@ bool intel_opregion_headless_sku(struct drm_i915_private *i915)
>
> void intel_opregion_register(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (!opregion->header)
> + if (!opregion)
> return;
>
> if (opregion->acpi) {
> @@ -1180,7 +1226,7 @@ void intel_opregion_register(struct drm_i915_private *i915)
>
> static void intel_opregion_resume_display(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> if (opregion->acpi) {
> intel_didl_outputs(i915);
> @@ -1206,9 +1252,9 @@ static void intel_opregion_resume_display(struct drm_i915_private *i915)
>
> void intel_opregion_resume(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (!opregion->header)
> + if (!opregion)
> return;
>
> if (HAS_DISPLAY(i915))
> @@ -1219,12 +1265,12 @@ void intel_opregion_resume(struct drm_i915_private *i915)
>
> static void intel_opregion_suspend_display(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> if (opregion->asle)
> opregion->asle->ardy = ASLE_ARDY_NOT_READY;
>
> - cancel_work_sync(&i915->display.opregion.asle_work);
> + cancel_work_sync(&opregion->asle_work);
>
> if (opregion->acpi)
> opregion->acpi->drdy = 0;
> @@ -1232,9 +1278,9 @@ static void intel_opregion_suspend_display(struct drm_i915_private *i915)
>
> void intel_opregion_suspend(struct drm_i915_private *i915, pci_power_t state)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (!opregion->header)
> + if (!opregion)
> return;
>
> intel_opregion_notify_adapter(i915, state);
> @@ -1245,11 +1291,11 @@ void intel_opregion_suspend(struct drm_i915_private *i915, pci_power_t state)
>
> void intel_opregion_unregister(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> intel_opregion_suspend(i915, PCI_D1);
>
> - if (!opregion->header)
> + if (!opregion)
> return;
>
> if (opregion->acpi_notifier.notifier_call) {
> @@ -1260,36 +1306,25 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
>
> void intel_opregion_cleanup(struct drm_i915_private *i915)
> {
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (!opregion->header)
> + if (!opregion)
> return;
>
> - /* just clear all opregion memory pointers now */
> memunmap(opregion->header);
> - if (opregion->rvda) {
> + if (opregion->rvda)
> memunmap(opregion->rvda);
> - opregion->rvda = NULL;
> - }
> - if (opregion->vbt_firmware) {
> - kfree(opregion->vbt_firmware);
> - opregion->vbt_firmware = NULL;
> - }
> - opregion->header = NULL;
> - opregion->acpi = NULL;
> - opregion->swsci = NULL;
> - opregion->asle = NULL;
> - opregion->asle_ext = NULL;
> - opregion->vbt = NULL;
> - opregion->lid_state = NULL;
> + kfree(opregion->vbt_firmware);
> + kfree(opregion);
> + i915->display.opregion = NULL;
> }
>
> static int intel_opregion_show(struct seq_file *m, void *unused)
> {
> struct drm_i915_private *i915 = m->private;
> - struct intel_opregion *opregion = &i915->display.opregion;
> + struct intel_opregion *opregion = i915->display.opregion;
>
> - if (opregion->header)
> + if (opregion)
> seq_write(m, opregion->header, OPREGION_SIZE);
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
> index d084b30e8703..0bec224f711f 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> @@ -25,38 +25,13 @@
> #ifndef _INTEL_OPREGION_H_
> #define _INTEL_OPREGION_H_
>
> -#include <linux/workqueue.h>
> #include <linux/pci.h>
> +#include <linux/types.h>
>
> struct drm_i915_private;
> struct intel_connector;
> struct intel_encoder;
>
> -struct opregion_header;
> -struct opregion_acpi;
> -struct opregion_swsci;
> -struct opregion_asle;
> -struct opregion_asle_ext;
> -
> -struct intel_opregion {
> - struct opregion_header *header;
> - struct opregion_acpi *acpi;
> - struct opregion_swsci *swsci;
> - u32 swsci_gbda_sub_functions;
> - u32 swsci_sbcb_sub_functions;
> - struct opregion_asle *asle;
> - struct opregion_asle_ext *asle_ext;
> - void *rvda;
> - void *vbt_firmware;
> - const void *vbt;
> - u32 vbt_size;
> - u32 *lid_state;
> - struct work_struct asle_work;
> - struct notifier_block acpi_notifier;
> -};
> -
> -#define OPREGION_SIZE (8 * 1024)
> -
> #ifdef CONFIG_ACPI
>
> int intel_opregion_setup(struct drm_i915_private *dev_priv);
> --
> 2.39.2
>
next prev parent reply other threads:[~2024-01-12 0:15 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-11 17:21 [PATCH 0/6] drm/i915/opregion: better abstractions Jani Nikula
2024-01-11 17:21 ` [PATCH 1/6] drm/i915/bios: move i915_vbt debugfs to intel_bios.c Jani Nikula
2024-01-11 23:11 ` Radhakrishna Sripada
2024-01-11 17:21 ` [PATCH 2/6] drm/i915/opregion: move i915_opregion debugfs to intel_opregion.c Jani Nikula
2024-01-11 23:20 ` Radhakrishna Sripada
2024-01-11 17:21 ` [PATCH 3/6] drm/i915/opregion: abstract getting the opregion VBT Jani Nikula
2024-01-11 23:22 ` Radhakrishna Sripada
2024-01-11 17:21 ` [PATCH 4/6] drm/i915/opregion: abstract ASLE presence check Jani Nikula
2024-01-12 0:03 ` Radhakrishna Sripada
2024-01-12 10:17 ` Jani Nikula
2024-01-12 19:36 ` Radhakrishna Sripada
2024-01-15 13:48 ` Jani Nikula
2024-01-16 9:57 ` Jani Nikula
2024-01-11 17:21 ` [PATCH 5/6] drm/i915/gvt: use local INTEL_GVT_OPREGION_SIZE Jani Nikula
2024-01-12 0:15 ` Radhakrishna Sripada
2024-01-11 17:21 ` [PATCH 6/6] drm/i915/opregion: make struct intel_opregion opaque Jani Nikula
2024-01-12 0:13 ` Radhakrishna Sripada [this message]
2024-01-17 11:25 ` Ville Syrjälä
2024-01-17 12:43 ` Jani Nikula
2024-01-11 18:19 ` ✗ Fi.CI.SPARSE: warning for drm/i915/opregion: better abstractions Patchwork
2024-01-11 18:37 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-01-15 14:14 ` ✗ Fi.CI.SPARSE: warning for drm/i915/opregion: better abstractions (rev2) Patchwork
2024-01-15 14:26 ` ✓ Fi.CI.BAT: success " Patchwork
2024-01-15 16:55 ` ✗ Fi.CI.IGT: failure " Patchwork
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=ZaCENhzNGDuZNF3H@invictus \
--to=radhakrishna.sripada@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/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.