| Message ID | 53045DD1.5010406@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Feb 19, 2014 at 03:31:29PM +0800, Aaron Lu wrote: > DID2 is in system memory region and has some assigned value like 0x400 > when we read it. For this case it is easy since there is only one output > device that is of type LVDS so we can match it to connector of type eDP > or LVDS, suppose there is only one such connector. But for output > devices' whose _ADR has the value of 0x301, 0x302, etc. I have no idea > how to match them up to the connectors of that type as we can't be sure > the probe order we have used in i915 driver is the same as BIOS'. Non-standard _ADR values are assigend by the GPU vendor, so Intel should be able to provide you with the correct interpretations.
On 02/19/2014 03:33 PM, Matthew Garrett wrote: > On Wed, Feb 19, 2014 at 03:31:29PM +0800, Aaron Lu wrote: > >> DID2 is in system memory region and has some assigned value like 0x400 >> when we read it. For this case it is easy since there is only one output >> device that is of type LVDS so we can match it to connector of type eDP >> or LVDS, suppose there is only one such connector. But for output >> devices' whose _ADR has the value of 0x301, 0x302, etc. I have no idea >> how to match them up to the connectors of that type as we can't be sure >> the probe order we have used in i915 driver is the same as BIOS'. > > Non-standard _ADR values are assigend by the GPU vendor, so Intel should > be able to provide you with the correct interpretations. It doesn't seem the _ADR value has to be the format defined by _DOD, as the example of the ACPI spec gives: Method (_ADR, 0) { return(0x0100) } So that is not the problem here. The problem is, we don't have any way of matching an ACPI output device node to a drm connector of the same type when there are more than 1 of those with the same type, i.e. we don't know how the index value are assigned by BIOS. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 19, 2014 at 04:59:06PM +0800, Aaron Lu wrote: > On 02/19/2014 03:33 PM, Matthew Garrett wrote: > > On Wed, Feb 19, 2014 at 03:31:29PM +0800, Aaron Lu wrote: > > > >> DID2 is in system memory region and has some assigned value like 0x400 > >> when we read it. For this case it is easy since there is only one output > >> device that is of type LVDS so we can match it to connector of type eDP > >> or LVDS, suppose there is only one such connector. But for output > >> devices' whose _ADR has the value of 0x301, 0x302, etc. I have no idea > >> how to match them up to the connectors of that type as we can't be sure > >> the probe order we have used in i915 driver is the same as BIOS'. > > > > Non-standard _ADR values are assigend by the GPU vendor, so Intel should > > be able to provide you with the correct interpretations. > > It doesn't seem the _ADR value has to be the format defined by _DOD, as > the example of the ACPI spec gives: > Method (_ADR, 0) { > return(0x0100) > } > So that is not the problem here. > > The problem is, we don't have any way of matching an ACPI output device > node to a drm connector of the same type when there are more than 1 of > those with the same type, i.e. we don't know how the index value are > assigned by BIOS. I've thought the OpRegion spec has some additional fields in there indicating the port number, which we could match up at least on modern platforms (where there's only ever port A-E). But that's very hazy recollection from a really old OpRegion spec, i.e. I have no bloody clue at all ;-) If I misremember this then we need to start on a begging tour again and ask the windows guys how this is all supposed to work ... -Daniel
We have a new bug report that has the same problem: https://bugzilla.kernel.org/show_bug.cgi?id=88941 The posted patch solves the problem. I know it's not perfect, but it doesn't seem it would do any harm to existing systems so should be safe. Better, if someone can shed some light on how this should be properly handled, that would be great. Thanks, Aaron On 03/04/2014 10:45 PM, Daniel Vetter wrote: > On Wed, Feb 19, 2014 at 04:59:06PM +0800, Aaron Lu wrote: >> On 02/19/2014 03:33 PM, Matthew Garrett wrote: >>> On Wed, Feb 19, 2014 at 03:31:29PM +0800, Aaron Lu wrote: >>> >>>> DID2 is in system memory region and has some assigned value like 0x400 >>>> when we read it. For this case it is easy since there is only one output >>>> device that is of type LVDS so we can match it to connector of type eDP >>>> or LVDS, suppose there is only one such connector. But for output >>>> devices' whose _ADR has the value of 0x301, 0x302, etc. I have no idea >>>> how to match them up to the connectors of that type as we can't be sure >>>> the probe order we have used in i915 driver is the same as BIOS'. >>> >>> Non-standard _ADR values are assigend by the GPU vendor, so Intel should >>> be able to provide you with the correct interpretations. >> >> It doesn't seem the _ADR value has to be the format defined by _DOD, as >> the example of the ACPI spec gives: >> Method (_ADR, 0) { >> return(0x0100) >> } >> So that is not the problem here. >> >> The problem is, we don't have any way of matching an ACPI output device >> node to a drm connector of the same type when there are more than 1 of >> those with the same type, i.e. we don't know how the index value are >> assigned by BIOS. > > I've thought the OpRegion spec has some additional fields in there > indicating the port number, which we could match up at least on modern > platforms (where there's only ever port A-E). But that's very hazy > recollection from a really old OpRegion spec, i.e. I have no bloody clue > at all ;-) > > If I misremember this then we need to start on a begging tour again and > ask the windows guys how this is all supposed to work ... > -Daniel > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 08 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote: > We have a new bug report that has the same problem: > https://bugzilla.kernel.org/show_bug.cgi?id=88941 > > The posted patch solves the problem. I know it's not perfect, but it > doesn't seem it would do any harm to existing systems so should be safe. > > Better, if someone can shed some light on how this should be properly > handled, that would be great. There was a bug report that I can't find right now that had a similar problem. I wrote a few patches, even somewhat polished ones (that I now pushed to [1] for reference) to handle extended DIDL. Unfortunately this didn't help the bug reporter because the right one was beyond the extended DIDL too, so I don't think I even sent these to the list. Anyway, just one more data point. This might help your reporter, so worth a try. But it doesn't solve everything. BR, Jani. > > Thanks, > Aaron > > On 03/04/2014 10:45 PM, Daniel Vetter wrote: >> On Wed, Feb 19, 2014 at 04:59:06PM +0800, Aaron Lu wrote: >>> On 02/19/2014 03:33 PM, Matthew Garrett wrote: >>>> On Wed, Feb 19, 2014 at 03:31:29PM +0800, Aaron Lu wrote: >>>> >>>>> DID2 is in system memory region and has some assigned value like 0x400 >>>>> when we read it. For this case it is easy since there is only one output >>>>> device that is of type LVDS so we can match it to connector of type eDP >>>>> or LVDS, suppose there is only one such connector. But for output >>>>> devices' whose _ADR has the value of 0x301, 0x302, etc. I have no idea >>>>> how to match them up to the connectors of that type as we can't be sure >>>>> the probe order we have used in i915 driver is the same as BIOS'. >>>> >>>> Non-standard _ADR values are assigend by the GPU vendor, so Intel should >>>> be able to provide you with the correct interpretations. >>> >>> It doesn't seem the _ADR value has to be the format defined by _DOD, as >>> the example of the ACPI spec gives: >>> Method (_ADR, 0) { >>> return(0x0100) >>> } >>> So that is not the problem here. >>> >>> The problem is, we don't have any way of matching an ACPI output device >>> node to a drm connector of the same type when there are more than 1 of >>> those with the same type, i.e. we don't know how the index value are >>> assigned by BIOS. >> >> I've thought the OpRegion spec has some additional fields in there >> indicating the port number, which we could match up at least on modern >> platforms (where there's only ever port A-E). But that's very hazy >> recollection from a really old OpRegion spec, i.e. I have no bloody clue >> at all ;-) >> >> If I misremember this then we need to start on a begging tour again and >> ask the windows guys how this is all supposed to work ... >> -Daniel >> >
On Mon, 08 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 08 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote: >> We have a new bug report that has the same problem: >> https://bugzilla.kernel.org/show_bug.cgi?id=88941 >> >> The posted patch solves the problem. I know it's not perfect, but it >> doesn't seem it would do any harm to existing systems so should be safe. >> >> Better, if someone can shed some light on how this should be properly >> handled, that would be great. > > There was a bug report that I can't find right now that had a similar > problem. I wrote a few patches, even somewhat polished ones (that I now > pushed to [1] for reference) to handle extended DIDL. Unfortunately this > didn't help the bug reporter because the right one was beyond the > extended DIDL too, so I don't think I even sent these to the list. > > Anyway, just one more data point. This might help your reporter, so > worth a try. But it doesn't solve everything. [1] http://cgit.freedesktop.org/~jani/drm/log/?h=didl
On 12/08/2014 07:04 PM, Jani Nikula wrote: > On Mon, 08 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Mon, 08 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote: >>> We have a new bug report that has the same problem: >>> https://bugzilla.kernel.org/show_bug.cgi?id=88941 >>> >>> The posted patch solves the problem. I know it's not perfect, but it >>> doesn't seem it would do any harm to existing systems so should be safe. >>> >>> Better, if someone can shed some light on how this should be properly >>> handled, that would be great. >> >> There was a bug report that I can't find right now that had a similar >> problem. I wrote a few patches, even somewhat polished ones (that I now >> pushed to [1] for reference) to handle extended DIDL. Unfortunately this >> didn't help the bug reporter because the right one was beyond the >> extended DIDL too, so I don't think I even sent these to the list. >> >> Anyway, just one more data point. This might help your reporter, so >> worth a try. But it doesn't solve everything. > > [1] http://cgit.freedesktop.org/~jani/drm/log/?h=didl Thanks for the info, I've asked Dmitry to give it a try. Regards, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a4ffc021c317..55956a517a77 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -204,6 +204,9 @@ struct intel_connector { /* since POLL and HPD connectors may use the same HPD line keep the native state of connector->polled in case hotplug storm detection changes it */ u8 polled; + + /* device id with type and index information */ + u32 disp_id; }; typedef struct dpll { diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 68459605bd12..ba08c894ce9a 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -221,11 +221,11 @@ struct opregion_asle { #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 ACPI_OTHER_OUTPUT 0 +#define ACPI_VGA_OUTPUT 1 +#define ACPI_TV_OUTPUT 2 +#define ACPI_DIGITAL_OUTPUT 3 +#define ACPI_LVDS_OUTPUT 4 #define MAX_DSLP 1500 @@ -600,78 +600,20 @@ static struct notifier_block intel_opregion_notifier = { .notifier_call = intel_opregion_video_event, }; -/* - * Initialise the DIDL field in opregion. This passes a list of devices to - * the firmware. Values are defined by section B.4.2 of the ACPI specification - * (version 3) - */ - -static void intel_didl_outputs(struct drm_device *dev) +static void intel_connector_getid(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_opregion *opregion = &dev_priv->opregion; - struct drm_connector *connector; - acpi_handle handle; - struct acpi_device *acpi_dev, *acpi_cdev, *acpi_video_bus = NULL; - unsigned long long device_id; - acpi_status status; - u32 temp; int i = 0; + int index[5] = {0}; + struct intel_connector *connector; - handle = ACPI_HANDLE(&dev->pdev->dev); - if (!handle || acpi_bus_get_device(handle, &acpi_dev)) - return; - - if (acpi_is_video_device(handle)) - acpi_video_bus = acpi_dev; - else { - list_for_each_entry(acpi_cdev, &acpi_dev->children, node) { - if (acpi_is_video_device(acpi_cdev->handle)) { - acpi_video_bus = acpi_cdev; - break; - } - } - } - - if (!acpi_video_bus) { - pr_warn("No ACPI video bus found\n"); - return; - } - - list_for_each_entry(acpi_cdev, &acpi_video_bus->children, node) { - if (i >= 8) { - dev_dbg(&dev->pdev->dev, - "More than 8 outputs detected via ACPI\n"); - return; - } - status = - acpi_evaluate_integer(acpi_cdev->handle, "_ADR", - NULL, &device_id); - if (ACPI_SUCCESS(status)) { - if (!device_id) - goto blind_set; - iowrite32((u32)(device_id & 0x0f0f), - &opregion->acpi->didl[i]); - i++; - } - } - -end: - /* If fewer than 8 outputs, the list must be null terminated */ - if (i < 8) - iowrite32(0, &opregion->acpi->didl[i]); - return; - -blind_set: - i = 0; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + list_for_each_entry(connector, &dev->mode_config.connector_list, base.head) { int output_type = ACPI_OTHER_OUTPUT; if (i >= 8) { dev_dbg(&dev->pdev->dev, "More than 8 outputs in connector list\n"); return; } - switch (connector->connector_type) { + switch (connector->base.connector_type) { case DRM_MODE_CONNECTOR_VGA: case DRM_MODE_CONNECTOR_DVIA: output_type = ACPI_VGA_OUTPUT; @@ -690,15 +632,35 @@ blind_set: output_type = ACPI_DIGITAL_OUTPUT; break; case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: output_type = ACPI_LVDS_OUTPUT; break; } - temp = ioread32(&opregion->acpi->didl[i]); - iowrite32(temp | (1<<31) | output_type | i, - &opregion->acpi->didl[i]); + connector->disp_id = (output_type << 8) | index[output_type]++; i++; } - goto end; +} + +/* + * Initialise the DIDL field in opregion. This passes a list of devices to + * the firmware. Values are defined by section B.4.2 of the ACPI specification + * (version 3) + */ + +static void intel_didl_outputs(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_opregion *opregion = &dev_priv->opregion; + struct intel_connector *connector; + int i = 0; + + list_for_each_entry(connector, &dev->mode_config.connector_list, base.head) + iowrite32(connector->disp_id, &opregion->acpi->didl[i++]); + + if (i < 8) + iowrite32(0, &opregion->acpi->didl[i]); + + return; } static void intel_setup_cadls(struct drm_device *dev) @@ -730,6 +692,7 @@ void intel_opregion_init(struct drm_device *dev) if (opregion->acpi) { if (drm_core_check_feature(dev, DRIVER_MODESET)) { + intel_connector_getid(dev); intel_didl_outputs(dev); intel_setup_cadls(dev); }