Re: [MIPI SEQ PARSING v2 PATCH 03/11] drm/i915: Parsing VBT if size of VBT exceeds 6KB

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 22 Sep 2015, "Deepak, M" <m.deepak@xxxxxxxxx> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx]
>>Sent: Thursday, September 17, 2015 5:41 PM
>>To: Deepak, M; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>Cc: Deepak, M
>>Subject: Re:  [MIPI SEQ PARSING v2 PATCH 03/11] drm/i915: Parsing
>>VBT if size of VBT exceeds 6KB
>>
>>On Thu, 10 Sep 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.
>>>
>>> v2: - Moving the validate_vbt to opregion file (Jani)
>>>     - Fix the i915_opregion() in debugfs (Jani)
>>>
>>> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c   |   24 ++-
>>>  drivers/gpu/drm/i915/i915_drv.h       |    4 +
>>>  drivers/gpu/drm/i915/intel_bios.c     |   49 +-----
>>>  drivers/gpu/drm/i915/intel_opregion.c |  279
>>> +++++++++------------------------
>>> drivers/gpu/drm/i915/intel_opregion.h |  230
>>> +++++++++++++++++++++++++++
>>>  5 files changed, 329 insertions(+), 257 deletions(-)  create mode
>>> 100644 drivers/gpu/drm/i915/intel_opregion.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 41629fa..5534aa2 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -26,6 +26,8 @@
>>>   *
>>>   */
>>>
>>> +#include <linux/acpi.h>
>>> +#include <acpi/video.h>
>>>  #include <linux/seq_file.h>
>>>  #include <linux/circ_buf.h>
>>>  #include <linux/ctype.h>
>>> @@ -39,6 +41,7 @@
>>>  #include "intel_ringbuffer.h"
>>>  #include <drm/i915_drm.h>
>>>  #include "i915_drv.h"
>>> +#include "intel_opregion.h"
>>>
>>>  enum {
>>>  	ACTIVE_LIST,
>>> @@ -1832,7 +1835,7 @@ static int i915_opregion(struct seq_file *m, void
>>*unused)
>>>  	struct drm_device *dev = node->minor->dev;
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  	struct intel_opregion *opregion = &dev_priv->opregion;
>>> -	void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL);
>>> +	void *data = kmalloc(OPREGION_VBT_OFFSET, GFP_KERNEL);
>>>  	int ret;
>>>
>>>  	if (data == NULL)
>>> @@ -1843,12 +1846,25 @@ static int i915_opregion(struct seq_file *m, void
>>*unused)
>>>  		goto out;
>>>
>>>  	if (opregion->header) {
>>> -		memcpy_fromio(data, opregion->header, OPREGION_SIZE);
>>> -		seq_write(m, data, OPREGION_SIZE);
>>> +		memcpy_fromio(data, opregion->header,
>>OPREGION_VBT_OFFSET);
>>> +		seq_write(m, data, OPREGION_VBT_OFFSET);
>>> +		kfree(data);
>>> +		if (opregion->asle->rvda) {
>>> +			data = kmalloc(opregion->asle->rvds, GFP_KERNEL);
>>> +			memcpy_fromio(data,
>>> +				(const void __iomem *) opregion->asle->rvda,
>>> +				opregion->asle->rvds);
>>> +			seq_write(m, data, opregion->asle->rvds);
>>> +		} else {
>>> +			data = kmalloc(OPREGION_SIZE -
>>OPREGION_VBT_OFFSET,
>>> +					GFP_KERNEL);
>>> +			memcpy_fromio(data, opregion->vbt,
>>> +					OPREGION_SIZE -
>>OPREGION_VBT_OFFSET);
>>> +			seq_write(m, data, OPREGION_SIZE -
>>OPREGION_VBT_OFFSET);
>>> +		}
>>
>>If rvda != 0, this debugfs file no longer represents the opregion contents.
>>Mailboxes #4 and #5 are dropped from the output. BTW, what is mailbox #4
>>expected to contain when rvda != 0? (I still don't have access to the latest
>>opregion spec version, so can't check what it actually says.)
>>
>>I am beginning to think we should leave "i915_opregion" debugfs file intact,
>>and add a new "i915_vbt" file that contains either mailbox #4 or the data in
>>rvda. This might be a cleaner approach.
>>
>>See my comments below, and you'll see how this would be feasible.
>>
> [Deepak M] I was thinking of splitting this function into 5 for dumping each mailbox. Which 
> I felt will be cleaner.

Please have i915_opregion and i915_vbt as I explained.

>>>  	}
>>>
>>>  	mutex_unlock(&dev->struct_mutex);
>>> -
>>>  out:
>>>  	kfree(data);
>>>  	return 0;
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h index 1287007..507d57a 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1780,6 +1780,7 @@ struct drm_i915_private {
>>>  	struct i915_hotplug hotplug;
>>>  	struct i915_fbc fbc;
>>>  	struct i915_drrs drrs;
>>> +	const struct bdb_header *bdb_start;
>>
>>What is wrong with using dev_priv->opregion.vbt for this?
>>
>>>  	struct intel_opregion opregion;
>>>  	struct intel_vbt_data vbt;
>>>
>>> @@ -3306,6 +3307,9 @@ intel_opregion_notify_adapter(struct drm_device
>>> *dev, pci_power_t state)  }  #endif
>>>
>>> +const struct bdb_header *validate_vbt(const void __iomem *_vbt,
>>> +			size_t size, const char *source);
>>> +
>>>  /* intel_acpi.c */
>>>  #ifdef CONFIG_ACPI
>>>  extern void intel_register_dsm_handler(void); diff --git
>>> a/drivers/gpu/drm/i915/intel_bios.c
>>> b/drivers/gpu/drm/i915/intel_bios.c
>>> index 0bf0942..1932a86 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -1227,49 +1227,6 @@ static const struct dmi_system_id
>>intel_no_opregion_vbt[] = {
>>>  	{ }
>>>  };
>>>
>>> -static const struct bdb_header *validate_vbt(const void __iomem *_base,
>>> -					     size_t size,
>>> -					     const void __iomem *_vbt,
>>> -					     const char *source)
>>> -{
>>> -	/*
>>> -	 * This is the one place where we explicitly discard the address space
>>> -	 * (__iomem) of the BIOS/VBT. (And this will cause a sparse
>>complaint.)
>>> -	 * From now on everything is based on 'base', and treated as regular
>>> -	 * memory.
>>> -	 */
>>> -	const void *base = (const void *) _base;
>>> -	size_t offset = _vbt - _base;
>>> -	const struct vbt_header *vbt = base + offset;
>>> -	const struct bdb_header *bdb;
>>> -
>>> -	if (offset + sizeof(struct vbt_header) > size) {
>>> -		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>>> -		return NULL;
>>> -	}
>>> -
>>> -	if (memcmp(vbt->signature, "$VBT", 4)) {
>>> -		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>> -		return NULL;
>>> -	}
>>> -
>>> -	offset += vbt->bdb_offset;
>>> -	if (offset + sizeof(struct bdb_header) > size) {
>>> -		DRM_DEBUG_DRIVER("BDB header incomplete\n");
>>> -		return NULL;
>>> -	}
>>> -
>>> -	bdb = base + offset;
>>> -	if (offset + bdb->bdb_size > size) {
>>> -		DRM_DEBUG_DRIVER("BDB incomplete\n");
>>> -		return NULL;
>>> -	}
>>> -
>>> -	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
>>> -		      source, vbt->signature);
>>> -	return bdb;
>>> -}
>>> -
>>
>>Moving of this function should be a separate prep patch.
>>
>>>  static const struct bdb_header *find_vbt(void __iomem *bios, size_t
>>> size)  {
>>>  	const struct bdb_header *bdb = NULL; @@ -1278,7 +1235,7 @@ static
>>> const struct bdb_header *find_vbt(void __iomem *bios, size_t size)
>>>  	/* Scour memory looking for the VBT signature. */
>>>  	for (i = 0; i + 4 < size; i++) {
>>>  		if (ioread32(bios + i) == *((const u32 *) "$VBT")) {
>>> -			bdb = validate_vbt(bios, size, bios + i, "PCI ROM");
>>> +			bdb = validate_vbt(bios + i, size - i, "PCI ROM");
>>>  			break;
>>>  		}
>>>  	}
>>> @@ -1308,10 +1265,8 @@ 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");
>>> +		bdb = dev_priv->bdb_start;
>>
>>This should be dev_priv->opregion.vbt.
> [Deepak M] validate_vbt function always returns the bdb header and our parsing starts from the bdb_header and not form the dev_priv->opregion.vbt where the VBT header starts. So I have added bdb_start in the dev_priv structure.
>>

Please don't add new fields when you can reuse existing ones.

>>>
>>>  	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 f46231f..3a43db9 100644
>>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> @@ -32,210 +32,7 @@
>>>  #include <drm/i915_drm.h>
>>>  #include "i915_drv.h"
>>>  #include "intel_drv.h"
>>> -
>>> -#define PCI_ASLE		0xe4
>>> -#define PCI_ASLS		0xfc
>>> -#define PCI_SWSCI		0xe8
>>> -#define PCI_SWSCI_SCISEL	(1 << 15)
>>> -#define PCI_SWSCI_GSSCIE	(1 << 0)
>>> -
>>> -#define OPREGION_HEADER_OFFSET 0
>>> -#define OPREGION_ACPI_OFFSET   0x100
>>> -#define   ACPI_CLID 0x01ac /* current lid state indicator */
>>> -#define   ACPI_CDCK 0x01b0 /* current docking state indicator */
>>> -#define OPREGION_SWSCI_OFFSET  0x200
>>> -#define OPREGION_ASLE_OFFSET   0x300
>>> -#define OPREGION_VBT_OFFSET    0x400
>>> -
>>> -#define OPREGION_SIGNATURE "IntelGraphicsMem"
>>> -#define MBOX_ACPI      (1<<0)
>>> -#define MBOX_SWSCI     (1<<1)
>>> -#define MBOX_ASLE      (1<<2)
>>> -#define MBOX_ASLE_EXT  (1<<4)
>>> -
>>> -struct opregion_header {
>>> -	u8 signature[16];
>>> -	u32 size;
>>> -	u32 opregion_ver;
>>> -	u8 bios_ver[32];
>>> -	u8 vbios_ver[16];
>>> -	u8 driver_ver[16];
>>> -	u32 mboxes;
>>> -	u32 driver_model;
>>> -	u32 pcon;
>>> -	u8 dver[32];
>>> -	u8 rsvd[124];
>>> -} __packed;
>>> -
>>> -/* OpRegion mailbox #1: public ACPI methods */ -struct opregion_acpi
>>> {
>>> -	u32 drdy;       /* driver readiness */
>>> -	u32 csts;       /* notification status */
>>> -	u32 cevt;       /* current event */
>>> -	u8 rsvd1[20];
>>> -	u32 didl[8];    /* supported display devices ID list */
>>> -	u32 cpdl[8];    /* currently presented display list */
>>> -	u32 cadl[8];    /* currently active display list */
>>> -	u32 nadl[8];    /* next active devices list */
>>> -	u32 aslp;       /* ASL sleep time-out */
>>> -	u32 tidx;       /* toggle table index */
>>> -	u32 chpd;       /* current hotplug enable indicator */
>>> -	u32 clid;       /* current lid state*/
>>> -	u32 cdck;       /* current docking state */
>>> -	u32 sxsw;       /* Sx state resume */
>>> -	u32 evts;       /* ASL supported events */
>>> -	u32 cnot;       /* current OS notification */
>>> -	u32 nrdy;       /* driver status */
>>> -	u32 did2[7];	/* extended supported display devices ID list */
>>> -	u32 cpd2[7];	/* extended attached display devices list */
>>> -	u8 rsvd2[4];
>>> -} __packed;
>>> -
>>> -/* OpRegion mailbox #2: SWSCI */
>>> -struct opregion_swsci {
>>> -	u32 scic;       /* SWSCI command|status|data */
>>> -	u32 parm;       /* command parameters */
>>> -	u32 dslp;       /* driver sleep time-out */
>>> -	u8 rsvd[244];
>>> -} __packed;
>>> -
>>> -/* OpRegion mailbox #3: ASLE */
>>> -struct opregion_asle {
>>> -	u32 ardy;       /* driver readiness */
>>> -	u32 aslc;       /* ASLE interrupt command */
>>> -	u32 tche;       /* technology enabled indicator */
>>> -	u32 alsi;       /* current ALS illuminance reading */
>>> -	u32 bclp;       /* backlight brightness to set */
>>> -	u32 pfit;       /* panel fitting state */
>>> -	u32 cblv;       /* current brightness level */
>>> -	u16 bclm[20];   /* backlight level duty cycle mapping table */
>>> -	u32 cpfm;       /* current panel fitting mode */
>>> -	u32 epfm;       /* enabled panel fitting modes */
>>> -	u8 plut[74];    /* panel LUT and identifier */
>>> -	u32 pfmb;       /* PWM freq and min brightness */
>>> -	u32 cddv;       /* color correction default values */
>>> -	u32 pcft;       /* power conservation features */
>>> -	u32 srot;       /* supported rotation angles */
>>> -	u32 iuer;       /* IUER events */
>>> -	u64 fdss;
>>> -	u32 fdsp;
>>> -	u32 stat;
>>> -	u64 rvda;	/* Physical address of raw vbt data */
>>> -	u32 rvds;	/* Size of raw vbt data */
>>> -	u8 rsvd[58];
>>> -} __packed;
>>> -
>>> -/* Driver readiness indicator */
>>> -#define ASLE_ARDY_READY		(1 << 0)
>>> -#define ASLE_ARDY_NOT_READY	(0 << 0)
>>> -
>>> -/* ASLE Interrupt Command (ASLC) bits */
>>> -#define ASLC_SET_ALS_ILLUM		(1 << 0)
>>> -#define ASLC_SET_BACKLIGHT		(1 << 1)
>>> -#define ASLC_SET_PFIT			(1 << 2)
>>> -#define ASLC_SET_PWM_FREQ		(1 << 3)
>>> -#define ASLC_SUPPORTED_ROTATION_ANGLES	(1 << 4)
>>> -#define ASLC_BUTTON_ARRAY		(1 << 5)
>>> -#define ASLC_CONVERTIBLE_INDICATOR	(1 << 6)
>>> -#define ASLC_DOCKING_INDICATOR		(1 << 7)
>>> -#define ASLC_ISCT_STATE_CHANGE		(1 << 8)
>>> -#define ASLC_REQ_MSK			0x1ff
>>> -/* response bits */
>>> -#define ASLC_ALS_ILLUM_FAILED		(1 << 10)
>>> -#define ASLC_BACKLIGHT_FAILED		(1 << 12)
>>> -#define ASLC_PFIT_FAILED		(1 << 14)
>>> -#define ASLC_PWM_FREQ_FAILED		(1 << 16)
>>> -#define ASLC_ROTATION_ANGLES_FAILED	(1 << 18)
>>> -#define ASLC_BUTTON_ARRAY_FAILED	(1 << 20)
>>> -#define ASLC_CONVERTIBLE_FAILED		(1 << 22)
>>> -#define ASLC_DOCKING_FAILED		(1 << 24)
>>> -#define ASLC_ISCT_STATE_FAILED		(1 << 26)
>>> -
>>> -/* Technology enabled indicator */
>>> -#define ASLE_TCHE_ALS_EN	(1 << 0)
>>> -#define ASLE_TCHE_BLC_EN	(1 << 1)
>>> -#define ASLE_TCHE_PFIT_EN	(1 << 2)
>>> -#define ASLE_TCHE_PFMB_EN	(1 << 3)
>>> -
>>> -/* ASLE backlight brightness to set */
>>> -#define ASLE_BCLP_VALID                (1<<31)
>>> -#define ASLE_BCLP_MSK          (~(1<<31))
>>> -
>>> -/* ASLE panel fitting request */
>>> -#define ASLE_PFIT_VALID         (1<<31)
>>> -#define ASLE_PFIT_CENTER (1<<0)
>>> -#define ASLE_PFIT_STRETCH_TEXT (1<<1) -#define
>>ASLE_PFIT_STRETCH_GFX
>>> (1<<2)
>>> -
>>> -/* PWM frequency and minimum brightness */ -#define
>>> ASLE_PFMB_BRIGHTNESS_MASK (0xff) -#define
>>ASLE_PFMB_BRIGHTNESS_VALID
>>> (1<<8) -#define ASLE_PFMB_PWM_MASK (0x7ffffe00) -#define
>>> ASLE_PFMB_PWM_VALID (1<<31)
>>> -
>>> -#define ASLE_CBLV_VALID         (1<<31)
>>> -
>>> -/* IUER */
>>> -#define ASLE_IUER_DOCKING		(1 << 7)
>>> -#define ASLE_IUER_CONVERTIBLE		(1 << 6)
>>> -#define ASLE_IUER_ROTATION_LOCK_BTN	(1 << 4)
>>> -#define ASLE_IUER_VOLUME_DOWN_BTN	(1 << 3)
>>> -#define ASLE_IUER_VOLUME_UP_BTN		(1 << 2)
>>> -#define ASLE_IUER_WINDOWS_BTN		(1 << 1)
>>> -#define ASLE_IUER_POWER_BTN		(1 << 0)
>>> -
>>> -/* Software System Control Interrupt (SWSCI) */
>>> -#define SWSCI_SCIC_INDICATOR		(1 << 0)
>>> -#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT	1
>>> -#define SWSCI_SCIC_MAIN_FUNCTION_MASK	(0xf << 1)
>>> -#define SWSCI_SCIC_SUB_FUNCTION_SHIFT	8
>>> -#define SWSCI_SCIC_SUB_FUNCTION_MASK	(0xff << 8)
>>> -#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT	8
>>> -#define SWSCI_SCIC_EXIT_PARAMETER_MASK	(0xff << 8)
>>> -#define SWSCI_SCIC_EXIT_STATUS_SHIFT	5
>>> -#define SWSCI_SCIC_EXIT_STATUS_MASK	(7 << 5)
>>> -#define SWSCI_SCIC_EXIT_STATUS_SUCCESS	1
>>> -
>>> -#define SWSCI_FUNCTION_CODE(main, sub) \
>>> -	((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
>>> -	 (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
>>> -
>>> -/* SWSCI: Get BIOS Data (GBDA) */
>>> -#define SWSCI_GBDA			4
>>> -#define SWSCI_GBDA_SUPPORTED_CALLS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
>>> -#define SWSCI_GBDA_REQUESTED_CALLBACKS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
>>> -#define SWSCI_GBDA_BOOT_DISPLAY_PREF
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
>>> -#define SWSCI_GBDA_PANEL_DETAILS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
>>> -#define SWSCI_GBDA_TV_STANDARD
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
>>> -#define SWSCI_GBDA_INTERNAL_GRAPHICS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
>>> -#define SWSCI_GBDA_SPREAD_SPECTRUM
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
>>> -
>>> -/* SWSCI: System BIOS Callbacks (SBCB) */
>>> -#define SWSCI_SBCB			6
>>> -#define SWSCI_SBCB_SUPPORTED_CALLBACKS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
>>> -#define SWSCI_SBCB_INIT_COMPLETION
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
>>> -#define SWSCI_SBCB_PRE_HIRES_SET_MODE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
>>> -#define SWSCI_SBCB_POST_HIRES_SET_MODE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
>>> -#define SWSCI_SBCB_DISPLAY_SWITCH
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
>>> -#define SWSCI_SBCB_SET_TV_FORMAT
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
>>> -#define SWSCI_SBCB_ADAPTER_POWER_STATE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
>>> -#define SWSCI_SBCB_DISPLAY_POWER_STATE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
>>> -#define SWSCI_SBCB_SET_BOOT_DISPLAY
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
>>> -#define SWSCI_SBCB_SET_PANEL_DETAILS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10)
>>> -#define SWSCI_SBCB_SET_INTERNAL_GFX
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
>>> -#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
>>> -#define SWSCI_SBCB_SUSPEND_RESUME
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
>>> -#define SWSCI_SBCB_SET_SPREAD_SPECTRUM
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
>>> -#define SWSCI_SBCB_POST_VBE_PM
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
>>> -#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>>> -
>>> -#define ACPI_OTHER_OUTPUT (0<<8)
>>> -#define ACPI_VGA_OUTPUT (1<<8)
>>> -#define ACPI_TV_OUTPUT (2<<8)
>>> -#define ACPI_DIGITAL_OUTPUT (3<<8)
>>> -#define ACPI_LVDS_OUTPUT (4<<8)
>>> -
>>> -#define MAX_DSLP	1500
>>> +#include "intel_opregion.h"
>>
>>As said, I don't see the need to move these defines to a separate header. This
>>is definitely stuff that we want to keep hidden in one place, and nobody
>>outside of intel_opregion.c should use these.
>>
> [Deepak M] Need some of these definitions in the debugfs.c file, because of this
> had created a new file for macros.

Please rework your debugfs handling to not need these. If needed, add an
interface to intel_opregion.c to do what you want.

>
>>>
>>>  #ifdef CONFIG_ACPI
>>>  static int swsci(struct drm_device *dev, u32 function, u32 parm, u32
>>> *parm_out) @@ -892,13 +689,55 @@ static void swsci_setup(struct
>>> drm_device *dev)  static inline void swsci_setup(struct drm_device
>>> *dev) {}  #endif  /* CONFIG_ACPI */
>>>
>>> +const struct bdb_header *validate_vbt(const void __iomem *_vbt,
>>> +					size_t size,
>>> +					const char *source)
>>> +{
>>> +	/*
>>> +	 * This is the one place where we explicitly discard the address space
>>> +	 * (__iomem) of the BIOS/VBT. (And this will cause a sparse
>>complaint.)
>>> +	 */
>>> +	const struct vbt_header *vbt = (const struct vbt_header *)_vbt;
>>> +	const struct bdb_header *bdb;
>>> +	size_t offset;
>>> +
>>> +	if (sizeof(struct vbt_header) > size) {
>>> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	if (memcmp(vbt->signature, "$VBT", 4)) {
>>> +		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	offset = vbt->bdb_offset;
>>> +	if (offset + sizeof(struct bdb_header) > size) {
>>> +		DRM_DEBUG_DRIVER("BDB header incomplete\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	bdb = (const void *)_vbt + offset;
>>> +	if (offset + bdb->bdb_size > size) {
>>> +		DRM_DEBUG_DRIVER("BDB incomplete\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
>>> +			source, vbt->signature);
>>> +	return bdb;
>>> +}
>>> +
>>>  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)];
>>> +	const struct bdb_header *bdb = NULL;
>>> +	size_t size;
>>>  	int err = 0;
>>>
>>>  	BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100); @@ -
>>917,7
>>> +756,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);
>>
>>Now you leave out mailbox #5. I don't think there's any reason *not* to
>>ioremap all of opregion here.
>>
>>>  	if (!base)
>>>  		return -ENOMEM;
>>>
>>> @@ -929,7 +768,33 @@ int intel_opregion_setup(struct drm_device *dev)
>>>  		goto err_out;
>>>  	}
>>>  	opregion->header = base;
>>> -	opregion->vbt = base + OPREGION_VBT_OFFSET;
>>> +	/* Assigning the alse to the mailbox 3 of the opregion */
>>> +	opregion->asle = base + OPREGION_ASLE_OFFSET;
>>
>>That's assigned towards the end of the function *if* the mbox is supported.
>>
>>> +
>>> +	/*
>>> +	 * Non-zero value in rvda field is an indication to driver that a
>>> +	 * valid Raw VBT is stored in that address and driver should not refer
>>> +	 * to mailbox4 for getting VBT.
>>> +	 */
>>> +	if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) {
>>> +			size = opregion->asle->rvds;
>>> +			vbt_base = acpi_os_ioremap(opregion->asle->rvda,
>>> +					size);
>>> +	} else {
>>> +			size = OPREGION_SIZE - OPREGION_VBT_OFFSET;
>>> +			vbt_base = acpi_os_ioremap(asls +
>>OPREGION_VBT_OFFSET,
>>> +					size);
>>> +	}
>>> +
>>> +	bdb = validate_vbt(vbt_base, size, "OpRegion");
>>> +
>>> +	if (bdb == NULL) {
>>> +		err = -EINVAL;
>>> +		goto err_vbt;
>>> +	}
>>> +
>>> +	dev_priv->bdb_start = bdb;
>>
>>Again, I don't see why you should store this here. Nor I see the need to
>>actually validate vbt here. You can just as well do it in intel_bios like before, as
>>long as you assign opregion->vbt here appropriately.
>>
>>You'll also need to iounmap vbt_base in intel_opregion_fini. That may need
>>some additional checks if you unconditionally ioremap all of opregion and
>>conditionally ioremap rvda, and point opregion->vbt to it.
>>
> [Deepak M] Okay will handle this.
>>> +	opregion->vbt = vbt_base;
>>>
>>>  	opregion->lid_state = base + ACPI_CLID;
>>>
>>> @@ -953,6 +818,8 @@ int intel_opregion_setup(struct drm_device *dev)
>>>
>>>  	return 0;
>>>
>>> +err_vbt:
>>> +	iounmap(vbt_base);
>>>  err_out:
>>>  	iounmap(base);
>>>  	return err;
>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.h
>>> b/drivers/gpu/drm/i915/intel_opregion.h
>>> new file mode 100644
>>> index 0000000..bcb45ec
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.h
>>> @@ -0,0 +1,230 @@
>>> +/*
>>> + * Copyright 2008 Intel Corporation <hong.liu@xxxxxxxxx>
>>> + * Copyright 2008 Red Hat <mjg@xxxxxxxxxx>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> +obtaining
>>> + * a copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction,
>>> +including
>>> + * without limitation the rights to use, copy, modify, merge,
>>> +publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and
>>> +to
>>> + * permit persons to whom the Software is furnished to do so, subject
>>> +to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including
>>> +the
>>> + * next paragraph) shall be included in all copies or substantial
>>> + * portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>>KIND,
>>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
>>WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>>> + * NON-INFRINGEMENT.  IN NO EVENT SHALL INTEL AND/OR ITS SUPPLIERS
>>BE
>>> + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF
>>OR IN
>>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>THE
>>> + * SOFTWARE.
>>> + *
>>> + */
>>> +
>>> +#define PCI_ASLE		0xe4
>>> +#define PCI_ASLS		0xfc
>>> +#define PCI_SWSCI		0xe8
>>> +#define PCI_SWSCI_SCISEL	(1 << 15)
>>> +#define PCI_SWSCI_GSSCIE	(1 << 0)
>>> +
>>> +#define OPREGION_HEADER_OFFSET 0
>>> +#define OPREGION_ACPI_OFFSET   0x100
>>> +#define   ACPI_CLID 0x01ac /* current lid state indicator */
>>> +#define   ACPI_CDCK 0x01b0 /* current docking state indicator */
>>> +#define OPREGION_SWSCI_OFFSET  0x200
>>> +#define OPREGION_ASLE_OFFSET   0x300
>>> +#define OPREGION_VBT_OFFSET    0x400
>>> +
>>> +#define OPREGION_SIGNATURE "IntelGraphicsMem"
>>> +#define MBOX_ACPI      (1<<0)
>>> +#define MBOX_SWSCI     (1<<1)
>>> +#define MBOX_ASLE      (1<<2)
>>> +#define MBOX_ASLE_EXT  (1<<4)
>>> +
>>> +struct opregion_header {
>>> +	u8 signature[16];
>>> +	u32 size;
>>> +	u32 opregion_ver;
>>> +	u8 bios_ver[32];
>>> +	u8 vbios_ver[16];
>>> +	u8 driver_ver[16];
>>> +	u32 mboxes;
>>> +	u32 driver_model;
>>> +	u32 pcon;
>>> +	u8 dver[32];
>>> +	u8 rsvd[124];
>>> +} __packed;
>>> +
>>> +/* OpRegion mailbox #1: public ACPI methods */ struct opregion_acpi {
>>> +	u32 drdy;       /* driver readiness */
>>> +	u32 csts;       /* notification status */
>>> +	u32 cevt;       /* current event */
>>> +	u8 rsvd1[20];
>>> +	u32 didl[8];    /* supported display devices ID list */
>>> +	u32 cpdl[8];    /* currently presented display list */
>>> +	u32 cadl[8];    /* currently active display list */
>>> +	u32 nadl[8];    /* next active devices list */
>>> +	u32 aslp;       /* ASL sleep time-out */
>>> +	u32 tidx;       /* toggle table index */
>>> +	u32 chpd;       /* current hotplug enable indicator */
>>> +	u32 clid;       /* current lid state*/
>>> +	u32 cdck;       /* current docking state */
>>> +	u32 sxsw;       /* Sx state resume */
>>> +	u32 evts;       /* ASL supported events */
>>> +	u32 cnot;       /* current OS notification */
>>> +	u32 nrdy;       /* driver status */
>>> +	u32 did2[7];	/* extended supported display devices ID list */
>>> +	u32 cpd2[7];	/* extended attached display devices list */
>>> +	u8 rsvd2[4];
>>> +} __packed;
>>> +
>>> +/* OpRegion mailbox #2: SWSCI */
>>> +struct opregion_swsci {
>>> +	u32 scic;       /* SWSCI command|status|data */
>>> +	u32 parm;       /* command parameters */
>>> +	u32 dslp;       /* driver sleep time-out */
>>> +	u8 rsvd[244];
>>> +} __packed;
>>> +
>>> +/* OpRegion mailbox #3: ASLE */
>>> +struct opregion_asle {
>>> +	u32 ardy;       /* driver readiness */
>>> +	u32 aslc;       /* ASLE interrupt command */
>>> +	u32 tche;       /* technology enabled indicator */
>>> +	u32 alsi;       /* current ALS illuminance reading */
>>> +	u32 bclp;       /* backlight brightness to set */
>>> +	u32 pfit;       /* panel fitting state */
>>> +	u32 cblv;       /* current brightness level */
>>> +	u16 bclm[20];   /* backlight level duty cycle mapping table */
>>> +	u32 cpfm;       /* current panel fitting mode */
>>> +	u32 epfm;       /* enabled panel fitting modes */
>>> +	u8 plut[74];    /* panel LUT and identifier */
>>> +	u32 pfmb;       /* PWM freq and min brightness */
>>> +	u32 cddv;       /* color correction default values */
>>> +	u32 pcft;       /* power conservation features */
>>> +	u32 srot;       /* supported rotation angles */
>>> +	u32 iuer;       /* IUER events */
>>> +	u64 fdss;
>>> +	u32 fdsp;
>>> +	u32 stat;
>>> +	u64 rvda;	/* Physical address of raw vbt data */
>>> +	u32 rvds;	/* Size of raw vbt data */
>>> +	u8 rsvd[58];
>>> +} __packed;
>>> +
>>> +/* Driver readiness indicator */
>>> +#define ASLE_ARDY_READY		(1 << 0)
>>> +#define ASLE_ARDY_NOT_READY	(0 << 0)
>>> +
>>> +/* ASLE Interrupt Command (ASLC) bits */
>>> +#define ASLC_SET_ALS_ILLUM		(1 << 0)
>>> +#define ASLC_SET_BACKLIGHT		(1 << 1)
>>> +#define ASLC_SET_PFIT			(1 << 2)
>>> +#define ASLC_SET_PWM_FREQ		(1 << 3)
>>> +#define ASLC_SUPPORTED_ROTATION_ANGLES	(1 << 4)
>>> +#define ASLC_BUTTON_ARRAY		(1 << 5)
>>> +#define ASLC_CONVERTIBLE_INDICATOR	(1 << 6)
>>> +#define ASLC_DOCKING_INDICATOR		(1 << 7)
>>> +#define ASLC_ISCT_STATE_CHANGE		(1 << 8)
>>> +#define ASLC_REQ_MSK			0x1ff
>>> +/* response bits */
>>> +#define ASLC_ALS_ILLUM_FAILED		(1 << 10)
>>> +#define ASLC_BACKLIGHT_FAILED		(1 << 12)
>>> +#define ASLC_PFIT_FAILED		(1 << 14)
>>> +#define ASLC_PWM_FREQ_FAILED		(1 << 16)
>>> +#define ASLC_ROTATION_ANGLES_FAILED	(1 << 18)
>>> +#define ASLC_BUTTON_ARRAY_FAILED	(1 << 20)
>>> +#define ASLC_CONVERTIBLE_FAILED		(1 << 22)
>>> +#define ASLC_DOCKING_FAILED		(1 << 24)
>>> +#define ASLC_ISCT_STATE_FAILED		(1 << 26)
>>> +
>>> +/* Technology enabled indicator */
>>> +#define ASLE_TCHE_ALS_EN	(1 << 0)
>>> +#define ASLE_TCHE_BLC_EN	(1 << 1)
>>> +#define ASLE_TCHE_PFIT_EN	(1 << 2)
>>> +#define ASLE_TCHE_PFMB_EN	(1 << 3)
>>> +
>>> +/* ASLE backlight brightness to set */
>>> +#define ASLE_BCLP_VALID                (1<<31)
>>> +#define ASLE_BCLP_MSK          (~(1<<31))
>>> +
>>> +/* ASLE panel fitting request */
>>> +#define ASLE_PFIT_VALID         (1<<31)
>>> +#define ASLE_PFIT_CENTER (1<<0)
>>> +#define ASLE_PFIT_STRETCH_TEXT (1<<1) #define
>>ASLE_PFIT_STRETCH_GFX
>>> +(1<<2)
>>> +
>>> +/* PWM frequency and minimum brightness */ #define
>>> +ASLE_PFMB_BRIGHTNESS_MASK (0xff) #define
>>ASLE_PFMB_BRIGHTNESS_VALID
>>> +(1<<8) #define ASLE_PFMB_PWM_MASK (0x7ffffe00) #define
>>> +ASLE_PFMB_PWM_VALID (1<<31)
>>> +
>>> +#define ASLE_CBLV_VALID         (1<<31)
>>> +
>>> +/* IUER */
>>> +#define ASLE_IUER_DOCKING		(1 << 7)
>>> +#define ASLE_IUER_CONVERTIBLE		(1 << 6)
>>> +#define ASLE_IUER_ROTATION_LOCK_BTN	(1 << 4)
>>> +#define ASLE_IUER_VOLUME_DOWN_BTN	(1 << 3)
>>> +#define ASLE_IUER_VOLUME_UP_BTN		(1 << 2)
>>> +#define ASLE_IUER_WINDOWS_BTN		(1 << 1)
>>> +#define ASLE_IUER_POWER_BTN		(1 << 0)
>>> +
>>> +/* Software System Control Interrupt (SWSCI) */
>>> +#define SWSCI_SCIC_INDICATOR		(1 << 0)
>>> +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT	1
>>> +#define SWSCI_SCIC_MAIN_FUNCTION_MASK	(0xf << 1)
>>> +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT	8
>>> +#define SWSCI_SCIC_SUB_FUNCTION_MASK	(0xff << 8)
>>> +#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT	8
>>> +#define SWSCI_SCIC_EXIT_PARAMETER_MASK	(0xff << 8)
>>> +#define SWSCI_SCIC_EXIT_STATUS_SHIFT	5
>>> +#define SWSCI_SCIC_EXIT_STATUS_MASK	(7 << 5)
>>> +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS	1
>>> +
>>> +#define SWSCI_FUNCTION_CODE(main, sub) \
>>> +	((main) << SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \
>>> +	 (sub) << SWSCI_SCIC_SUB_FUNCTION_SHIFT)
>>> +
>>> +/* SWSCI: Get BIOS Data (GBDA) */
>>> +#define SWSCI_GBDA			4
>>> +#define SWSCI_GBDA_SUPPORTED_CALLS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0)
>>> +#define SWSCI_GBDA_REQUESTED_CALLBACKS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1)
>>> +#define SWSCI_GBDA_BOOT_DISPLAY_PREF
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4)
>>> +#define SWSCI_GBDA_PANEL_DETAILS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5)
>>> +#define SWSCI_GBDA_TV_STANDARD
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6)
>>> +#define SWSCI_GBDA_INTERNAL_GRAPHICS
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7)
>>> +#define SWSCI_GBDA_SPREAD_SPECTRUM
>>	SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10)
>>> +
>>> +/* SWSCI: System BIOS Callbacks (SBCB) */
>>> +#define SWSCI_SBCB			6
>>> +#define SWSCI_SBCB_SUPPORTED_CALLBACKS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0)
>>> +#define SWSCI_SBCB_INIT_COMPLETION
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1)
>>> +#define SWSCI_SBCB_PRE_HIRES_SET_MODE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3)
>>> +#define SWSCI_SBCB_POST_HIRES_SET_MODE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4)
>>> +#define SWSCI_SBCB_DISPLAY_SWITCH
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5)
>>> +#define SWSCI_SBCB_SET_TV_FORMAT
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6)
>>> +#define SWSCI_SBCB_ADAPTER_POWER_STATE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7)
>>> +#define SWSCI_SBCB_DISPLAY_POWER_STATE
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8)
>>> +#define SWSCI_SBCB_SET_BOOT_DISPLAY
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 9)
>>> +#define SWSCI_SBCB_SET_PANEL_DETAILS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10)
>>> +#define SWSCI_SBCB_SET_INTERNAL_GFX
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 11)
>>> +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16)
>>> +#define SWSCI_SBCB_SUSPEND_RESUME
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17)
>>> +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
>>> +#define SWSCI_SBCB_POST_VBE_PM
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
>>> +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO
>>	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
>>> +
>>> +#define ACPI_OTHER_OUTPUT (0<<8)
>>> +#define ACPI_VGA_OUTPUT (1<<8)
>>> +#define ACPI_TV_OUTPUT (2<<8)
>>> +#define ACPI_DIGITAL_OUTPUT (3<<8)
>>> +#define ACPI_LVDS_OUTPUT (4<<8)
>>> +
>>> +#define MAX_DSLP	1500
>>
>>The bottom line here is that I think you could get all of this done with
>>*much* smaller changes. If we get a regression report pointing at this patch,
>>we'll have a lot of debugging to do to figure out what failed.
>>
> [Deepak M] Okay will try to break this patch into smaller one`s.
>>BR,
>>Jani.
>>
>>
>>
>>> --
>>> 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

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux