On Tue, 28 Jul 2015, Deepak M <m.deepak@xxxxxxxxx> wrote:
> Currently the iomap for VBT works only if the size of the
> VBT is less than 6KB, but if the size of the VBT exceeds
> 6KB than the physical address and the size of the VBT to
> be iomapped is specified in the mailbox3 and is iomapped
> accordingly.
>
> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_bios.c | 13 +++++++----
> drivers/gpu/drm/i915/intel_opregion.c | 39 ++++++++++++++++++++++++++++++---
> 2 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 2583587..1b9164e 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1219,6 +1219,7 @@ intel_parse_bios(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct pci_dev *pdev = dev->pdev;
> const struct bdb_header *bdb = NULL;
> + const struct vbt_header *vbt = NULL;
> u8 __iomem *bios = NULL;
>
> if (HAS_PCH_NOP(dev))
> @@ -1226,10 +1227,14 @@ intel_parse_bios(struct drm_device *dev)
>
> init_vbt_defaults(dev_priv);
>
> - /* XXX Should this validation be moved to intel_opregion.c? */
> - if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt)
> - bdb = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
> - dev_priv->opregion.vbt, "OpRegion");
> + if (!dmi_check_system(intel_no_opregion_vbt) &&
> + dev_priv->opregion.vbt) {
> + vbt = (struct vbt_header *)dev_priv->opregion.vbt;
> + bdb = (struct bdb_header *)(dev_priv->opregion.vbt +
> + vbt->bdb_offset);
> + DRM_DEBUG_KMS("Using VBT from Opregion: %20s\n",
> + vbt->signature);
> + }
Please read the comment in the beginning of validate_vbt. Please keep
things that way. I put in some effort to clean this up and get rid of a
bunch of extra casts, so please don't add new ones back.
You should probably move some of this stuff to intel_opregion.c and have
a function to return the struct bdb_header *pointer from there.
Please also look into in i915_opregion in i915_debugfs.c, and fix that.
>
> if (bdb == NULL) {
> size_t size;
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 71e87ab..1372e39 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -50,6 +50,7 @@
> #define OPREGION_VBT_OFFSET 0x400
>
> #define OPREGION_SIGNATURE "IntelGraphicsMem"
> +#define VBT_SIGNATURE "$VBT"
Adding this does you no good when you're duplicating this from
intel_bios.c and not removing any from there... really the check should
be in one place only.
> #define MBOX_ACPI (1<<0)
> #define MBOX_SWSCI (1<<1)
> #define MBOX_ASLE (1<<2)
> @@ -113,7 +114,12 @@ struct opregion_asle {
> u32 pcft; /* power conservation features */
> u32 srot; /* supported rotation angles */
> u32 iuer; /* IUER events */
> - u8 rsvd[86];
> + u64 fdss; /* DSS Buffer address allocated for IFFS feature */
> + u32 fdsp; /* Size of DSS Buffer */
> + u32 stat; /* State Indicator */
> + u64 rvda; /* Physical address of raw vbt data */
> + u32 rvds; /* Size of raw vbt data */
> + u8 rsvd[58];
These should be a prep patch that could be merged quickly with a check
against the spec.
> } __packed;
>
> /* Driver readiness indicator */
> @@ -858,8 +864,10 @@ int intel_opregion_setup(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_opregion *opregion = &dev_priv->opregion;
> void __iomem *base;
> + void __iomem *vbt_base;
> u32 asls, mboxes;
> char buf[sizeof(OPREGION_SIGNATURE)];
> + char vbt_sig_buf[sizeof(VBT_SIGNATURE)];
> int err = 0;
>
> pci_read_config_dword(dev->pdev, PCI_ASLS, &asls);
> @@ -873,7 +881,7 @@ int intel_opregion_setup(struct drm_device *dev)
> INIT_WORK(&opregion->asle_work, asle_work);
> #endif
>
> - base = acpi_os_ioremap(asls, OPREGION_SIZE);
> + base = acpi_os_ioremap(asls, OPREGION_VBT_OFFSET);
> if (!base)
> return -ENOMEM;
>
> @@ -884,8 +892,31 @@ int intel_opregion_setup(struct drm_device *dev)
> err = -EINVAL;
> goto err_out;
> }
> +
> opregion->header = base;
> - opregion->vbt = base + OPREGION_VBT_OFFSET;
> + opregion->asle = base + OPREGION_ASLE_OFFSET;
Why is this addition needed or even correct?
> +
> + if (opregion->header->opregion_ver >= 2) {
> + if (opregion->asle->rvda)
> + vbt_base = acpi_os_ioremap(opregion->asle->rvda,
> + opregion->asle->rvds);
> + else
> + vbt_base = acpi_os_ioremap(asls + OPREGION_VBT_OFFSET,
> + OPREGION_SIZE - OPREGION_VBT_OFFSET);
> + } else
> + vbt_base = acpi_os_ioremap(asls + OPREGION_VBT_OFFSET,
> + OPREGION_SIZE - OPREGION_VBT_OFFSET);
The two else branches are identical.
> +
> +
> + memcpy_fromio(vbt_sig_buf, vbt_base, sizeof(vbt_sig_buf));
This is silly, just do what validate_vbt does. (I know, there's
silliness for the opregion signature there already.)
> +
> + if (memcmp(vbt_sig_buf, VBT_SIGNATURE, 4)) {
> + DRM_ERROR("VBT signature mismatch\n");
> + err = -EINVAL;
> + goto err_vbt;
> + }
> +
> + opregion->vbt = vbt_base;
>
> opregion->lid_state = base + ACPI_CLID;
>
> @@ -909,6 +940,8 @@ int intel_opregion_setup(struct drm_device *dev)
>
> return 0;
>
> +err_vbt:
> + iounmap(vbt_base);
> err_out:
> iounmap(base);
> return err;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
