On Mon, Dec 14, 2015 at 04:34:50PM +0200, Jani Nikula wrote:
> On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote:
> >> Make the validation function a boolean operating on a buffer of given
> >> size, removing the extra pointer dances.
> >>
> >> Move the OpRegion based VBT validation to intel_opregion_setup(), only
> >> initializing opregion->vbt if it's valid.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.h | 1 +
> >> drivers/gpu/drm/i915/intel_bios.c | 63 ++++++++++++++++++-----------------
> >> drivers/gpu/drm/i915/intel_opregion.c | 9 +++--
> >> 3 files changed, 40 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index a2bd4c6a86a1..ef15f1710b50 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
> >>
> >> /* intel_bios.c */
> >> int intel_bios_init(struct drm_device *dev);
> >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size);
> >>
> >> /* intel_opregion.c */
> >> #ifdef CONFIG_ACPI
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index f2b79b7d12b8..007b2b759600 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt)
> >> return _vbt + vbt->bdb_offset;
> >> }
> >>
> >> -static const struct vbt_header *validate_vbt(const void *base,
> >> - size_t size,
> >> - const void *_vbt)
> >> +/**
> >> + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT
> >> + * @buf: pointer to a buffer to validate
> >> + * @size: size of the buffer
> >> + *
> >> + * Returns true on valid VBT.
> >> + */
> >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size)
> >> {
> >> - size_t offset = _vbt - base;
> >> - const struct vbt_header *vbt = _vbt;
> >> + const struct vbt_header *vbt = buf;
> >> const struct bdb_header *bdb;
> >>
> >> if (!vbt)
> >> - return NULL;
> >> + return false;
> >>
> >> - if (offset + sizeof(struct vbt_header) > size) {
> >> + if (sizeof(struct vbt_header) > size) {
> >> DRM_DEBUG_DRIVER("VBT header incomplete\n");
> >> - return NULL;
> >> + return false;
> >> }
> >>
> >> if (memcmp(vbt->signature, "$VBT", 4)) {
> >> DRM_DEBUG_DRIVER("VBT invalid signature\n");
> >> - return NULL;
> >> + return false;
> >> }
> >>
> >> - offset += vbt->bdb_offset;
> >> - if (offset + sizeof(struct bdb_header) > size) {
> >> + if (vbt->bdb_offset + sizeof(struct bdb_header) > size) {
> >> DRM_DEBUG_DRIVER("BDB header incomplete\n");
> >> - return NULL;
> >> + return false;
> >> }
> >>
> >> bdb = get_bdb_header(vbt);
> >> - if (offset + bdb->bdb_size > size) {
> >> + if (vbt->bdb_offset + bdb->bdb_size > size) {
> >> DRM_DEBUG_DRIVER("BDB incomplete\n");
> >> - return NULL;
> >> + return false;
> >> }
> >>
> >> return vbt;
> >> @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base,
> >>
> >> static const struct vbt_header *find_vbt(void __iomem *bios, size_t size)
> >> {
> >> - const struct vbt_header *vbt = NULL;
> >> size_t i;
> >>
> >> /* Scour memory looking for the VBT signature. */
> >> for (i = 0; i + 4 < size; i++) {
> >> - if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
> >> - /*
> >> - * This is the one place where we explicitly discard the
> >> - * address space (__iomem) of the BIOS/VBT. From now on
> >> - * everything is based on 'base', and treated as regular
> >> - * memory.
> >> - */
> >> - void *_bios = (void __force *) bios;
> >> + void *vbt;
> >>
> >> - vbt = validate_vbt(_bios, size, _bios + i);
> >> - break;
> >> - }
> >> + if (ioread32(bios + i) != *((const u32 *) "$VBT"))
> >> + continue;
> >> +
> >> + /*
> >> + * This is the one place where we explicitly discard the address
> >> + * space (__iomem) of the BIOS/VBT.
> >> + */
> >> + vbt = (void __force *) bios + i;
> >> + if (intel_bios_is_valid_vbt(vbt, size - i))
> >> + return vbt;
> >> +
> >> + break;
> >> }
> >>
> >> - return vbt;
> >> + return NULL;
> >> }
> >>
> >> /**
> >> @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev)
> >> {
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct pci_dev *pdev = dev->pdev;
> >> - const struct vbt_header *vbt;
> >> + const struct vbt_header *vbt = dev_priv->opregion.vbt;
> >> const struct bdb_header *bdb;
> >> u8 __iomem *bios = NULL;
> >>
> >> @@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev)
> >>
> >> init_vbt_defaults(dev_priv);
> >>
> >> - /* XXX Should this validation be moved to intel_opregion.c? */
> >> - vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE,
> >> - dev_priv->opregion.vbt);
> >> if (vbt) {
> >> DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n",
> >> vbt->signature);
> >
> > Maybe the debug message should be moved too? It looks a bit out of place
> > after the validation gets moved.
>
> This is intentional (see patch 4). In a way this is where the decision
> is made to use the opregion VBT. The opregion code just validates it,
> but doesn't actually *use* it.
But when you add the RVDA thing, the debug print for that is in the
opregion code, so with that we get two debug prints?
>
> BR,
> Jani.
>
>
>
> >
> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> >> index 5b9fc790d300..9f992e7c6185 100644
> >> --- a/drivers/gpu/drm/i915/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> >> @@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev)
> >> if (mboxes & MBOX_ASLE_EXT)
> >> DRM_DEBUG_DRIVER("ASLE extension supported\n");
> >>
> >> - if (!dmi_check_system(intel_no_opregion_vbt))
> >> - opregion->vbt = base + OPREGION_VBT_OFFSET;
> >> + if (!dmi_check_system(intel_no_opregion_vbt)) {
> >> + void *vbt = base + OPREGION_VBT_OFFSET;
> >> + u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
> >> +
> >> + if (intel_bios_is_valid_vbt(vbt, vbt_size))
> >> + opregion->vbt = vbt;
> >> + }
> >>
> >> return 0;
> >>
> >> --
> >> 2.1.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
