Re: [MIPI SEQ PARSING v1 PATCH 2/9] drm/i915: Parsing VBT if size of VBT exceeds 6KB

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 28 Jul 2015, Deepak M <m.deepak@intel.com> 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@intel.com>
> ---
>  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx