On Tue, 20 Feb 2024, Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> wrote:
> VBT read from firmware is currently nested within opregion vbt read.
> Extract it and place it together with other vbt read mechanisms and
> dis-associate vbt-firmware from opregion structure.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 48 ++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_opregion.c | 46 ------------------
> 2 files changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 2624a4528b21..4cd338ed362d 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -25,6 +25,8 @@
> *
> */
>
> +#include <linux/firmware.h>
> +
> #include <drm/display/drm_dp_helper.h>
> #include <drm/display/drm_dsc_helper.h>
> #include <drm/drm_edid.h>
> @@ -2950,6 +2952,47 @@ bool intel_bios_is_valid_vbt(struct drm_i915_private *i915,
> return vbt;
> }
>
> +static struct vbt_header *firmware_get_vbt(struct drm_i915_private *i915,
> + size_t *size)
> +{
> + struct vbt_header *vbt = NULL;
> + const struct firmware *fw = NULL;
> + const char *name = i915->display.params.vbt_firmware;
> + int ret;
> +
> + if (!name || !*name)
> + return vbt;
Please return NULL directly.
> +
> + ret = request_firmware(&fw, name, i915->drm.dev);
> + if (ret) {
> + drm_err(&i915->drm,
> + "Requesting VBT firmware \"%s\" failed (%d)\n",
> + name, ret);
> + return vbt;
Please return NULL directly.
> + }
> +
> + if (intel_bios_is_valid_vbt(i915, fw->data, fw->size)) {
> + vbt = kmemdup(fw->data, fw->size, GFP_KERNEL);
> + if (vbt) {
> + drm_dbg_kms(&i915->drm,
> + "Found valid VBT firmware \"%s\"\n", name);
if (size)
> + *size = fw->size;
> + } else {
> + drm_err(&i915->drm,
> + "Requesting VBT firmware \"%s\" failed (%d)\n",
> + name, ENOMEM);
No error messages on ENOMEM, please.
> + vbt = NULL;
You just checked if vbt is NULL.
> + }
> + } else {
> + drm_dbg_kms(&i915->drm, "Invalid VBT firmware \"%s\"\n",
> + name);
> + }
> +
> + release_firmware(fw);
> +
> + return vbt;
> +}
> +
> static u32 intel_spi_read(struct intel_uncore *uncore, u32 offset)
> {
> intel_uncore_write(uncore, PRIMARY_SPI_ADDRESS, offset);
> @@ -3100,7 +3143,10 @@ void intel_bios_init(struct drm_i915_private *i915)
>
> init_vbt_defaults(i915);
>
> - vbt = intel_opregion_get_vbt(i915, NULL);
> + vbt = firmware_get_vbt(i915, &size);
> +
> + if (!vbt)
> + vbt = intel_opregion_get_vbt(i915, NULL);
Crash boom bang without the NULL check.
>
> /*
> * If the OpRegion does not have VBT, look in SPI flash through MMIO or
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index fcbb083318a7..5d07a002edaa 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -27,7 +27,6 @@
>
> #include <linux/acpi.h>
> #include <linux/dmi.h>
> -#include <linux/firmware.h>
> #include <acpi/video.h>
>
> #include <drm/drm_edid.h>
> @@ -263,7 +262,6 @@ struct intel_opregion {
> struct opregion_asle *asle;
> struct opregion_asle_ext *asle_ext;
> void *rvda;
> - void *vbt_firmware;
> const void *vbt;
> u32 vbt_size;
> struct work_struct asle_work;
> @@ -869,46 +867,6 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
> { }
> };
>
> -static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv)
> -{
> - struct intel_opregion *opregion = dev_priv->display.opregion;
> - const struct firmware *fw = NULL;
> - const char *name = dev_priv->display.params.vbt_firmware;
> - int ret;
> -
> - if (!name || !*name)
> - return -ENOENT;
> -
> - ret = request_firmware(&fw, name, dev_priv->drm.dev);
> - if (ret) {
> - drm_err(&dev_priv->drm,
> - "Requesting VBT firmware \"%s\" failed (%d)\n",
> - name, ret);
> - return ret;
> - }
> -
> - if (intel_bios_is_valid_vbt(dev_priv, fw->data, fw->size)) {
> - opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL);
> - if (opregion->vbt_firmware) {
> - drm_dbg_kms(&dev_priv->drm,
> - "Found valid VBT firmware \"%s\"\n", name);
> - opregion->vbt = opregion->vbt_firmware;
> - opregion->vbt_size = fw->size;
> - ret = 0;
> - } else {
> - ret = -ENOMEM;
> - }
> - } else {
> - drm_dbg_kms(&dev_priv->drm, "Invalid VBT firmware \"%s\"\n",
> - name);
> - ret = -EINVAL;
> - }
> -
> - release_firmware(fw);
> -
> - return ret;
> -}
> -
> int intel_opregion_setup(struct drm_i915_private *dev_priv)
> {
> struct intel_opregion *opregion;
> @@ -1006,9 +964,6 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
> drm_dbg(&dev_priv->drm, "Mailbox #2 for backlight present\n");
> }
>
> - if (intel_load_vbt_firmware(dev_priv) == 0)
> - goto out;
> -
> if (dmi_check_system(intel_no_opregion_vbt))
> goto out;
>
> @@ -1312,7 +1267,6 @@ void intel_opregion_cleanup(struct drm_i915_private *i915)
> memunmap(opregion->header);
> if (opregion->rvda)
> memunmap(opregion->rvda);
> - kfree(opregion->vbt_firmware);
> kfree(opregion);
> i915->display.opregion = NULL;
> }
--
Jani Nikula, Intel
