From: Jani Nikula <jani.nikula@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>,
intel-gfx@lists.freedesktop.org
Cc: lucas.demarchi@intel.com
Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/opregion: Abstract opregion function
Date: Mon, 07 Feb 2022 12:59:35 +0200 [thread overview]
Message-ID: <874k5brlgo.fsf@intel.com> (raw)
In-Reply-To: <20220206144311.5053-2-anshuman.gupta@intel.com>
On Sun, 06 Feb 2022, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> Abstract opregion operations like get opregion base, get rvda and
> opregion cleanup in form of i915_opregion_ops.
> This will be required to converge igfx and dgfx opregion.
> This keeps intel_opregion_setup() as static function, and adds
> a new function intel_opregion_init().
Too many things at once.
If it's a function pointer abstraction, don't make *any* functional
changes at the same time, i.e. first let the dgfx and igfx paths be the
same like they are now. Don't add a separate dgfx check.
It's *much* easier to review this if there are no functional changes,
and it's easier to review the follow-up when that adds the functional
changes for dgfx.
BR,
Jani.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_opregion.c | 215 ++++++++++++++----
> drivers/gpu/drm/i915/display/intel_opregion.h | 8 +-
> drivers/gpu/drm/i915/i915_driver.c | 2 +-
> 3 files changed, 172 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index af9d30f56cc1..19f0558c0fbf 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -137,6 +137,13 @@ struct opregion_asle_ext {
> u8 rsvd[764];
> } __packed;
>
> +struct i915_opregion_func {
> + void *(*alloc_opregion)(struct drm_i915_private *i915);
> + void *(*alloc_rvda)(struct drm_i915_private *i915);
> + void (*free_rvda)(struct drm_i915_private *i915);
> + void (*free_opregion)(struct drm_i915_private *i915);
> +};
> +
> /* Driver readiness indicator */
> #define ASLE_ARDY_READY (1 << 0)
> #define ASLE_ARDY_NOT_READY (0 << 0)
> @@ -839,13 +846,10 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> -int intel_opregion_setup(struct drm_i915_private *dev_priv)
> +static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> {
> struct intel_opregion *opregion = &dev_priv->opregion;
> - struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> - u32 asls, mboxes;
> - char buf[sizeof(OPREGION_SIGNATURE)];
> - int err = 0;
> + u32 mboxes;
> void *base;
> const void *vbt;
> u32 vbt_size;
> @@ -856,27 +860,12 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> BUILD_BUG_ON(sizeof(struct opregion_asle) != 0x100);
> BUILD_BUG_ON(sizeof(struct opregion_asle_ext) != 0x400);
>
> - pci_read_config_dword(pdev, ASLS, &asls);
> - drm_dbg(&dev_priv->drm, "graphic opregion physical addr: 0x%x\n",
> - asls);
> - if (asls == 0) {
> - drm_dbg(&dev_priv->drm, "ACPI OpRegion not supported!\n");
> - return -ENOTSUPP;
> - }
> -
> INIT_WORK(&opregion->asle_work, asle_work);
>
> - base = memremap(asls, OPREGION_SIZE, MEMREMAP_WB);
> - if (!base)
> - return -ENOMEM;
> + base = opregion->opregion_func->alloc_opregion(dev_priv);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
>
> - memcpy(buf, base, sizeof(buf));
> -
> - if (memcmp(buf, OPREGION_SIGNATURE, 16)) {
> - drm_dbg(&dev_priv->drm, "opregion signature mismatch\n");
> - err = -EINVAL;
> - goto err_out;
> - }
> opregion->header = base;
> opregion->lid_state = base + ACPI_CLID;
>
> @@ -924,23 +913,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
>
> if (opregion->header->over.major >= 2 && opregion->asle &&
> opregion->asle->rvda && opregion->asle->rvds) {
> - resource_size_t rvda = opregion->asle->rvda;
> -
> - /*
> - * opregion 2.0: rvda is the physical VBT address.
> - *
> - * opregion 2.1+: rvda is unsigned, relative offset from
> - * opregion base, and should never point within opregion.
> - */
> - if (opregion->header->over.major > 2 ||
> - opregion->header->over.minor >= 1) {
> - drm_WARN_ON(&dev_priv->drm, rvda < OPREGION_SIZE);
> -
> - rvda += asls;
> - }
>
> - opregion->rvda = memremap(rvda, opregion->asle->rvds,
> - MEMREMAP_WB);
> + opregion->rvda = opregion->opregion_func->alloc_rvda(dev_priv);
>
> vbt = opregion->rvda;
> vbt_size = opregion->asle->rvds;
> @@ -953,8 +927,7 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> } else {
> drm_dbg_kms(&dev_priv->drm,
> "Invalid VBT in ACPI OpRegion (RVDA)\n");
> - memunmap(opregion->rvda);
> - opregion->rvda = NULL;
> + opregion->opregion_func->free_rvda(dev_priv);
> }
> }
>
> @@ -982,9 +955,6 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> out:
> return 0;
>
> -err_out:
> - memunmap(base);
> - return err;
> }
>
> static int intel_use_opregion_panel_type_callback(const struct dmi_system_id *id)
> @@ -1169,11 +1139,9 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
> }
>
> /* just clear all opregion memory pointers now */
> - memunmap(opregion->header);
> - if (opregion->rvda) {
> - memunmap(opregion->rvda);
> - opregion->rvda = NULL;
> - }
> + opregion->opregion_func->free_rvda(i915);
> + opregion->opregion_func->free_opregion(i915);
> +
> if (opregion->vbt_firmware) {
> kfree(opregion->vbt_firmware);
> opregion->vbt_firmware = NULL;
> @@ -1186,3 +1154,152 @@ void intel_opregion_unregister(struct drm_i915_private *i915)
> opregion->vbt = NULL;
> opregion->lid_state = NULL;
> }
> +
> +static int
> +intel_opregion_get_asls(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> + struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> + u32 asls;
> +
> + pci_read_config_dword(pdev, ASLS, &asls);
> + drm_dbg(&i915->drm, "graphic opregion physical addr: 0x%x\n",
> + asls);
> + if (asls == 0) {
> + drm_dbg(&i915->drm, "ACPI OpRegion not supported!\n");
> + return -EINVAL;
> + }
> +
> + opregion->asls = asls;
> +
> + return 0;
> +}
> +
> +static void *intel_igfx_alloc_opregion(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> + char buf[sizeof(OPREGION_SIGNATURE)];
> + int err = 0;
> + void *base;
> +
> + err = intel_opregion_get_asls(i915);
> + if (err)
> + return ERR_PTR(err);
> +
> + base = memremap(opregion->asls, OPREGION_SIZE, MEMREMAP_WB);
> + if (!base)
> + return ERR_PTR(-ENOMEM);
> +
> + memcpy(buf, base, sizeof(buf));
> +
> + if (memcmp(buf, OPREGION_SIGNATURE, 16)) {
> + drm_dbg(&i915->drm, "opregion signature mismatch\n");
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + return base;
> +
> +err_out:
> + memunmap(base);
> +
> + return ERR_PTR(err);
> +}
> +
> +static void *intel_igfx_alloc_rvda(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> + resource_size_t rvda;
> + void *opreg_rvda;
> +
> + drm_WARN_ON(&i915->drm, !opregion->asls || !opregion->header);
> +
> + rvda = opregion->asle->rvda;
> +
> + /*
> + * opregion 2.0: rvda is the physical VBT address.
> + *
> + * opregion 2.1+: rvda is unsigned, relative offset from
> + * opregion base, and should never point within opregion.
> + */
> + if (opregion->header->over.major > 2 ||
> + opregion->header->over.minor >= 1) {
> + drm_WARN_ON(&i915->drm, rvda < OPREGION_SIZE);
> +
> + rvda += opregion->asls;
> + }
> +
> + opreg_rvda = memremap(rvda, opregion->asle->rvds, MEMREMAP_WB);
> + if (!opreg_rvda)
> + return ERR_PTR(-ENOMEM);
> +
> + return opreg_rvda;
> +}
> +
> +static void intel_igfx_free_rvda(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> +
> + if (opregion->rvda) {
> + memunmap(opregion->rvda);
> + opregion->rvda = NULL;
> + }
> +}
> +
> +static void intel_igfx_free_opregion(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> +
> + if (opregion->header)
> + memunmap(opregion->header);
> +}
> +
> +static void *intel_dgfx_alloc_opregion(struct drm_i915_private *i915)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static void *intel_dgfx_alloc_rvda(struct drm_i915_private *i915)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static void intel_dgfx_free_rvda(struct drm_i915_private *i915)
> +{
> +}
> +
> +static void intel_dgfx_free_opregion(struct drm_i915_private *i915)
> +{
> +}
> +
> +static const struct i915_opregion_func igfx_opregion_func = {
> + .alloc_opregion = intel_igfx_alloc_opregion,
> + .alloc_rvda = intel_igfx_alloc_rvda,
> + .free_rvda = intel_igfx_free_rvda,
> + .free_opregion = intel_igfx_free_opregion,
> +};
> +
> +static const struct i915_opregion_func dgfx_opregion_func = {
> + .alloc_opregion = intel_dgfx_alloc_opregion,
> + .alloc_rvda = intel_dgfx_alloc_rvda,
> + .free_rvda = intel_dgfx_free_rvda,
> + .free_opregion = intel_dgfx_free_opregion,
> +};
> +
> +/**
> + * intel_opregion_init() - Init ACPI opregion.
> + * @i915 i915 device priv data.
> + * It initialize the dgfx/igfx opregion function pointers
> + * and setup the ACPI opregions.
> + */
> +int intel_opregion_init(struct drm_i915_private *i915)
> +{
> + struct intel_opregion *opregion = &i915->opregion;
> +
> + if (IS_DGFX(i915))
> + opregion->opregion_func = &dgfx_opregion_func;
> + else
> + opregion->opregion_func = &igfx_opregion_func;
> +
> + return intel_opregion_setup(i915);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
> index 82cc0ba34af7..4ff48c445044 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> @@ -37,6 +37,7 @@ struct opregion_acpi;
> struct opregion_swsci;
> struct opregion_asle;
> struct opregion_asle_ext;
> +struct i915_opregion_func;
>
> struct intel_opregion {
> struct opregion_header *header;
> @@ -46,6 +47,8 @@ struct intel_opregion {
> u32 swsci_sbcb_sub_functions;
> struct opregion_asle *asle;
> struct opregion_asle_ext *asle_ext;
> + const struct i915_opregion_func *opregion_func;
> + resource_size_t asls;
> void *rvda;
> void *vbt_firmware;
> const void *vbt;
> @@ -59,8 +62,7 @@ struct intel_opregion {
>
> #ifdef CONFIG_ACPI
>
> -int intel_opregion_setup(struct drm_i915_private *dev_priv);
> -
> +int intel_opregion_init(struct drm_i915_private *i915);
> void intel_opregion_register(struct drm_i915_private *dev_priv);
> void intel_opregion_unregister(struct drm_i915_private *dev_priv);
>
> @@ -78,7 +80,7 @@ struct edid *intel_opregion_get_edid(struct intel_connector *connector);
>
> #else /* CONFIG_ACPI*/
>
> -static inline int intel_opregion_setup(struct drm_i915_private *dev_priv)
> +static inline int intel_opregion_init(struct drm_i915_private *i915)
> {
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 3d41f532a5d6..f7cb34f5fa4a 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -634,7 +634,7 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> if (ret)
> goto err_msi;
>
> - intel_opregion_setup(dev_priv);
> + intel_opregion_init(dev_priv);
>
> ret = intel_pcode_init(dev_priv);
> if (ret)
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2022-02-07 10:59 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-06 14:43 [Intel-gfx] [PATCH 0/4] DGFX OpRegion Anshuman Gupta
2022-02-06 14:43 ` [Intel-gfx] [PATCH 1/4] drm/i915/opregion: Abstract opregion function Anshuman Gupta
2022-02-07 10:59 ` Jani Nikula [this message]
2022-02-06 14:43 ` [Intel-gfx] [PATCH 2/4] drm/i915/opregion: Register opreg func only for disp parts Anshuman Gupta
2022-02-07 11:02 ` Jani Nikula
2022-02-06 14:43 ` [Intel-gfx] [PATCH 3/4] drm/i915/dgfx: OPROM OpRegion Setup Anshuman Gupta
2022-02-06 17:15 ` kernel test robot
2022-02-06 14:43 ` [Intel-gfx] [PATCH 4/4] drm/i915/dgfx: Get VBT from rvda Anshuman Gupta
2022-02-06 15:14 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for DGFX OpRegion Patchwork
2022-02-06 15:15 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-02-06 15:48 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-02-06 17:05 ` [Intel-gfx] ✗ 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=874k5brlgo.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@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.