Re: radeon testing

[Luca Tettamanti]

On Mon, Aug 20, 2012 at 10:24:01AM -0400, Alex Deucher wrote:
> > I just tested the rebased acpi_patches branch: ACPI part works fine, but
> > I see a big slow down during radeon driver initialization when the
> > screen goes black for a couple of seconds (with 3.5.0 + acpi patches the
> > screen just flickers during boot). With initcall_debug I see:
> >
> > [2.355300] calling  radeon_init+0x0/0x1000 [radeon] @ 552
> > ...
> > [5.530310] initcall radeon_init+0x0/0x1000 [radeon] returned 0 after 
> > 3102147 usecs
> >
> > For reference 3.5 takes ~1 sec. With drm.debug=2 the log (attached) is not 
> > very
> > informative, I'll try and get more info...
> 
> Can you bisect?

I've put in some printk, this is the result:

[2.403138] evergreen_mc_program
[2.403196] evergreen_mc_stop
...
[4.268491] evergreen_mc_resume done
[4.268548] evergreen_mc_program done

This is the patch:

--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1349,6 +1349,8 @@ void evergreen_mc_program(struct radeon_device *rdev)
u32 tmp;
int i, j;
 
+   printk(KERN_INFO "%s\n", __func__);
+
/* Initialize HDP */
for (i = 0, j = 0; i < 32; i++, j += 0x18) {
WREG32((0x2c14 + j), 0x);
@@ -1359,10 +1361,14 @@ void evergreen_mc_program(struct radeon_device *rdev)
}
WREG32(HDP_REG_COHERENCY_FLUSH_CNTL, 0);
 
+   printk(KERN_INFO "evergreen_mc_stop\n");
evergreen_mc_stop(rdev, &save);
+// printk(KERN_INFO "evergreen_mc_wait_for_idle\n");
if (evergreen_mc_wait_for_idle(rdev)) {
dev_warn(rdev->dev, "Wait for MC idle timedout !\n");
}
+// printk(KERN_INFO "evergreen_mc_wait_for_idle done\n");
+
/* Lockout access through VGA aperture*/
WREG32(VGA_HDP_CONTROL, VGA_MEMORY_DISABLE);
/* Update configuration */
@@ -1411,10 +1417,13 @@ void evergreen_mc_program(struct radeon_device *rdev)
WREG32(MC_VM_AGP_TOP, 0x0FFF);
WREG32(MC_VM_AGP_BOT, 0x0FFF);
}
+// printk(KERN_INFO "evergreen_mc_wait_for_idle\n");
if (evergreen_mc_wait_for_idle(rdev)) {
dev_warn(rdev->dev, "Wait for MC idle timedout !\n");
}
+// printk(KERN_INFO "evergreen_mc_resume\n");
evergreen_mc_resume(rdev, &save);
+   printk(KERN_INFO "evergreen_mc_resume done\n");
/* we need to own VRAM, so turn off the VGA renderer here
 * to stop it overwriting our objects */
rv515_vga_render_disable(rdev);


Any printk between evergreen_mc_stop and evergreen_mc_resume locks up
the machine. The likely culprit is commit 023e188e:

commit 023e188ec102330eb05ad264f675e869637478eb
Author: Alex Deucher 
Date:   Wed Aug 15 17:18:42 2012 -0400

drm/radeon: properly handle mc_stop/mc_resume on evergreen+

- Stop the displays from accessing the FB
- Block CPU access
- Turn off MC client access

This should fix issues some users have seen, especially
with UEFI, when changing the MC FB location that result
in hangs or display corruption.



I haven't tried backing out the commit yet, but looking at the diff I
see that you call radeon_wait_for_vblank and radeon_get_vblank_counter,
but evergreen_mc_program is called way before IRQ is set up. Is the
vblank counter running? Looks like we just hitting the timeout here...




Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: radeon testing

[Luca Tettamanti]

On Tue, Aug 21, 2012 at 09:51:46AM -0400, Alex Deucher wrote:
> On Mon, Aug 20, 2012 at 3:30 PM, Luca Tettamanti  wrote:
> > Any printk between evergreen_mc_stop and evergreen_mc_resume locks up
> > the machine. The likely culprit is commit 023e188e:
> 
> yeah, vram is locked out at that point.  I guess we probably need to
> block anyone from trying to access it.

I see; the 2 dev_warn would probably lock up the machine as well right?

> > I haven't tried backing out the commit yet, but looking at the diff I
> > see that you call radeon_wait_for_vblank and radeon_get_vblank_counter,
> > but evergreen_mc_program is called way before IRQ is set up. Is the
> > vblank counter running? Looks like we just hitting the timeout here...
> 
> We aren't waiting for an interrupt, just polling the current crtc
> status until it enters the vblank region.  The status and counters
> should be working as we only wait on displays that are enabled.

It appears that all the crtcs are considered active:

[4.260766] crtc 0 enabled 272696081 (this is the value of crtc_enabled)
[4.260766] crtc 0 wait for vblank 0x1 (0x1 means no timeout)
[4.260766] crtc 0: waited 33 [10] (number of loops of 
radeon_get_vblank_counter)
[4.260766] crtc 1 enabled 272630544
[4.260766] crtc 1 wait for vblank 0x1
[4.260766] crtc 1: waited 10 [10]
[4.260766] crtc 2 enabled 4195088
[4.260766] crtc 2 wait for vblank 0x1
[4.260766] crtc 2: waited 10 [10]
[4.260766] crtc 3 enabled 4195088
[4.260766] crtc 3 wait for vblank 0x1
[4.260766] crtc 3: waited 10 [10]
[4.260766] crtc 4 enabled 4195088
[4.260766] crtc 4 wait for vblank 0x1
[4.260766] crtc 4: waited 10 [10]
[4.260766] crtc 5 enabled 4195088
[4.260766] crtc 5 wait for vblank 0x1
[4.260766] crtc 5: waited 10 [10]

Maybe the code should be checking EVERGREEN_CRTC_MASTER_EN?

I'm testing this patch and the boot is fast again:

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index 2308c7d..72bf721 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1251,7 +1251,8 @@ void evergreen_mc_stop(struct radeon_device *rdev, struct 
evergreen_mc_save *sav
WREG32(VGA_RENDER_CONTROL, 0);
/* blank the display controllers */
for (i = 0; i < rdev->num_crtc; i++) {
-   crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL + crtc_offsets[i]);
+   crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL + crtc_offsets[i]) 
&
+   EVERGREEN_CRTC_MASTER_EN;
if (crtc_enabled) {
save->crtc_enabled[i] = true;
if (ASIC_IS_DCE6(rdev)) {


Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] omap: add omapdrm support

[Luca Tettamanti]

Hi Rob,
I've a couple of (minor) comments:

On Wed, Mar 21, 2012 at 01:00:36PM -0500, Rob Clark wrote:
> --- /dev/null
> +++ b/omap/omap_drm.c
[...]
> +/* allocate a new (un-tiled) buffer object */
> +struct omap_bo * omap_bo_new(struct omap_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + if (flags & OMAP_BO_TILED) {
> + return NULL;
> + }
> + return omap_bo_new_impl(dev, (union omap_gem_size){
> + .bytes = size,
> + }, flags);

Hum, the indentation of the anonymous union looks weird (but maybe it's just
me...)

> +}
> +
> +/* allocate a new buffer object */
> +struct omap_bo * omap_bo_new_tiled(struct omap_device *dev,
> + uint32_t width, uint32_t height, uint32_t flags)
> +{
> + if (!(flags & OMAP_BO_TILED)) {
> + return NULL;
> + }
> + return omap_bo_new_impl(dev, (union omap_gem_size){
> + .tiled = {
> + .width = width,
> + .height = height,
> + },
> + }, flags);
> +}

Here too :-) What about this:

return omap_bo_new_impl(dev, (union omap_gem_size)
{
.stuff = blah,
});

Or just use a temp var?

> +
> +/* get buffer info */
> +static int get_buffer_info(struct omap_bo *bo)
> +{
> + struct drm_omap_gem_info req = {
> + .handle = bo->handle,
> + };
> + int ret = drmCommandWriteRead(bo->dev->fd, DRM_OMAP_GEM_INFO,
> + &req, sizeof(req));
> + if (ret) {
> + return ret;
> + }
> +
> + /* really all we need for now is mmap offset */
> + bo->offset = req.offset;
> + bo->size = req.size;
> +
> + return 0;
> +}
> +
> +/* import a buffer object from DRI2 name */
> +struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name)
> +{
> + struct omap_bo *bo = calloc(sizeof(*bo), 1);
> + struct drm_gem_open req = {
> + .name = name,
> + };
> +
> + if (drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req)) {
> + goto fail;
> + }

bo may be NULL here:

> +
> + bo->dev = dev;
> + bo->name = name;
> + bo->handle = req.handle;

I also woundn't use the calloc in the initialization block, I prefer to
keep the allocation and the check together:

bo = alloc_stuff();
if (!bo)
oh_crap();

I find it more easy to check visually.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 24/24] drm/radeon: add faulty command buffer dump facilities

[Luca Tettamanti]

Hi Jerome,

On Wed, Apr 25, 2012 at 9:03 PM,   wrote:
> From: Jerome Glisse 
>
> This add a command buffer dumping facilities, that will
> dump command buffer and all associated bo that most likely
> triggered a lockup.
[cut]

I spotted this:

> +void radeon_fence_blob_faulty_ib(struct radeon_device *rdev, int ring)
> +{
> +       struct radeon_fence *fence;
> +       struct list_head *i;
> +       unsigned long irq_flags;
> +       uint32_t seq;
> +
> +       write_lock_irqsave(&rdev->fence_lock, irq_flags);
> +       seq = radeon_fence_read(rdev, ring);
> +       list_for_each(i, &rdev->fence_drv[ring].emitted) {
> +               fence = list_entry(i, struct radeon_fence, list);
> +               if (fence->seq != seq && fence->ib) {
> +                       radeon_lockup_build_blob(rdev, fence->ib);

radeon_lockup_build_blob() will take a mutex and call vmalloc() inside
an atomic context.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Linux 3.4-rc4

[Luca Tettamanti]

On Mon, Apr 30, 2012 at 11:07 AM, Maarten Maathuis  wrote:
> On Mon, Apr 30, 2012 at 12:37 AM, Dmitry Torokhov
>  wrote:
>> On Sat, Apr 28, 2012 at 11:33:50AM -0400, Nick Bowler wrote:
>>> On 2012-04-28 02:19 -0400, Alex Deucher wrote:
>>> > On Fri, Apr 27, 2012 at 8:39 PM, Nick Bowler  
>>> > wrote:
>>> > > Hi Ben,
>>> > >
>>> > > On 2012-04-27 15:20 +1000, Ben Skeggs wrote:
>>> > >> Does this patch help you at all?
>>> > >>
>>> > >> http://cgit.freedesktop.org/nouveau/linux-2.6/commit/?id=a3a285f17867f0018de798b5ee85731ec1268305
>>> > >
>>> > > Yes.  I cherry-picked this patch on top of Linus' master (3.4-rc4+) and
>>> > > this appears to solve the "black screen on VGA" problem described in the
>>> > > original report.  Thanks!
>>> > >
>>> > > Unfortunately, that's not the end of my VGA-related regressions. :(
>>> > >
>>> > > While tracking down the black screen issue, I've been having the monitor
>>> > > directly connected to the video card the whole time, but now when I'm
>>> > > connected through my KVM switch (an IOGear GCS1804), it appears that
>>> > > something's going wrong with reading the EDID, because the available
>>> > > modes are all screwed up (both console and X decide they want to drive
>>> > > the display at 1024x768).  Here's the output of xrandr on 3.2.15:
>>> > >
>>> > >  % xrandr
>>> > >  Screen 1: minimum 320 x 200, current 1600 x 1200, maximum 4096 x 4096
>>> > >  VGA-1 connected 1600x1200+0+0 (normal left inverted right x axis y 
>>> > > axis) 352mm x 264mm
>>> > >     1600x1200      75.0*+   70.0     65.0     60.0
>>> > >     1280x1024      85.0 +   75.0     60.0
>>> > >     1920x1440      60.0
>>> > >     1856x1392      60.0
>>> > >     1792x1344      60.0
>>> > >     1920x1200      74.9     59.9
>>> > >     1680x1050      84.9     74.9     60.0
>>> > >     1400x1050      85.0     74.9     60.0
>>> > >     1440x900       84.8     75.0     59.9
>>> > >     1280x960       85.0     60.0
>>> > >     1360x768       60.0
>>> > >     1280x800       84.9     74.9     59.8
>>> > >     1152x864       75.0
>>> > >     1280x768       84.8     74.9     59.9
>>> > >     1024x768       85.0     75.1     75.0     70.1     60.0     43.5    
>>> > >  43.5
>>> > >     832x624        74.6
>>> > >     800x600        85.1     72.2     75.0     60.3     56.2
>>> > >     848x480        60.0
>>> > >     640x480        85.0     75.0     72.8     72.8     66.7     60.0    
>>> > >  59.9
>>> > >     720x400        85.0     87.8     70.1
>>> > >     640x400        85.1
>>> > >     640x350        85.1
>>> > >     320x200       165.1
>>> > >
>>> > > And on 3.4-rc4+ (with your patch cherry-picked):
>>> > >
>>> > >  % xrandr
>>> > >  Screen 1: minimum 320 x 200, current 1024 x 768, maximum 4096 x 4096
>>> > >  VGA-1 connected 1024x768+0+0 (normal left inverted right x axis y 
>>> > > axis) 0mm x 0mm
>>> > >     1024x768       60.0*
>>> > >     800x600        60.3     56.2
>>> > >     848x480        60.0
>>> > >     640x480        59.9
>>> > >     320x200       165.1
>>> > >
>>> > > Running xrandr on 3.4-rc4+ also causes the screen to go black for a
>>> > > second when it does not on 3.2.15.  It also causes several messages of
>>> > > the form
>>> > >
>>> > >  [drm] nouveau :01:00.0: Load detected on output B
>>> > >
>>> > > to be logged.  Also, looking at /sys/class/drm/card0-VGA-1/edid I see
>>> > > that it is empty on 3.4-rc4+ and it is correct on 3.2.15.  Things seem
>>> > > to work OK when the KVM is not involved.
>>> >
>>> > Were you ever able to fetch a EDID with the KVM involved?  KVMs are
>>> > notorious for not connecting the ddc pins.
>>>
>>> Yes, it works on 3.2.15 as described above.
>>
>> I have the same (or similar) KVM (not in the office at the moment) and I
>> can confirm that with newer kernels EDID fecthing in flaky. It's 50/50
>> if EDED retrieval succeeds or if it fails with:
>>
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.936336] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.955317] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.973879] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.087659] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.107147] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.126908] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.146277] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.297659] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.317063] [drm:d

AMD GPU: Toshiba, brightness and ATIF

[Luca Tettamanti]

Hello,
I'm trying to figure out how to control the brightness of the screen of my
laptop, a Toshiba L885-10W.

The ACPI DSDT table contains the standard control methods, the setter (_BCM),
however, only sets a value in the record returned by ATIF method and send out a
notification:

CreateByteField (ATIB, 0x0C, BRIL) // ATIB is returned by ATIF
Store (Arg0, BRIL)
Notify (VGA, 0x81)

When using the ACPI sysfs backlight interface nothing happens (obviously).

0x81 seems also strange, according to the specification is

  "Output Device Status Change: Used to notify OSPM whenever the state of any
  output devices attached to the VGA controller has been changed. This event
  will, for example, be generated when the user plugs-in or remove a CRT from
  the VGA port. In this case, OSPM will re-enumerate all devices attached to
  VGA"

and confuses KDE when using more than one screen (but this is a
different issue).

I suspect that the notification means that the radeon driver is supposed to
change the brightness; the GPU is a:

01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI 
Thames XT/GL [Radeon HD 7600M Series] [1002:6840]

kernel drivers says:
[drm] initializing kernel modesetting (TURKS 0x1002:0x6840 0x1179:0xFB22)

Poking around with avivotools it seems that AVIVO_LVDS_BACKLIGHT_CNTL (0x7af8)
is not present anymore...

Furthermore ATIF returns a different structure based on the parameter passed by
the caller; radeon drm driver always passes "1", the BRIL field is mentioned
only when passing "2" (it shouldn't matter, the field is set unconditionally,
but it seems some king of versioning); it seems a versioned data structure...

Do you have any information that you can share about ATIF and brightness
control on modern GPUs?

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Wed, Jul 25, 2012 at 01:38:09PM -0400, alexdeuc...@gmail.com wrote:
> From: Alex Deucher 
> 
> Add a new header that defines the AMD ACPI interface used
> for laptops, PowerXpress, and chipset specific functionality
> and update the current code to use it.

Great! Now my DSDT makes sense ;)

> Todo:
> - properly verify the ACPI interfaces
> - hook up and handle ACPI notifications

I see a problem here, I've hacked up a standalone test module, but:

> +#define ATIF_FUNCTION_GET_SYSTEM_PARAMETERS0x1
[...]
> + * flags
> + * bits 1:0:
> + * 0 - Notify(VGA, 0x81) is not used for notification
> + * 1 - Notify(VGA, 0x81) is used for notification

My system has this bit set, and the brightness control method does send
the notification.
My module register itself with register_acpi_notifier and gets 2
notifications, one with ACPI_VIDEO_NOTIFY_PROBE (0x81) and the other
with ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS (or DEC, depending on what I
press).

The standard acpi "video" module, however, in response to
ACPI_VIDEO_NOTIFY_PROBE generates a key press sending
KEY_SWITCHVIDEOMODE.

This greatly confuses KDE which messes up my dual screen configuration;
I guess that the spurious KEY_SWITCHVIDEOMODE may be problematic also
with other DEs...

In more detail what happens is the following:
- I press the brightness hotkey, firmware generates a notification on
  the relevant device (_SB.PCI0.PEG0.VGA.LCD)
- ACPI video module gets the ACPI_VIDEO_NOTIFY_{DEC,INC}_BRIGHTNESS
  notification and tries to adjust the brightness with
  acpi_video_device_lcd_set_level, which in turns calls _BCM
- _BCM sets the relevant bits in the AMD-specific structure and does a
  Notify (VGA, 0x81)
- again ACPI video module gets the nodification (in this case
  ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the hypothetical
  radeon driver does its magic to adjust the screen.

My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the
acpi notifier chain, and allow the handlers to veto the key press (like
it's done for ACPI_VIDEO_NOTIFY_SWITCH).

Zhang Rui what do you think about this?

The other missing bit is how to actually change the brightness... Alex,
do you know what registers to poke?

My card is a:

01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI 
Thames XT/GL [Radeon HD 600M Series] [1002:6840]

on a Toshiba L855.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
> On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti  wrote:
> > The other missing bit is how to actually change the brightness... Alex,
> > do you know what registers to poke?
> 
> You need to check if the GPU controls the backlight or the system
> does.  I think the attached patches should point you in the right
> direction.

Yep :)

0050:  ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability :
  0050:  (union) ATOM_FIRMWARE_CAPABILITY sbfAccess  :
USHORT GPUControlsBL:1  = 0x0001 (1)

The panel is using the INTERNAL_UNIPHY encoder, and I see the
UNIPHYTransmitterControl command table.

Interaction with video.ko is still a bit messy...

Do you already have code for handling the notifications? I'll work on it
in the weekend otherwise ;)

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Fri, Jul 27, 2012 at 12:46:50PM +0800, joeyli wrote:
> 於 四，2012-07-26 於 23:31 -0400，Alex Deucher 提到：
> > On Thu, Jul 26, 2012 at 10:50 PM, joeyli  wrote:
> > > 於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
> > >> - again ACPI video module gets the nodification (in this case
> > >>   ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
> > >> - KDE seems this and muck with the screen configuration :(
> > >> - meanwhile the brightness notification is propagated, the
> > >> hypothetical
> > >>   radeon driver does its magic to adjust the screen.
> > >>
> > >> My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to
> > >> the
> > >> acpi notifier chain, and allow the handlers to veto the key press
> > >> (like
> > >> it's done for ACPI_VIDEO_NOTIFY_SWITCH).
> > >
> > > I welcome this approach!
> > >
> > > On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when
> > > AC-power unplug, because BIOS want to nodify video driver do some power
> > > saving stuff.
> > > It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power
> > > unplug.
> > >
> > > At least acpi/video driver need to know this 0x81 event is filed by BIOS
> > > to radeon-acpi for notify but not do video mode switch. That means the
> > > radeon drm need take the video switch responsibility.
> > 
> > Probably we'd just want the radeon acpi handler to just forward the
> > events to userspace so that the user can choose what to do with it
> > (xrandr command, etc.), otherwise we'll need to define policy in the
> > driver.
> 
> Any kernel module can issue KEY_SWITCHVIDEOMODE to user space, then
> gnome-settings-daemon(on Gnome) and  krandr(on KDE) will call xrandr
> library to switch video mode.

Exacly, and if we have pending system bios requests then the event is
not a real ACPI_VIDEO_NOTIFY_PROBE and (IMHO) it should not be forwarded
to the userspace as such.
The "vanilla" ACPI_VIDEO_NOTIFY_PROBE is already forwared to userspace.

> The 0x81 ACPI event for acpi/video driver is ACPI_VIDEO_NOTIFY_PROBE,
> means need issue KEY_SWITCHVIDEOMODE. But, 0x81 for radeon is a general
> notification event.
> I didn't see probe state in GET_SYSTEM_BIOS_REQUESTS, how can we
> distinguish this 0x81 is a ACPI_VIDEO_NOTIFY_PROBE or a ATI general
> notification event?

+#define ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS 0x2
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS
+ * ARG1: none
+ * OUTPUT:
+ * WORD  - structure size in bytes (includes size field)
+ * DWORD - pending sbios requests
+ * BYTE  - panel expansion mode
+ * BYTE  - thermal state: target gfx controller
+ * BYTE  - thermal state: state id (0: exit state, non-0: state)
+ * BYTE  - forced power state: target gfx controller
+ * BYTE  - forced power state: state id
+ * BYTE  - system power source
+ * BYTE  - panel backlight level (0-255)

I guess that if "pending sbios requests" == 0 then the event is the
general purpose one, and is not handled by radeon driver.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Jul 26, 2012 at 03:42:26PM -0400, Alex Deucher wrote:
> On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti  wrote:
> > On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
> >> On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti  
> >> wrote:
> >> > The other missing bit is how to actually change the brightness... Alex,
> >> > do you know what registers to poke?
> >>
> >> You need to check if the GPU controls the backlight or the system
> >> does.  I think the attached patches should point you in the right
> >> direction.
> >
> > Yep :)
> >
> > 0050:  ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability :
> >   0050:  (union) ATOM_FIRMWARE_CAPABILITY sbfAccess  :
> > USHORT GPUControlsBL:1  = 0x0001 (1)
> >
> > The panel is using the INTERNAL_UNIPHY encoder, and I see the
> > UNIPHYTransmitterControl command table.
> >
> > Interaction with video.ko is still a bit messy...
> >
> > Do you already have code for handling the notifications? I'll work on it
> > in the weekend otherwise ;)
> 
> I don't have patches for that.  Please feel free to work on it :)

I just found the first problem (probably a BIOS bug):
ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
I intended to use the method to set up the notification handler but now
my BIOS says that it's not there even if it is...
Can I assume some default values (e.g. notifications are enabled and will
use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
different)?

thanks,
Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti  wrote:
> > I just found the first problem (probably a BIOS bug):
> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > I intended to use the method to set up the notification handler but now
> > my BIOS says that it's not there even if it is...
> > Can I assume some default values (e.g. notifications are enabled and will
> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > different)?
> 
> The spec says that the bits in the supported functions vector mean
> that if bit n is set, function n+1 exists, 

Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?

Maybe if the bit n is set then functions 0..n are available? That would
(almost) match what I see...

> but it's possible that the
> spec is wrong and it's actually a 1 to 1 mapping; if bit n is set,
> function n is supported.  In which case the the supported functions
> vector bits should be:
> +/* supported functions vector */
> +#   define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED   (1 << 1)
> +#   define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED(1 << 2)
> +#   define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED  (1 << 3)
> +#   define ATIF_GET_LID_STATE_SUPPORTED   (1 << 4)
> +#   define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED   (1 << 5)
> +#   define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 6)
> +#   define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED  (1 << 7)
> +#   define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED(1 << 8)
> +#   define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 13)
> +#   define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED   (1 << 15)
> 
> See if that lines up better. 

Not really... the value returned by VERIFY_INTERFACE is 0x2, but in the
DSDT I see:

ATIF_FUNCTION_GET_SYSTEM_PARAMETERS
ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS
ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS
ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS

The implementation of the first one makes sense, the second is used for
brightness control. The other two _might_ be a leftover (the machine
does not have an analog TV out).

> I'm still new to these ACPI interfaces
> so I'm not an expert yet.

I've been exposed to a lot of ACPI code (I wrote the asus_atk0110
driver), in my experience the DSDT is full of crap: code copied&pasted
from other machines, leftover no longer used, and other stuff that's
plainly wrong.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
> Hi Luca, 
> 
> 於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
> > I just found the first problem (probably a BIOS bug):
> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > I intended to use the method to set up the notification handler but now
> > my BIOS says that it's not there even if it is...
> > Can I assume some default values (e.g. notifications are enabled and will
> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > different)?
> > 
> 
> Did you check your DSDT for there have some "Notify (VGA, 0x81)"
> statement in AFN0..AFN15?
> If YES, I think that means your machine in case the 0x81 is for ATI used
> by default.

Yes, my point is that the nofication is there, but since
GET_SYSTEM_PARAMETERS is not announced I have not way to check it.
IOW, what is implemented in the DSDT does not match what is announced by
VERIFY_INTERFACE.
For reference this is the DSDT: http://pastebin.com/KKS7ZsTt

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

Hi,
I'm attaching a first draft of my work. The first 3 patches are
infrastructure work, the fourth wires the notification handler and
retrieves the requests from the system BIOS, but it does not actually
change brightness yet.

The problem here is how to get the correct encoder: should I just scan
encoder_list checking for ATOM_DEVICE_LCD_SUPPORT and see if it has a
backlight device attached?
Hopefully there is only one encoder with it, right?

I'm also toying with the idea of creating structures matching the output
of the various ACPI methods, this would remove some ugly pointer
arithmetics, but it _might_ make it easier to read past the buffer if
one does not check the size before using the struct. What do you think?

Luca
>From e485a3a4075c25ba1747ad61f6168c3734d5f0a9 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |   23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..8816698 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,10 +14,8 @@
 #include 
 
 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object* radeon_atif_call(acpi_handle handle, int function)
 {
 	acpi_status status;
 	union acpi_object atif_arg_elements[2];
@@ -28,7 +26,7 @@ static int radeon_atif_call(acpi_handle handle)
 	atif_arg.pointer = &atif_arg_elements[0];
 
 	atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-	atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
+	atif_arg_elements[0].integer.value = function;
 	atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
 	atif_arg_elements[1].integer.value = 0;
 
@@ -39,18 +37,18 @@ static int radeon_atif_call(acpi_handle handle)
 		DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
  acpi_format_exception(status));
 		kfree(buffer.pointer);
-		return 1;
+		return NULL;
 	}
 
-	kfree(buffer.pointer);
-	return 0;
+	return buffer.pointer;
 }
 
 /* Call all ACPI methods here */
 int radeon_acpi_init(struct radeon_device *rdev)
 {
 	acpi_handle handle;
-	int ret;
+	union acpi_object *info;
+	int ret = 0;
 
 	/* Get the device handle */
 	handle = DEVICE_ACPI_HANDLE(&rdev->pdev->dev);
@@ -60,10 +58,11 @@ int radeon_acpi_init(struct radeon_device *rdev)
 		return 0;
 
 	/* Call the ATIF method */
-	ret = radeon_atif_call(handle);
-	if (ret)
-		return ret;
+	info = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE);
+	if (!info)
+		ret = -EIO;
 
-	return 0;
+	kfree(info);
+	return ret;
 }
 
-- 
1.7.10.4

>From b40c42f6808d84e36095f8b21daad28b3928 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:19:07 +0200
Subject: [PATCH 2/4] drm/radeon: implement radeon_atif_verify_interface

Wrap the call to VERIFY_INTERFACE and add the parsing of the support
vectors.

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon.h  |   40 
 drivers/gpu/drm/radeon/radeon_acpi.c |   67 +++---
 2 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index fefcca5..0db98eb 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1456,6 +1456,44 @@ struct r600_vram_scratch {
 	u64gpu_addr;
 };
 
+/*
+ * ACPI
+ */
+struct radeon_atif_notification_cfg {
+	bool enabled;
+	int command_code;
+};
+
+struct radeon_atif_notifications {
+	bool display_switch;
+	bool expansion_mode_change;
+	bool thermal_state;
+	bool forced_power_state;
+	bool system_power_state;
+	bool display_conf_change;
+	bool px_gfx_switch;
+	bool brightness_change;
+	bool dgpu_display_event;
+};
+
+struct radeon_atif_functions {
+	bool system_params;
+	bool sbios_requests;
+	bool select_active_disp;
+	bool lid_state;
+	bool get_tv_standard;
+	bool set_tv_standard;
+	bool get_panel_expansion_mode;
+	bool set_panel_expansion_mode;
+	bool temperature_change;
+	bool graphics_device_types;
+};
+
+struct radeon_atif {
+	struct radeon_atif_notifications notifications;
+	struct radeon_atif_functions functions;
+	struct radeon_atif_notification_cfg notification_cfg;
+};
 
 /*
  * Core structure, functions and helpers.
@@ -1548,6 +1586,8 @@ struct radeon_device {
 	unsigned 		debugfs_count;
 	/* virtual memory */
 	struct radeon_vm_manager	vm_manager;
+	/* ACPI interface */
+	struct radeon_atif		atif;
 };
 
 int radeon_device_init(struct radeon_device *rdev,
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 8816698..d35bf8a 100644
--- a/drive

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 04:32:47PM +0800, joeyli wrote:
> 於 日，2012-07-29 於 15:10 +0200，Luca Tettamanti 提到：
> > On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
> > > Hi Luca, 
> > > 
> > > 於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
> > > > I just found the first problem (probably a BIOS bug):
> > > > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > > > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > > > I intended to use the method to set up the notification handler but now
> > > > my BIOS says that it's not there even if it is...
> > > > Can I assume some default values (e.g. notifications are enabled and 
> > > > will
> > > > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > > > different)?
> > > > 
> > > 
> > > Did you check your DSDT for there have some "Notify (VGA, 0x81)"
> > > statement in AFN0..AFN15?
> > > If YES, I think that means your machine in case the 0x81 is for ATI used
> > > by default.
> > 
> > Yes, my point is that the nofication is there, but since
> > GET_SYSTEM_PARAMETERS is not announced I have not way to check it.
> > IOW, what is implemented in the DSDT does not match what is announced by
> > VERIFY_INTERFACE.
> > For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
> > 
> > Luca
> > 
> 
> Yes, saw the problem in your DSDT:
> 
> Method (AF00, 0, NotSerialized)
> {
> CreateWordField (ATIB, Zero, SSZE)
>   ...
> Store (0x80, NMSK)
> Store (0x02, SFUN)<=== 10b, bit 0 is 0
> Return (ATIB)
> }
> 
> But, AF01 still supported in ATIF on this machine, maybe we should still
> try GET_SYSTEM_PARAMETERS even the function vector set to 0?
> No idea...

That's what I'm doing right now... if SBIOS_REQUESTS is supported I try
and call GET_SYSTEM_PARAMETERS even if it's not announced.

> On the other hand, 
> My patch to avoid 0x81 event conflict with acpi/video driver like below.
> This patch woks on my notebook. Your patches do much more things then
> mine, so I think my patch just for reference.  

I ignored the event handling for now... I'd like to hear something back
from ACPI camp before committing to this solution.

> There have a problem is:
> If we want use acpi_notifier_call_chain to check does event consume by
> any notifier in acpi's blocking notifier chain, then we need return
> NOTIFY_BAD in radeon_acpi but not NOTIFY_OK.
> 
> So, I suggest radeon_acpi should register to acpi notifier chain by
> itself but not append on radeon_acpi_event in radeon_pm. 

It shouldn't matter, once I change radeon_atif_handler to return
NOTIFY_BAD the call chain will be stopped anyway.

> And,
> suggest also check the device class is ACPI_VIDEO_CLASS like following:
> 
> +static int radeon_acpi_video_event(struct notifier_block *nb,
> ...
> + if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
> + return NOTIFY_DONE;
> +

Will do. I'll use the package structs in the next iteration.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
> On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti  wrote:
> > On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
> >> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti  
> >> wrote:
> >> > I just found the first problem (probably a BIOS bug):
> >> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> >> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> >> > I intended to use the method to set up the notification handler but now
> >> > my BIOS says that it's not there even if it is...
> >> > Can I assume some default values (e.g. notifications are enabled and will
> >> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> >> > different)?
> >>
> >> The spec says that the bits in the supported functions vector mean
> >> that if bit n is set, function n+1 exists,
> >
> > Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
> > mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
> >
> > Maybe if the bit n is set then functions 0..n are available? That would
> > (almost) match what I see...
> 
> From the spec:
> 
> Supported DWORD Bit vector providing supported functions
> information. Each bit marks
> Functions Bit  support for one specific function of the
> ATIF method. Bit n, if set,
> Vectorindicates that Function n+1 is supported.

Sorry, I still don't understand it... what's "Function n+1" in this
context?
Does this mean that if bit n is set then the function defined as
1 << (n+1) is supported?

> I don't know how wonky bioses in the wild are however.  I can see what
> our internal teams do in these sort of cases.

That would be helpful :)
Note that on this machine (Toshiba L855-10W) brightness control under
windows does not work with the stock catalyst driver, it works only with
the (oldish) driver supplied by Toshiba.

Anyway, I'm attaching v2 of my patches, I've incorporated the
suggestions and some bits of code from joeyli, and now brightness
control is actually implemented.

Still missing is the issue of event 0x81 and the conflict with video.ko;
the handler should probably return NOTIFY_BAD to veto the keypress.

Luca
>From f0f8699eabee0d47b93fba14f8126b821cc106a5 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.
v2: add support for the 2nd parameter (from Lee, Chun-Yi
).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |   38 --
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..158e514 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,23 +14,30 @@
 #include 
 
 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object *radeon_atif_call(acpi_handle handle, int function,
+   struct acpi_buffer *params)
 {
acpi_status status;
union acpi_object atif_arg_elements[2];
struct acpi_object_list atif_arg;
-   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
atif_arg.count = 2;
atif_arg.pointer = &atif_arg_elements[0];
 
atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
-   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[1].integer.value = 0;
+   atif_arg_elements[0].integer.value = function;
+
+   if (params) {
+   atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+   atif_arg_elements[1].buffer.length = params->length;
+   atif_arg_elements[1].buffer.pointer = params->pointer;
+   } else {
+   /* We need a second fake parameter */
+   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
+   atif_arg_elements[1].integer.value = 0;
+   }
 
status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);
 
@@ -39,18 +46,18 @@ static int radeon_atif_call(acpi_handle handle)
DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 acpi_format_exception(status));
kfree(buffer.pointer);
-   return 1;
+ 

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:30 PM, Alex Deucher  wrote:
> On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti  wrote:
>> On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
>>> Supported DWORD Bit vector providing supported functions
>>> information. Each bit marks
>>> Functions Bit  support for one specific function of the
>>> ATIF method. Bit n, if set,
>>> Vectorindicates that Function n+1 is supported.
>>
>> Sorry, I still don't understand it... what's "Function n+1" in this
>> context?
>> Does this mean that if bit n is set then the function defined as
>> 1 << (n+1) is supported?
>
> It means if bit n is set in teh supported function vector, function
> n+1 is supported.  E.g., if bit 1 is set, function 2
> (ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported.  If bit 3 is
> set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.

Great, just had an epiphany ;-) "n+1" refers to the value that's
passed down to ATIF, I was still thinking in terms of bitmasks...
Ok, so my code is correct, BIOS is botched... meh.

L
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher  wrote:
> Regarding patches 3 and 4, it might be easier to just store a pointer
> to the relevant encoder when you verify ATIF rather than walking the
> encoder list every time.

Makes sense, I was unsure about the lifetime of the encoders, but
AFAICS they're destroyed only when the module in unloaded.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Fwd: Brightness on HP EliteBook 8460p

[Luca Tettamanti]

On Sat, Jul 28, 2012 at 4:47 PM, Pali Rohár  wrote:
> Hello, I have some good news. Radeon patches from this post
> http://lists.freedesktop.org/archives/dri-devel/2012-July/025535.html
> export brightness file /sys/class/backlight/radeon_bl/brightness
> And finally with these patches I'm able to change brightness on my EliteBook.

Nice, in your older mail I read:

> The PEGP.DGFX acpi device was binding to
> acpi/video driver, the above ASL code emit a 0xD0 bus event to
> video.c but cann't process it. Even we add a new bus event in
> video.c and generate a acpi event, there still need another acpi
> driver should take care it.

That event (0xd0) is probably a custom event announced through ATIF.
I'm working on the code to deal with it, the latest patches are here:

http://lists.freedesktop.org/archives/dri-devel/2012-July/025646.html

You first need this one with the ACPI header:
http://lists.freedesktop.org/archives/dri-devel/2012-July/025517.html

and the other 3 that you've already tested. With my patches on top you
should be able to change the brightness using the hotkeys.

I'd be grateful if you could test them :-)

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Fwd: Brightness on HP EliteBook 8460p

[Luca Tettamanti]

I'm putting back the CC and adding Alex.

On Tue, Jul 31, 2012 at 5:07 PM, Pali Rohár  wrote:
> Thanks, now working. When I write to acpi video brightness file
> it change brightness and in dmesg is:
>
> [   47.200998] [drm:radeon_atif_handler], event, device_class =
> video, type = 0xd0
> [   47.201102] [drm:radeon_atif_get_sbios_requests], SBIOS
> pending requests: 0x80
> [   47.201104] [drm:radeon_atif_handler], ATIF: 1 pending SBIOS
> requests
> [   47.201105] [drm:radeon_atif_handler], Changing brightness to
> 11

Great! I'll send an updated patch to Alex soon.

> I think that acpi only sent event about brightness key pressed,
> because nothing happened when I pressed it.
>
> Also for windows is needed special HP application (hp hotkey) for
> brightness keys. Without it brightness keys not working too...

I've looked at hp-wmi: the hotkey is indeed dispatched via WMI
(hp-wmi) so you need something in userspace (KDE, Gnome, etc) to
respond to that key press.

> And there is one problem with /sys/class/backlight/radeon_bl.
> When I enable Ambient Light Sensor which auto adjust brightness
> based on sensor data, writing value 0 (min) or 255 (max) to
> /sys/class/backlight/radeon_bl/brightness turn off display.

Ok, that's weird. 0 turns off the panel by design, 255 should not...

> All
> other values (1-254) are OK (adjust brightness level). When I
> turn off Ambient Light Sensor (via hp-wmi kernel module) then
> values 0 and 255 also set brightness level (min and max). My
> suggestion is to convert value 0 to 1 and 255 to 254 to prevent
> this problem.

No idea what's going on here... might be some weird vendor magic.
The WMI code is rather obscure...

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Tue, Jul 31, 2012 at 09:58:04AM -0400, Alex Deucher wrote:
> On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti  wrote:
> > On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher  
> > wrote:
> >> Regarding patches 3 and 4, it might be easier to just store a pointer
> >> to the relevant encoder when you verify ATIF rather than walking the
> >> encoder list every time.

Done.

> > Makes sense, I was unsure about the lifetime of the encoders, but
> > AFAICS they're destroyed only when the module in unloaded.
> 
> They are present for the life of the driver.

I had to move to call to radeon_acpi_init after radeon_modeset_init,
otherwise the encoders are not present yet (the tear down code path is
correct, acpi first, then modeset).
Latest and greatest version is attached; I fixed notifications when
using custom command codes (tested by Pali Rohár) and implemented your
suggestion. 

Luca
>From f0f8699eabee0d47b93fba14f8126b821cc106a5 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.
v2: add support for the 2nd parameter (from Lee, Chun-Yi
).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |   38 --
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..158e514 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,23 +14,30 @@
 #include 
 
 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object *radeon_atif_call(acpi_handle handle, int function,
+   struct acpi_buffer *params)
 {
acpi_status status;
union acpi_object atif_arg_elements[2];
struct acpi_object_list atif_arg;
-   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
atif_arg.count = 2;
atif_arg.pointer = &atif_arg_elements[0];
 
atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
-   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[1].integer.value = 0;
+   atif_arg_elements[0].integer.value = function;
+
+   if (params) {
+   atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+   atif_arg_elements[1].buffer.length = params->length;
+   atif_arg_elements[1].buffer.pointer = params->pointer;
+   } else {
+   /* We need a second fake parameter */
+   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
+   atif_arg_elements[1].integer.value = 0;
+   }
 
status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);
 
@@ -39,18 +46,18 @@ static int radeon_atif_call(acpi_handle handle)
DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 acpi_format_exception(status));
kfree(buffer.pointer);
-   return 1;
+   return NULL;
}
 
-   kfree(buffer.pointer);
-   return 0;
+   return buffer.pointer;
 }
 
 /* Call all ACPI methods here */
 int radeon_acpi_init(struct radeon_device *rdev)
 {
acpi_handle handle;
-   int ret;
+   union acpi_object *info;
+   int ret = 0;
 
/* Get the device handle */
handle = DEVICE_ACPI_HANDLE(&rdev->pdev->dev);
@@ -60,10 +67,11 @@ int radeon_acpi_init(struct radeon_device *rdev)
return 0;
 
/* Call the ATIF method */
-   ret = radeon_atif_call(handle);
-   if (ret)
-   return ret;
+   info = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE, NULL);
+   if (!info)
+   ret = -EIO;
 
-   return 0;
+   kfree(info);
+   return ret;
 }
 
-- 
1.7.10.4

>From 427002ddf653b0abd0fb820b09322bf2a0b281af Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Mon, 30 Jul 2012 21:11:58 +0200
Subject: [PATCH 2/4] drm/radeon: implement radeon_atif_verify_interface

Wrap the call to VERIFY_INTERFACE and add the parsing of the support
vectors.
v2: use a packed struct for handling the output of ACPI calls, hides
ugly pointer arithmetics (Lee, Chun-Yi ).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon.h  |   40 +
 drivers/gpu/drm/radeon/radeon_acpi.c |   79 +++---
 2 files changed, 113 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index fefcca5..0db98eb 100644
--- a/drivers

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
> Patches look good.  I picked them up and combined them with may
> patches plus a few other small fixes.  They are available here:
> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
> Let me know what you think.

Looks ok, I lost one fix along the road though, I'm attaching the patch.

Luca
>From 0f71d5b56b9e5eee3194b5b926767511281ea0a6 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Wed, 1 Aug 2012 10:53:19 +0200
Subject: [PATCH] drm/radeon: fix, enable notifications with device specific
 command code

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 14ae8aa..a812b9a 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -214,6 +214,7 @@ static int radeon_atif_get_notification_params(acpi_handle 
handle,
err = -EINVAL;
goto out;
}
+   n->enabled = true;
n->command_code = params.command_code;
}
 
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RFC] drm/radeon: ACPI: veto the keypress on ATIF events

[Luca Tettamanti]

AMD ACPI interface may overload the standard event
ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
userspace because the user did not press the mode switch key (the
spurious keypress confuses the DE which usually changes the
display configuration and messes up a dual-screen setup).
This patch gives the radeon driver the chance to examine the event and
block the keypress if the event is an "AMD event".

Signed-off-by: Luca Tettamanti 
---
Any comment from ACPI front?

 drivers/acpi/video.c |   10 --
 drivers/gpu/drm/radeon/radeon_acpi.c |7 ++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..a8592c4 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1457,7 +1457,12 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
acpi_video_device_enumerate(video);
acpi_video_device_rebind(video);
acpi_bus_generate_proc_event(device, event, 0);
-   keycode = KEY_SWITCHVIDEOMODE;
+   /* This event is also overloaded by AMD ACPI interface, don't
+* send the key press if the event has been handled elsewhere
+* (e.g. radeon DRM driver).
+*/
+   if (!acpi_notifier_call_chain(device, event, 0))
+   keycode = KEY_SWITCHVIDEOMODE;
break;
 
case ACPI_VIDEO_NOTIFY_CYCLE:   /* Cycle Display output hotkey pressed. 
*/
@@ -1479,7 +1484,8 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
break;
}
 
-   if (event != ACPI_VIDEO_NOTIFY_SWITCH)
+   if (event != ACPI_VIDEO_NOTIFY_SWITCH &&
+   event != ACPI_VIDEO_NOTIFY_PROBE)
acpi_notifier_call_chain(device, event, 0);
 
if (keycode) {
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 96de08d..ee0d29e 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev,
}
/* TODO: check other events */
 
-   return NOTIFY_OK;
+   /* We've handled the event, stop the notifier chain. The ACPI interface
+* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to
+* userspace if the event was generated only to signal a SBIOS
+* request.
+*/
+   return NOTIFY_BAD;
 }
 
 /* Call all ACPI methods here */
-- 
1.7.10.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC] drm/radeon: ACPI: veto the keypress on ATIF events

[Luca Tettamanti]

On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
> On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
> > AMD ACPI interface may overload the standard event
> > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
> > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
> > userspace because the user did not press the mode switch key (the
> > spurious keypress confuses the DE which usually changes the
> > display configuration and messes up a dual-screen setup).
> > This patch gives the radeon driver the chance to examine the event and
> > block the keypress if the event is an "AMD event".
> > 
> > Signed-off-by: Luca Tettamanti 
> > ---
> > Any comment from ACPI front?
> > 
> it looks good to me.
> But I'm wondering if we can use the following code for ACPI part, which
> looks cleaner.
> I know this may change the behavior of other events, but in theory, we
> should not send any input event if we know something wrong in kernel.
> 
> what do you think?

I like it, it's cleaner.
I've split the patch in two pieces (one for video, the other for
radeon) and adopted your suggestion.

BTW, I'm leaving for vacation in a few hours, I'll be offline till mid
August.

Luca
>From acce30c96b90d1bc550e82a9e7f19226fa194d5e Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Thu, 2 Aug 2012 15:30:27 +0200
Subject: [PATCH 1/2] ACPI video: allow events handlers to veto the keypress

The standard video events may be overloaded for device specific
purposes. For example AMD ACPI interface overloads
ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
userspace because the user did not press the mode switch key (the
spurious keypress confuses the DE which usually changes the
display configuration and messes up a dual-screen setup).
This patch gives the handlers the chance to examine the event and
block the keypress if the event is device specific.
v2: refactor as suggested by Zhang Rui 

Signed-off-by: Luca Tettamanti 
---
 drivers/acpi/video.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..d75642a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1448,8 +1448,7 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
case ACPI_VIDEO_NOTIFY_SWITCH:  /* User requested a switch,
 * most likely via hotkey. */
acpi_bus_generate_proc_event(device, event, 0);
-   if (!acpi_notifier_call_chain(device, event, 0))
-   keycode = KEY_SWITCHVIDEOMODE;
+   keycode = KEY_SWITCHVIDEOMODE;
break;
 
case ACPI_VIDEO_NOTIFY_PROBE:   /* User plugged in or removed a video
@@ -1479,8 +1478,9 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
break;
}
 
-   if (event != ACPI_VIDEO_NOTIFY_SWITCH)
-   acpi_notifier_call_chain(device, event, 0);
+   if (acpi_notifier_call_chain(device, event, 0))
+   /* Something vetoed the keypress. */
+   keycode = 0;
 
if (keycode) {
    input_report_key(input, keycode, 1);
-- 
1.7.10.4

>From def5119d8f617eef0fac2cd35d7bb18c176ff8f6 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Thu, 2 Aug 2012 15:33:03 +0200
Subject: [PATCH 2/2] drm/radeon: block the keypress on ATIF events

The AMD ACPI interface may use ACPI_VIDEO_NOTIFY_PROBE to signal SBIOS
requests; block the keypress in this case since the user did not
actually press the mode switch key.

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 96de08d..ee0d29e 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev,
}
/* TODO: check other events */
 
-   return NOTIFY_OK;
+   /* We've handled the event, stop the notifier chain. The ACPI interface
+* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to
+* userspace if the event was generated only to signal a SBIOS
+* request.
+*/
+   return NOTIFY_BAD;
 }
 
 /* Call all ACPI methods here */
-- 
1.7.10.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher  wrote:
> I admit I'm not really an ACPI expert, but thinking about this more,
> I'm wondering if maybe we should just send the appropriate brightness
> change, switch display, etc. event to userspace rather than handling
> it directly in the radeon driver, then let userspace callback down via
> the bl interface, etc.  With backlight for example, does handling it
> in the kernel driver as per your patch prevent userspace from seeing
> the brightness up/down event?  Wouldn't that break things like OSD
> brightness displays and such?

No, the event is sent to userspace by the standard ACPI video driver,
it works as before.
Changing brightness usually goes like this:
1) user presses a hotkey
2) a notification is generated (0x86 or 0x87)
3) video.ko handles the notification and calls into ACPI to change the
level (_BCM) and firmware does its magic
4) a key press (brightness up/down) is sent to userspace

With ATIF step 3 does not actually change the brightness, it just send
out another event (VIDEO_PROBE, or one of the device specific ones) so
we need to take care of that too. The rest of the process, including
the delivery of the key presses, stays the same.

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/kms: disable ss fixed ref divide

[Luca Tettamanti]

On Mon, Dec 13, 2010 at 5:27 AM, Alex Deucher  wrote:
> Seems to cause problems on certain laptops
>
> Fixes:
> https://bugzilla.kernel.org/show_bug.cgi?id=24462
>
> Signed-off-by: Alex Deucher 
Tested-by: Luca Tettamanti 

> ---
>  drivers/gpu/drm/radeon/atombios_crtc.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c 
> b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 4d2eb92..6a4f090 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -554,7 +554,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
>                                        dp_clock = dig_connector->dp_clock;
>                                }
>                        }
> -
> +#if 0 /* doesn't work properly on some laptops */
>                        /* use recommended ref_div for ss */
>                        if (radeon_encoder->devices & 
> (ATOM_DEVICE_LCD_SUPPORT)) {
>                                if (ss_enabled) {
> @@ -564,7 +564,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
>                                        }
>                                }
>                        }
> -
> +#endif
>                        if (ASIC_IS_AVIVO(rdev)) {
>                                /* DVO wants 2x pixel clock if the DVO chip is 
> in 12 bit mode */
>                                if (radeon_encoder->encoder_id == 
> ENCODER_OBJECT_ID_INTERNAL_KLDSCP_DVO1)
> --
> 1.7.1.1
>
> ___
> 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: [DRM] BUG: sleeping function called from invalid context, drm_lastclose

[Luca Tettamanti]

On Wed, Aug 11, 2010 at 10:50 AM, Dave Airlie  wrote:
> On Wed, Aug 11, 2010 at 6:48 PM, Luca Tettamanti  wrote:
>> Hi Arnd,
>> this commit:
>>
>> commit 58374713c9dfb4d231f8c56cac089f6fbdedc2ec
>> Author: Arnd Bergmann 
>> Date:   Sat Jul 10 23:51:39 2010 +0200
>>
>>    drm: kill BKL from common code
>>
>>
>> moved the call to (inside drm_release) drm_lastclose inside dev->count_lock
>> spinlock.
>> drm_lastclose however takes dev->struct_mutex (now inside an atomic
>> context):
>
> I have a patch from Chris Wilson that I need to push to fix this,
> basically reducing the spin lock coverage,
> and relying on the global mutex to handle the open race.

Ok, thank you :)

Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: question regarding nvc0_instmem_suspend()

[Luca Tettamanti]

On Fri, Aug 13, 2010 at 11:39 PM, Dan Carpenter  wrote:
> Smatch thinks there is a buffer overflow in nvc0_instmem_suspend() and
> I've looked at it, but I don't understand the code.
>
> drivers/gpu/drm/nouveau/nvc0_instmem.c +152 nvc0_instmem_suspend(10)
>        error: buffer overflow 'dev_priv->susres.ramin_copy' 16384 <= 1835008
>
>   141  int
>   142  nvc0_instmem_suspend(struct drm_device *dev)
>   143  {
>   144          struct drm_nouveau_private *dev_priv = dev->dev_private;
>   145          int i;
>   146
>   147          dev_priv->susres.ramin_copy = vmalloc(65536);
>
>        dev_priv->susres.ramin_copy is an array of 16384 u32 elements
>        (65536 bytes).
>
>   148          if (!dev_priv->susres.ramin_copy)
>   149                  return -ENOMEM;
>   150
>   151          for (i = 0x70; i < 0x71; i += 4)
>   152                  dev_priv->susres.ramin_copy[i/4] = nv_rd32(dev, i);
>
>        0x70 / 4 is 1835008 so we're way past the end of the array
>        and then we get larger.

I guess that it should be something like:

base = 0x70;
for (i = 0; i < 0x1; i += 4)
dev_priv->susres.ramin_copy[i/4] = nv_rd32(dev, base + i);


Luca
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 24/24] drm/radeon: add faulty command buffer dump facilities

[Luca Tettamanti]

Hi Jerome,

On Wed, Apr 25, 2012 at 9:03 PM,   wrote:
> From: Jerome Glisse 
>
> This add a command buffer dumping facilities, that will
> dump command buffer and all associated bo that most likely
> triggered a lockup.
[cut]

I spotted this:

> +void radeon_fence_blob_faulty_ib(struct radeon_device *rdev, int ring)
> +{
> + ? ? ? struct radeon_fence *fence;
> + ? ? ? struct list_head *i;
> + ? ? ? unsigned long irq_flags;
> + ? ? ? uint32_t seq;
> +
> + ? ? ? write_lock_irqsave(&rdev->fence_lock, irq_flags);
> + ? ? ? seq = radeon_fence_read(rdev, ring);
> + ? ? ? list_for_each(i, &rdev->fence_drv[ring].emitted) {
> + ? ? ? ? ? ? ? fence = list_entry(i, struct radeon_fence, list);
> + ? ? ? ? ? ? ? if (fence->seq != seq && fence->ib) {
> + ? ? ? ? ? ? ? ? ? ? ? radeon_lockup_build_blob(rdev, fence->ib);

radeon_lockup_build_blob() will take a mutex and call vmalloc() inside
an atomic context.

Luca

Linux 3.4-rc4

[Luca Tettamanti]

On Mon, Apr 30, 2012 at 11:07 AM, Maarten Maathuis  
wrote:
> On Mon, Apr 30, 2012 at 12:37 AM, Dmitry Torokhov
>  wrote:
>> On Sat, Apr 28, 2012 at 11:33:50AM -0400, Nick Bowler wrote:
>>> On 2012-04-28 02:19 -0400, Alex Deucher wrote:
>>> > On Fri, Apr 27, 2012 at 8:39 PM, Nick Bowler >> > elliptictech.com> wrote:
>>> > > Hi Ben,
>>> > >
>>> > > On 2012-04-27 15:20 +1000, Ben Skeggs wrote:
>>> > >> Does this patch help you at all?
>>> > >>
>>> > >> http://cgit.freedesktop.org/nouveau/linux-2.6/commit/?id=a3a285f17867f0018de798b5ee85731ec1268305
>>> > >
>>> > > Yes. ?I cherry-picked this patch on top of Linus' master (3.4-rc4+) and
>>> > > this appears to solve the "black screen on VGA" problem described in the
>>> > > original report. ?Thanks!
>>> > >
>>> > > Unfortunately, that's not the end of my VGA-related regressions. :(
>>> > >
>>> > > While tracking down the black screen issue, I've been having the monitor
>>> > > directly connected to the video card the whole time, but now when I'm
>>> > > connected through my KVM switch (an IOGear GCS1804), it appears that
>>> > > something's going wrong with reading the EDID, because the available
>>> > > modes are all screwed up (both console and X decide they want to drive
>>> > > the display at 1024x768). ?Here's the output of xrandr on 3.2.15:
>>> > >
>>> > > ?% xrandr
>>> > > ?Screen 1: minimum 320 x 200, current 1600 x 1200, maximum 4096 x 4096
>>> > > ?VGA-1 connected 1600x1200+0+0 (normal left inverted right x axis y 
>>> > > axis) 352mm x 264mm
>>> > > ? ? 1600x1200 ? ? ?75.0*+ ? 70.0 ? ? 65.0 ? ? 60.0
>>> > > ? ? 1280x1024 ? ? ?85.0 + ? 75.0 ? ? 60.0
>>> > > ? ? 1920x1440 ? ? ?60.0
>>> > > ? ? 1856x1392 ? ? ?60.0
>>> > > ? ? 1792x1344 ? ? ?60.0
>>> > > ? ? 1920x1200 ? ? ?74.9 ? ? 59.9
>>> > > ? ? 1680x1050 ? ? ?84.9 ? ? 74.9 ? ? 60.0
>>> > > ? ? 1400x1050 ? ? ?85.0 ? ? 74.9 ? ? 60.0
>>> > > ? ? 1440x900 ? ? ? 84.8 ? ? 75.0 ? ? 59.9
>>> > > ? ? 1280x960 ? ? ? 85.0 ? ? 60.0
>>> > > ? ? 1360x768 ? ? ? 60.0
>>> > > ? ? 1280x800 ? ? ? 84.9 ? ? 74.9 ? ? 59.8
>>> > > ? ? 1152x864 ? ? ? 75.0
>>> > > ? ? 1280x768 ? ? ? 84.8 ? ? 74.9 ? ? 59.9
>>> > > ? ? 1024x768 ? ? ? 85.0 ? ? 75.1 ? ? 75.0 ? ? 70.1 ? ? 60.0 ? ? 43.5 ? 
>>> > > ? 43.5
>>> > > ? ? 832x624 ? ? ? ?74.6
>>> > > ? ? 800x600 ? ? ? ?85.1 ? ? 72.2 ? ? 75.0 ? ? 60.3 ? ? 56.2
>>> > > ? ? 848x480 ? ? ? ?60.0
>>> > > ? ? 640x480 ? ? ? ?85.0 ? ? 75.0 ? ? 72.8 ? ? 72.8 ? ? 66.7 ? ? 60.0 ? 
>>> > > ? 59.9
>>> > > ? ? 720x400 ? ? ? ?85.0 ? ? 87.8 ? ? 70.1
>>> > > ? ? 640x400 ? ? ? ?85.1
>>> > > ? ? 640x350 ? ? ? ?85.1
>>> > > ? ? 320x200 ? ? ? 165.1
>>> > >
>>> > > And on 3.4-rc4+ (with your patch cherry-picked):
>>> > >
>>> > > ?% xrandr
>>> > > ?Screen 1: minimum 320 x 200, current 1024 x 768, maximum 4096 x 4096
>>> > > ?VGA-1 connected 1024x768+0+0 (normal left inverted right x axis y 
>>> > > axis) 0mm x 0mm
>>> > > ? ? 1024x768 ? ? ? 60.0*
>>> > > ? ? 800x600 ? ? ? ?60.3 ? ? 56.2
>>> > > ? ? 848x480 ? ? ? ?60.0
>>> > > ? ? 640x480 ? ? ? ?59.9
>>> > > ? ? 320x200 ? ? ? 165.1
>>> > >
>>> > > Running xrandr on 3.4-rc4+ also causes the screen to go black for a
>>> > > second when it does not on 3.2.15. ?It also causes several messages of
>>> > > the form
>>> > >
>>> > > ?[drm] nouveau :01:00.0: Load detected on output B
>>> > >
>>> > > to be logged. ?Also, looking at /sys/class/drm/card0-VGA-1/edid I see
>>> > > that it is empty on 3.4-rc4+ and it is correct on 3.2.15. ?Things seem
>>> > > to work OK when the KVM is not involved.
>>> >
>>> > Were you ever able to fetch a EDID with the KVM involved? ?KVMs are
>>> > notorious for not connecting the ddc pins.
>>>
>>> Yes, it works on 3.2.15 as described above.
>>
>> I have the same (or similar) KVM (not in the office at the moment) and I
>> can confirm that with newer kernels EDID fecthing in flaky. It's 50/50
>> if EDED retrieval succeeds or if it fails with:
>>
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.936336] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.955317] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 26 13:06:57 dtor-d630 kernel: [13464.973879] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.087659] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.107147] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.126908] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.146277] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [44602.297659] [drm:drm_edid_block_valid] 
>> *ERROR* EDID checksum is invalid, remainder is 208
>> Apr 27 09:13:03 dtor-d630 kernel: [4460

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
> Patches look good.  I picked them up and combined them with may
> patches plus a few other small fixes.  They are available here:
> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
> Let me know what you think.

Looks ok, I lost one fix along the road though, I'm attaching the patch.

Luca
-- next part --
>From 0f71d5b56b9e5eee3194b5b926767511281ea0a6 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Wed, 1 Aug 2012 10:53:19 +0200
Subject: [PATCH] drm/radeon: fix, enable notifications with device specific
 command code

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 14ae8aa..a812b9a 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -214,6 +214,7 @@ static int radeon_atif_get_notification_params(acpi_handle 
handle,
err = -EINVAL;
goto out;
}
+   n->enabled = true;
n->command_code = params.command_code;
}

-- 
1.7.10.4

[PATCH/RFC] drm/radeon: ACPI: veto the keypress on ATIF events

[Luca Tettamanti]

AMD ACPI interface may overload the standard event
ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
userspace because the user did not press the mode switch key (the
spurious keypress confuses the DE which usually changes the
display configuration and messes up a dual-screen setup).
This patch gives the radeon driver the chance to examine the event and
block the keypress if the event is an "AMD event".

Signed-off-by: Luca Tettamanti 
---
Any comment from ACPI front?

 drivers/acpi/video.c |   10 --
 drivers/gpu/drm/radeon/radeon_acpi.c |7 ++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..a8592c4 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1457,7 +1457,12 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
acpi_video_device_enumerate(video);
acpi_video_device_rebind(video);
acpi_bus_generate_proc_event(device, event, 0);
-   keycode = KEY_SWITCHVIDEOMODE;
+   /* This event is also overloaded by AMD ACPI interface, don't
+* send the key press if the event has been handled elsewhere
+* (e.g. radeon DRM driver).
+*/
+   if (!acpi_notifier_call_chain(device, event, 0))
+   keycode = KEY_SWITCHVIDEOMODE;
break;

case ACPI_VIDEO_NOTIFY_CYCLE:   /* Cycle Display output hotkey pressed. 
*/
@@ -1479,7 +1484,8 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
break;
}

-   if (event != ACPI_VIDEO_NOTIFY_SWITCH)
+   if (event != ACPI_VIDEO_NOTIFY_SWITCH &&
+   event != ACPI_VIDEO_NOTIFY_PROBE)
acpi_notifier_call_chain(device, event, 0);

if (keycode) {
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 96de08d..ee0d29e 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev,
}
/* TODO: check other events */

-   return NOTIFY_OK;
+   /* We've handled the event, stop the notifier chain. The ACPI interface
+* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to
+* userspace if the event was generated only to signal a SBIOS
+* request.
+*/
+   return NOTIFY_BAD;
 }

 /* Call all ACPI methods here */
-- 
1.7.10.4

[PATCH/RFC] drm/radeon: ACPI: veto the keypress on ATIF events

[Luca Tettamanti]

On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
> On ?, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
> > AMD ACPI interface may overload the standard event
> > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
> > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
> > userspace because the user did not press the mode switch key (the
> > spurious keypress confuses the DE which usually changes the
> > display configuration and messes up a dual-screen setup).
> > This patch gives the radeon driver the chance to examine the event and
> > block the keypress if the event is an "AMD event".
> > 
> > Signed-off-by: Luca Tettamanti 
> > ---
> > Any comment from ACPI front?
> > 
> it looks good to me.
> But I'm wondering if we can use the following code for ACPI part, which
> looks cleaner.
> I know this may change the behavior of other events, but in theory, we
> should not send any input event if we know something wrong in kernel.
> 
> what do you think?

I like it, it's cleaner.
I've split the patch in two pieces (one for video, the other for
radeon) and adopted your suggestion.

BTW, I'm leaving for vacation in a few hours, I'll be offline till mid
August.

Luca
-- next part --
>From acce30c96b90d1bc550e82a9e7f19226fa194d5e Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Thu, 2 Aug 2012 15:30:27 +0200
Subject: [PATCH 1/2] ACPI video: allow events handlers to veto the keypress

The standard video events may be overloaded for device specific
purposes. For example AMD ACPI interface overloads
ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
userspace because the user did not press the mode switch key (the
spurious keypress confuses the DE which usually changes the
display configuration and messes up a dual-screen setup).
This patch gives the handlers the chance to examine the event and
block the keypress if the event is device specific.
v2: refactor as suggested by Zhang Rui 

Signed-off-by: Luca Tettamanti 
---
 drivers/acpi/video.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..d75642a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1448,8 +1448,7 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
case ACPI_VIDEO_NOTIFY_SWITCH:  /* User requested a switch,
 * most likely via hotkey. */
acpi_bus_generate_proc_event(device, event, 0);
-   if (!acpi_notifier_call_chain(device, event, 0))
-   keycode = KEY_SWITCHVIDEOMODE;
+   keycode = KEY_SWITCHVIDEOMODE;
break;

case ACPI_VIDEO_NOTIFY_PROBE:   /* User plugged in or removed a video
@@ -1479,8 +1478,9 @@ static void acpi_video_bus_notify(struct acpi_device 
*device, u32 event)
break;
}

-   if (event != ACPI_VIDEO_NOTIFY_SWITCH)
-   acpi_notifier_call_chain(device, event, 0);
+   if (acpi_notifier_call_chain(device, event, 0))
+   /* Something vetoed the keypress. */
+   keycode = 0;

if (keycode) {
input_report_key(input, keycode, 1);
-- 
1.7.10.4

-- next part --
>From def5119d8f617eef0fac2cd35d7bb18c176ff8f6 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Thu, 2 Aug 2012 15:33:03 +0200
Subject: [PATCH 2/2] drm/radeon: block the keypress on ATIF events

The AMD ACPI interface may use ACPI_VIDEO_NOTIFY_PROBE to signal SBIOS
requests; block the keypress in this case since the user did not
actually press the mode switch key.

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 96de08d..ee0d29e 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev,
}
/* TODO: check other events */

-   return NOTIFY_OK;
+   /* We've handled the event, stop the notifier chain. The ACPI interface
+* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to
+* userspace if the event was generated only to signal a SBIOS
+* request.
+*/
+   return NOTIFY_BAD;
 }

 /* Call all ACPI methods here */
-- 
1.7.10.4

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher  wrote:
> I admit I'm not really an ACPI expert, but thinking about this more,
> I'm wondering if maybe we should just send the appropriate brightness
> change, switch display, etc. event to userspace rather than handling
> it directly in the radeon driver, then let userspace callback down via
> the bl interface, etc.  With backlight for example, does handling it
> in the kernel driver as per your patch prevent userspace from seeing
> the brightness up/down event?  Wouldn't that break things like OSD
> brightness displays and such?

No, the event is sent to userspace by the standard ACPI video driver,
it works as before.
Changing brightness usually goes like this:
1) user presses a hotkey
2) a notification is generated (0x86 or 0x87)
3) video.ko handles the notification and calls into ACPI to change the
level (_BCM) and firmware does its magic
4) a key press (brightness up/down) is sent to userspace

With ATIF step 3 does not actually change the brightness, it just send
out another event (VIDEO_PROBE, or one of the device specific ones) so
we need to take care of that too. The rest of the process, including
the delivery of the key presses, stays the same.

Luca

radeon testing

[Luca Tettamanti]

On Thu, Aug 16, 2012 at 11:51:10AM -0400, Alex Deucher wrote:
> I've gathered up the various outstanding radeon patches and put them
> up in a tree for testing:
> http://cgit.freedesktop.org/~agd5f/linux/log/?h=drm-next-3.7-wip
> Some of these may end up in -fixes, but most of them are -next
> material.  I haven't had a chance to go through Christian's last set
> of VM patches yet.
> 
> Highlights:
> - rom fetch fixes (UEFI, thunderbolt docks)
> - major ACPI rework
> - backlight control for newer asics

Hi Alex,
I just tested the rebased acpi_patches branch: ACPI part works fine, but
I see a big slow down during radeon driver initialization when the
screen goes black for a couple of seconds (with 3.5.0 + acpi patches the
screen just flickers during boot). With initcall_debug I see:

[2.355300] calling  radeon_init+0x0/0x1000 [radeon] @ 552
...
[5.530310] initcall radeon_init+0x0/0x1000 [radeon] returned 0 after 
3102147 usecs

For reference 3.5 takes ~1 sec. With drm.debug=2 the log (attached) is not very
informative, I'll try and get more info...

Luca
-- next part --
[0.00] BIOS-e820: [mem 0xaa99f000-0xaf2befff] usable
[0.00] BIOS-e820: [mem 0xaf2bf000-0xaf6befff] reserved
[0.00] BIOS-e820: [mem 0xaf6bf000-0xaf7befff] ACPI NVS
[0.00] BIOS-e820: [mem 0xaf7bf000-0xaf7fefff] ACPI data
[0.00] BIOS-e820: [mem 0xaf7ff000-0xaf7f] usable
[0.00] BIOS-e820: [mem 0xaf80-0xafff] reserved
[0.00] BIOS-e820: [mem 0xe000-0xefff] reserved
[0.00] BIOS-e820: [mem 0xfeb0-0xfeb03fff] reserved
[0.00] BIOS-e820: [mem 0xfec0-0xfec00fff] reserved
[0.00] BIOS-e820: [mem 0xfed1-0xfed19fff] reserved
[0.00] BIOS-e820: [mem 0xfed1c000-0xfed1] reserved
[0.00] BIOS-e820: [mem 0xfee0-0xfee00fff] reserved
[0.00] BIOS-e820: [mem 0xffd8-0x] reserved
[0.00] BIOS-e820: [mem 0x0001-0x0001ceff] usable
[0.00] NX (Execute Disable) protection: active
[0.00] DMI 2.7 present.
[0.00] DMI: TOSHIBA SATELLITE L855/Portable PC, BIOS 1.60 04/20/2012
[0.00] e820: update [mem 0x-0x] usable ==> reserved
[0.00] e820: remove [mem 0x000a-0x000f] usable
[0.00] No AGP bridge found
[0.00] e820: last_pfn = 0x1cf000 max_arch_pfn = 0x4
[0.00] MTRR default type: uncachable
[0.00] MTRR fixed ranges enabled:
[0.00]   0-9 write-back
[0.00]   A-B uncachable
[0.00]   C-E7FFF write-protect
[0.00]   E8000-E write-combining
[0.00]   F-F write-protect
[0.00] MTRR variable ranges enabled:
[0.00]   0 base 0 mask F8000 write-back
[0.00]   1 base 08000 mask FC000 write-back
[0.00]   2 base 0AF80 mask FFF80 uncachable
[0.00]   3 base 0B000 mask FF000 uncachable
[0.00]   4 base 0FFC0 mask FFFC0 write-protect
[0.00]   5 base 1 mask F write-back
[0.00]   6 base 1CF00 mask FFF00 uncachable
[0.00]   7 base 1D000 mask FF000 uncachable
[0.00]   8 base 1E000 mask FE000 uncachable
[0.00]   9 disabled
[0.00] x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106
[0.00] e820: last_pfn = 0xaf800 max_arch_pfn = 0x4
[0.00] found SMP MP-table at [mem 0x000fe1b0-0x000fe1bf] mapped at 
[880fe1b0]
[0.00] initial memory mapped: [mem 0x-0x1fff]
[0.00] Base memory trampoline at [88097000] 97000 size 24576
[0.00] init_memory_mapping: [mem 0x-0xaf7f]
[0.00]  [mem 0x-0xaf7f] page 2M
[0.00] kernel direct mapping tables up to 0xaf7f @ [mem 
0x1fa8-0x1fff]
[0.00] init_memory_mapping: [mem 0x1-0x1ceff]
[0.00]  [mem 0x1-0x1ceff] page 2M
[0.00] kernel direct mapping tables up to 0x1ceff @ [mem 
0xaf2b6000-0xaf2befff]
[0.00] RAMDISK: [mem 0x3763c000-0x37b15fff]
[0.00] ACPI: RSDP 000fe020 00024 (v02 TOSINV)
[0.00] ACPI: XSDT af7fe210 0009C (v01 TOSINV TOSINV00 0001  
0113)
[0.00] ACPI: FACP af7fb000 0010C (v05 TOSINV TOSINV00 0001 
ACPI 0004)
[0.00] ACPI: DSDT af7f 07F66 (v01 TOSINV TOSINV00  
ACPI 0004)
[0.00] ACPI: FACS af7bb000 00040
[0.00] ACPI: UEFI af7fd000 00236 (v01 TOSINV TOSINV00 0001 
ACPI 0004)
[0.00] ACPI: ASF! af7fc000 000A5 (v32 TOSINV TOSINV00 0001 
ACPI 0004)
[0.00] A

radeon testing

[Luca Tettamanti]

On Mon, Aug 20, 2012 at 10:24:01AM -0400, Alex Deucher wrote:
> > I just tested the rebased acpi_patches branch: ACPI part works fine, but
> > I see a big slow down during radeon driver initialization when the
> > screen goes black for a couple of seconds (with 3.5.0 + acpi patches the
> > screen just flickers during boot). With initcall_debug I see:
> >
> > [2.355300] calling  radeon_init+0x0/0x1000 [radeon] @ 552
> > ...
> > [5.530310] initcall radeon_init+0x0/0x1000 [radeon] returned 0 after 
> > 3102147 usecs
> >
> > For reference 3.5 takes ~1 sec. With drm.debug=2 the log (attached) is not 
> > very
> > informative, I'll try and get more info...
> 
> Can you bisect?

I've put in some printk, this is the result:

[2.403138] evergreen_mc_program
[2.403196] evergreen_mc_stop
...
[4.268491] evergreen_mc_resume done
[4.268548] evergreen_mc_program done

This is the patch:

--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1349,6 +1349,8 @@ void evergreen_mc_program(struct radeon_device *rdev)
u32 tmp;
int i, j;

+   printk(KERN_INFO "%s\n", __func__);
+
/* Initialize HDP */
for (i = 0, j = 0; i < 32; i++, j += 0x18) {
WREG32((0x2c14 + j), 0x);
@@ -1359,10 +1361,14 @@ void evergreen_mc_program(struct radeon_device *rdev)
}
WREG32(HDP_REG_COHERENCY_FLUSH_CNTL, 0);

+   printk(KERN_INFO "evergreen_mc_stop\n");
evergreen_mc_stop(rdev, &save);
+// printk(KERN_INFO "evergreen_mc_wait_for_idle\n");
if (evergreen_mc_wait_for_idle(rdev)) {
dev_warn(rdev->dev, "Wait for MC idle timedout !\n");
}
+// printk(KERN_INFO "evergreen_mc_wait_for_idle done\n");
+
/* Lockout access through VGA aperture*/
WREG32(VGA_HDP_CONTROL, VGA_MEMORY_DISABLE);
/* Update configuration */
@@ -1411,10 +1417,13 @@ void evergreen_mc_program(struct radeon_device *rdev)
WREG32(MC_VM_AGP_TOP, 0x0FFF);
WREG32(MC_VM_AGP_BOT, 0x0FFF);
}
+// printk(KERN_INFO "evergreen_mc_wait_for_idle\n");
if (evergreen_mc_wait_for_idle(rdev)) {
dev_warn(rdev->dev, "Wait for MC idle timedout !\n");
}
+// printk(KERN_INFO "evergreen_mc_resume\n");
evergreen_mc_resume(rdev, &save);
+   printk(KERN_INFO "evergreen_mc_resume done\n");
/* we need to own VRAM, so turn off the VGA renderer here
 * to stop it overwriting our objects */
rv515_vga_render_disable(rdev);


Any printk between evergreen_mc_stop and evergreen_mc_resume locks up
the machine. The likely culprit is commit 023e188e:

commit 023e188ec102330eb05ad264f675e869637478eb
Author: Alex Deucher 
Date:   Wed Aug 15 17:18:42 2012 -0400

drm/radeon: properly handle mc_stop/mc_resume on evergreen+

- Stop the displays from accessing the FB
- Block CPU access
- Turn off MC client access

This should fix issues some users have seen, especially
with UEFI, when changing the MC FB location that result
in hangs or display corruption.



I haven't tried backing out the commit yet, but looking at the diff I
see that you call radeon_wait_for_vblank and radeon_get_vblank_counter,
but evergreen_mc_program is called way before IRQ is set up. Is the
vblank counter running? Looks like we just hitting the timeout here...




Luca

radeon testing

[Luca Tettamanti]

On Tue, Aug 21, 2012 at 09:51:46AM -0400, Alex Deucher wrote:
> On Mon, Aug 20, 2012 at 3:30 PM, Luca Tettamanti  
> wrote:
> > Any printk between evergreen_mc_stop and evergreen_mc_resume locks up
> > the machine. The likely culprit is commit 023e188e:
> 
> yeah, vram is locked out at that point.  I guess we probably need to
> block anyone from trying to access it.

I see; the 2 dev_warn would probably lock up the machine as well right?

> > I haven't tried backing out the commit yet, but looking at the diff I
> > see that you call radeon_wait_for_vblank and radeon_get_vblank_counter,
> > but evergreen_mc_program is called way before IRQ is set up. Is the
> > vblank counter running? Looks like we just hitting the timeout here...
> 
> We aren't waiting for an interrupt, just polling the current crtc
> status until it enters the vblank region.  The status and counters
> should be working as we only wait on displays that are enabled.

It appears that all the crtcs are considered active:

[4.260766] crtc 0 enabled 272696081 (this is the value of crtc_enabled)
[4.260766] crtc 0 wait for vblank 0x1 (0x1 means no timeout)
[4.260766] crtc 0: waited 33 [10] (number of loops of 
radeon_get_vblank_counter)
[4.260766] crtc 1 enabled 272630544
[4.260766] crtc 1 wait for vblank 0x1
[4.260766] crtc 1: waited 10 [10]
[4.260766] crtc 2 enabled 4195088
[4.260766] crtc 2 wait for vblank 0x1
[4.260766] crtc 2: waited 10 [10]
[4.260766] crtc 3 enabled 4195088
[4.260766] crtc 3 wait for vblank 0x1
[4.260766] crtc 3: waited 10 [10]
[4.260766] crtc 4 enabled 4195088
[4.260766] crtc 4 wait for vblank 0x1
[4.260766] crtc 4: waited 10 [10]
[4.260766] crtc 5 enabled 4195088
[4.260766] crtc 5 wait for vblank 0x1
[4.260766] crtc 5: waited 10 [10]

Maybe the code should be checking EVERGREEN_CRTC_MASTER_EN?

I'm testing this patch and the boot is fast again:

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index 2308c7d..72bf721 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1251,7 +1251,8 @@ void evergreen_mc_stop(struct radeon_device *rdev, struct 
evergreen_mc_save *sav
WREG32(VGA_RENDER_CONTROL, 0);
/* blank the display controllers */
for (i = 0; i < rdev->num_crtc; i++) {
-   crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL + crtc_offsets[i]);
+   crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL + crtc_offsets[i]) 
&
+   EVERGREEN_CRTC_MASTER_EN;
if (crtc_enabled) {
save->crtc_enabled[i] = true;
if (ASIC_IS_DCE6(rdev)) {


Luca

[DRM] BUG: sleeping function called from invalid context, drm_lastclose

[Luca Tettamanti]

On Wed, Aug 11, 2010 at 10:50 AM, Dave Airlie  wrote:
> On Wed, Aug 11, 2010 at 6:48 PM, Luca Tettamanti  
> wrote:
>> Hi Arnd,
>> this commit:
>>
>> commit 58374713c9dfb4d231f8c56cac089f6fbdedc2ec
>> Author: Arnd Bergmann 
>> Date: ? Sat Jul 10 23:51:39 2010 +0200
>>
>> ? ?drm: kill BKL from common code
>>
>>
>> moved the call to (inside drm_release) drm_lastclose inside dev->count_lock
>> spinlock.
>> drm_lastclose however takes dev->struct_mutex (now inside an atomic
>> context):
>
> I have a patch from Chris Wilson that I need to push to fix this,
> basically reducing the spin lock coverage,
> and relying on the global mutex to handle the open race.

Ok, thank you :)

Luca

question regarding nvc0_instmem_suspend()

[Luca Tettamanti]

On Fri, Aug 13, 2010 at 11:39 PM, Dan Carpenter  wrote:
> Smatch thinks there is a buffer overflow in nvc0_instmem_suspend() and
> I've looked at it, but I don't understand the code.
>
> drivers/gpu/drm/nouveau/nvc0_instmem.c +152 nvc0_instmem_suspend(10)
> ? ? ? ?error: buffer overflow 'dev_priv->susres.ramin_copy' 16384 <= 1835008
>
> ? 141 ?int
> ? 142 ?nvc0_instmem_suspend(struct drm_device *dev)
> ? 143 ?{
> ? 144 ? ? ? ? ?struct drm_nouveau_private *dev_priv = dev->dev_private;
> ? 145 ? ? ? ? ?int i;
> ? 146
> ? 147 ? ? ? ? ?dev_priv->susres.ramin_copy = vmalloc(65536);
>
> ? ? ? ?dev_priv->susres.ramin_copy is an array of 16384 u32 elements
> ? ? ? ?(65536 bytes).
>
> ? 148 ? ? ? ? ?if (!dev_priv->susres.ramin_copy)
> ? 149 ? ? ? ? ? ? ? ? ?return -ENOMEM;
> ? 150
> ? 151 ? ? ? ? ?for (i = 0x70; i < 0x71; i += 4)
> ? 152 ? ? ? ? ? ? ? ? ?dev_priv->susres.ramin_copy[i/4] = nv_rd32(dev, i);
>
> ? ? ? ?0x70 / 4 is 1835008 so we're way past the end of the array
> ? ? ? ?and then we get larger.

I guess that it should be something like:

base = 0x70;
for (i = 0; i < 0x1; i += 4)
dev_priv->susres.ramin_copy[i/4] = nv_rd32(dev, base + i);


Luca

[PATCH] drm/radeon/kms: disable ss fixed ref divide

[Luca Tettamanti]

On Mon, Dec 13, 2010 at 5:27 AM, Alex Deucher  wrote:
> Seems to cause problems on certain laptops
>
> Fixes:
> https://bugzilla.kernel.org/show_bug.cgi?id=24462
>
> Signed-off-by: Alex Deucher 
Tested-by: Luca Tettamanti 

> ---
> ?drivers/gpu/drm/radeon/atombios_crtc.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c 
> b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 4d2eb92..6a4f090 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -554,7 +554,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dp_clock = dig_connector->dp_clock;
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ?}
> -
> +#if 0 /* doesn't work properly on some laptops */
> ? ? ? ? ? ? ? ? ? ? ? ?/* use recommended ref_div for ss */
> ? ? ? ? ? ? ? ? ? ? ? ?if (radeon_encoder->devices & 
> (ATOM_DEVICE_LCD_SUPPORT)) {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (ss_enabled) {
> @@ -564,7 +564,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ?}
> -
> +#endif
> ? ? ? ? ? ? ? ? ? ? ? ?if (ASIC_IS_AVIVO(rdev)) {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* DVO wants 2x pixel clock if the DVO chip is 
> in 12 bit mode */
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (radeon_encoder->encoder_id == 
> ENCODER_OBJECT_ID_INTERNAL_KLDSCP_DVO1)
> --
> 1.7.1.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

AMD GPU: Toshiba, brightness and ATIF

[Luca Tettamanti]

Hello,
I'm trying to figure out how to control the brightness of the screen of my
laptop, a Toshiba L885-10W.

The ACPI DSDT table contains the standard control methods, the setter (_BCM),
however, only sets a value in the record returned by ATIF method and send out a
notification:

CreateByteField (ATIB, 0x0C, BRIL) // ATIB is returned by ATIF
Store (Arg0, BRIL)
Notify (VGA, 0x81)

When using the ACPI sysfs backlight interface nothing happens (obviously).

0x81 seems also strange, according to the specification is

  "Output Device Status Change: Used to notify OSPM whenever the state of any
  output devices attached to the VGA controller has been changed. This event
  will, for example, be generated when the user plugs-in or remove a CRT from
  the VGA port. In this case, OSPM will re-enumerate all devices attached to
  VGA"

and confuses KDE when using more than one screen (but this is a
different issue).

I suspect that the notification means that the radeon driver is supposed to
change the brightness; the GPU is a:

01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI 
Thames XT/GL [Radeon HD 7600M Series] [1002:6840]

kernel drivers says:
[drm] initializing kernel modesetting (TURKS 0x1002:0x6840 0x1179:0xFB22)

Poking around with avivotools it seems that AVIVO_LVDS_BACKLIGHT_CNTL (0x7af8)
is not present anymore...

Furthermore ATIF returns a different structure based on the parameter passed by
the caller; radeon drm driver always passes "1", the BRIL field is mentioned
only when passing "2" (it shouldn't matter, the field is set unconditionally,
but it seems some king of versioning); it seems a versioned data structure...

Do you have any information that you can share about ATIF and brightness
control on modern GPUs?

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Wed, Jul 25, 2012 at 01:38:09PM -0400, alexdeucher at gmail.com wrote:
> From: Alex Deucher 
> 
> Add a new header that defines the AMD ACPI interface used
> for laptops, PowerXpress, and chipset specific functionality
> and update the current code to use it.

Great! Now my DSDT makes sense ;)

> Todo:
> - properly verify the ACPI interfaces
> - hook up and handle ACPI notifications

I see a problem here, I've hacked up a standalone test module, but:

> +#define ATIF_FUNCTION_GET_SYSTEM_PARAMETERS0x1
[...]
> + * flags
> + * bits 1:0:
> + * 0 - Notify(VGA, 0x81) is not used for notification
> + * 1 - Notify(VGA, 0x81) is used for notification

My system has this bit set, and the brightness control method does send
the notification.
My module register itself with register_acpi_notifier and gets 2
notifications, one with ACPI_VIDEO_NOTIFY_PROBE (0x81) and the other
with ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS (or DEC, depending on what I
press).

The standard acpi "video" module, however, in response to
ACPI_VIDEO_NOTIFY_PROBE generates a key press sending
KEY_SWITCHVIDEOMODE.

This greatly confuses KDE which messes up my dual screen configuration;
I guess that the spurious KEY_SWITCHVIDEOMODE may be problematic also
with other DEs...

In more detail what happens is the following:
- I press the brightness hotkey, firmware generates a notification on
  the relevant device (_SB.PCI0.PEG0.VGA.LCD)
- ACPI video module gets the ACPI_VIDEO_NOTIFY_{DEC,INC}_BRIGHTNESS
  notification and tries to adjust the brightness with
  acpi_video_device_lcd_set_level, which in turns calls _BCM
- _BCM sets the relevant bits in the AMD-specific structure and does a
  Notify (VGA, 0x81)
- again ACPI video module gets the nodification (in this case
  ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the hypothetical
  radeon driver does its magic to adjust the screen.

My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the
acpi notifier chain, and allow the handlers to veto the key press (like
it's done for ACPI_VIDEO_NOTIFY_SWITCH).

Zhang Rui what do you think about this?

The other missing bit is how to actually change the brightness... Alex,
do you know what registers to poke?

My card is a:

01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI 
Thames XT/GL [Radeon HD 600M Series] [1002:6840]

on a Toshiba L855.

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
> On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti  
> wrote:
> > The other missing bit is how to actually change the brightness... Alex,
> > do you know what registers to poke?
> 
> You need to check if the GPU controls the backlight or the system
> does.  I think the attached patches should point you in the right
> direction.

Yep :)

0050:  ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability :
  0050:  (union) ATOM_FIRMWARE_CAPABILITY sbfAccess  :
USHORT GPUControlsBL:1  = 0x0001 (1)

The panel is using the INTERNAL_UNIPHY encoder, and I see the
UNIPHYTransmitterControl command table.

Interaction with video.ko is still a bit messy...

Do you already have code for handling the notifications? I'll work on it
in the weekend otherwise ;)

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Fri, Jul 27, 2012 at 12:46:50PM +0800, joeyli wrote:
> ? ??2012-07-26 ? 23:31 -0400?Alex Deucher ???
> > On Thu, Jul 26, 2012 at 10:50 PM, joeyli  wrote:
> > > ? ??2012-07-26 ? 14:58 +0200?Luca Tettamanti ???
> > >> - again ACPI video module gets the nodification (in this case
> > >>   ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
> > >> - KDE seems this and muck with the screen configuration :(
> > >> - meanwhile the brightness notification is propagated, the
> > >> hypothetical
> > >>   radeon driver does its magic to adjust the screen.
> > >>
> > >> My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to
> > >> the
> > >> acpi notifier chain, and allow the handlers to veto the key press
> > >> (like
> > >> it's done for ACPI_VIDEO_NOTIFY_SWITCH).
> > >
> > > I welcome this approach!
> > >
> > > On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when
> > > AC-power unplug, because BIOS want to nodify video driver do some power
> > > saving stuff.
> > > It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power
> > > unplug.
> > >
> > > At least acpi/video driver need to know this 0x81 event is filed by BIOS
> > > to radeon-acpi for notify but not do video mode switch. That means the
> > > radeon drm need take the video switch responsibility.
> > 
> > Probably we'd just want the radeon acpi handler to just forward the
> > events to userspace so that the user can choose what to do with it
> > (xrandr command, etc.), otherwise we'll need to define policy in the
> > driver.
> 
> Any kernel module can issue KEY_SWITCHVIDEOMODE to user space, then
> gnome-settings-daemon(on Gnome) and  krandr(on KDE) will call xrandr
> library to switch video mode.

Exacly, and if we have pending system bios requests then the event is
not a real ACPI_VIDEO_NOTIFY_PROBE and (IMHO) it should not be forwarded
to the userspace as such.
The "vanilla" ACPI_VIDEO_NOTIFY_PROBE is already forwared to userspace.

> The 0x81 ACPI event for acpi/video driver is ACPI_VIDEO_NOTIFY_PROBE,
> means need issue KEY_SWITCHVIDEOMODE. But, 0x81 for radeon is a general
> notification event.
> I didn't see probe state in GET_SYSTEM_BIOS_REQUESTS, how can we
> distinguish this 0x81 is a ACPI_VIDEO_NOTIFY_PROBE or a ATI general
> notification event?

+#define ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS 0x2
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS
+ * ARG1: none
+ * OUTPUT:
+ * WORD  - structure size in bytes (includes size field)
+ * DWORD - pending sbios requests
+ * BYTE  - panel expansion mode
+ * BYTE  - thermal state: target gfx controller
+ * BYTE  - thermal state: state id (0: exit state, non-0: state)
+ * BYTE  - forced power state: target gfx controller
+ * BYTE  - forced power state: state id
+ * BYTE  - system power source
+ * BYTE  - panel backlight level (0-255)

I guess that if "pending sbios requests" == 0 then the event is the
general purpose one, and is not handled by radeon driver.

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Thu, Jul 26, 2012 at 03:42:26PM -0400, Alex Deucher wrote:
> On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti  
> wrote:
> > On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
> >> On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti  
> >> wrote:
> >> > The other missing bit is how to actually change the brightness... Alex,
> >> > do you know what registers to poke?
> >>
> >> You need to check if the GPU controls the backlight or the system
> >> does.  I think the attached patches should point you in the right
> >> direction.
> >
> > Yep :)
> >
> > 0050:  ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability :
> >   0050:  (union) ATOM_FIRMWARE_CAPABILITY sbfAccess  :
> > USHORT GPUControlsBL:1  = 0x0001 (1)
> >
> > The panel is using the INTERNAL_UNIPHY encoder, and I see the
> > UNIPHYTransmitterControl command table.
> >
> > Interaction with video.ko is still a bit messy...
> >
> > Do you already have code for handling the notifications? I'll work on it
> > in the weekend otherwise ;)
> 
> I don't have patches for that.  Please feel free to work on it :)

I just found the first problem (probably a BIOS bug):
ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
I intended to use the method to set up the notification handler but now
my BIOS says that it's not there even if it is...
Can I assume some default values (e.g. notifications are enabled and will
use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
different)?

thanks,
Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti  
> wrote:
> > I just found the first problem (probably a BIOS bug):
> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > I intended to use the method to set up the notification handler but now
> > my BIOS says that it's not there even if it is...
> > Can I assume some default values (e.g. notifications are enabled and will
> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > different)?
> 
> The spec says that the bits in the supported functions vector mean
> that if bit n is set, function n+1 exists, 

Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?

Maybe if the bit n is set then functions 0..n are available? That would
(almost) match what I see...

> but it's possible that the
> spec is wrong and it's actually a 1 to 1 mapping; if bit n is set,
> function n is supported.  In which case the the supported functions
> vector bits should be:
> +/* supported functions vector */
> +#   define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED   (1 << 1)
> +#   define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED(1 << 2)
> +#   define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED  (1 << 3)
> +#   define ATIF_GET_LID_STATE_SUPPORTED   (1 << 4)
> +#   define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED   (1 << 5)
> +#   define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 6)
> +#   define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED  (1 << 7)
> +#   define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED(1 << 8)
> +#   define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 13)
> +#   define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED   (1 << 15)
> 
> See if that lines up better. 

Not really... the value returned by VERIFY_INTERFACE is 0x2, but in the
DSDT I see:

ATIF_FUNCTION_GET_SYSTEM_PARAMETERS
ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS
ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS
ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS

The implementation of the first one makes sense, the second is used for
brightness control. The other two _might_ be a leftover (the machine
does not have an analog TV out).

> I'm still new to these ACPI interfaces
> so I'm not an expert yet.

I've been exposed to a lot of ACPI code (I wrote the asus_atk0110
driver), in my experience the DSDT is full of crap: code copied&pasted
from other machines, leftover no longer used, and other stuff that's
plainly wrong.

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
> Hi Luca, 
> 
> ? ??2012-07-28 ? 16:56 +0200?Luca Tettamanti ???
> > I just found the first problem (probably a BIOS bug):
> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > I intended to use the method to set up the notification handler but now
> > my BIOS says that it's not there even if it is...
> > Can I assume some default values (e.g. notifications are enabled and will
> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > different)?
> > 
> 
> Did you check your DSDT for there have some "Notify (VGA, 0x81)"
> statement in AFN0..AFN15?
> If YES, I think that means your machine in case the 0x81 is for ATI used
> by default.

Yes, my point is that the nofication is there, but since
GET_SYSTEM_PARAMETERS is not announced I have not way to check it.
IOW, what is implemented in the DSDT does not match what is announced by
VERIFY_INTERFACE.
For reference this is the DSDT: http://pastebin.com/KKS7ZsTt

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

Hi,
I'm attaching a first draft of my work. The first 3 patches are
infrastructure work, the fourth wires the notification handler and
retrieves the requests from the system BIOS, but it does not actually
change brightness yet.

The problem here is how to get the correct encoder: should I just scan
encoder_list checking for ATOM_DEVICE_LCD_SUPPORT and see if it has a
backlight device attached?
Hopefully there is only one encoder with it, right?

I'm also toying with the idea of creating structures matching the output
of the various ACPI methods, this would remove some ugly pointer
arithmetics, but it _might_ make it easier to read past the buffer if
one does not check the size before using the struct. What do you think?

Luca
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-radeon-refactor-radeon_atif_call.patch
Type: text/x-diff
Size: 2233 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: 0002-drm-radeon-implement-radeon_atif_verify_interface.patch
Type: text/x-diff
Size: 5331 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: 0003-drm-radeon-implement-wrapper-for-GET_SYSTEM_PARAMS.patch
Type: text/x-diff
Size: 3101 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: 0004-drm-radeon-implement-skeleton-handler-for-ACPI-event.patch
Type: text/x-diff
Size: 3696 bytes
Desc: not available
URL: 

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 04:32:47PM +0800, joeyli wrote:
> ? ??2012-07-29 ? 15:10 +0200?Luca Tettamanti ???
> > On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
> > > Hi Luca, 
> > > 
> > > ? ??2012-07-28 ? 16:56 +0200?Luca Tettamanti ???
> > > > I just found the first problem (probably a BIOS bug):
> > > > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> > > > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> > > > I intended to use the method to set up the notification handler but now
> > > > my BIOS says that it's not there even if it is...
> > > > Can I assume some default values (e.g. notifications are enabled and 
> > > > will
> > > > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> > > > different)?
> > > > 
> > > 
> > > Did you check your DSDT for there have some "Notify (VGA, 0x81)"
> > > statement in AFN0..AFN15?
> > > If YES, I think that means your machine in case the 0x81 is for ATI used
> > > by default.
> > 
> > Yes, my point is that the nofication is there, but since
> > GET_SYSTEM_PARAMETERS is not announced I have not way to check it.
> > IOW, what is implemented in the DSDT does not match what is announced by
> > VERIFY_INTERFACE.
> > For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
> > 
> > Luca
> > 
> 
> Yes, saw the problem in your DSDT:
> 
> Method (AF00, 0, NotSerialized)
> {
> CreateWordField (ATIB, Zero, SSZE)
>   ...
> Store (0x80, NMSK)
> Store (0x02, SFUN)<=== 10b, bit 0 is 0
> Return (ATIB)
> }
> 
> But, AF01 still supported in ATIF on this machine, maybe we should still
> try GET_SYSTEM_PARAMETERS even the function vector set to 0?
> No idea...

That's what I'm doing right now... if SBIOS_REQUESTS is supported I try
and call GET_SYSTEM_PARAMETERS even if it's not announced.

> On the other hand, 
> My patch to avoid 0x81 event conflict with acpi/video driver like below.
> This patch woks on my notebook. Your patches do much more things then
> mine, so I think my patch just for reference.  

I ignored the event handling for now... I'd like to hear something back
from ACPI camp before committing to this solution.

> There have a problem is:
> If we want use acpi_notifier_call_chain to check does event consume by
> any notifier in acpi's blocking notifier chain, then we need return
> NOTIFY_BAD in radeon_acpi but not NOTIFY_OK.
> 
> So, I suggest radeon_acpi should register to acpi notifier chain by
> itself but not append on radeon_acpi_event in radeon_pm. 

It shouldn't matter, once I change radeon_atif_handler to return
NOTIFY_BAD the call chain will be stopped anyway.

> And,
> suggest also check the device class is ACPI_VIDEO_CLASS like following:
> 
> +static int radeon_acpi_video_event(struct notifier_block *nb,
> ...
> + if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
> + return NOTIFY_DONE;
> +

Will do. I'll use the package structs in the next iteration.

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
> On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti  
> wrote:
> > On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
> >> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti  
> >> wrote:
> >> > I just found the first problem (probably a BIOS bug):
> >> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> >> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> >> > I intended to use the method to set up the notification handler but now
> >> > my BIOS says that it's not there even if it is...
> >> > Can I assume some default values (e.g. notifications are enabled and will
> >> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> >> > different)?
> >>
> >> The spec says that the bits in the supported functions vector mean
> >> that if bit n is set, function n+1 exists,
> >
> > Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
> > mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
> >
> > Maybe if the bit n is set then functions 0..n are available? That would
> > (almost) match what I see...
> 
> From the spec:
> 
> Supported DWORD Bit vector providing supported functions
> information. Each bit marks
> Functions Bit  support for one specific function of the
> ATIF method. Bit n, if set,
> Vectorindicates that Function n+1 is supported.

Sorry, I still don't understand it... what's "Function n+1" in this
context?
Does this mean that if bit n is set then the function defined as
1 << (n+1) is supported?

> I don't know how wonky bioses in the wild are however.  I can see what
> our internal teams do in these sort of cases.

That would be helpful :)
Note that on this machine (Toshiba L855-10W) brightness control under
windows does not work with the stock catalyst driver, it works only with
the (oldish) driver supplied by Toshiba.

Anyway, I'm attaching v2 of my patches, I've incorporated the
suggestions and some bits of code from joeyli, and now brightness
control is actually implemented.

Still missing is the issue of event 0x81 and the conflict with video.ko;
the handler should probably return NOTIFY_BAD to veto the keypress.

Luca
-- next part --
>From f0f8699eabee0d47b93fba14f8126b821cc106a5 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.
v2: add support for the 2nd parameter (from Lee, Chun-Yi
).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |   38 --
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..158e514 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,23 +14,30 @@
 #include 

 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object *radeon_atif_call(acpi_handle handle, int function,
+   struct acpi_buffer *params)
 {
acpi_status status;
union acpi_object atif_arg_elements[2];
struct acpi_object_list atif_arg;
-   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

atif_arg.count = 2;
atif_arg.pointer = &atif_arg_elements[0];

atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
-   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[1].integer.value = 0;
+   atif_arg_elements[0].integer.value = function;
+
+   if (params) {
+   atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+   atif_arg_elements[1].buffer.length = params->length;
+   atif_arg_elements[1].buffer.pointer = params->pointer;
+   } else {
+   /* We need a second fake parameter */
+   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
+   atif_arg_elements[1].integer.value = 0;
+   }

status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);

@@ -39,18 +46,18 @@ static int radeon_atif_call(acpi_handle handle)
DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 acpi_format_exception(status));
kfree(buffer.pointer);
- 

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:30 PM, Alex Deucher  wrote:
> On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti  
> wrote:
>> On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
>>> Supported DWORD Bit vector providing supported functions
>>> information. Each bit marks
>>> Functions Bit  support for one specific function of the
>>> ATIF method. Bit n, if set,
>>> Vectorindicates that Function n+1 is supported.
>>
>> Sorry, I still don't understand it... what's "Function n+1" in this
>> context?
>> Does this mean that if bit n is set then the function defined as
>> 1 << (n+1) is supported?
>
> It means if bit n is set in teh supported function vector, function
> n+1 is supported.  E.g., if bit 1 is set, function 2
> (ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported.  If bit 3 is
> set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.

Great, just had an epiphany ;-) "n+1" refers to the value that's
passed down to ATIF, I was still thinking in terms of bitmasks...
Ok, so my code is correct, BIOS is botched... meh.

L

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher  wrote:
> Regarding patches 3 and 4, it might be easier to just store a pointer
> to the relevant encoder when you verify ATIF rather than walking the
> encoder list every time.

Makes sense, I was unsure about the lifetime of the encoders, but
AFAICS they're destroyed only when the module in unloaded.

Luca

Fwd: Brightness on HP EliteBook 8460p

[Luca Tettamanti]

On Sat, Jul 28, 2012 at 4:47 PM, Pali Roh?r  wrote:
> Hello, I have some good news. Radeon patches from this post
> http://lists.freedesktop.org/archives/dri-devel/2012-July/025535.html
> export brightness file /sys/class/backlight/radeon_bl/brightness
> And finally with these patches I'm able to change brightness on my EliteBook.

Nice, in your older mail I read:

> The PEGP.DGFX acpi device was binding to
> acpi/video driver, the above ASL code emit a 0xD0 bus event to
> video.c but cann't process it. Even we add a new bus event in
> video.c and generate a acpi event, there still need another acpi
> driver should take care it.

That event (0xd0) is probably a custom event announced through ATIF.
I'm working on the code to deal with it, the latest patches are here:

http://lists.freedesktop.org/archives/dri-devel/2012-July/025646.html

You first need this one with the ACPI header:
http://lists.freedesktop.org/archives/dri-devel/2012-July/025517.html

and the other 3 that you've already tested. With my patches on top you
should be able to change the brightness using the hotkeys.

I'd be grateful if you could test them :-)

Luca

Fwd: Brightness on HP EliteBook 8460p

[Luca Tettamanti]

I'm putting back the CC and adding Alex.

On Tue, Jul 31, 2012 at 5:07 PM, Pali Roh?r  wrote:
> Thanks, now working. When I write to acpi video brightness file
> it change brightness and in dmesg is:
>
> [   47.200998] [drm:radeon_atif_handler], event, device_class =
> video, type = 0xd0
> [   47.201102] [drm:radeon_atif_get_sbios_requests], SBIOS
> pending requests: 0x80
> [   47.201104] [drm:radeon_atif_handler], ATIF: 1 pending SBIOS
> requests
> [   47.201105] [drm:radeon_atif_handler], Changing brightness to
> 11

Great! I'll send an updated patch to Alex soon.

> I think that acpi only sent event about brightness key pressed,
> because nothing happened when I pressed it.
>
> Also for windows is needed special HP application (hp hotkey) for
> brightness keys. Without it brightness keys not working too...

I've looked at hp-wmi: the hotkey is indeed dispatched via WMI
(hp-wmi) so you need something in userspace (KDE, Gnome, etc) to
respond to that key press.

> And there is one problem with /sys/class/backlight/radeon_bl.
> When I enable Ambient Light Sensor which auto adjust brightness
> based on sensor data, writing value 0 (min) or 255 (max) to
> /sys/class/backlight/radeon_bl/brightness turn off display.

Ok, that's weird. 0 turns off the panel by design, 255 should not...

> All
> other values (1-254) are OK (adjust brightness level). When I
> turn off Ambient Light Sensor (via hp-wmi kernel module) then
> values 0 and 255 also set brightness level (min and max). My
> suggestion is to convert value 0 to 1 and 255 to 254 to prevent
> this problem.

No idea what's going on here... might be some weird vendor magic.
The WMI code is rather obscure...

Luca

[PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[Luca Tettamanti]

On Tue, Jul 31, 2012 at 09:58:04AM -0400, Alex Deucher wrote:
> On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti  
> wrote:
> > On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher  
> > wrote:
> >> Regarding patches 3 and 4, it might be easier to just store a pointer
> >> to the relevant encoder when you verify ATIF rather than walking the
> >> encoder list every time.

Done.

> > Makes sense, I was unsure about the lifetime of the encoders, but
> > AFAICS they're destroyed only when the module in unloaded.
> 
> They are present for the life of the driver.

I had to move to call to radeon_acpi_init after radeon_modeset_init,
otherwise the encoders are not present yet (the tear down code path is
correct, acpi first, then modeset).
Latest and greatest version is attached; I fixed notifications when
using custom command codes (tested by Pali Roh?r) and implemented your
suggestion. 

Luca
-- next part --
>From f0f8699eabee0d47b93fba14f8126b821cc106a5 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.
v2: add support for the 2nd parameter (from Lee, Chun-Yi
).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon_acpi.c |   38 --
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c 
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..158e514 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,23 +14,30 @@
 #include 

 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object *radeon_atif_call(acpi_handle handle, int function,
+   struct acpi_buffer *params)
 {
acpi_status status;
union acpi_object atif_arg_elements[2];
struct acpi_object_list atif_arg;
-   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+   struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

atif_arg.count = 2;
atif_arg.pointer = &atif_arg_elements[0];

atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
-   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
-   atif_arg_elements[1].integer.value = 0;
+   atif_arg_elements[0].integer.value = function;
+
+   if (params) {
+   atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+   atif_arg_elements[1].buffer.length = params->length;
+   atif_arg_elements[1].buffer.pointer = params->pointer;
+   } else {
+   /* We need a second fake parameter */
+   atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
+   atif_arg_elements[1].integer.value = 0;
+   }

status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);

@@ -39,18 +46,18 @@ static int radeon_atif_call(acpi_handle handle)
DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 acpi_format_exception(status));
kfree(buffer.pointer);
-   return 1;
+   return NULL;
}

-   kfree(buffer.pointer);
-   return 0;
+   return buffer.pointer;
 }

 /* Call all ACPI methods here */
 int radeon_acpi_init(struct radeon_device *rdev)
 {
acpi_handle handle;
-   int ret;
+   union acpi_object *info;
+   int ret = 0;

/* Get the device handle */
handle = DEVICE_ACPI_HANDLE(&rdev->pdev->dev);
@@ -60,10 +67,11 @@ int radeon_acpi_init(struct radeon_device *rdev)
return 0;

/* Call the ATIF method */
-   ret = radeon_atif_call(handle);
-   if (ret)
-   return ret;
+   info = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE, NULL);
+   if (!info)
+   ret = -EIO;

-   return 0;
+   kfree(info);
+   return ret;
 }

-- 
1.7.10.4

-- next part --
>From 427002ddf653b0abd0fb820b09322bf2a0b281af Mon Sep 17 00:00:00 2001
From: Luca Tettamanti 
Date: Mon, 30 Jul 2012 21:11:58 +0200
Subject: [PATCH 2/4] drm/radeon: implement radeon_atif_verify_interface

Wrap the call to VERIFY_INTERFACE and add the parsing of the support
vectors.
v2: use a packed struct for handling the output of ACPI calls, hides
ugly pointer arithmetics (Lee, Chun-Yi ).

Signed-off-by: Luca Tettamanti 
---
 drivers/gpu/drm/radeon/radeon.h  |   40 +
 drivers/gpu/drm/radeon/radeon_acpi.c |   79 +++---
 2 files changed, 113 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeo

[PATCH libdrm] omap: add omapdrm support

[Luca Tettamanti]

Hi Rob,
I've a couple of (minor) comments:

On Wed, Mar 21, 2012 at 01:00:36PM -0500, Rob Clark wrote:
> --- /dev/null
> +++ b/omap/omap_drm.c
[...]
> +/* allocate a new (un-tiled) buffer object */
> +struct omap_bo * omap_bo_new(struct omap_device *dev,
> + uint32_t size, uint32_t flags)
> +{
> + if (flags & OMAP_BO_TILED) {
> + return NULL;
> + }
> + return omap_bo_new_impl(dev, (union omap_gem_size){
> + .bytes = size,
> + }, flags);

Hum, the indentation of the anonymous union looks weird (but maybe it's just
me...)

> +}
> +
> +/* allocate a new buffer object */
> +struct omap_bo * omap_bo_new_tiled(struct omap_device *dev,
> + uint32_t width, uint32_t height, uint32_t flags)
> +{
> + if (!(flags & OMAP_BO_TILED)) {
> + return NULL;
> + }
> + return omap_bo_new_impl(dev, (union omap_gem_size){
> + .tiled = {
> + .width = width,
> + .height = height,
> + },
> + }, flags);
> +}

Here too :-) What about this:

return omap_bo_new_impl(dev, (union omap_gem_size)
{
.stuff = blah,
});

Or just use a temp var?

> +
> +/* get buffer info */
> +static int get_buffer_info(struct omap_bo *bo)
> +{
> + struct drm_omap_gem_info req = {
> + .handle = bo->handle,
> + };
> + int ret = drmCommandWriteRead(bo->dev->fd, DRM_OMAP_GEM_INFO,
> + &req, sizeof(req));
> + if (ret) {
> + return ret;
> + }
> +
> + /* really all we need for now is mmap offset */
> + bo->offset = req.offset;
> + bo->size = req.size;
> +
> + return 0;
> +}
> +
> +/* import a buffer object from DRI2 name */
> +struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name)
> +{
> + struct omap_bo *bo = calloc(sizeof(*bo), 1);
> + struct drm_gem_open req = {
> + .name = name,
> + };
> +
> + if (drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req)) {
> + goto fail;
> + }

bo may be NULL here:

> +
> + bo->dev = dev;
> + bo->name = name;
> + bo->handle = req.handle;

I also woundn't use the calloc in the initialization block, I prefer to
keep the allocation and the check together:

bo = alloc_stuff();
if (!bo)
oh_crap();

I find it more easy to check visually.

Luca