Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/opregion: Abstract opregion function
On 2022-03-16 at 11:25:33 +0200, Jani Nikula wrote:
> On Sun, 20 Feb 2022, Anshuman Gupta 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.
> >
> > v2:
> > - Keep only function pointer abstraction stuff. [Jani]
> > - Add alloc_rvda error handling.
> >
> > v3:
> > - Added necessary credit to Manasi for static analysis fix around
> > drm_WARN_ON(>drm, !opregion->asls || !opregion->header)
> >
> > Cc: Jani Nikula
> > Cc: Rodrigo Vivi
> > Cc: Badal Nilawar
> > Cc: Uma Shankar
> > Signed-off-by: Manasi Navare
> > Signed-off-by: Anshuman Gupta
> > ---
> > drivers/gpu/drm/i915/display/intel_opregion.c | 179 +-
> > drivers/gpu/drm/i915/display/intel_opregion.h | 3 +
> > 2 files changed, 134 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> > b/drivers/gpu/drm/i915/display/intel_opregion.c
> > index 9b56064ddb5d..94eb7c23fcb4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> > @@ -138,6 +138,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)
> > @@ -876,10 +883,7 @@ static int intel_load_vbt_firmware(struct
> > drm_i915_private *dev_priv)
> > static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> > {
> > struct intel_opregion *opregion = _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;
> > @@ -890,27 +894,12 @@ static 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, );
> > - drm_dbg(_priv->drm, "graphic opregion physical addr: 0x%x\n",
> > - asls);
> > - if (asls == 0) {
> > - drm_dbg(_priv->drm, "ACPI OpRegion not supported!\n");
> > - return -ENOTSUPP;
> > - }
> > -
> > INIT_WORK(>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(_priv->drm, "opregion signature mismatch\n");
> > - err = -EINVAL;
> > - goto err_out;
> > - }
> > opregion->header = base;
> > opregion->lid_state = base + ACPI_CLID;
> >
> > @@ -970,23 +959,10 @@ static 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(_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);
>
> It's a basic principle of interfaces to do corresponding things at the
> same abstraction level.
>
> Now, you set opregion->rvda at the level that calls ->alloc_rvda,
> however ->free_rvda accesses opregion->rvda directly.
>
> Similarly for ->alloc_opregion and ->free_opregion.
Thanks for review comment.
Could you please shed light to make same abstraction level.
free_rvda(void *rvda)
{
IS_ERR(rvda)
return;
if (rvda) {
memunmap(opregion->rvda);
rvda = NULL;
}
}
I could think above kind of approach
Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/opregion: Abstract opregion function
On Sun, 20 Feb 2022, Anshuman Gupta 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.
>
> v2:
> - Keep only function pointer abstraction stuff. [Jani]
> - Add alloc_rvda error handling.
>
> v3:
> - Added necessary credit to Manasi for static analysis fix around
> drm_WARN_ON(>drm, !opregion->asls || !opregion->header)
>
> Cc: Jani Nikula
> Cc: Rodrigo Vivi
> Cc: Badal Nilawar
> Cc: Uma Shankar
> Signed-off-by: Manasi Navare
> Signed-off-by: Anshuman Gupta
> ---
> drivers/gpu/drm/i915/display/intel_opregion.c | 179 +-
> drivers/gpu/drm/i915/display/intel_opregion.h | 3 +
> 2 files changed, 134 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 9b56064ddb5d..94eb7c23fcb4 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -138,6 +138,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)
> @@ -876,10 +883,7 @@ static int intel_load_vbt_firmware(struct
> drm_i915_private *dev_priv)
> static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> {
> struct intel_opregion *opregion = _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;
> @@ -890,27 +894,12 @@ static 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, );
> - drm_dbg(_priv->drm, "graphic opregion physical addr: 0x%x\n",
> - asls);
> - if (asls == 0) {
> - drm_dbg(_priv->drm, "ACPI OpRegion not supported!\n");
> - return -ENOTSUPP;
> - }
> -
> INIT_WORK(>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(_priv->drm, "opregion signature mismatch\n");
> - err = -EINVAL;
> - goto err_out;
> - }
> opregion->header = base;
> opregion->lid_state = base + ACPI_CLID;
>
> @@ -970,23 +959,10 @@ static 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(_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);
> + if (IS_ERR(opregion->rvda))
> + goto mbox4_vbt;
>
> vbt = opregion->rvda;
> vbt_size = opregion->asle->rvds;
> @@ -999,11 +975,12 @@ static int intel_opregion_setup(struct drm_i915_private
> *dev_priv)
> } else {
> drm_dbg_kms(_priv->drm,
> "Invalid VBT in ACPI OpRegion (RVDA)\n");
> - memunmap(opregion->rvda);
> - opregion->rvda = NULL;
> + opregion->opregion_func->free_rvda(dev_priv);
> }
> }
>
> +mbox4_vbt:
> +
> vbt = base + OPREGION_VBT_OFFSET;
> /*
>* The VBT specification says that if the ASLE ext mailbox is not used
> @@ -1028,9 +1005,6 @@ static int intel_opregion_setup(struct
Re: [Intel-gfx] [PATCH v3 2/6] drm/i915/opregion: Abstract opregion function
On Sun, 20 Feb 2022, Anshuman Gupta 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.
>
> v2:
> - Keep only function pointer abstraction stuff. [Jani]
> - Add alloc_rvda error handling.
>
> v3:
> - Added necessary credit to Manasi for static analysis fix around
> drm_WARN_ON(>drm, !opregion->asls || !opregion->header)
>
> Cc: Jani Nikula
> Cc: Rodrigo Vivi
> Cc: Badal Nilawar
> Cc: Uma Shankar
> Signed-off-by: Manasi Navare
> Signed-off-by: Anshuman Gupta
> ---
> drivers/gpu/drm/i915/display/intel_opregion.c | 179 +-
> drivers/gpu/drm/i915/display/intel_opregion.h | 3 +
> 2 files changed, 134 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 9b56064ddb5d..94eb7c23fcb4 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -138,6 +138,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)
> @@ -876,10 +883,7 @@ static int intel_load_vbt_firmware(struct
> drm_i915_private *dev_priv)
> static int intel_opregion_setup(struct drm_i915_private *dev_priv)
> {
> struct intel_opregion *opregion = _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;
> @@ -890,27 +894,12 @@ static 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, );
> - drm_dbg(_priv->drm, "graphic opregion physical addr: 0x%x\n",
> - asls);
> - if (asls == 0) {
> - drm_dbg(_priv->drm, "ACPI OpRegion not supported!\n");
> - return -ENOTSUPP;
> - }
> -
> INIT_WORK(>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(_priv->drm, "opregion signature mismatch\n");
> - err = -EINVAL;
> - goto err_out;
> - }
> opregion->header = base;
> opregion->lid_state = base + ACPI_CLID;
>
> @@ -970,23 +959,10 @@ static 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(_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);
It's a basic principle of interfaces to do corresponding things at the
same abstraction level.
Now, you set opregion->rvda at the level that calls ->alloc_rvda,
however ->free_rvda accesses opregion->rvda directly.
Similarly for ->alloc_opregion and ->free_opregion.
> + if (IS_ERR(opregion->rvda))
> + goto mbox4_vbt;
>
> vbt = opregion->rvda;
> vbt_size = opregion->asle->rvds;
> @@ -999,11 +975,12 @@ static int intel_opregion_setup(struct drm_i915_private
> *dev_priv)
> } else {
> drm_dbg_kms(_priv->drm,
> "Invalid VBT in ACPI OpRegion (RVDA)\n");
> - memunmap(opregion->rvda);
> - opregion->rvda = NULL;
> +
[Intel-gfx] [PATCH v3 2/6] drm/i915/opregion: Abstract opregion function
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.
v2:
- Keep only function pointer abstraction stuff. [Jani]
- Add alloc_rvda error handling.
v3:
- Added necessary credit to Manasi for static analysis fix around
drm_WARN_ON(>drm, !opregion->asls || !opregion->header)
Cc: Jani Nikula
Cc: Rodrigo Vivi
Cc: Badal Nilawar
Cc: Uma Shankar
Signed-off-by: Manasi Navare
Signed-off-by: Anshuman Gupta
---
drivers/gpu/drm/i915/display/intel_opregion.c | 179 +-
drivers/gpu/drm/i915/display/intel_opregion.h | 3 +
2 files changed, 134 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
b/drivers/gpu/drm/i915/display/intel_opregion.c
index 9b56064ddb5d..94eb7c23fcb4 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -138,6 +138,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)
@@ -876,10 +883,7 @@ static int intel_load_vbt_firmware(struct drm_i915_private
*dev_priv)
static int intel_opregion_setup(struct drm_i915_private *dev_priv)
{
struct intel_opregion *opregion = _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;
@@ -890,27 +894,12 @@ static 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, );
- drm_dbg(_priv->drm, "graphic opregion physical addr: 0x%x\n",
- asls);
- if (asls == 0) {
- drm_dbg(_priv->drm, "ACPI OpRegion not supported!\n");
- return -ENOTSUPP;
- }
-
INIT_WORK(>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(_priv->drm, "opregion signature mismatch\n");
- err = -EINVAL;
- goto err_out;
- }
opregion->header = base;
opregion->lid_state = base + ACPI_CLID;
@@ -970,23 +959,10 @@ static 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(_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);
+ if (IS_ERR(opregion->rvda))
+ goto mbox4_vbt;
vbt = opregion->rvda;
vbt_size = opregion->asle->rvds;
@@ -999,11 +975,12 @@ static int intel_opregion_setup(struct drm_i915_private
*dev_priv)
} else {
drm_dbg_kms(_priv->drm,
"Invalid VBT in ACPI OpRegion (RVDA)\n");
- memunmap(opregion->rvda);
- opregion->rvda = NULL;
+ opregion->opregion_func->free_rvda(dev_priv);
}
}
+mbox4_vbt:
+
vbt = base + OPREGION_VBT_OFFSET;
/*
* The VBT specification says that if the ASLE ext mailbox is not used
@@ -1028,9 +1005,6 @@ static int intel_opregion_setup(struct drm_i915_private
*dev_priv)
out:
return 0;
-err_out:
- memunmap(base);
- return err;
}
static int
