Re: [PATCH] ACPI/Intel: Rework Opregion support
On Wed, Mar 16, 2011 at 2:26 AM, Indan Zupancic in...@nul.nu wrote: On Wed, March 16, 2011 03:17, Alex Deucher wrote: It's not HDCP, encrypted bluray is the main issue. And while there are hacks for bluray around already, contractual obligations don't care whether existing hacks are available or not. So the contract says to keep it secret, not to make it secure. Wonderful. Without going into detail, they are very intertwined. The hw was designed long before we planned to support open source as much as we are now. Fortunately, we have been working with our hw teams to make future UVD implementations take the open source driver into account. It has nothing to do with open source, if you need to trust the driver you're already screwed from a security point of view. It would be nice to have an open source Fusion based media player/ IPTV decoder, but I guess that's hoping for too much. I understand if AMD/ATI wants to keep its 3D driver secret, but hardware video decoding?! If you have to keep it secret it means shortcuts were taken and it's all insecure anyway. That if it gets broken the Open source driver gets blamed is ridiculous and more politics than anything else. We don't keep the 3D engine secret. Most of the code we've written and specs we've released are 3D engine stuff. Fortunately 3D is less tied up in drm compared to video. That's not what I said, I was talking about the driver. There are always details not documented, and plenty of optimisation tricks that make the hardware go fast. Just compare the performance of the Windows driver with the open source Linux driver. It doesn't give the impression that AMD is putting much effort in the open source drivers or that it takes it seriously. People are over thinking and believe secret recipes is what actually make driver fast. From my experience i would say that it's just the open source driver stack that is limiting performances, mostly because it require a huge amount of work. I believe there is several hundred of engineers working on the closed driver just to optimize it and improve it while the open source stacks have couple dozen and not all working on same hw but working on AMD, Intel, NVidia. You can check that with some of the test that were in r600 demo, iirc correctly with direct hw access perf were close to theoretical hw performance. Cheers, Jerome ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, 15 Mar 2011 02:18:02 +0100 (CET), Indan Zupancic in...@nul.nu wrote: Hello, Some nitpicks below. On Mon, March 14, 2011 18:59, Chris Wilson wrote: Note: neither the opregion_dev interface or the alse_set_* properly report failures. As such we have a slight change in behaviour on Ironlake+ platforms and an uncorrected bug for earlier chipsets. -Chris What uncorrected bug? For later chipsets we currently report the failure to respond to the ALSE requests, for earlier we do not. The patch harmonises the two code paths reusing the earlier code for later chipsets, hence the change in behaviour and potential regression. Alternatively, actually reporting the failure for earlier chipsets may also break existing setups. And are there earlier chipsets with ASLE support at all, besides gen 4? If there are no gen 2 or gen 3 chipsets with ASLE then the backlight code can be simplified further. The OpRegion interface was devised midway through gen3 (afaik), and you find it on some i915-class hw and not others. In theory, there is nothing to prevent a BIOS/ACPI from being rewritten for it to be of use in gen2, and who knows one such beast may already exist (considering much to our horror you can still buy gen2 chipsets). diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 4e5ff59..51565bb 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -261,8 +261,8 @@ intel_panel_detect(struct drm_device *dev) * appears to be either the BIOS or Linux ACPI fault */ #if 0 /* Assume that the BIOS does not lie through the OpRegion... */ - if (dev_priv-opregion.lid_state) - return ioread32(dev_priv-opregion.lid_state) 0x1 ? + if (dev_priv-opregion_dev.opregion.acpi) + return ioread32(dev_priv-opregion_dev.opregion.acpi-clid) 0x1 ? What guarantees that opregion.acpi != NULL here? You mean other than the explicit test for opregion.acpi != NULL? Or perhaps just remove that #if 0 code chunk altogether? Read the changelog and thread on the patch that disabled this logic, the failure (or at least inconsistent behaviour with the expectations of the HP BIOS authors) appears to be in how we initialise ACPI on the HP machines that causes the initial value of lid state to be incorrect. Since one of the laptops that Dave tests drm-next on is a HP, he was bitten by the bug and temporarily (we hope) disabled the logic. Or else once again, we will continue to light up the panel on a closed laptop. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, March 15, 2011 09:37, Chris Wilson wrote: On Tue, 15 Mar 2011 02:18:02 +0100 (CET), Indan Zupancic in...@nul.nu wrote: Hello, Some nitpicks below. On Mon, March 14, 2011 18:59, Chris Wilson wrote: Note: neither the opregion_dev interface or the alse_set_* properly report failures. As such we have a slight change in behaviour on Ironlake+ platforms and an uncorrected bug for earlier chipsets. -Chris What uncorrected bug? For later chipsets we currently report the failure to respond to the ALSE requests, for earlier we do not. The patch harmonises the two code paths reusing the earlier code for later chipsets, hence the change in behaviour and potential regression. Alternatively, actually reporting the failure for earlier chipsets may also break existing setups. Ah, okay, for the ASLE_SET_ALS_ILLUM/ASLE_SET_PFIT/ASLE_SET_PWM_FREQ cases. Hopefully this change doesn't cause regressions, that would be sad. And are there earlier chipsets with ASLE support at all, besides gen 4? If there are no gen 2 or gen 3 chipsets with ASLE then the backlight code can be simplified further. The OpRegion interface was devised midway through gen3 (afaik), and you find it on some i915-class hw and not others. In theory, there is nothing to prevent a BIOS/ACPI from being rewritten for it to be of use in gen2, and who knows one such beast may already exist (considering much to our horror you can still buy gen2 chipsets). Pity, if they're still sold any bets are off. diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 4e5ff59..51565bb 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -261,8 +261,8 @@ intel_panel_detect(struct drm_device *dev) * appears to be either the BIOS or Linux ACPI fault */ #if 0 /* Assume that the BIOS does not lie through the OpRegion... */ - if (dev_priv-opregion.lid_state) - return ioread32(dev_priv-opregion.lid_state) 0x1 ? + if (dev_priv-opregion_dev.opregion.acpi) + return ioread32(dev_priv-opregion_dev.opregion.acpi-clid) 0x1 ? What guarantees that opregion.acpi != NULL here? You mean other than the explicit test for opregion.acpi != NULL? I'm blind. I checked all the rest of the code, but not the line just above it. Gah! Or perhaps just remove that #if 0 code chunk altogether? Read the changelog and thread on the patch that disabled this logic, the I just subscribed to intel-gfx, seemed like a good idea after the reject. failure (or at least inconsistent behaviour with the expectations of the HP BIOS authors) appears to be in how we initialise ACPI on the HP machines that causes the initial value of lid state to be incorrect. Since one of the laptops that Dave tests drm-next on is a HP, he was bitten by the bug and temporarily (we hope) disabled the logic. Or else once again, we will continue to light up the panel on a closed laptop. Hopefully it's fixed by the next BIOS upgrade by HP... Everything would be a lot simpler if the BIOSes were open source. It's shocking with what you guys have to deal with. Good luck and thanks for all the hard work! Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
Indan Zupancic wrote: Everything would be a lot simpler if the BIOSes were open source. coreboot has existed for about eleven years and some 250 mainboards of varying shapes and sizes (from laptop to server) are supported, but it's only just recently that things are really taking off, with the code release from AMD to initialize their most recent Fusion platform. http://www.coreboot.org/Welcome_to_coreboot http://blogs.amd.com/work/2011/02/28/amd-coreboot/ http://news.slashdot.org/story/11/03/04/1736241/AMD-Provides-Fusion-Support-For-Coreboot //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, March 15, 2011 12:27, Peter Stuge wrote: coreboot has existed for about eleven years and some 250 mainboards of varying shapes and sizes (from laptop to server) are supported, but it's I've been wanting to get rid of BIOSes and use Coreboot for ages, but the amount of hassle needed to get it working for my hardware and the lack of features (suspend), as well the chance of bricking the system always put me off. only just recently that things are really taking off, with the code release from AMD to initialize their most recent Fusion platform. They don't give their Linux devs any Fusion hardware, nor do they open the UVD spec, but at least they release info like this. Greetings, Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, Mar 15, 2011 at 7:46 AM, Indan Zupancic in...@nul.nu wrote: On Tue, March 15, 2011 12:27, Peter Stuge wrote: coreboot has existed for about eleven years and some 250 mainboards of varying shapes and sizes (from laptop to server) are supported, but it's I've been wanting to get rid of BIOSes and use Coreboot for ages, but the amount of hassle needed to get it working for my hardware and the lack of features (suspend), as well the chance of bricking the system always put me off. only just recently that things are really taking off, with the code release from AMD to initialize their most recent Fusion platform. They don't give their Linux devs any Fusion hardware, nor do they open the UVD spec, but at least they release info like this. They do give us fusion hw; before launch even. That's why we had Linux support before hw was available publicly. My board just happened to get bricked recently during a failed bios upgrade. A new one is on the way. Also we are looking into a potential release of UVD, but unfortunately, our decode hw is intimately tied in with our drm implementation and if someone managed to use the released information to compromise the drm in our windows driver it would very negatively impact our ability to sell into the windows market and would probably get the open source graphics initiative shut down. Alex Greetings, Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, March 15, 2011 17:06, Alex Deucher wrote: On Tue, Mar 15, 2011 at 7:46 AM, Indan Zupancic in...@nul.nu wrote: They don't give their Linux devs any Fusion hardware, nor do they open the UVD spec, but at least they release info like this. They do give us fusion hw; before launch even. That's why we had Linux support before hw was available publicly. My board just happened to get bricked recently during a failed bios upgrade. A new one is on the way. Okay, that's better than I thought. I remember a dev saying that no one had Fusion hardware, that's where I got this notion from. Also we are looking into a potential release of UVD, but unfortunately, our decode hw is intimately tied in with our drm implementation and if someone managed to use the released information to compromise the drm in our windows driver it would very negatively impact our ability to sell into the windows market and would probably get the open source graphics initiative shut down. Are you talking about HDCP or something else? Because the HDCP master key already leaked, so the whole security aspect of it is already a joke, open source UVD won't make any difference. Basically you're telling me that I or someone else should reverse engineer de Catalyst driver and break all drm before you consider opening up UVD? I'd argue that opening up UVD now is more secure because it takes away the only morivation to break UVD's drm. Alternatively, can't you open up UVD spec except the drm bits, so people can at least write their own UVD driver to watch unencrypted data? It would be nice to have an open source Fusion based media player/ IPTV decoder, but I guess that's hoping for too much. I understand if AMD/ATI wants to keep its 3D driver secret, but hardware video decoding?! If you have to keep it secret it means shortcuts were taken and it's all insecure anyway. That if it gets broken the Open source driver gets blamed is ridiculous and more politics than anything else. This is getting more and more off-topic though, sorry for the noise. Greetings, Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, Mar 15, 2011 at 8:02 PM, Indan Zupancic in...@nul.nu wrote: On Tue, March 15, 2011 17:06, Alex Deucher wrote: On Tue, Mar 15, 2011 at 7:46 AM, Indan Zupancic in...@nul.nu wrote: They don't give their Linux devs any Fusion hardware, nor do they open the UVD spec, but at least they release info like this. They do give us fusion hw; before launch even. That's why we had Linux support before hw was available publicly. My board just happened to get bricked recently during a failed bios upgrade. A new one is on the way. Okay, that's better than I thought. I remember a dev saying that no one had Fusion hardware, that's where I got this notion from. Also we are looking into a potential release of UVD, but unfortunately, our decode hw is intimately tied in with our drm implementation and if someone managed to use the released information to compromise the drm in our windows driver it would very negatively impact our ability to sell into the windows market and would probably get the open source graphics initiative shut down. Are you talking about HDCP or something else? Because the HDCP master key already leaked, so the whole security aspect of it is already a joke, open source UVD won't make any difference. It's not HDCP, encrypted bluray is the main issue. And while there are hacks for bluray around already, contractual obligations don't care whether existing hacks are available or not. Basically you're telling me that I or someone else should reverse engineer de Catalyst driver and break all drm before you consider opening up UVD? I'd argue that opening up UVD now is more secure because it takes away the only morivation to break UVD's drm. Alternatively, can't you open up UVD spec except the drm bits, so people can at least write their own UVD driver to watch unencrypted data? Without going into detail, they are very intertwined. The hw was designed long before we planned to support open source as much as we are now. Fortunately, we have been working with our hw teams to make future UVD implementations take the open source driver into account. It would be nice to have an open source Fusion based media player/ IPTV decoder, but I guess that's hoping for too much. I understand if AMD/ATI wants to keep its 3D driver secret, but hardware video decoding?! If you have to keep it secret it means shortcuts were taken and it's all insecure anyway. That if it gets broken the Open source driver gets blamed is ridiculous and more politics than anything else. We don't keep the 3D engine secret. Most of the code we've written and specs we've released are 3D engine stuff. Fortunately 3D is less tied up in drm compared to video. It's all about mitigating risk. Imagine you run a company and someone asks you this: Can we release programming info for a part of our chip to support 2-3% of our market that may put at risk 90% of our market? What would you do? The reason the review is taking so long is that we want to be real sure that the risk is safe. This is getting more and more off-topic though, sorry for the noise. Agreed. Alex Greetings, Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
Read the changelog and thread on the patch that disabled this logic, the failure (or at least inconsistent behaviour with the expectations of the HP BIOS authors) appears to be in how we initialise ACPI on the HP machines that causes the initial value of lid state to be incorrect. Since one of the laptops that Dave tests drm-next on is a HP, he was bitten by the bug and temporarily (we hope) disabled the logic. Or else once again, we will continue to light up the panel on a closed laptop. Yeah I've no idea if the ACPI implementation is wrong or not :-( I suspects its the BIOS actually, the BIOS wants _INI called before _REG. The ACPI video device isn't in the EC address space, so it ends up in the default region address space. So we'd really need to know how Windows sets up the _REG calls for the non-EC spaces and where its calls the _INI methods wrt to that. The thing is I think the actual ACPI lid device is fine, its just opregion isn't updated until we get an event later. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] ACPI/Intel: Rework Opregion support
Hello, Some nitpicks below. On Mon, March 14, 2011 18:59, Chris Wilson wrote: From: Matthew Garrett m...@redhat.com The Integrated Graphics Device opregion specification defines a mechanism for the OS and system firmware to collaborate on various graphics-related functionality. This is currently implemented in the i915 driver but isn't strictly limited to these devices. Move it to a more generic location and remove the i915 dependency, while simultaneously reworking i915 to make use of the new driver. Signed-off-by: Matthew Garrett m...@redhat.com Acked-by: Acked-by: Jesse Barnes jbar...@virtuousgeek.org Cc: Alan Cox a...@linux.intel.com Cc: Keith Packard kei...@keithp.com Cc: Len Brown l...@kernel.org [ickle: rebase onto drm-core-next] Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- Note: neither the opregion_dev interface or the alse_set_* properly report failures. As such we have a slight change in behaviour on Ironlake+ platforms and an uncorrected bug for earlier chipsets. -Chris What uncorrected bug? And are there earlier chipsets with ASLE support at all, besides gen 4? If there are no gen 2 or gen 3 chipsets with ASLE then the backlight code can be simplified further. --- drivers/acpi/Kconfig |8 + drivers/acpi/Makefile |1 + drivers/acpi/acpi_igd_opregion.c | 411 ++ drivers/gpu/drm/Kconfig |1 + drivers/gpu/drm/i915/Makefile |1 - drivers/gpu/drm/i915/i915_debugfs.c |2 +- drivers/gpu/drm/i915/i915_dma.c | 13 +- drivers/gpu/drm/i915/i915_drv.c |6 +- drivers/gpu/drm/i915/i915_drv.h | 34 +-- drivers/gpu/drm/i915/i915_irq.c |7 +- drivers/gpu/drm/i915/intel_bios.c |8 +- drivers/gpu/drm/i915/intel_lvds.c |2 +- drivers/gpu/drm/i915/intel_opregion.c | 516 - drivers/gpu/drm/i915/intel_panel.c|4 +- include/acpi/acpi_igd_opregion.h | 105 +++ 15 files changed, 552 insertions(+), 567 deletions(-) create mode 100644 drivers/acpi/acpi_igd_opregion.c delete mode 100644 drivers/gpu/drm/i915/intel_opregion.c create mode 100644 include/acpi/acpi_igd_opregion.h diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 2aa042a..7ace174 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -383,4 +383,12 @@ config ACPI_HED source drivers/acpi/apei/Kconfig +config ACPI_IGD_OPREGION + tristate ACPI Integrated Graphics Device OpRegion support + help + This driver adds support for the Intel ACPI Integrated Graphics + Device OpRegion specification, allowing communication between + the firmware and graphics driver on mobile systems with Intel + graphics + Isn't this option always selected automatically when needed? That is, it shouldn't show up as an option at all, so people can't accidentally select it when they don't need it? Though that doesn't quite work with modules I suppose, never mind. endif# ACPI diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index d113fa5..dcade4b 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -62,6 +62,7 @@ obj-$(CONFIG_ACPI_SBS) += sbs.o obj-$(CONFIG_ACPI_POWER_METER) += power_meter.o obj-$(CONFIG_ACPI_HED) += hed.o obj-$(CONFIG_ACPI_EC_DEBUGFS)+= ec_sys.o +obj-$(CONFIG_ACPI_IGD_OPREGION) += acpi_igd_opregion.o # processor has its own processor. module_param namespace processor-y := processor_driver.o processor_throttling.o diff --git a/drivers/acpi/acpi_igd_opregion.c b/drivers/acpi/acpi_igd_opregion.c new file mode 100644 index 000..0015ca8 --- /dev/null +++ b/drivers/acpi/acpi_igd_opregion.c @@ -0,0 +1,411 @@ +/* + * Copyright 2008 Intel Corporation hong@intel.com + * Copyright 2008 Red Hat m...@redhat.com + * + * 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,
Re: [PATCH] ACPI/Intel: Rework Opregion support
On Tue, Mar 15, 2011 at 02:18:02AM +0100, Indan Zupancic wrote:
+
+ if (dev-set_backlight)
+ dev-set_backlight(dev-drm_dev, bclp * max / 255);
I would hide the max backlight from the opregion code and move this
calculation into set_brightness. Then change the interface to one
based on a scale from 0-255 or 0-100, which fits better with what
opregion actually does. On the other hand, it's the same code in
a different place, so it doesn't make much difference.
That would require an extra layer of indirection in every driver, so
keeping it here seems reasonable.
+void igd_opregion_enable_asle(struct opregion_dev *dev)
+{
+ struct opregion_asle *asle = dev-opregion.asle;
+
+ if (asle dev-enable_asle) {
+ dev-enable_asle(dev-drm_dev);
+
+ asle-tche = ASLE_ALS_EN | ASLE_BLC_EN | ASLE_PFIT_EN |
+ ASLE_PFMB_EN;
Shouldn't which flags are set depend on which callback functions are set?
Then you can remove all the function != NULL tests too. e.g:
Our experience is that we should give BIOSes the full set of support
flags even if we don't actually do anything with them. That way we avoid
cases where BIOS authors refuse to do anything with partial
implementations (even if we support everything they use) and we get
feedback when we actually find cases of advanced functionality in the
real world.
+ if (IS_MOBILE(dev)) {
+ dev_priv-opregion_dev.max_backlight =
intel_panel_get_max_backlight(dev);
+ dev_priv-opregion_dev.set_backlight =
intel_panel_set_backlight;
+ }
+ dev_priv-opregion_dev.enable_asle = intel_enable_asle;
+ dev_priv-opregion_dev.drm_dev = dev;
So we have a drm_device-drm_i915_private-opregion_dev.drm_device loop,
which is a bit messy, but consistent with other existing code.
Anyone who wants to clean that up is welcome to do so, but really I
optimised for ease of transition here.
- /* XXX Should this validation be moved to intel_opregion.c? */
- if (dev_priv-opregion.vbt) {
- struct vbt_header *vbt = dev_priv-opregion.vbt;
+ /* XXX Should this validation be moved to acpi_igd_opregion.c? */
Should it? Seems like a good moment to do so.
Now that we've got multiple consumers it's probably not helpful to move
the (potentially chip-specific) VBT handling to general code. We've got
zero documentation on how GMA500 handles VBT, and not a great deal more
for i915.
About the naming, as this is Intel-only intel_opregion seems clearer than
igd-opregion, which sounds like it could be anything.
It's Intel-only in implementation, not in specification. As much as I
dislike vendor-neutral specs that have been pushed and implemented by a
single vendor, I'd rather not make the naming Intel-specific if there's
a chance someone else can end up depending on it.
--
Matthew Garrett | mj...@srcf.ucam.org
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
