[PATCH] drm/i915: Optimize DIV_ROUND_CLOSEST call
DIV_ROUND_CLOSEST is faster if the compiler knows it will only be
dealing with unsigned dividends. This optimization rips 32 bytes of
binary code on x86_64.
Signed-off-by: Jean Delvare
Cc: Guenter Roeck
Cc: Andrew Morton
Cc: Daniel Vetter
Cc: David Airlie
---
Already sent on: 2012-09-03.
Daniel, I think we can safely assume ia_freq can't be negative?
drivers/gpu/drm/i915/intel_pm.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-3.7-rc5.orig/drivers/gpu/drm/i915/intel_pm.c 2012-11-12
09:30:56.796836818 +0100
+++ linux-3.7-rc5/drivers/gpu/drm/i915/intel_pm.c 2012-11-12
10:49:38.241676096 +0100
@@ -2547,7 +2547,8 @@ static void gen6_update_ring_freq(struct
{
struct drm_i915_private *dev_priv = dev->dev_private;
int min_freq = 15;
- int gpu_freq, ia_freq, max_ia_freq;
+ int gpu_freq;
+ unsigned int ia_freq, max_ia_freq;
int scaling_factor = 180;
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Optimize DIV_ROUND_CLOSEST call
On Mon, 12 Nov 2012 15:02:26 +0100, Daniel Vetter wrote: > On Mon, Nov 12, 2012 at 02:18:02PM +0100, Jean Delvare wrote: > > DIV_ROUND_CLOSEST is faster if the compiler knows it will only be > > dealing with unsigned dividends. This optimization rips 32 bytes of > > binary code on x86_64. > > > > Signed-off-by: Jean Delvare > > Cc: Guenter Roeck > > Cc: Andrew Morton > > Cc: Daniel Vetter > > Cc: David Airlie > > --- > > Already sent on: 2012-09-03. > > > > Daniel, I think we can safely assume ia_freq can't be negative? > > Queued for -next, thanks for the patch. Sorry that I've missed it on first > submission. No problem, thanks :) -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V4] drm: edid: add support for E-DDC
Hi all,
Sorry for breaking message threading but I was not included in
iterations 3 and 4 of this patch.
Random comments about v4:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
> unsigned
> char *buf,
> int block, int len)
> {
> unsigned char start = block * EDID_LENGTH;
> + unsigned char segment = block >> 1;
> + unsigned char xfers = segment ? 3 : 2;
> int ret, retries = 5;
>
> /* The core i2c driver will automatically retry the transfer if the
> @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
> unsigned char *buf,
> */
> do {
> struct i2c_msg msgs[] = {
> - {
> + { /*set segment pointer */
> + .addr = DDC_SEGMENT_ADDR,
> + .flags = segment ? 0 : I2C_M_IGNORE_NAK,
I don't get the idea. If segment == 0, this message is never sent, so the
value of field flags doesn't matter. So flags will always be 0 when this
message is sent, so it can be hard-coded.
But from previous discussions my understanding was an agreement on always
using I2C_M_IGNORE_NAK for improved compatibility. So I2C_M_IGNORE_NAK
should be hard-coded, not 0?
> + .len= 1,
> + .buf= &segment,
> + }, {
> .addr = DDC_ADDR,
> .flags = 0,
> .len= 1,
> @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
> unsigned char *buf,
> .buf= buf,
> }
> };
> - ret = i2c_transfer(adapter, msgs, 2);
> + /* Avoid sending the segment addr to not upset non-compliant ddc
> +* monitors.
> +*/
s/segment addr/segment/, plus it's abot E-DCC compliance as I understand it,
not DDC.
> + if (!segment)
> + ret = i2c_transfer(adapter, &msgs[1], xfers);
> + else
> + ret = i2c_transfer(adapter, msgs, xfers);
> +
This can be written:
ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
Which is more compact and, I suspect, faster.
> if (ret == -ENXIO) {
> DRM_DEBUG_KMS("drm: skipping non-existent adapter
> %s\n",
> adapter->name);
> break;
> }
> - } while (ret != 2 && --retries);
> + } while (ret != xfers && --retries);
>
> - return ret == 2 ? 0 : -1;
> + return ret == xfers ? 0 : -1;
> }
>
> static bool drm_edid_is_zero(u8 *in_edid, int length)
Other than this, your code looks reasonable, not so different from what
I submitted 8 months ago actually. But ISTU you can test the code with
real hardware while I couldn't.
With the changes above applied, you can add:
Reviewed-by: Jean Delvare
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Optimize DIV_ROUND_CLOSEST call
From: Jean Delvare
Subject: drm/i915: Optimize DIV_ROUND_CLOSEST call
DIV_ROUND_CLOSEST is faster if the compiler knows it will only be
dealing with unsigned dividends.
Signed-off-by: Jean Delvare
Cc: Guenter Roeck
Cc: Andrew Morton
Cc: Daniel Vetter
Cc: David Airlie
---
Daniel, I think we can safely assume ia_freq can't be negative?
drivers/gpu/drm/i915/intel_pm.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-3.6-rc3.orig/drivers/gpu/drm/i915/intel_pm.c 2012-08-28
14:46:22.0 +0200
+++ linux-3.6-rc3/drivers/gpu/drm/i915/intel_pm.c 2012-09-01
21:17:32.074619227 +0200
@@ -2499,7 +2499,8 @@ static void gen6_update_ring_freq(struct
{
struct drm_i915_private *dev_priv = dev->dev_private;
int min_freq = 15;
- int gpu_freq, ia_freq, max_ia_freq;
+ int gpu_freq;
+ unsigned int ia_freq, max_ia_freq;
int scaling_factor = 180;
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Linux 3.4-rc4
etrieval 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:drm_edid_block_valid] *ERROR* EDID checksum is invalid, > >> remainder is 208 > >> > >> Earlier kernels were able to retrieve EDEDs reliably. > >> > >> This is with: > >> > >> [1.678392] [drm] nouveau :01:00.0: Detected an NV50 > >> generation card (0x086b00a2) > > > > Just a crazy thought, but didn't we change some timings related to > > EDID retrieval? To make it faster. > > Hum, this commit: > > commit 1849ecb22fb3b5d57b65e7369a3957adf9f26f39 > Author: Jean Delvare > Date: Sat Jan 28 11:07:09 2012 +0100 > > drm/kms: Make i2c buses faster > > doubled the data rate but only for radeon and intel drivers. nouveau > doesn't use the standard i2c-algo-bit helpers (BTW: the > cond_resched() has been removed), and AFAICS it's using 1us delay; > the other drivers are using 10us, 1us seems a bit too low... As I read the code, it is actually using a 6 us delay. This is fast but reasonable, especially when the code handles clock stretching Ben Skeggs (Cc'd) rewrote the I2C handling code in the nouveau driver completely in kernel 3.3: commit f553b79c03f0dbd52f6f03abe8233a2bef8cbd0d Author: Ben Skeggs Date: Wed Dec 21 18:09:12 2011 +1000 drm/nouveau/i2c: handle bit-banging ourselves i2c-algo-bit doesn't actually work very well on one card I have access to (NVS 300), random single-bit errors occur most of the time - what we're doing now is closer to what xf86i2c.c does. The original plan was to figure out why i2c-algo-bit fails on the NVS 300, and fix it. However, while investigating I discovered i2c-algo-bit calls cond_resched(), which makes it a bad idea for us to be using as we execute VBIOS scripts from a tasklet, and there may very well be i2c transfers as a result. So, since I already wrote this code in userspace to track down the NVS 300 bug, and it's not really much code - lets use it. Signed-off-by: Ben Skeggs So if the regression happened between 3.2.15 and 3.4-rc4, that would be a good candidate. BTW, Ben, there were two interesting fixes to i2c-algo-bit meanwhile, you may want to try using it again. Maarten, another commit you may want to try reverting is 9292f37e1f5c79400254dca46f83313488093825 . If none of the above works, it would be great if you could test your KVM with another graphics adapter, so that we know if we are looking for a nouveau-specific bug or rather an issue in the common i2c or edid code. Otherwise a plain bisection is probably the way to go. -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"
Hi Florian, On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote: > On Sat, 04 Jun 2011 19:34:56 - > Jean Delvare wrote: > > > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a > > hang when loading the eeprom driver (see bug #35572.) GMBUS will be > > re-enabled later, differently. > > > > Signed-off-by: Jean Delvare > > Reported-by: Marek Otahal > > Tested-by: Yermandu Patapitafious > > Tested-by: Andrew Lutomirski > > Acked-by: Chris Wilson > > Cc: David Airlie > > is this[1] resolved some other way in the meantime? > > Regards, > Flo > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572 Not that I know of (and I don't see any other way at least for 2.6.39.) This is a shame, really, my revert patch should have been applied several days ago already. Keith, Chris, David, can you please get it rolling? This is a regression presumably affecting a lot of users, we should really fix it quickly, both in 2.6.39.x and 3.0-rc. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"
Hi Dave, On Tue, 14 Jun 2011 13:39:35 +1000, Dave Airlie wrote: > On Sat, Jun 11, 2011 at 10:58 PM, Jean Delvare wrote: > > Hi Florian, > > > > On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote: > >> On Sat, 04 Jun 2011 19:34:56 - > >> Jean Delvare wrote: > >> > >> > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a > >> > hang when loading the eeprom driver (see bug #35572.) GMBUS will be > >> > re-enabled later, differently. > >> > > >> > Signed-off-by: Jean Delvare > >> > Reported-by: Marek Otahal > >> > Tested-by: Yermandu Patapitafious > >> > Tested-by: Andrew Lutomirski > >> > Acked-by: Chris Wilson > >> > Cc: David Airlie > >> > >> is this[1] resolved some other way in the meantime? > >> > >> Regards, > >> Flo > >> > >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572 > > > > Not that I know of (and I don't see any other way at least for 2.6.39.) > > This is a shame, really, my revert patch should have been applied > > several days ago already. > > > > Keith, Chris, David, can you please get it rolling? This is a > > regression presumably affecting a lot of users, we should really fix it > > quickly, both in 2.6.39.x and 3.0-rc. > > This patch really had no info other than the bug link to tell me wtf its > doing, The patch I sent on June 4th (Message-ID: <20110604213456.7ac5588e@endymion.delvare>) says: Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a hang when loading the eeprom driver (see bug #35572.) GMBUS will be re-enabled later, differently. Seems clear enough to me. > I actually don't think reverting this is the correct fix, since it looks like > the code path thats screws with the mutex still happens on GEN2 machines > which unlucky for them. No. Without the revert, the problem happens on every chip except GEN2. With the revert, the problems shouldn't happen on any chip. At least this is how I understand the code. Anyway, you may not like the fix, but the fact is that commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f caused a regression, so at least for kernel 2.6.39, reverting it is the right thing to do. For 3.0.0, either someone comes up with an alternative fix and we can apply it, or reverting the faulty commit is the way to go too. We have a known regression affecting many users, we have the fix, let's please apply it quickly. Further discussions should happen _later_. > Chris, also I don't see an ack anywhere on the list, only some > discussion in the bug, I would indeed love to get a ack from Chris. > I suspect the correct fix is to remove all the offending code instead > of just putting back the piece > of plaster that fell out of the wall. Which "offending code" are you talking about? We are fixing a regression here. It has to do with being fast and safe, not with "the correct fix". Thanks, -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] drm/radeon: Fix Asus M2A-VM HDMI EDID error flooding problem
A-VM HDMI DDC quirk: Log EDID retrieval status here once,
> + * instead of periodically dumping data and kernel errors into the
> + * logs, if a monitor is not connected to HDMI */
> if (edid) {
> + DRM_INFO("Radeon display connector %s: Found valid EDID",
> + drm_get_connector_name(connector));
> kfree(edid);
> + } else {
> + DRM_INFO("Radeon display connector %s: No display connected or
> invalid EDID",
> + drm_get_connector_name(connector));
> }
> return ret;
> }
> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> b/drivers/gpu/drm/radeon/radeon_i2c.c
> index 781196d..1d6decd 100644
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -63,6 +63,66 @@ bool radeon_ddc_probe(struct radeon_connector
> *radeon_connector)
> return false;
> }
>
> +/**
> + * Probe EDID information via I2C.
> + *
> + * \param adapter : i2c device adaptor
> + * \param buf : EDID data buffer to be filled
> + * \param len : EDID data buffer length
> + * \return 0 on success or -1 on failure.
It doesn't look like the standard kernel documentation style. At least
I've never seen such \ before. And worse, it doesn't describe the
function below at all.
> + *
> + * Try to fetch EDID information by calling i2c driver function and
> + * probe for EDID header information.
> + *
> + * Remark:
> + * This function has been added, because there are integrated ATI Radeon
> + * chipset implementations (e. g. Asus M2A-VM HDMI that indicate the
> + * availability of a DDC even when there's no monitor connected.
> + * In this case, drm_get_edid and drm_edid_block_valid periodically dump
> + * data and kernel errors into the logs and onto the terminal, which lead to
> + * an unacceptable system behaviour.
> + */
> +bool radeon_ddc_edid_probe(struct radeon_connector *radeon_connector)
> +{
> + u8 out_buf[] = { 0x0, 0x0};
You only use the first byte (same in radeon_ddc_probe, could be
optimized.)
> + u8 block[20];
> + int ret;
> + struct i2c_msg msgs[] = {
> + {
> + .addr = 0x50,
> + .flags = 0,
> + .len= 1,
> + .buf= out_buf,
> + }, {
> + .addr = 0x50,
> + .flags = I2C_M_RD,
> + .len= 20,
> + .buf= block,
> + }
> + };
> +
> + ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
You are reading 20 bytes when you really only need 10. It would be more
efficient to read 8 bytes first, and only if needed, the two remaining
ones..
> + if (ret == 2)
> + if ((block[0] == 0x00) &&
> + (block[7] == 0x00) &&
> + (block[1] == 0xff) &&
> + (block[2] == 0xff) &&
> + (block[3] == 0xff) &&
> + (block[4] == 0xff) &&
> + (block[5] == 0xff) &&
> + (block[6] == 0xff))
> + /* EDID header starts with:
> + * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00;
> + * seems to be an EDID */
> + if ((block[18] != 0x00) || (block[19] != 0x00))
You can drop many parentheses in these tests.
> + /* EDID headers end with EDID version and
> + * revision number: EDID version is not 0.0 =>
> + * EDID should be available */
> + return true;
> + /* Couldn't find an accessible EDID on this connector. */
> + return false;
> +}
> +
> /* bit banging i2c */
>
> static void radeon_i2c_do_lock(struct radeon_i2c_chan *i2c, int lock_state)
> diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
> b/drivers/gpu/drm/radeon/radeon_mode.h
> index 6df4e3c..14710fc 100644
> --- a/drivers/gpu/drm/radeon/radeon_mode.h
> +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> @@ -515,6 +515,7 @@ extern void radeon_i2c_put_byte(struct radeon_i2c_chan
> *i2c,
> extern void radeon_router_select_ddc_port(struct radeon_connector
> *radeon_connector);
> extern void radeon_router_select_cd_port(struct radeon_connector
> *radeon_connector);
> extern bool radeon_ddc_probe(struct radeon_connector *radeon_connector);
> +extern bool radeon_ddc_edid_probe(struct radeon_connector *radeon_connector);
> extern int radeon_ddc_get_modes(struct radeon_connector *radeon_connector);
>
> extern struct drm_encoder *radeon_best_encoder(struct drm_connector
> *connector);
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] drm/radeon: Fix Asus M2A-VM HDMI EDID error flooding problem
u8 buf[2];
+ u8 out = 0x0;
+ u8 buf[8];
int ret;
struct i2c_msg msgs[] = {
{
.addr = 0x50,
.flags = 0,
.len = 1,
- .buf = out_buf,
+ .buf = &out,
},
{
.addr = 0x50,
.flags = I2C_M_RD,
- .len = 1,
+ .len = 8,
.buf = buf,
}
};
@@ -57,7 +57,7 @@ bool radeon_ddc_probe(struct radeon_conn
radeon_router_select_ddc_port(radeon_connector);
ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
- if (ret == 2)
+ if (ret == 2 && drm_edid_header_valid(buf) >= 6)
return true;
return false;
--- linux-3.0-rc4.orig/include/drm/drm_crtc.h 2011-06-21 10:30:19.0
+0200
+++ linux-3.0-rc4/include/drm/drm_crtc.h2011-06-22 12:02:24.0
+0200
@@ -802,6 +802,7 @@ extern struct drm_display_mode *drm_gtf_
extern int drm_add_modes_noedid(struct drm_connector *connector,
int hdisplay, int vdisplay);
+extern int drm_edid_header_valid(const u8 *raw_edid);
extern bool drm_edid_is_valid(struct edid *edid);
struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
int hsize, int vsize, int fresh);
But if Alex believes that the case is rare enough that a board-specific
workaround is better, that's totally fine with me too. He is the master!
> Point 4:
> I've checked your comments and updated the patch. Hopefully, it fits now
> better to the linux kernel coding style. Thank you for the hints.
>
>
> A subsequent mail will contain the updated patch proposal.
[1] I wonder why radeon_router_select_ddc_port() isn't part of
pre_xfer().
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon: Fix Asus M2A-VM HDMI EDID error flooding problem
On Fri, 24 Jun 2011 09:36:56 -0400, Alex Deucher wrote:
> On Fri, Jun 24, 2011 at 12:02 AM, Thomas Reim wrote:
> >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > b/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > index 781196d..7e93cf9 100644
> >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > @@ -34,7 +34,7 @@
> >> > */
> >> > bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> >> > {
> >> > - u8 out_buf[] = { 0x0, 0x0};
> >> > + u8 out = 0x0;
> >> > u8 buf[2];
> >> > int ret;
> >> > struct i2c_msg msgs[] = {
> >> > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector
> >> > *radeon_connector)
> >> > .addr = 0x50,
> >> > .flags = 0,
> >> > .len = 1,
> >> > - .buf = out_buf,
> >> > + .buf = &out,
> >> > },
> >> > {
> >> > .addr = 0x50,
> >>
> >>
> >> The change above doesn't seem to be related.
> >
> > This was a comment from Jean who complained about the ineffective usage
> > of the i2c bus. But I can also restore the old code. What's your
> > preference?
>
> Ah, I missed that. Let's make that a separate patch, or fix it when
> you add support for the extended edid check.
>
> Thanks for fixing this up.
I'll send a patch for that one, as I found it. It is indeed unrelated
to the problem, I mentioned it only to avoid the same mistake in a
newly added function.
It's a very minor cleanup / optimization, BTW.
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Fix EDID dump format
* print_hex_dump_bytes() already includes a log level, so we shouldn't
add one. That log level is KERN_DEBUG so use the same for our header
for consistency.
* print_hex_dump_bytes() properly puts a newline at the end of the
last line, so there is no reason why we would add another one.
Signed-off-by: Jean Delvare
Cc: David Airlie
---
drivers/gpu/drm/drm_edid.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- linux-3.0-rc4.orig/drivers/gpu/drm/drm_edid.c 2011-06-22
16:55:11.0 +0200
+++ linux-3.0-rc4/drivers/gpu/drm/drm_edid.c2011-06-27 15:11:53.0
+0200
@@ -184,9 +184,9 @@ drm_edid_block_valid(u8 *raw_edid)
bad:
if (raw_edid) {
- printk(KERN_ERR "Raw EDID:\n");
- print_hex_dump_bytes(KERN_ERR, DUMP_PREFIX_NONE, raw_edid,
EDID_LENGTH);
- printk(KERN_ERR "\n");
+ printk(KERN_DEBUG "Raw EDID:\n");
+ print_hex_dump_bytes("", DUMP_PREFIX_OFFSET,
+raw_edid, EDID_LENGTH);
}
return 0;
}
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Shorten buffers in radeon_ddc_probe
No need for 2-byte buffers, we only send one byte and receive one
byte.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
drivers/gpu/drm/radeon/radeon_i2c.c |7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
--- linux-3.0-rc4.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2011-06-27
15:39:33.0 +0200
+++ linux-3.0-rc4/drivers/gpu/drm/radeon/radeon_i2c.c 2011-06-27
15:40:35.0 +0200
@@ -34,21 +34,20 @@
*/
bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
{
- u8 out_buf[] = { 0x0, 0x0};
- u8 buf[2];
+ u8 out = 0x0, in;
int ret;
struct i2c_msg msgs[] = {
{
.addr = 0x50,
.flags = 0,
.len = 1,
- .buf = out_buf,
+ .buf = &out,
},
{
.addr = 0x50,
.flags = I2C_M_RD,
.len = 1,
- .buf = buf,
+ .buf = &in,
}
};
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/kms: Shorten buffers in radeon_ddc_probe
On Mon, 27 Jun 2011 22:44:42 +0200, Jean Delvare wrote:
> No need for 2-byte buffers, we only send one byte and receive one
> byte.
>
> Signed-off-by: Jean Delvare
> Cc: David Airlie
> Cc: Alex Deucher
> ---
> drivers/gpu/drm/radeon/radeon_i2c.c |7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> --- linux-3.0-rc4.orig/drivers/gpu/drm/radeon/radeon_i2c.c2011-06-27
> 15:39:33.0 +0200
> +++ linux-3.0-rc4/drivers/gpu/drm/radeon/radeon_i2c.c 2011-06-27
> 15:40:35.0 +0200
> @@ -34,21 +34,20 @@
> */
> bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> {
> - u8 out_buf[] = { 0x0, 0x0};
> - u8 buf[2];
> + u8 out = 0x0, in;
> int ret;
> struct i2c_msg msgs[] = {
> {
> .addr = 0x50,
> .flags = 0,
> .len = 1,
> - .buf = out_buf,
> + .buf = &out,
> },
> {
> .addr = 0x50,
> .flags = I2C_M_RD,
> .len = 1,
> - .buf = buf,
> + .buf = &in,
> }
> };
Please disregard this patch, it is superseded by a more important
workaround by Thomas Reim.
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [lm-sensors] [PATCH 01/34] System Firmware Interface
On Mon, 18 Jul 2011 09:08:15 -0400, Prarit Bhargava wrote: > This patch introduces a general System Firmware interface to the kernel, > called > sysfw. > > Inlcluded in this interface is the ability to search a standard set of fields, > sysfw_lookup(). The fields are currently based upon the x86 and ia64 SMBIOS > fields but exapandable to fields that other arches may introduce. Also > included is the ability to search and match against those fields, and run > a callback function against the matches, sysfw_callback(). > > Modify module code to use sysfw instead of old DMI interface. This is a HUGE patch set. You'd need to have a good reason for such a big and intrusive change, yet I see no such reason explained. I understand that we _can_ abstract system information interfaces, but just because we can doesn't mean we have to. I would at least wait for a second DMI-like interface to be widely implemented and support before any attempt to abstract, otherwise your design is bound to be missing the target. And even then, you'd still need to convince me that there is a need for a unified interface to access both backends at once. I would guess that you know what backend is present on a system when you try to identify it. At this point, I see the work needed to review your patches, the risk of regressions due to the large size of the patch set, but I don't see any immediate benefit. Thus I am not going to look into it at all, sorry. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Fix I2C mask definitions
Commit 9b9fe724 accidentally used RADEON_GPIO_EN_* where
RADEON_GPIO_MASK_* was intended. This caused improper initialization
of I2C buses, mostly visible when setting i2c_algo_bit.bit_test=1.
Using the right constants fixes the problem.
Signed-off-by: Jean Delvare
Cc: Alex Deucher
Cc: Jerome Glisse
Cc: sta...@kernel.org
---
This needs testing on more combios-based Radeon cards, please. I
could only test it on one Radeon 9200 (RV280) card.
drivers/gpu/drm/radeon/radeon_combios.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- linux-3.0.orig/drivers/gpu/drm/radeon/radeon_combios.c 2011-10-06
14:52:59.0 +0200
+++ linux-3.0/drivers/gpu/drm/radeon/radeon_combios.c 2011-10-06
14:53:23.0 +0200
@@ -620,8 +620,8 @@ static struct radeon_i2c_bus_rec combios
i2c.y_data_mask = 0x80;
} else {
/* default masks for ddc pads */
- i2c.mask_clk_mask = RADEON_GPIO_EN_1;
- i2c.mask_data_mask = RADEON_GPIO_EN_0;
+ i2c.mask_clk_mask = RADEON_GPIO_MASK_1;
+ i2c.mask_data_mask = RADEON_GPIO_MASK_0;
i2c.a_clk_mask = RADEON_GPIO_A_1;
i2c.a_data_mask = RADEON_GPIO_A_0;
i2c.en_clk_mask = RADEON_GPIO_EN_1;
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Simplify I2C post_xfer function
There is no point in re-doing in post_xfer all the initialization
that was already done by pre_xfer. Instead, only do the work which
differs from pre_xfer.
Signed-off-by: Jean Delvare
Reviewed-by: Alex Deucher
---
drivers/gpu/drm/radeon/radeon_i2c.c | 48 ---
1 file changed, 22 insertions(+), 26 deletions(-)
--- linux-3.0.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-06
18:39:04.0 +0200
+++ linux-3.0/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-06
18:45:27.0 +0200
@@ -81,8 +81,9 @@ bool radeon_ddc_probe(struct radeon_conn
/* bit banging i2c */
-static void radeon_i2c_do_lock(struct radeon_i2c_chan *i2c, int lock_state)
+static int pre_xfer(struct i2c_adapter *i2c_adap)
{
+ struct radeon_i2c_chan *i2c = i2c_get_adapdata(i2c_adap);
struct radeon_device *rdev = i2c->dev->dev_private;
struct radeon_i2c_bus_rec *rec = &i2c->rec;
uint32_t temp;
@@ -137,19 +138,30 @@ static void radeon_i2c_do_lock(struct ra
WREG32(rec->en_data_reg, temp);
/* mask the gpio pins for software use */
- temp = RREG32(rec->mask_clk_reg);
- if (lock_state)
- temp |= rec->mask_clk_mask;
- else
- temp &= ~rec->mask_clk_mask;
+ temp = RREG32(rec->mask_clk_reg) | rec->mask_clk_mask;
WREG32(rec->mask_clk_reg, temp);
temp = RREG32(rec->mask_clk_reg);
+ temp = RREG32(rec->mask_data_reg) | rec->mask_data_mask;
+ WREG32(rec->mask_data_reg, temp);
temp = RREG32(rec->mask_data_reg);
- if (lock_state)
- temp |= rec->mask_data_mask;
- else
- temp &= ~rec->mask_data_mask;
+
+ return 0;
+}
+
+static void post_xfer(struct i2c_adapter *i2c_adap)
+{
+ struct radeon_i2c_chan *i2c = i2c_get_adapdata(i2c_adap);
+ struct radeon_device *rdev = i2c->dev->dev_private;
+ struct radeon_i2c_bus_rec *rec = &i2c->rec;
+ uint32_t temp;
+
+ /* unmask the gpio pins for software use */
+ temp = RREG32(rec->mask_clk_reg) & ~rec->mask_clk_mask;
+ WREG32(rec->mask_clk_reg, temp);
+ temp = RREG32(rec->mask_clk_reg);
+
+ temp = RREG32(rec->mask_data_reg) & ~rec->mask_data_mask;
WREG32(rec->mask_data_reg, temp);
temp = RREG32(rec->mask_data_reg);
}
@@ -209,22 +221,6 @@ static void set_data(void *i2c_priv, int
WREG32(rec->en_data_reg, val);
}
-static int pre_xfer(struct i2c_adapter *i2c_adap)
-{
- struct radeon_i2c_chan *i2c = i2c_get_adapdata(i2c_adap);
-
- radeon_i2c_do_lock(i2c, 1);
-
- return 0;
-}
-
-static void post_xfer(struct i2c_adapter *i2c_adap)
-{
- struct radeon_i2c_chan *i2c = i2c_get_adapdata(i2c_adap);
-
- radeon_i2c_do_lock(i2c, 0);
-}
-
/* hw i2c */
static u32 radeon_get_i2c_prescale(struct radeon_device *rdev)
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there
Hi Dave, Eugeni, Alex,
On Tue, 18 Oct 2011 11:02:00 +0100, Dave Airlie wrote:
> > This allows to avoid talking to a non-existent bus repeatedly until we
> > finally timeout. The non-existent bus is signalled by -ENXIO error,
> > provided by i2c_algo_bit:bit_doAddress call.
> >
> > As the advantage of such change, all the other routines which use
> > drm_get_edid would benefit for this timeout.
> >
> > This change should fix
> > https://bugs.freedesktop.org/show_bug.cgi?id=41059 and improve overall
> > edid detection timing by 10-30% in most cases, and by a much larger margin
> > in case of phantom outputs.
>
> Jean, Alex,
>
> I'm thinking of thowing this into -next, any objections?
This seems to be the wrong place for the test. The code below is really
testing for non-responsive (possibly not present) EDID, NOT
"non-existent adapter". Non-existent adapter should be checked in the
firmware if possible, or failing that, by testing the bus lines at bus
creation time, as i2c_algo_bit.bit_test=1 is doing. While this setting
has been known to cause trouble recently (not per se but because it was
triggering a bug somewhere else in the radeon driver), it might still
have value, and could be changed to a per-adapter setting by exporting
the test function (I have a patch ready doing exactly this) and letting
video drivers test their I2C buses and discard the non-working ones if
they want.
I am worried that the patch below will cause regressions on other
machines. There's a comment right before the loop which reads:
/* The core i2c driver will automatically retry the transfer if the
* adapter reports EAGAIN. However, we find that bit-banging transfers
* are susceptible to errors under a heavily loaded machine and
* generate spurious NAKs and timeouts. Retrying the transfer
* of the individual block a few times seems to overcome this.
*/
So the retries are there for a reason, and -ENXIO is exactly what you
get on spurious NAKs.
One thing which may make sense would be to make the retry count a
module parameter, instead of a hard-coded value, so that users can
lower the value if they care more about boot time than reliability. But
again, ideally problematic buses would not have been created in the
first place.
> >
> > Signed-off-by: Eugeni Dodonov
> > ---
> > drivers/gpu/drm/drm_edid.c | 5 +
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 7425e5c..1bca6d7 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -265,6 +265,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
> > unsigned char *buf,
> > }
> > };
> > ret = i2c_transfer(adapter, msgs, 2);
> > + if (ret == -ENXIO) {
> > + DRM_DEBUG_KMS("drm: skipping non-existent adapter
> > %s\n",
> > + adapter->name);
> > + break;
> > + }
> > } while (ret != 2 && --retries);
> >
> > return ret == 2 ? 0 : -1;
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there
On Tue, 18 Oct 2011 11:37:38 -0200, Eugeni Dodonov wrote: > On 10/18/2011 11:14, Jean Delvare wrote: > > Hi Dave, Eugeni, Alex, > > > > On Tue, 18 Oct 2011 11:02:00 +0100, Dave Airlie wrote: > >>> This allows to avoid talking to a non-existent bus repeatedly until we > >>> finally timeout. The non-existent bus is signalled by -ENXIO error, > >>> provided by i2c_algo_bit:bit_doAddress call. > >>> > >>> As the advantage of such change, all the other routines which use > >>> drm_get_edid would benefit for this timeout. > >>> > >>> This change should fix > >>> https://bugs.freedesktop.org/show_bug.cgi?id=41059 and improve overall > >>> edid detection timing by 10-30% in most cases, and by a much larger margin > >>> in case of phantom outputs. > >> > >> Jean, Alex, > >> > >> I'm thinking of thowing this into -next, any objections? > > > > This seems to be the wrong place for the test. The code below is really > > testing for non-responsive (possibly not present) EDID, NOT > > "non-existent adapter". Non-existent adapter should be checked in the > > firmware if possible, or failing that, by testing the bus lines at bus > > creation time, as i2c_algo_bit.bit_test=1 is doing. While this setting > > has been known to cause trouble recently (not per se but because it was > > triggering a bug somewhere else in the radeon driver), it might still > > have value, and could be changed to a per-adapter setting by exporting > > the test function (I have a patch ready doing exactly this) and letting > > video drivers test their I2C buses and discard the non-working ones if > > they want. > > > > I am worried that the patch below will cause regressions on other > > machines. There's a comment right before the loop which reads: > > > > /* The core i2c driver will automatically retry the transfer if the > > * adapter reports EAGAIN. However, we find that bit-banging transfers > > * are susceptible to errors under a heavily loaded machine and > > * generate spurious NAKs and timeouts. Retrying the transfer > > * of the individual block a few times seems to overcome this. > > */ > > > > So the retries are there for a reason, and -ENXIO is exactly what you > > get on spurious NAKs. > > You are right about the bit_test=1 testing, my initial attempt at the > fix did exactly that > (https://bugs.freedesktop.org/show_bug.cgi?id=41059, comments 14 and 15). > > The problem is that for some buses, namely HDMI ones in my testing, > bit_test-like tests always consider them as non-existent when the > connectivity is not setup; and as working when it is. Just to clarify: by "connectivity is setup", do you mean code in the driver setting the GPIO pin direction etc., or a display being connected to the graphics card? I admit I am a little surprised. I2C buses should have their lines up by default, thanks to pull-up resistors, and masters and slaves should merely drive the lines low as needed. The absence of slaves should have no impact on the line behavior. If the Intel graphics chips (or the driver) rely on the display to hold the lines high, I'd say this is a seriously broken design. As a side note, I thought that HDMI had the capability of presence detection, so there may be a better way to know if a display is connected or not, and this information could used to dynamically instantiate and delete the I2C buses? That way we could skip probing for EDID when there is no chance of success. In the specific case of the user report that started this discussion, the card has no HDMI port to start with, so it seems weird to even attempt to create a DDC bus for HDMI. If there no way the i915 driver can detect this case and skip the whole HDMI setup? As I understand it the radeon driver is able to do that. If the user report is an isolated case of faulty BIOS/firmware, you could consider handling it specifically (as the radeon driver does, too.) > So in any case, we > could not just blacklist them - when they do, they are gone for good, > and won't work anymore, and we need to keep re-trying them at each attempt. > > And in case of continuous pre-testing for the stuck bits and like when > driver attempts to get the edid (for example, when xrandr is run), we > still hit the same issue - the drm_do_probe_ddc_edid will continue to > retry the attempts until it reaches the maximum number of retries. E.g., there > is no way to tell drm_do_probe_ddc_edid to treat any return code as a > permanent failure and make it give up faster. Well, you could always do manual line testing of the I2C
Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there
Forgot to attach the patch, sorry. Here it is.
--
Jean Delvare
---
drivers/i2c/algos/i2c-algo-bit.c |8 ++--
include/linux/i2c-algo-bit.h |3 +++
2 files changed, 9 insertions(+), 2 deletions(-)
--- linux-3.1-rc10.orig/drivers/i2c/algos/i2c-algo-bit.c 2011-10-20 12:03:05.0 +0200
+++ linux-3.1-rc10/drivers/i2c/algos/i2c-algo-bit.c 2011-10-20 12:57:20.0 +0200
@@ -231,8 +231,11 @@ static int i2c_inb(struct i2c_adapter *i
/*
* Sanity check for the adapter hardware - check the reaction of
* the bus lines only if it seems to be idle.
+ * Must be called with i2c_adap->bus_lock held if adapter is registered.
+ * This is done by surrounding the call to i2c_bit_test_bus() with
+ * i2c_lock_adapter(i2c_adap) and i2c_unlock_adapter(i2c_adap).
*/
-static int test_bus(struct i2c_adapter *i2c_adap)
+int i2c_bit_test_bus(struct i2c_adapter *i2c_adap)
{
struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
const char *name = i2c_adap->name;
@@ -320,6 +323,7 @@ bailout:
return -ENODEV;
}
+EXPORT_SYMBOL_GPL(i2c_bit_test_bus);
/* - Utility functions
*/
@@ -623,7 +627,7 @@ static int __i2c_bit_add_bus(struct i2c_
int ret;
if (bit_test) {
- ret = test_bus(adap);
+ ret = i2c_bit_test_bus(adap);
if (ret < 0)
return -ENODEV;
}
--- linux-3.1-rc10.orig/include/linux/i2c-algo-bit.h 2011-07-22 04:17:23.0 +0200
+++ linux-3.1-rc10/include/linux/i2c-algo-bit.h 2011-10-20 12:54:33.0 +0200
@@ -50,4 +50,7 @@ struct i2c_algo_bit_data {
int i2c_bit_add_bus(struct i2c_adapter *);
int i2c_bit_add_numbered_bus(struct i2c_adapter *);
+/* Must be called with bus_lock held if adapter is registered */
+int i2c_bit_test_bus(struct i2c_adapter *);
+
#endif /* _LINUX_I2C_ALGO_BIT_H */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there
On Thu, 20 Oct 2011 14:33:39 +0200, Jean Delvare wrote: > That being said, even then the whole probe sequence shouldn't exceed > 10 ms, which I wouldn't expect a user to notice. The user-reported 4 > second delay when running xrandr can't be caused by this. 4 seconds for > 15 attempts is 250 ms per attempt, this looks like a timeout due to > non-functional bus, not a nack. Not that 250 ms is 1 jiffy for HZ=250, D'oh. Today is not my day, I can't believe I wrote this :/ 1 jiffy is obviously 4 ms only at HZ=250. So I can't explain why it is taking so much time on the reporter's machine. > which I guess was what the reporting user was running. So even with > your patch, there's still 750 ms to wait, I don't think this is > acceptable. You really have to fix that I2C bus or skip probing it. This conclusion probably still holds. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/kms: Make i2c buses faster
A udelay value of 20 leads to an I2C bus running at only 25 kbps. A
value of 40 as the nouveau driver has is even slower at 12.5 kbps. I2C
devices can typically operate faster than this, 50 kbps should be fine
for all devices (and compliant devices can always stretch the clock is
needed.)
FWIW, the vast majority of framebuffer drivers set udelay to 10
already. So set it to 10 in DRM drivers too, this will make EDID block
reads faster. We might even lower the udelay value later if no problem
is reported.
Signed-off-by: Jean Delvare
Cc: Eugeni Dodonov
Cc: Dave Airlie
Cc: Keith Packard
Cc: Alex Deucher
---
drivers/gpu/drm/i915/intel_i2c.c |2 +-
drivers/gpu/drm/nouveau/nouveau_i2c.c |2 +-
drivers/gpu/drm/radeon/radeon_i2c.c |2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
--- linux-3.1-rc10.orig/drivers/gpu/drm/i915/intel_i2c.c2011-07-22
04:17:23.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/i915/intel_i2c.c 2011-10-20
14:59:11.0 +0200
@@ -36,7 +36,7 @@
/* Intel GPIO access functions */
-#define I2C_RISEFALL_TIME 20
+#define I2C_RISEFALL_TIME 10
static inline struct intel_gmbus *
to_intel_gmbus(struct i2c_adapter *i2c)
--- linux-3.1-rc10.orig/drivers/gpu/drm/nouveau/nouveau_i2c.c 2011-07-22
04:17:23.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/nouveau/nouveau_i2c.c2011-10-20
15:14:36.0 +0200
@@ -217,7 +217,7 @@ nouveau_i2c_init(struct drm_device *dev,
if (entry->port_type < 6) {
i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 40;
+ i2c->bit.udelay = 10;
i2c->bit.timeout = usecs_to_jiffies(5000);
i2c->bit.data = i2c;
ret = i2c_bit_add_bus(&i2c->adapter);
--- linux-3.1-rc10.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-20
14:41:33.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-20
14:58:17.0 +0200
@@ -928,7 +928,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
i2c->algo.bit.setscl = set_clock;
i2c->algo.bit.getsda = get_data;
i2c->algo.bit.getscl = get_clock;
- i2c->algo.bit.udelay = 20;
+ i2c->algo.bit.udelay = 10;
/* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
* make this, 2 jiffies is a lot more reliable */
i2c->algo.bit.timeout = 2;
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/kms: Use the standard VESA timeout for DDC channels
The VESA specification suggests a 2.2 ms timeout on DDC channels.
Only the intel DRM driver implements this properly today, align all
drivers to the proper implementation.
Signed-off-by: Jean Delvare
Cc: Eugeni Dodonov
Cc: Dave Airlie
Cc: Keith Packard
Cc: Alex Deucher
---
drivers/gpu/drm/i915/intel_i2c.c |2 +-
drivers/gpu/drm/nouveau/nouveau_i2c.c |2 +-
drivers/gpu/drm/radeon/radeon_i2c.c |4 +---
3 files changed, 3 insertions(+), 5 deletions(-)
--- linux-3.1-rc10.orig/drivers/gpu/drm/i915/intel_i2c.c2011-10-20
14:59:11.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/i915/intel_i2c.c 2011-10-20
15:24:33.0 +0200
@@ -183,7 +183,7 @@ intel_gpio_create(struct drm_i915_privat
gpio->algo.getsda = get_data;
gpio->algo.getscl = get_clock;
gpio->algo.udelay = I2C_RISEFALL_TIME;
- gpio->algo.timeout = usecs_to_jiffies(2200);
+ gpio->algo.timeout = usecs_to_jiffies(2200);/* from VESA */
gpio->algo.data = gpio;
if (i2c_bit_add_bus(&gpio->adapter))
--- linux-3.1-rc10.orig/drivers/gpu/drm/nouveau/nouveau_i2c.c 2011-10-20
15:14:36.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/nouveau/nouveau_i2c.c2011-10-20
15:24:37.0 +0200
@@ -218,7 +218,7 @@ nouveau_i2c_init(struct drm_device *dev,
if (entry->port_type < 6) {
i2c->adapter.algo_data = &i2c->bit;
i2c->bit.udelay = 10;
- i2c->bit.timeout = usecs_to_jiffies(5000);
+ i2c->bit.timeout = usecs_to_jiffies(2200); /* from VESA */
i2c->bit.data = i2c;
ret = i2c_bit_add_bus(&i2c->adapter);
} else {
--- linux-3.1-rc10.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-20
14:58:17.0 +0200
+++ linux-3.1-rc10/drivers/gpu/drm/radeon/radeon_i2c.c 2011-10-20
15:24:41.0 +0200
@@ -929,9 +929,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
i2c->algo.bit.getsda = get_data;
i2c->algo.bit.getscl = get_clock;
i2c->algo.bit.udelay = 10;
- /* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
-* make this, 2 jiffies is a lot more reliable */
- i2c->algo.bit.timeout = 2;
+ i2c->algo.bit.timeout = usecs_to_jiffies(2200); /* from VESA */
i2c->algo.bit.data = i2c;
ret = i2c_bit_add_bus(&i2c->adapter);
if (ret) {
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/kms: Make i2c buses faster
Hi Alan, On Friday 21 October 2011 03:32:44 pm Alan Cox wrote: > On Fri, 21 Oct 2011 09:08:30 +0200 > > Jean Delvare wrote: > > A udelay value of 20 leads to an I2C bus running at only 25 kbps. A > > value of 40 as the nouveau driver has is even slower at 12.5 kbps. > > I2C devices can typically operate faster than this, 50 kbps should > > be fine for all devices (and compliant devices can always stretch > > the clock is needed.) > > That depends on your cable, drive and signal quality. It's not > something you just turn up because it seems a good idea. Reliability > is MUCH more important than shaving microseconds off monitor probe > times. We're talking milliseconds here, not microseconds. Namely 23 ms per 128- byte EDID block for intel and radeon, 69 ms for nouveau. I very much doubt that cable quality is an issue here. DDC is a very slow bus (even at the maximum speed of 100 kbps) compared to the video signal which is running through the other wires in the VGA or DDC cable. If you really reach the point where DDC becomes unreliable, I doubt the video signal is anywhere next to usable. More importantly, my initial motivation for sending this patch is that it may help prevent problems due to the software nature of bit-banged I2C. A faster clock means shorter (time-wise) messages, which in turn means less risks to be disturbed by interrupts or CPU overload. Last but not least, I can't believe that so many framebuffer drivers would have been using these settings for years if it caused trouble. Does anyone know at which speed hardware I2C engines are running the DDC bus on various graphics cards? -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/kms: Make i2c buses faster
Hi Alex, On Friday 21 October 2011 08:05:48 pm Alex Deucher wrote: > On Fri, Oct 21, 2011 at 10:16 AM, Jean Delvare > > Does anyone know at which speed hardware I2C engines are running > > the DDC bus on various graphics cards? > > IIRC, we generally target the radeon hw i2c engines to run at 50 khz. Then it doesn't seem unreasonable to try and achieve the same for bit- banged I2C. That's exactly what my patch is doing. -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there
Hi Eugeni, On Mon, 24 Oct 2011 12:40:14 -0200, Eugeni Dodonov wrote: > On Thu, Oct 20, 2011 at 10:33, Jean Delvare wrote: > > > Just to clarify: by "connectivity is setup", do you mean code in the > > driver setting the GPIO pin direction etc., or a display being > > connected to the graphics card? > > > > I admit I am a little surprised. I2C buses should have their lines up > > by default, thanks to pull-up resistors, and masters and slaves should > > merely drive the lines low as needed. The absence of slaves should have > > no impact on the line behavior. If the Intel graphics chips (or the > > driver) rely on the display to hold the lines high, I'd say this is a > > seriously broken design. > > > > As a side note, I thought that HDMI had the capability of presence > > detection, so there may be a better way to know if a display is > > connected or not, and this information could used to dynamically > > instantiate and delete the I2C buses? That way we could skip probing > > for EDID when there is no chance of success. > > Yes, I think so too. > > I admit I haven't got to the root of the problem here. My test was really > simple, borrowed from the test_bus() at i2c-algo-bit.c - without HDMI cable > plugged in, I was getting the "SDA stuck high" messages; with the cable > plugged in, I wasn't getting any of those. Just the HDMI cable, or with a screen at the other end? Either way, this smells like bad hardware design. The graphics card itself should pull the I2C bus lines up by default, it shouldn't rely on a cable (I'm not familiar with HDMI but I'm not sure if that makes sense at all) or external display (more likely) to do it. But I can also imagine that this could be a driver issue, maybe the GPIO lines aren't setup properly by the driver? I'm not familiar enough with the Intel graphics hardware and driver to tell, you'll know better. > But in any case, I haven't investigated it deeper in the hardware direction > after figuring out that drm_get_edid would retry its attempts for retreiving > the edid for 15 times, getting the -ENXIO error for all of them. > > > > Well, you could always do manual line testing of the I2C bus _before_ > > calling drm_do_probe_ddc_edid()? And skip the call if the test fails > > (i.e. the bus isn't ready for use.) As said before, I am willing to > > export bit_test if it helps any. I've attached a patch doing exactly > > this. Let me know if you want me to commit it. > > Yes, surely, I can do this. I did a similar test in the i915-specific patch, > checking if we can talk to i2c adapter pior to call drm_do_probe_ddc_edid, > but I thought that perhaps it would be easier for all the cards relying on > drm_get_edid to have a faster return path in case of problems. > > I am fine with any solution, if this problem is happening to appear on i915 > cards only, we could do this in our driver. It is that 15 attempts looks a > bit overkill. Yes, I agree, 15 retries makes no sense. And I like your original patch, it makes a lot of sense. > > Your proposed patch is better than I first thought. I had forgotten > > that i2c-algo-bit tries up to 3 times to get a first ack from a slave. > > So if i2c_transfer returns -ENXIO, this means that i2c-algo-bit has > > already attempted 3 times to contact the slave, with no reply, so > > there's probably no point going on. A communication problem with a > > present slave would have returned a different error code. > > > > Without your patch, a missing slave would cause 3 * 5 = 15 retries, > > which is definitely too much. > > > > That being said, even then the whole probe sequence shouldn't exceed > > 10 ms, which I wouldn't expect a user to notice. The user-reported 4 > > second delay when running xrandr can't be caused by this. 4 seconds for > > 15 attempts is 250 ms per attempt, this looks like a timeout due to > > non-functional bus, not a nack. Not that 250 ms is 1 jiffy for HZ=250, > > which I guess was what the reporting user was running. So even with > > your patch, there's still 750 ms to wait, I don't think this is > > acceptable. You really have to fix that I2C bus or skip probing it. > > Yep, true. I've followed the easiest route so far - find out where the > initial problem happens, and attempt at solving it. This change in > drm_get_edid solves the delay at its origin, but we certainly could have > additional delays propagated somewhere else. I couldn't reproduce the > original reporter's huge delay, so I looked at what was within my reach. Your fix is not really "at the o
Re: [PATCH] drm/radeon/kms: fix up gpio i2c mask bits for r4xx for real
On Mon, 21 Nov 2011 12:10:14 -0500, alexdeuc...@gmail.com wrote:
> From: Alex Deucher
>
> Fixes i2c test failures when i2c_algo_bit.bit_test=1.
>
> The hw doesn't actually require a mask, so just set it
> to the default mask bits for r1xx-r4xx radeon ddc.
>
> I missed this part the first time through.
>
> Signed-off-by: Alex Deucher
> Cc: sta...@kernel.org
> Cc: Jean Delvare
Acked-by: Jean Delvare
And the fix was tested successfully by one openSUSE 11.4 user, see:
https://bugzilla.novell.com/show_bug.cgi?id=691052#c37
Thanks Alex!
Note for stable: now that this missing part is ready, the first part
(6c47e5c23aa2a7c54ad7ac13af4bd56cd9e703bf) can finally find its way to
stable trees too.
>
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/radeon/radeon_atombios.c | 12
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
> b/drivers/gpu/drm/radeon/radeon_atombios.c
> index fecd705..933a2cd 100644
> --- a/drivers/gpu/drm/radeon/radeon_atombios.c
> +++ b/drivers/gpu/drm/radeon/radeon_atombios.c
> @@ -181,6 +181,18 @@ void radeon_atombios_i2c_init(struct radeon_device *rdev)
> gpio = &i2c_info->asGPIO_Info[i];
> i2c.valid = false;
>
> + /* r4xx mask is technically not used by the hw, so
> patch in the legacy mask bits */
> + if ((rdev->family == CHIP_R420) ||
> + (rdev->family == CHIP_R423) ||
> + (rdev->family == CHIP_RV410)) {
> + if ((le16_to_cpu(gpio->usClkMaskRegisterIndex)
> == 0x0018) ||
> + (le16_to_cpu(gpio->usClkMaskRegisterIndex)
> == 0x0019) ||
> + (le16_to_cpu(gpio->usClkMaskRegisterIndex)
> == 0x001a)) {
> + gpio->ucClkMaskShift = 0x19;
> + gpio->ucDataMaskShift = 0x18;
> + }
> + }
> +
> /* some evergreen boards have bad data for this entry */
> if (ASIC_IS_DCE4(rdev)) {
> if ((i == 7) &&
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Fix module parameter description format
From: Jean Delvare Subject: drm/radeon/kms: Fix module parameter description format Module parameter descriptions don't take a trailing \n, otherwise it breaks formatting of modinfo's output. Also add missing space after comma. Signed-off-by: Jean Delvare Cc: David Airlie Cc: Alex Deucher Cc: Jerome Glisse --- drivers/gpu/drm/radeon/radeon_drv.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.2-rc3.orig/drivers/gpu/drm/radeon/radeon_drv.c 2011-11-24 10:13:11.0 +0100 +++ linux-3.2-rc3/drivers/gpu/drm/radeon/radeon_drv.c 2011-11-29 10:25:27.0 +0100 @@ -140,7 +140,7 @@ module_param_named(vramlimit, radeon_vra MODULE_PARM_DESC(agpmode, "AGP Mode (-1 == PCI)"); module_param_named(agpmode, radeon_agpmode, int, 0444); -MODULE_PARM_DESC(gartsize, "Size of PCIE/IGP gart to setup in megabytes (32,64, etc)\n"); +MODULE_PARM_DESC(gartsize, "Size of PCIE/IGP gart to setup in megabytes (32, 64, etc)"); module_param_named(gartsize, radeon_gart_size, int, 0600); MODULE_PARM_DESC(benchmark, "Run benchmark"); -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau: Fix module parameter description formats
Module parameter descriptions don't take a trailing \n, otherwise it breaks formatting of modinfo's output. Also remove trailing space. Signed-off-by: Jean Delvare Cc: David Airlie Cc: Ben Skeggs --- drivers/gpu/drm/nouveau/nouveau_drv.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) --- linux-3.2-rc3.orig/drivers/gpu/drm/nouveau/nouveau_drv.c2011-11-09 15:53:32.0 +0100 +++ linux-3.2-rc3/drivers/gpu/drm/nouveau/nouveau_drv.c 2011-11-29 10:33:37.0 +0100 @@ -89,7 +89,7 @@ MODULE_PARM_DESC(override_conntype, "Ign int nouveau_override_conntype = 0; module_param_named(override_conntype, nouveau_override_conntype, int, 0400); -MODULE_PARM_DESC(tv_disable, "Disable TV-out detection\n"); +MODULE_PARM_DESC(tv_disable, "Disable TV-out detection"); int nouveau_tv_disable = 0; module_param_named(tv_disable, nouveau_tv_disable, int, 0400); @@ -104,23 +104,23 @@ module_param_named(tv_norm, nouveau_tv_n MODULE_PARM_DESC(reg_debug, "Register access debug bitmask:\n" "\t\t0x1 mc, 0x2 video, 0x4 fb, 0x8 extdev,\n" "\t\t0x10 crtc, 0x20 ramdac, 0x40 vgacrtc, 0x80 rmvio,\n" - "\t\t0x100 vgaattr, 0x200 EVO (G80+). "); + "\t\t0x100 vgaattr, 0x200 EVO (G80+)"); int nouveau_reg_debug; module_param_named(reg_debug, nouveau_reg_debug, int, 0600); -MODULE_PARM_DESC(perflvl, "Performance level (default: boot)\n"); +MODULE_PARM_DESC(perflvl, "Performance level (default: boot)"); char *nouveau_perflvl; module_param_named(perflvl, nouveau_perflvl, charp, 0400); -MODULE_PARM_DESC(perflvl_wr, "Allow perflvl changes (warning: dangerous!)\n"); +MODULE_PARM_DESC(perflvl_wr, "Allow perflvl changes (warning: dangerous!)"); int nouveau_perflvl_wr; module_param_named(perflvl_wr, nouveau_perflvl_wr, int, 0400); -MODULE_PARM_DESC(msi, "Enable MSI (default: off)\n"); +MODULE_PARM_DESC(msi, "Enable MSI (default: off)"); int nouveau_msi; module_param_named(msi, nouveau_msi, int, 0400); -MODULE_PARM_DESC(ctxfw, "Use external HUB/GPC ucode (fermi)\n"); +MODULE_PARM_DESC(ctxfw, "Use external HUB/GPC ucode (fermi)"); int nouveau_ctxfw; module_param_named(ctxfw, nouveau_ctxfw, int, 0400); -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/radeon/kms: Hide debugging message
Use the proper macro to issue the debugging message in
radeon_atif_call(). Otherwise we spam the log of many systems with a
message which looks like an error message of unknown origin, and could
thus confuse the user. Commit dc77de12dde95c8da39e4c417eb70c7d445cf84b
was a first step in this direction, but was not sufficient IMHO.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
Might be considered for stable, this is not a critical bug but it can
waste time of users and developers alike.
drivers/gpu/drm/radeon/radeon_acpi.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-3.2-rc3.orig/drivers/gpu/drm/radeon/radeon_acpi.c 2011-11-29
17:47:02.0 +0100
+++ linux-3.2-rc3/drivers/gpu/drm/radeon/radeon_acpi.c 2011-11-29
18:12:02.0 +0100
@@ -35,7 +35,8 @@ static int radeon_atif_call(acpi_handle
/* Fail only if calling the method fails and ATIF is supported */
if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
- printk(KERN_DEBUG "failed to evaluate ATIF got %s\n",
acpi_format_exception(status));
+ DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
+acpi_format_exception(status));
kfree(buffer.pointer);
return 1;
}
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/radeon/kms: Skip ACPI call to ATIF when possible
I am under the impression that it only makes sense to call the ATIF
method if the graphics device has an ACPI handle attached. So we could
skip the call altogether if there is no such handle.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
This is only tested on a system where the Radeon device has no ACPI
handle and there is no ATIF method. This should also be tested on
systems with ATIF, presumably laptops.
drivers/gpu/drm/radeon/radeon_acpi.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
--- linux-3.2-rc3.orig/drivers/gpu/drm/radeon/radeon_acpi.c 2011-11-29
18:12:02.0 +0100
+++ linux-3.2-rc3/drivers/gpu/drm/radeon/radeon_acpi.c 2011-11-29
18:59:46.0 +0100
@@ -51,13 +51,13 @@ int radeon_acpi_init(struct radeon_devic
acpi_handle handle;
int ret;
- /* No need to proceed if we're sure that ATIF is not supported */
- if (!ASIC_IS_AVIVO(rdev) || !rdev->bios)
- return 0;
-
/* Get the device handle */
handle = DEVICE_ACPI_HANDLE(&rdev->pdev->dev);
+ /* No need to proceed if we're sure that ATIF is not supported */
+ if (!ASIC_IS_AVIVO(rdev) || !rdev->bios || !handle)
+ return 0;
+
/* Call the ATIF method */
ret = radeon_atif_call(handle);
if (ret)
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/edid: Add support for extension blocks beyond the first
When 2 or more EDID extension blocks are present, segment must be
selected prior to reading the extended EDID block over the DDC
channel. Add support for this.
Signed-off-by: Jean Delvare
Cc: Adam Jackson
---
This needs testing by someone with access to such a display.
drivers/gpu/drm/drm_edid.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
--- linux-3.2-rc3.orig/drivers/gpu/drm/drm_edid.c 2011-11-09
15:53:31.0 +0100
+++ linux-3.2-rc3/drivers/gpu/drm/drm_edid.c2011-12-03 10:12:47.0
+0100
@@ -242,7 +242,8 @@ static int
drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
int block, int len)
{
- unsigned char start = block * EDID_LENGTH;
+ unsigned char segment = block >> 1;
+ unsigned char start = (block & 0x01) * EDID_LENGTH;
int ret, retries = 5;
/* The core i2c driver will automatically retry the transfer if the
@@ -254,6 +255,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter
do {
struct i2c_msg msgs[] = {
{
+ .addr = DDC_SEGMENT_ADDR,
+ .flags = 0,
+ .len= 1,
+ .buf= &segment,
+ }, {
.addr = DDC_ADDR,
.flags = 0,
.len= 1,
@@ -265,7 +271,18 @@ drm_do_probe_ddc_edid(struct i2c_adapter
.buf= buf,
}
};
- ret = i2c_transfer(adapter, msgs, 2);
+
+ /* Don't write segment if it is 0, for compatibility */
+ if (segment) {
+ ret = i2c_transfer(adapter, msgs, 3);
+ /* The E-DDC specification says that the first ack is
+* optional, so retry in ignore-nak mode if we get no
+* ack at first.
+*/
+ if (ret == -ENXIO)
+ msgs[0].flags |= I2C_M_IGNORE_NAK;
+ } else
+ ret = i2c_transfer(adapter, msgs + 1, 2);
} while (ret != 2 && --retries);
return ret == 2 ? 0 : -1;
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/kms: bail on BTC parts if MC ucode is missing
On Fri, 9 Dec 2011 11:31:54 -0500, alexdeuc...@gmail.com wrote:
> From: Alex Deucher
>
> We already do this for cayman, need to also do it for
> BTC parts. The default memory and voltage setup is not
> adequate for advanced operation. Continuing will
> result in an unusable display.
>
> Signed-off-by: Alex Deucher
> Cc: sta...@kernel.org
> Cc: Jean Delvare
Thanks Alex. I tested this fix and it works to some degree, but it
doesn't solve all the issues.
What works:
* No garbage on the screen.
* X falls back to the vesa driver.
What doesn't work:
* No console messages displayed.
* Trying to switch to a text console (e.g. Ctrl+Alt+F2) results in a
permanent black screen, I couldn't return to X after that.
So there must be some more integration work needed, probably not in the
radeon driver but rather in the drm_fb layer. But anyway, the situation
is definitely better with this patch than without, so it should be
applied and go to stable trees too:
Tested-by: Jean Delvare
> ---
> drivers/gpu/drm/radeon/evergreen.c | 12
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c
> b/drivers/gpu/drm/radeon/evergreen.c
> index 3e8054c..93c0348 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -3468,6 +3468,18 @@ int evergreen_init(struct radeon_device *rdev)
> evergreen_pcie_gart_fini(rdev);
> rdev->accel_working = false;
> }
> +
> + /* Don't start up if the MC ucode is missing on BTC parts.
> + * The default clocks and voltages before the MC ucode
> + * is loaded are not suffient for advanced operations.
> + */
> + if (ASIC_IS_DCE5(rdev)) {
> + if (!rdev->mc_fw && !(rdev->flags & RADEON_IS_IGP)) {
> + DRM_ERROR("radeon: MC ucode required for NI+.\n");
> + return -EINVAL;
> + }
> + }
> +
> return 0;
> }
>
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
i2c handling in nouveau driver
Hi Ben, I see in commit f553b79c03f0dbd52f6f03abe8233a2bef8cbd0d that you just changed the nouveau driver to use an internal i2c bit-banging implementation instead of i2c-algo-bit. Let me say here publicly that I disapprove this change. I believe this is a move in the wrong direction. Just because doing so fixed one bug you were seeing doesn't make it right. If i2c-algo-bit has problems, please report them to the maintainer (i.e. me) and have them fixed. If you have problems with it, others may do as well. If everyone stops using common pieces of code as soon as they have a problem, we will end up with a lot of duplicated code in the kernel, which is bad in many respects. In the commit message, you complain about the use of cond_resched() in i2c-algo-bit. You might as well be right, maybe it should be removed. It's been there pretty much forever (February 2002 at least) and while I can understand why it was put there, I would agree it isn't necessarily a good idea. Did you try removing it to see if it solved your problem? There is also a call to yield() somewhere else in the i2c-algo-bit driver. I remember having a discussion about it long ago, someone proposed to change it to cond_resched(), but it never happened. I admit I don't know the difference between cond_resched() and yield() so I can't really make a decision until someone explains it to me. I see that your reimplementation uses: #define T_HOLD 5000 which basically means you're running the I2C bus at ~100 kHz, while the original code had: port->bit.udelay = 40; which basically ran the bus at ~12.5 kHz, i.e. only a tad faster than the low limit allowed by SMBus. I warned some times ago that such low speeds could be problematic: http://lists.freedesktop.org/archives/dri-devel/2011-October/015512.html I even posted a patch: http://lists.freedesktop.org/archives/dri-devel/2011-October/015504.html Despite positive comments from the i915 and radeon driver maintainers, it was never applied. Did you try lowering the udelay value with i2c- algo-bit to see if it would solve your problem? David, any reason why you did not pick this patch? Should I resend it? In a split form maybe? Ben, again, I would really have appreciated if you had contacted me while investigating your NVS 300 issue, instead of silently running away from i2c-algo-bit. If the code is broken, let's fix it for the benefit of all users. Duplicating code around isn't going to help any. Thanks, -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/kms: Fix module parameter description format
On Wednesday 30 November 2011 05:59:57 pm Alex Deucher wrote: > On Wed, Nov 30, 2011 at 11:22 AM, Jean Delvare > wrote: > > From: Jean Delvare > > Subject: drm/radeon/kms: Fix module parameter description format > > > > Module parameter descriptions don't take a trailing \n, otherwise > > it breaks formatting of modinfo's output. Also add missing space > > after comma. > > > > Signed-off-by: Jean Delvare > > Cc: David Airlie > > Cc: Alex Deucher > > Cc: Jerome Glisse > > Reviewed-by: Alex Deucher David, Alex, can this patch be applied now? Thanks, -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/nouveau: Fix module parameter description formats
On Wednesday 30 November 2011 05:23:55 pm Jean Delvare wrote: > Module parameter descriptions don't take a trailing \n, otherwise it > breaks formatting of modinfo's output. Also remove trailing space. > > Signed-off-by: Jean Delvare > Cc: David Airlie > Cc: Ben Skeggs > --- > drivers/gpu/drm/nouveau/nouveau_drv.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > (...) David, Ben, can either of you please pick this patch now? It's pretty trivial and has been waiting for over a month already. Thanks, -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/kms: Make i2c buses faster
A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C devices can typically operate faster than this, 50 kbps should be fine for all devices (and compliant devices can always stretch the clock if needed.) FWIW, the vast majority of framebuffer drivers set udelay to 10 already. So set it to 10 in DRM drivers too, this will make EDID block reads faster. We might even lower the udelay value later if no problem is reported. Signed-off-by: Jean Delvare Acked-by: Eugeni Dodonov Cc: Dave Airlie Cc: Keith Packard Cc: Alex Deucher --- Already sent on: 2011-10-21. drivers/gpu/drm/i915/intel_i2c.c|2 +- drivers/gpu/drm/radeon/radeon_i2c.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- linux-3.3-rc1.orig/drivers/gpu/drm/i915/intel_i2c.c 2012-01-28 10:12:16.554480007 +0100 +++ linux-3.3-rc1/drivers/gpu/drm/i915/intel_i2c.c 2012-01-28 10:28:16.884668894 +0100 @@ -37,7 +37,7 @@ /* Intel GPIO access functions */ -#define I2C_RISEFALL_TIME 20 +#define I2C_RISEFALL_TIME 10 static inline struct intel_gmbus * to_intel_gmbus(struct i2c_adapter *i2c) --- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-28 10:12:16.555480007 +0100 +++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-28 10:28:16.886668894 +0100 @@ -924,7 +924,7 @@ struct radeon_i2c_chan *radeon_i2c_creat i2c->algo.bit.setscl = set_clock; i2c->algo.bit.getsda = get_data; i2c->algo.bit.getscl = get_clock; - i2c->algo.bit.udelay = 20; + i2c->algo.bit.udelay = 10; /* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always * make this, 2 jiffies is a lot more reliable */ i2c->algo.bit.timeout = 2; -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/radeon/kms: Use the standard VESA timeout for DDC channels
The VESA specification suggests a 2.2 ms timeout on DDC channels.
Use exactly that (as the i915 driver does) instead of hard-coding a
jiffy count.
Signed-off-by: Jean Delvare
Reviewed-by: Keith Packard
Cc: Dave Airlie
Cc: Alex Deucher
---
Already sent on: 2011-10-21.
drivers/gpu/drm/radeon/radeon_i2c.c |4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
--- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-28
10:37:51.722069517 +0100
+++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-28
10:39:26.996070929 +0100
@@ -925,9 +925,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
i2c->algo.bit.getsda = get_data;
i2c->algo.bit.getscl = get_clock;
i2c->algo.bit.udelay = 10;
- /* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always
-* make this, 2 jiffies is a lot more reliable */
- i2c->algo.bit.timeout = 2;
+ i2c->algo.bit.timeout = usecs_to_jiffies(2200); /* from VESA */
i2c->algo.bit.data = i2c;
ret = i2c_bit_add_bus(&i2c->adapter);
if (ret) {
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Fix device tree linkage of i2c buses
Properly set the parent device of i2c buses before registering them so
that they will show at the right place in the device tree (rather than
in /sys/devices directly.)
Signed-off-by: Jean Delvare
Cc: Dave Airlie
Cc: Alex Deucher
---
drivers/gpu/drm/radeon/radeon_i2c.c |1 +
1 file changed, 1 insertion(+)
--- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-26
09:12:40.0 +0100
+++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-27
22:58:58.235346502 +0100
@@ -897,6 +897,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
i2c->rec = *rec;
i2c->adapter.owner = THIS_MODULE;
i2c->adapter.class = I2C_CLASS_DDC;
+ i2c->adapter.dev.parent = &dev->pdev->dev;
i2c->dev = dev;
i2c_set_adapdata(&i2c->adapter, i2c);
if (rec->mm_i2c ||
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Fix device tree linkage of DP i2c buses too
Properly set the parent device of DP i2c buses before registering them
too.
Signed-off-by: Jean Delvare
Cc: Dave Airlie
Cc: Alex Deucher
---
I'm sorry, my previous patch missed the fact that DP i2c buses are
handled separately so they need the same fix.
drivers/gpu/drm/radeon/radeon_i2c.c |1 +
1 file changed, 1 insertion(+)
--- linux-3.3-rc1.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-31
08:55:20.596108280 +0100
+++ linux-3.3-rc1/drivers/gpu/drm/radeon/radeon_i2c.c 2012-01-31
08:55:22.334108306 +0100
@@ -956,6 +956,7 @@ struct radeon_i2c_chan *radeon_i2c_creat
i2c->rec = *rec;
i2c->adapter.owner = THIS_MODULE;
i2c->adapter.class = I2C_CLASS_DDC;
+ i2c->adapter.dev.parent = &dev->pdev->dev;
i2c->dev = dev;
snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"Radeon aux bus %s", name);
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/edid: Add support for extension blocks beyond the first
Hi Ville,
On Monday 13 February 2012 07:24:23 pm Ville Syrjälä wrote:
> On Wed, Dec 07, 2011 at 03:11:07PM +0100, Jean Delvare wrote:
> > When 2 or more EDID extension blocks are present, segment must be
> > selected prior to reading the extended EDID block over the DDC
> > channel. Add support for this.
> >
> > Signed-off-by: Jean Delvare
> > Cc: Adam Jackson
> > ---
> > This needs testing by someone with access to such a display.
> >
> > drivers/gpu/drm/drm_edid.c | 21 +++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > --- linux-3.2-rc3.orig/drivers/gpu/drm/drm_edid.c 2011-11-09
> > 15:53:31.0 +0100 +++
> > linux-3.2-rc3/drivers/gpu/drm/drm_edid.c2011-12-03
> > 10:12:47.0 +0100 @@ -242,7 +242,8 @@ static int
> > drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char
> > *buf, int block, int len)
> > {
> > - unsigned char start = block * EDID_LENGTH;
> > + unsigned char segment = block >> 1;
> > + unsigned char start = (block & 0x01) * EDID_LENGTH;
> > int ret, retries = 5;
> >
> > /* The core i2c driver will automatically retry the transfer if
> > the @@ -254,6 +255,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter
> > do {
> > struct i2c_msg msgs[] = {
> > {
> > + .addr = DDC_SEGMENT_ADDR,
> > + .flags = 0,
> > + .len= 1,
> > + .buf= &segment,
> > + }, {
> > .addr = DDC_ADDR,
> > .flags = 0,
> > .len= 1,
> > @@ -265,7 +271,18 @@ drm_do_probe_ddc_edid(struct i2c_adapter
> > .buf= buf,
> > }
> > };
> > - ret = i2c_transfer(adapter, msgs, 2);
> > +
> > + /* Don't write segment if it is 0, for compatibility */
> > + if (segment) {
> > + ret = i2c_transfer(adapter, msgs, 3);
> > + /* The E-DDC specification says that the first ack is
> > +* optional, so retry in ignore-nak mode if we get no
> > +* ack at first.
> > +*/
> > + if (ret == -ENXIO)
> > + msgs[0].flags |= I2C_M_IGNORE_NAK;
>
> This seems a bit wrong to me. The spec says that the ack for the
> segment address is "don't care", but for the segment pointer the ack
> is required (when segment != 0).
Correct.
> With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from
> a non E-DDC display, if we try to read segment != 0 from it. Of
> course we shouldn't do that unless the display lied to us about what
> extension blocks it provides.
Still correct.
> So I'm not sure if it would be better to trust that the display never
> lies about the extension blocks, or if we should just assume all
> E-DDC displays ack both segment addr and pointer.
I went for the former, as should be obvious from my proposed
implementation. Whether this is the best decision is impossible to tell
until the code is tested in the fields.
> The no-ack feature
> seems to there for backwards compatibility, for cases where the host
> always sends the segment addr/pointer even when reading segment 0
> (which your code doesn't do).
I agree.
> To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be
> split into two flags (one for addr, other for data).
This is correct, but this seemed a little overkill. I would only
implement this in i2c-core if it turns out to be absolutely necessary to
properly handle one real-world display.
I would suggest that my patch gets applied as is for now, and it can be
adjusted later if needed. It is certainly better than the current code
anyway.
Thanks for the review,
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/7] i2c: export bit-banging algo functions
Hi Daniel,
Sorry for the late reply.
On Tue, 14 Feb 2012 22:37:21 +0100, Daniel Vetter wrote:
> i915 has a hw i2c controller (gmbus) but for a bunch of stupid reasons
> we need to be able to fall back to the bit-banging algo on gpio pins.
>
> The current code sets up a 2nd i2c controller for the same i2c bus using
> the bit-banging algo. This has a bunch of issues, the major one being
> that userspace can directly access this fallback i2c adaptor behind
> the drivers back.
>
> But we need to frob a few registers before and after using fallback
> gpio bit-banging, so this horribly fails.
You could use the pre_xfer and post_xfer hooks together with a shared
mutex to solve the problem. But I agree its much cleaner to expose a
single adapter in the first place.
If you need to hot-switch between hardware and bit-banged I2C, I suggest
that you lock the bus while doing so, to avoid switching while a
transaction is in progress. This can be achieved with
i2c_lock_adapter() and i2c_unlock_adapter().
> The new plan is to only set up one i2c adaptor and transparently fall
> back to bit-banging by directly calling the xfer function of the bit-
> banging algo in the i2c core.
>
> To make that possible, export the 2 i2c algo functions.
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/i2c/algos/i2c-algo-bit.c | 12 +++-
> include/linux/i2c-algo-bit.h |4
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 525c734..ec1651a 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -531,8 +531,8 @@ static int bit_doAddress(struct i2c_adapter *i2c_adap,
> struct i2c_msg *msg)
> return 0;
> }
>
> -static int bit_xfer(struct i2c_adapter *i2c_adap,
> - struct i2c_msg msgs[], int num)
> +int i2c_bit_xfer(struct i2c_adapter *i2c_adap,
> + struct i2c_msg msgs[], int num)
> {
> struct i2c_msg *pmsg;
> struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> @@ -598,21 +598,23 @@ bailout:
> adap->post_xfer(i2c_adap);
> return ret;
> }
> +EXPORT_SYMBOL(i2c_bit_xfer);
>
> -static u32 bit_func(struct i2c_adapter *adap)
> +u32 i2c_bit_func(struct i2c_adapter *adap)
> {
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> I2C_FUNC_SMBUS_READ_BLOCK_DATA |
> I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
> }
> +EXPORT_SYMBOL(i2c_bit_func);
>
>
> /* -exported algorithm data: -
> */
>
> static const struct i2c_algorithm i2c_bit_algo = {
> - .master_xfer= bit_xfer,
> - .functionality = bit_func,
> + .master_xfer= i2c_bit_xfer,
> + .functionality = i2c_bit_func,
> };
>
> /*
> diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h
> index 4f98148..cdd6336 100644
> --- a/include/linux/i2c-algo-bit.h
> +++ b/include/linux/i2c-algo-bit.h
> @@ -47,6 +47,10 @@ struct i2c_algo_bit_data {
> int timeout;/* in jiffies */
> };
>
> +int i2c_bit_xfer(struct i2c_adapter *i2c_adap,
> + struct i2c_msg msgs[], int num);
> +u32 i2c_bit_func(struct i2c_adapter *adap);
> +
> int i2c_bit_add_bus(struct i2c_adapter *);
> int i2c_bit_add_numbered_bus(struct i2c_adapter *);
I have no problem with this in the principle. In the details, I don't
understand why you don't simply export i2c_bit_algo? This is one fewer
export and should serve the same purpose - even allowing faster
initializations in some cases.
Looking a bit more to the i2c-algo-bit code, I also notice that
bypassing i2c_bit_add_bus() means you'll miss some of its features,
namely bus testing, default retries value and warning on non-compliant
buses. While none of these are mandatory, it may make sense to export a
new function i2c_bit_add_numbered_bus() which would call
__i2c_bit_add_bus() with no callback function. If you do that, you may
not have to export anything else.
I leave it up to you which way you want to implement it, all are fine
with me, and we can always change later if more use cases show up which
would work better with a different implementation.
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/7] i2c: export bit-banging algo functions
On Mon, 27 Feb 2012 23:52:23 +0100, Daniel Vetter wrote: > On Mon, Feb 27, 2012 at 11:20:40PM +0100, Jean Delvare wrote: > > If you need to hot-switch between hardware and bit-banged I2C, I suggest > > that you lock the bus while doing so, to avoid switching while a > > transaction is in progress. This can be achieved with > > i2c_lock_adapter() and i2c_unlock_adapter(). > > The drm/i915 xfer function is currently protected by a single mutex > (because the hw i2c controller can only be used on one bus at a time). So > I think we're covered. Also we do the fallback in our xfer function when > we notice that things don't quite work as they should, so we actually want > to switch while a transfer is in progress. Dunno whether that's the best > approach, but the current code is structured like this. This seems perfectly sane. > (...) > I've noticed that the the bit-banging algo does some test upon > registration but figured that it's not worth the fuss to add a new init > function that avoids the registration. Note that thanks to the way things are implemented in i2c-algo-bit, you'd really only have to write a new wrapper around __i2c_bit_add_bus(), so this is really only 1 line of code. That being said, I am not insisting, it's really up to you. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] i2c: export bit-banging algo functions
On Tue, 28 Feb 2012 00:39:39 +0100, Daniel Vetter wrote:
> i915 has a hw i2c controller (gmbus) but for a bunch of stupid reasons
> we need to be able to fall back to the bit-banging algo on gpio pins.
>
> The current code sets up a 2nd i2c controller for the same i2c bus using
> the bit-banging algo. This has a bunch of issues, the major one being
> that userspace can directly access this fallback i2c adaptor behind
> the drivers back.
>
> But we need to frob a few registers before and after using fallback
> gpio bit-banging, so this horribly fails.
>
> The new plan is to only set up one i2c adaptor and transparently fall
> back to bit-banging by directly calling the xfer function of the bit-
> banging algo in the i2c core.
>
> To make that possible, export the 2 i2c algo functions.
>
> v2: As suggested by Jean Delvare, simply export the i2c_bit_algo
> vtable instead of the individual functions.
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/i2c/algos/i2c-algo-bit.c |3 ++-
> include/linux/i2c-algo-bit.h |1 +
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 525c734..ad0459c 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -610,10 +610,11 @@ static u32 bit_func(struct i2c_adapter *adap)
>
> /* -exported algorithm data: -
> */
>
> -static const struct i2c_algorithm i2c_bit_algo = {
> +const struct i2c_algorithm i2c_bit_algo = {
> .master_xfer= bit_xfer,
> .functionality = bit_func,
> };
> +EXPORT_SYMBOL(i2c_bit_algo);
>
> /*
> * registering functions to load algorithms at runtime
> diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h
> index 4f98148..584ffa0 100644
> --- a/include/linux/i2c-algo-bit.h
> +++ b/include/linux/i2c-algo-bit.h
> @@ -49,5 +49,6 @@ struct i2c_algo_bit_data {
>
> int i2c_bit_add_bus(struct i2c_adapter *);
> int i2c_bit_add_numbered_bus(struct i2c_adapter *);
> +extern const struct i2c_algorithm i2c_bit_algo;
>
> #endif /* _LINUX_I2C_ALGO_BIT_H */
Acked-by: Jean Delvare
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] i2c-algo-bit: Fix spurious SCL timeouts under heavy load
Hi Ville,
On Wed, 14 Mar 2012 10:32:52 +0200, Ville Syrjälä wrote:
> When the system is under heavy load, there can be a significant delay
> between the getscl() and time_after() calls inside sclhi(). That delay
> may cause the time_after() check to trigger after SCL has gone high,
> causing sclhi() to return -ETIMEDOUT.
>
> To fix the problem, double check that SCL is still low after the
> timeout has been reached, before deciding to return -ETIMEDOUT.
>
> Signed-off-by: Ville Syrjälä
> ---
> I can easily reproduce these spurious timeouts on my HP-compaq nc6000
> laptop with the radeon kms driver. It's enough to have a -j2 kernel
> build running, and simultaneosly issue xrandr commands in a
> terminal. Calling xrandr will cause the driver to re-read the EDID
> from the display. A significant number of the EDID reads will fail.
> With this fix I have yet to see any failed EDID reads.
Thanks for describing a test case, I was able to reproduce the problem
easily by following your instructions. The problem is real, even with
the pending fixes I have to radeon's I2C implementation.
I only have one concern about your implementation:
>
> drivers/i2c/algos/i2c-algo-bit.c |4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 525c734..d25112e 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -104,9 +104,11 @@ static int sclhi(struct i2c_algo_bit_data *adap)
>* are processing data internally.
>*/
> if (time_after(jiffies, start + adap->timeout))
> - return -ETIMEDOUT;
> + break;
> cond_resched();
> }
> + if (!getscl(adap))
> + return -ETIMEDOUT;
This means double-check even in the most common case where time_after()
didn't cause the loop break. From a performance perspective, this seems
undesirable. What would you think of the alternative fix below?
--- linux-3.3-rc7.orig/drivers/i2c/algos/i2c-algo-bit.c 2012-03-15
09:33:10.232176790 +0100
+++ linux-3.3-rc7/drivers/i2c/algos/i2c-algo-bit.c 2012-03-15
14:52:48.127778459 +0100
@@ -103,8 +103,14 @@ static int sclhi(struct i2c_algo_bit_dat
* chips may hold it low ("clock stretching") while they
* are processing data internally.
*/
- if (time_after(jiffies, start + adap->timeout))
+ if (time_after(jiffies, start + adap->timeout)) {
+ /* Test one last time, as we may have been preempted
+* between last check and timeout test.
+*/
+ if (getscl(adap))
+ break;
return -ETIMEDOUT;
+ }
cond_resched();
}
#ifdef DEBUG
Functionally it should be equivalent to your proposal, but faster. I'll
apply that (and send for stable inclusion.)
Thanks,
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i2c handling in nouveau driver
Hi Ben, I'm sorry for the very very late reply. Please do not jump to the conclusion that I do not care - I do! Just I am very busy, just as you. On Wednesday 11 January 2012 01:40:58 pm Ben Skeggs wrote: > On Wed, 2012-01-11 at 11:17 +0100, Jean Delvare wrote: > > In the commit message, you complain about the use of cond_resched() > > in i2c-algo-bit. You might as well be right, maybe it should be > > removed. It's been there pretty much forever (February 2002 at > > least) and while I can understand why it was put there, I would > > agree it isn't necessarily a good idea. Did you try removing it to > > see if it solved your problem? > > I don't disagree, and I held off a long time before finally > committing it to the nouveau repository due to not liking the > duplication. My final decision to do it was admittedly due to > having very very limited time and a *lot* of other things to work > on. And this solution worked now. > > The fact i2c-algo-bit can't be used from an atomic context was > probably a convenient excuse I guess, but I did also figure there > were good reasons for it and there'd likely be push-back at > attempting to change that. Again, due to having loads of other > things to do, I took the quick approach. I don't think there actually is any good reason, and not only I wouldn't push back if someone wants to get rid of the call to cond_resched(), but I would even push for it to happen ;) Now the question is, what do we replace it with? Nothing? cpu_relax()? yield()? udelay()? I guess yield() would have the same property of making the code not callable from atomic context. Not having anything might be harsh from a CPU and I/O load perspective, as it would be a tight loop lasting for several milliseconds. So this leaves us with cpu_relax() and udelay(). Any preference? > (...) > I'd be more than happy to have these issues (nvs300 and running from > an atomic context) solved in i2c-algo-bit, and will gladly revert > the nouveau changes once it is. Let me know how I can help you with > that. Incidentally I just sent a fix upstream for a bug reported by Ville Syrjälä in i2c-algo-bit, that would cause spurious timeouts on heavy CPU load. I am curious if it would have helped in your case. Please take a look: http://marc.info/?l=linux-kernel&m=133182203301053&w=2 And if you think it could help, I would appreciate if you could give it a try with the nouveau driver and see if it actually does. Thanks, -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/kms: Make i2c buses faster
Hi Keith, Sorry for the late reply. On Sunday 29 January 2012 02:26:25 am Keith Packard wrote: > On Sat, 28 Jan 2012 11:07:09 +0100, Jean Delvare > wrote: > > A udelay value of 20 leads to an I2C bus running at only 25 kbps. > > I2C devices can typically operate faster than this, 50 kbps should > > be fine for all devices (and compliant devices can always stretch > > the clock if needed.) > > > > FWIW, the vast majority of framebuffer drivers set udelay to 10 > > already. So set it to 10 in DRM drivers too, this will make EDID > > block reads faster. We might even lower the udelay value later if > > no problem is reported. > > That runs the DDC at a whopping 50kbps, which is half of the maximum > rate specified in the DDC/CI standard. I don't know if we can count > on clock stretching (http://www.i2c-bus.org/clock-stretching/), but > if so, I don't know why we wouldn't just go to the standard 100kbps > data rate and be done with it. We may end up doing that. I wanted to play it safe for now as at least Alan Cox expressed concerns with increasing the speed of DDC buses. I don't share them, but being cautious can't hurt. Clock stretching is optional, each slave is free to implement it or not. I very much doubt it is needed when reading and EDID though, even at 100 kbps. Typically what takes time is writing to EEPROMs, but in general EEPROMs will buffer the write and simply stop responding to their slave addresses until done. This is why most EEPROMs have a documented write page size. Displays must be interoperable by design, so I'd hope that every serious display maker would only use EEPROMs that can either cope with 100 kbps or do clock stretching as needed. I have no doubt crappy hardware exists, but I'd rather decrease the clock speed on repeated errors than default to a slow clock speed. Users with good hardware should get the best out of it. > Might be nice to see what frequency Windows uses for i2c; anyone want > to pull a vga cable apart and hook up a logic analyser? Can't do that, sorry. It would certainly be valuable if someone has the time, hardware and interest, however I don't think "Windows" uses a frequency, rather separate video drivers are likely to have their own implementation and speed (if nothing else, simply because recent video cards use hardware I2C engines rather than bit-banging.) -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/radeon/kms: Use the standard VESA timeout for DDC channels
Hi Keith, On Sunday 29 January 2012 02:34:05 am Keith Packard wrote: > On Sat, 28 Jan 2012 11:08:58 +0100, Jean Delvare > wrote: > > The VESA specification suggests a 2.2 ms timeout on DDC channels. > > Use exactly that (as the i915 driver does) instead of hard-coding a > > jiffy count. > > The Vesa spec seems to say 2ms; at least according to the DDC/CI spec > paragraph 6.6. To be honest, I did not read it, I don't even think it is publicly available, is it? I did naively trust the comment in radeon_i2c.c: "vesa says 2.2 ms is enough". Not sure where this value came from if you claim the spec says 2 ms. Jerome, you wrote this comment in the first place, want to comment on that? The Intel drivers are using 2.2 ms too, since November 2008 (added to i915 driver by Jesse Barnes.) Maybe this all originates from X11 driver code? > usecs_to_jiffies rounds the value it gets up, so we > should never get an interval less than 2ms if we pass 2000us to it. Technically I agree, all we need to agree on is the value, 2000 us or 2200 us. I don't mind either way, I am not aware of specific complaints about the current code, I was just trying to clean things up, as hard- coding jiffy counts is bad and having different timeouts in the various drivers makes little sense IMHO. -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/i915: Run DDC buses at 50 kbps
A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C devices can typically operate faster than this, 50 kbps should be fine for all devices (and compliant devices can always stretch the clock if needed.) FWIW, the vast majority of framebuffer drivers set udelay to 10 already. So set it to 10 in DRM drivers too, this will make EDID block reads faster. We might even lower the udelay value later if no problem is reported. Signed-off-by: Jean Delvare Acked-by: Eugeni Dodonov Cc: Dave Airlie Cc: Keith Packard --- Changes since v1: * Split per driver to make merging easier. * Make the subject line more accurate. drivers/gpu/drm/i915/intel_i2c.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.4-rc0.orig/drivers/gpu/drm/i915/intel_i2c.c 2012-03-21 13:43:33.750915151 +0100 +++ linux-3.4-rc0/drivers/gpu/drm/i915/intel_i2c.c 2012-03-21 13:44:05.923915628 +0100 @@ -37,7 +37,7 @@ /* Intel GPIO access functions */ -#define I2C_RISEFALL_TIME 20 +#define I2C_RISEFALL_TIME 10 static inline struct intel_gmbus * to_intel_gmbus(struct i2c_adapter *i2c) -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/radeon/kms: Run DDC buses at 50 kbps
A udelay value of 20 leads to an I2C bus running at only 25 kbps. I2C devices can typically operate faster than this, 50 kbps should be fine for all devices (and compliant devices can always stretch the clock if needed.) FWIW, the vast majority of framebuffer drivers set udelay to 10 already. So set it to 10 in DRM drivers too, this will make EDID block reads faster. We might even lower the udelay value later if no problem is reported. Signed-off-by: Jean Delvare Reviewed-by: Alex Deucher Cc: Dave Airlie --- Changes since v1: * Split per driver to make merging easier. * Make the subject line more accurate. drivers/gpu/drm/radeon/radeon_i2c.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-3.4-rc0.orig/drivers/gpu/drm/radeon/radeon_i2c.c 2012-03-21 13:43:33.753915151 +0100 +++ linux-3.4-rc0/drivers/gpu/drm/radeon/radeon_i2c.c 2012-03-21 13:48:01.619472673 +0100 @@ -925,7 +925,7 @@ struct radeon_i2c_chan *radeon_i2c_creat i2c->algo.bit.setscl = set_clock; i2c->algo.bit.getsda = get_data; i2c->algo.bit.getscl = get_clock; - i2c->algo.bit.udelay = 20; + i2c->algo.bit.udelay = 10; /* vesa says 2.2 ms is enough, 1 jiffy doesn't seem to always * make this, 2 jiffies is a lot more reliable */ i2c->algo.bit.timeout = 2; -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/i915: Run DDC buses at 50 kbps
On Thursday 22 March 2012 09:50:23 pm Daniel Vetter wrote: > On Wed, Mar 21, 2012 at 02:29:47PM +0100, Jean Delvare wrote: > > A udelay value of 20 leads to an I2C bus running at only 25 kbps. > > I2C devices can typically operate faster than this, 50 kbps should > > be fine for all devices (and compliant devices can always stretch > > the clock if needed.) > > > > FWIW, the vast majority of framebuffer drivers set udelay to 10 > > already. So set it to 10 in DRM drivers too, this will make EDID > > block reads faster. We might even lower the udelay value later if > > no problem is reported. > > > > Signed-off-by: Jean Delvare > > Acked-by: Eugeni Dodonov > > Cc: Dave Airlie > > Cc: Keith Packard > > Fyi this already got merged int Dave's tree (the unsplit version) as: > > commit 1849ecb22fb3b5d57b65e7369a3957adf9f26f39 > Author: Jean Delvare > Date: Sat Jan 28 11:07:09 2012 +0100 > > drm/kms: Make i2c buses faster Thanks Daniel, I just noticed this as it got merged into Linus tree last night. I had not received any ack from Dave and the git repository mentioned in MAINTAINERS is wrong so I couldn't check whether my patches were already applied or not. Anyway, the important thing is that they are in Linus' tree now. -- Jean Delvare Suse L3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [lm-sensors] Incorrect Temperature Readings
Hi Scott, Please don't top-post. On Mon, 26 Mar 2012 15:59:56 -0400, Scott Ondercin wrote: > -I've been running the computer for over an hour currently and it feels > perfectly fine. Even the hard drive feels only mildly warm. Nonetheless, > I'm getting 96C readings for my PCI Adapter. > > Is there a way to modify my /etc/sensors.conf file to change the input levels > or high/crit temps for the adapter? I may not be looking properly, but I > can't find its chip name. This default configuration file only deals with a limited number of chips for which a generic configuration is possible. In your case, please create a new configuration file under /etc/sensors.d and add your statements here, starting with chip "nouveau-pci-0100" statement. You should indeed be able to adjust the thermal thresholds of your graphics adapter with the nouveau driver. That being said... Either you trust the temperature value and you should be worried and not adjust the thresholds, or you don't trust it and it will be difficult to decide what values to set the thresholds to. Anyway, you said that you had to replace the fan a few months ago. Do you remember the temperatures you got before that? Usually there is a common cooling system for CPU and GPU in laptops, so I suspect that your broke cooling for the GPU when changing the fan. It's really easy to break, if the heatsink is no longer in straight contact with the GPU, cooling become totally inefficient. You could temporarily try the binary driver from nvidia to have a comparison point. If it returns a value much lower than nouveau is reporting, that would be a bug in nouveau. If it returns the same value then your hardware is really in danger. > Also, is there a way to run "module-init-tools" without getting > "stop/waiting"? This is a completely unrelated question and I don't even understand it. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon: only add the mm i2c bus if the hw_i2c module param is set
On Tue, 10 Apr 2012 12:14:27 -0400, alexdeuc...@gmail.com wrote: > From: Alex Deucher > > It seems it can corrupt the monitor EDID in certain cases on certain > boards when running sensors detect. It's rarely used anyway outside > of AIW boards. > > http://lists.lm-sensors.org/pipermail/lm-sensors/2012-April/035847.html > http://lists.freedesktop.org/archives/xorg/2011-January/052239.html > > Signed-off-by: Alex Deucher > Cc: sta...@vger.kernel.org > Cc: Jean Delvare > --- > drivers/gpu/drm/radeon/radeon_i2c.c |4 > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c > b/drivers/gpu/drm/radeon/radeon_i2c.c > index 85bcfc8..3edec1c 100644 > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > @@ -900,6 +900,10 @@ struct radeon_i2c_chan *radeon_i2c_create(struct > drm_device *dev, > struct radeon_i2c_chan *i2c; > int ret; > > + /* don't add the mm_i2c bus unless hw_i2c is enabled */ > + if (rec->mm_i2c && (radeon_hw_i2c == 0)) > + return NULL; > + > i2c = kzalloc(sizeof(struct radeon_i2c_chan), GFP_KERNEL); > if (i2c == NULL) > return NULL; Acked-by: Jean Delvare Thanks Alex! -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
glxgears crashes radeon driver (kernel 2.6.34-rc6-git3)
irrelevant, as I get many of these for Xorg when I stop the machine since months. Let me know if I can help debugging this by providing extra information or doing tests. This is low priority for me (I don't normally do 3D on that machine) but you probably want the bug fixed before 2.6.34-final. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: glxgears crashes radeon driver (kernel 2.6.34-rc6-git3)
On Wed, 5 May 2010 16:40:54 +0200, Jean Delvare wrote:
> With kernels 2.6.34-rc6-git{1,2,3}, glxgears crashes the radeon driver
> on my machine. Kernel 2.6.33.3 works fine. (...)
Still happens with -git6. Should I start bisecting?
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: glxgears crashes radeon driver (kernel 2.6.34-rc6-git3)
On Sun, 9 May 2010 13:36:35 +0200, Jean Delvare wrote:
> On Wed, 5 May 2010 16:40:54 +0200, Jean Delvare wrote:
> > With kernels 2.6.34-rc6-git{1,2,3}, glxgears crashes the radeon driver
> > on my machine. Kernel 2.6.33.3 works fine. (...)
>
> Still happens with -git6. Should I start bisecting?
Bisection says that the faulty commit is:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b4fe945405e477cded91772b4fec854705443dd5
It doesn't revert cleanly from 2.6.34-rc6+, I'll try a manual revert
tomorrow. My device is a Radeon 9200, so r200, if it helps.
Thanks,
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] radeon: Fix 3 regressions (was: glxgears crashes radeon driver)
Yes, it's me again ;)
On Sun, 9 May 2010 22:58:30 +0200, Jean Delvare wrote:
> Bisection says that the faulty commit is:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=b4fe945405e477cded91772b4fec854705443dd5
>
> It doesn't revert cleanly from 2.6.34-rc6+, I'll try a manual revert
> tomorrow. My device is a Radeon 9200, so r200, if it helps.
I can confirm that manually reverting the patch above helps somewhat.
The crash is gone. Now I get an essentially black image in glxgears,
which may be a different bug.
(gdb) file drivers/gpu/drm/radeon/radeon.o
Reading symbols from
/home/khali/src/linux-2.6.34-rc6/drivers/gpu/drm/radeon/radeon.o...done.
(gdb) list *(radeon_cp_cmdbuf+0x15e)
0xa58e is in radeon_cp_cmdbuf (drivers/gpu/drm/radeon/radeon_state.c:2921).
2916
2917drm_radeon_cmd_header_t *header;
2918header = drm_buffer_read_object(cmdbuf->buffer,
2919sizeof(stack_header), &stack_header);
2920
2921switch (header->header.cmd_type) {
2922case RADEON_CMD_PACKET:
2923DRM_DEBUG("RADEON_CMD_PACKET\n");
2924if (radeon_emit_packets
2925(dev_priv, file_priv, *header, cmdbuf)) {
(gdb)
I took a look at the above commit and found 3 bugs in it, one of which
is directly responsible for my crash. Candidate patch below.
Note 1: Why one would call radeon_cp_cmdbuf() with an empty buffer is
beyond me, but hey, what do I know about graphics drivers.
Note 2: Even with this patch, glxgears is all black unless I move the
window around, so there's one more bug to track. I guess I'm up for one
more bisection afternoon.
* * * * *
From: Jean Delvare
Subject: radeon: Fix 3 regressions
Commit b4fe945405e477cded91772b4fec854705443dd5 introduced 3 bugs,
fix them:
* Use the right command dword for second packet offset in
RADEON_CNTL_PAINT/BITBLT_MULTI.
* Don't leak memory if drm_buffer_copy_from_user() fails.
* Don't call drm_buffer_unprocessed() unless drm_buffer_alloc() and
drm_buffer_copy_from_user() have been called successfully first.
Signed-off-by: Jean Delvare
Cc: Pauli Nieminen
Cc: Dave Airlie
---
I can certainly provide 3 separate patches for clarity if you prefer.
drivers/gpu/drm/radeon/radeon_state.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
--- linux-2.6.34-rc6.orig/drivers/gpu/drm/radeon/radeon_state.c 2010-05-10
09:20:07.0 +0200
+++ linux-2.6.34-rc6/drivers/gpu/drm/radeon/radeon_state.c 2010-05-10
15:19:19.0 +0200
@@ -424,7 +424,7 @@ static __inline__ int radeon_check_and_f
if ((*cmd & RADEON_GMC_SRC_PITCH_OFFSET_CNTL) &&
(*cmd & RADEON_GMC_DST_PITCH_OFFSET_CNTL)) {
u32 *cmd3 = drm_buffer_pointer_to_dword(cmdbuf->buffer,
3);
- offset = *cmd << 10;
+ offset = *cmd3 << 10;
if (radeon_check_and_fixup_offset
(dev_priv, file_priv, &offset)) {
DRM_ERROR("Invalid second packet offset\n");
@@ -2895,9 +2895,12 @@ static int radeon_cp_cmdbuf(struct drm_d
return rv;
rv = drm_buffer_copy_from_user(cmdbuf->buffer, buffer,
cmdbuf->bufsz);
- if (rv)
+ if (rv) {
+ drm_buffer_free(cmdbuf->buffer);
return rv;
- }
+ }
+ } else
+ goto done;
orig_nbox = cmdbuf->nbox;
@@ -2905,8 +2908,7 @@ static int radeon_cp_cmdbuf(struct drm_d
int temp;
temp = r300_do_cp_cmdbuf(dev, file_priv, cmdbuf);
- if (cmdbuf->bufsz != 0)
- drm_buffer_free(cmdbuf->buffer);
+ drm_buffer_free(cmdbuf->buffer);
return temp;
}
@@ -3012,16 +3014,15 @@ static int radeon_cp_cmdbuf(struct drm_d
}
}
- if (cmdbuf->bufsz != 0)
- drm_buffer_free(cmdbuf->buffer);
+ drm_buffer_free(cmdbuf->buffer);
+ done:
DRM_DEBUG("DONE\n");
COMMIT_RING();
return 0;
err:
- if (cmdbuf->bufsz != 0)
- drm_buffer_free(cmdbuf->buffer);
+ drm_buffer_free(cmdbuf->buffer);
return -EINVAL;
}
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
Hi Justin, On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: > could be a right solution, could be wrong > here is the warning: > CC drivers/i2c/i2c-core.o > drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': > drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used > > Signed-off-by: Justin P. Mattock > > --- > drivers/i2c/i2c-core.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 1cca263..79c6c26 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > mutex_lock(&core_lock); > dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >__process_new_adapter); > + if(!dummy) > + dummy = 0; One word: scripts/checkpatch.pl In other news, the above is just plain wrong. First we force people to read the result of bus_for_each_drv() and then when they do and don't need the value, gcc complains, so we add one more layer of useless code, which developers and possibly tools will later wonder and complain about? I can easily imagine that a static code analyzer would spot the above code as being a potential bug. Let's stop this madness now please. Either __must_check goes away from bus_for_each_drv() and from every other function which raises this problem, or we must disable that new type of warning gcc 4.6.0 generates. Depends which warnings we value more, as we can't sanely have both. > mutex_unlock(&core_lock); > > return 0; -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
On Tue, 15 Jun 2010 09:20:39 -0700, David Daney wrote: > On 06/15/2010 04:40 AM, Jean Delvare wrote: > > __process_new_adapter() calls i2c_do_add_adapter() which always returns > > 0. Why should I check the return value of bus_for_each_drv() when I > > know it will always be 0 by construction? > > > > Also note that the same function is also called through > > bus_for_each_dev() somewhere else in i2c-core, and there is no warning > > there because bus_for_each_dev() is not marked __must_check. How > > consistent is this? If bus_for_each_dev() is OK without __must_check, > > then I can't see why bus_for_each_drv() wouldn't be. > > Well, I would advocate removing the __must_check then. I have just sent a patch to LKML doing exactly this. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon: Fix stack data leak
Always zero-init a structure on the stack which is returned by a
function. Otherwise you may leak random stack data from previous
function calls.
This fixes the following warning I was seeing:
CC [M] drivers/gpu/drm/radeon/radeon_atombios.o
drivers/gpu/drm/radeon/radeon_atombios.c: In function
"radeon_atom_get_hpd_info_from_gpio":
drivers/gpu/drm/radeon/radeon_atombios.c:261: warning: "hpd.plugged_state" is
used uninitialized in this function
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
As a side, note, passing whole structures on the stack this way seems
suboptimal. Passing structures by address would be faster (and safer,
as it stands.)
drivers/gpu/drm/radeon/radeon_atombios.c |2 ++
1 file changed, 2 insertions(+)
--- linux-2.6.36-rc0.orig/drivers/gpu/drm/radeon/radeon_atombios.c
2010-08-15 10:24:49.0 +0200
+++ linux-2.6.36-rc0/drivers/gpu/drm/radeon/radeon_atombios.c 2010-08-15
13:52:31.0 +0200
@@ -226,6 +226,8 @@ static struct radeon_hpd radeon_atom_get
struct radeon_hpd hpd;
u32 reg;
+ memset(&hpd, 0, sizeof(struct radeon_hpd));
+
if (ASIC_IS_DCE4(rdev))
reg = EVERGREEN_DC_GPIO_HPD_A;
else
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Error message on RV710: reserve failed for wait
Hi all, I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see the following error messages in the logs: Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: 88007c334000 reserve failed for wait Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: 88007c334000 reserve failed for wait Sometimes that's just one or two of these, sometimes a bunch of them. I didn't notice any major problem so far. What kind of issue may result from this error? I see the message comes from function radeon_bo_wait() but I have no idea what this function does. Here are the messages related to my Radeon card as found in the kernel boot log: [drm] radeon defaulting to kernel modesetting. [drm] radeon kernel modesetting enabled. alloc irq_desc for 30 on node -1 alloc kstat_irqs on node -1 radeon :07:00.0: PCI INT A -> GSI 30 (level, low) -> IRQ 30 radeon :07:00.0: setting latency timer to 64 [drm] initializing kernel modesetting (RV710 0x1002:0x954F). [drm] register mmio base: 0xFBEE [drm] register mmio size: 65536 ATOM BIOS: 113 radeon :07:00.0: VRAM: 512M 0x - 0x1FFF (512M used) radeon :07:00.0: GTT: 512M 0x2000 - 0x3FFF [drm] Detected VRAM RAM=512M, BAR=256M [drm] RAM width 64bits DDR [TTM] Zone kernel: Available graphics memory: 1024276 kiB. [TTM] Initializing pool allocator. [drm] radeon: 512M of VRAM memory ready [drm] radeon: 512M of GTT memory ready. alloc irq_desc for 64 on node -1 alloc kstat_irqs on node -1 radeon :07:00.0: irq 64 for MSI/MSI-X radeon :07:00.0: radeon: using MSI. [drm] radeon: irq initialized. [drm] GART: num cpu pages 131072, num gpu pages 131072 [drm] Loading RV710 Microcode [drm] ring test succeeded in 1 usecs [drm] radeon: ib pool ready. [drm] ib test succeeded in 0 usecs [drm] Enabling audio support failed to evaluate ATIF got AE_BAD_PARAMETER [drm] Default TV standard: NTSC [drm] Default TV standard: NTSC [drm] Radeon Display Connectors [drm] Connector 0: [drm] VGA [drm] DDC: 0x7e50 0x7e50 0x7e54 0x7e54 0x7e58 0x7e58 0x7e5c 0x7e5c [drm] Encoders: [drm] CRT2: INTERNAL_KLDSCP_DAC2 [drm] Connector 1: [drm] HDMI-A [drm] HPD1 [drm] DDC: 0x7e40 0x7e40 0x7e44 0x7e44 0x7e48 0x7e48 0x7e4c 0x7e4c [drm] Encoders: [drm] DFP1: INTERNAL_UNIPHY [drm] Connector 2: [drm] DVI-I [drm] HPD4 [drm] DDC: 0x7f10 0x7f10 0x7f14 0x7f14 0x7f18 0x7f18 0x7f1c 0x7f1c [drm] Encoders: [drm] CRT1: INTERNAL_KLDSCP_DAC1 [drm] DFP2: INTERNAL_UNIPHY2 [drm] Internal thermal controller with fan control [drm] radeon: power management initialized [drm] fb mappable at 0xD0142000 [drm] vram apper at 0xD000 [drm] size 8294400 [drm] fb depth is 24 [drm]pitch is 7680 checking generic (d000 100) vs hw (d000 2000) fb: conflicting fb hw usage radeondrmfb vs VESA VGA - removing generic driver Console: switching to colour dummy device 80x25 Console: switching to colour frame buffer device 240x67 fb0: radeondrmfb frame buffer device drm: registered panic notifier [drm] Initialized radeon 2.6.0 20080528 for :07:00.0 on minor 0 The only thing out of the ordinary I could spot is the "failed to evaluate ATIF" message, but apparently it is only a debug message, so I doubt it is related (but it would certainly be nice to solve that problem too.) Let me know if I can provide more information, or help in any way with debugging this issue. Thanks, -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Error message on RV710: reserve failed for wait
On Thu, 30 Sep 2010 15:18:27 +0200, Jean Delvare wrote: > I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see > the following error messages in the logs: > > Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: > 88007c334000 reserve failed for wait > Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: > 88007c334000 reserve failed for wait For completeness, I also saw these error messages when running kernels 2.6.34.4+ and 2.6.35.5+, so this isn't something new in 2.6.36. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Error message on RV710: reserve failed for wait
Hi again, On Thu, 30 Sep 2010 15:18:27 +0200, Jean Delvare wrote: > I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see > the following error messages in the logs: > > Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: > 88007c334000 reserve failed for wait > Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: > 88007c334000 reserve failed for wait > > Sometimes that's just one or two of these, sometimes a bunch of them. I > didn't notice any major problem so far. What kind of issue may result > from this error? I see the message comes from function radeon_bo_wait() > but I have no idea what this function does. Problem still present in 2.6.36-rc7. I have investigated it myself, and here's what I found. The error code returned by ttm_bo_reserve() to radeon_bo_wait() when I get this error message is EBUSY. This can only happen when ttm_bo_reserve() isn't allowed to wait (no_wait=true). The only caller passing no_wait=true is radeon_gem_busy_ioctl(). So, as a summary, I see an error message in the kernel log whenever radeon_gem_busy_ioctl() tries to process some ioctl but can't get access to some kernel data it needs. Sorry for being vague here but I'm not familiar with the code at all, so that's the best I can say. I don't know who exactly is calling radeon_gem_busy_ioctl(), as it can only be reached through a function pointer. I presume this is the X11 radeon driver, in user-space? Jerome, you wrote that part of the code, maybe you could suggest a course of action? Is it OK for the radeon kernel driver to return -EBUSY to user-space in that case? If it is OK, then the error message shouldn't be printed, or maybe it should be degraded to a debugging message. If it isn't OK, what can be done to avoid it? Thanks, -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon/kms: Silent spurious error message
I see the following error message in my kernel log from time to time:
radeon :07:00.0: 88007c334000 reserve failed for wait
radeon :07:00.0: 88007c334000 reserve failed for wait
After investigation, it turns out that there's nothing to be afraid of
and everything works as intended. So remove the spurious log message.
Signed-off-by: Jean Delvare
Cc: Jerome Glisse
---
drivers/gpu/drm/radeon/radeon_object.h |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
--- linux-2.6.36-rc7.orig/drivers/gpu/drm/radeon/radeon_object.h
2010-10-08 13:32:46.0 +0200
+++ linux-2.6.36-rc7/drivers/gpu/drm/radeon/radeon_object.h 2010-10-08
13:33:05.0 +0200
@@ -124,11 +124,8 @@ static inline int radeon_bo_wait(struct
int r;
r = ttm_bo_reserve(&bo->tbo, true, no_wait, false, 0);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- dev_err(bo->rdev->dev, "%p reserve failed for wait\n",
bo);
+ if (unlikely(r != 0))
return r;
- }
spin_lock(&bo->tbo.lock);
if (mem_type)
*mem_type = bo->tbo.mem.mem_type;
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Inlined functions in radeon_object.h
Hi Jerome, What was the rationale for inlining functions radeon_bo_reserve() and radeon_bo_wait()? These functions are called many times (especially radeon_bo_reserve, called 47 times) and they aren't particularly small (especially radeon_bo_wait). I found that un-inlining radeon_bo_wait() saves 187 bytes of code on radeon.o on x86-64: add/remove: 1/0 grow/shrink: 0/3 up/down: 178/-365 (-187) function old new delta radeon_bo_wait - 178+178 radeon_gem_wait_idle_ioctl 257 157-100 radeon_gem_busy_ioctl298 187-111 radeon_gem_set_domain251 97-154 And un-inlining radeon_bo_reserve() saves 2644 bytes of code on radeon.o on x86-64 (2947 if you include the string section): add/remove: 1/0 grow/shrink: 0/38 up/down: 100/-2744 (-2644) function old new delta radeon_bo_reserve - 100+100 rs600_gart_disable 178 143 -35 radeon_gart_table_vram_free 142 94 -48 r600_wb_enable 657 609 -48 r600_ih_ring_fini164 116 -48 radeon_ring_fini 189 139 -50 radeon_ib_pool_fini 221 171 -50 r600_suspend 231 181 -50 rv770_fini 262 210 -52 r600_blit_fini 146 94 -52 r600_wb_disable 194 141 -53 radeon_gem_get_tiling_ioctl 263 209 -54 radeon_bo_set_tiling_flags 151 97 -54 rv770_suspend231 175 -56 radeon_suspend_kms 540 481 -59 radeon_fb_find_or_create_single 15971538 -59 rv770_pcie_gart_disable 958 898 -60 rv370_pcie_gart_disable 947 887 -60 radeon_ring_init 382 322 -60 r600_pcie_gart_disable 14051345 -60 evergreen_pcie_gart_disable 1020 960 -60 radeon_gem_object_unpin 115 54 -61 radeon_ttm_fini 283 220 -63 radeon_ttm_init 997 929 -68 radeon_gem_object_pin158 90 -68 radeon_gart_table_vram_pin 243 174 -69 rv770_startup 11065 10995 -70 radeon_bo_list_reserve 126 56 -70 r600_blit_init 850 779 -71 radeonfb_destroy_pinned_object 178 106 -72 r600_startup32353163 -72 radeon_ib_pool_init 546 470 -76 r600_irq_init 26482572 -76 r100_wb_fini 281 197 -84 r100_wb_init 580 488 -92 radeon_test_moves 15241429 -95 radeon_crtc_set_base17741668-106 atombios_crtc_set_base 44774248-229 radeon_benchmark_move 1208 974-234 Would you take a patch un-inlining either or both of these functions? For radeon_bo_reserve(), an alternative would be to remove the error message. After all, we just decided that the same error message was needless in radeon_bo_wait(), maybe the same reasoning applies to radeon_bo_reserve(), in which case the function would become a one-liner which we can legitimately keep inlined. The binary size benefit is slightly smaller (-2294 bytes on x86-64) but the code would be slightly faster (one function call saved.) What do you think? -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/ttm: Simplify ttm_bo_wait_unreserved
Function ttm_bo_wait_unreserved can be slightly simplified.
Signed-off-by: Jean Delvare
Cc: Thomas Hellstrom
Cc: Jerome Glisse
---
drivers/gpu/drm/ttm/ttm_bo.c |9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
--- linux-2.6.36-rc7.orig/drivers/gpu/drm/ttm/ttm_bo.c 2010-10-09
13:38:39.0 +0200
+++ linux-2.6.36-rc7/drivers/gpu/drm/ttm/ttm_bo.c 2010-10-09
14:23:07.0 +0200
@@ -169,18 +169,13 @@ static void ttm_bo_release_list(struct k
int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo, bool interruptible)
{
-
if (interruptible) {
- int ret = 0;
-
- ret = wait_event_interruptible(bo->event_queue,
+ return wait_event_interruptible(bo->event_queue,
atomic_read(&bo->reserved) == 0);
- if (unlikely(ret != 0))
- return ret;
} else {
wait_event(bo->event_queue, atomic_read(&bo->reserved) == 0);
+ return 0;
}
- return 0;
}
EXPORT_SYMBOL(ttm_bo_wait_unreserved);
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Fix I2C adapter registration
Fix many small bugs in I2C adapter registration: * Properly reject unsupported GPIO pin. * Fix improper use of I2C_NAME_SIZE (which is the size of i2c_client.name, not i2c_adapter.name.) * Prefix adapter names with "i915" so that the user knows what the I2C channel is connected to. * Fix swapped characters in the string used to name the GPIO-based adapter. * Add missing comma in gmbus name table. Signed-off-by: Jean Delvare Tested-by: Randy Dunlap --- Chris, what's the status of this patch? I think it should go to Linus ASAP. drivers/gpu/drm/i915/intel_i2c.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) --- linux-2.6.37-rc1.orig/drivers/gpu/drm/i915/intel_i2c.c 2010-11-02 09:19:35.0 +0100 +++ linux-2.6.37-rc1/drivers/gpu/drm/i915/intel_i2c.c 2010-11-05 18:38:15.0 +0100 @@ -160,7 +160,7 @@ intel_gpio_create(struct drm_i915_privat }; struct intel_gpio *gpio; - if (pin < 1 || pin > 7) + if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) return NULL; gpio = kzalloc(sizeof(struct intel_gpio), GFP_KERNEL); @@ -172,7 +172,8 @@ intel_gpio_create(struct drm_i915_privat gpio->reg += PCH_GPIOA - GPIOA; gpio->dev_priv = dev_priv; - snprintf(gpio->adapter.name, I2C_NAME_SIZE, "GPIO%c", "?BACDEF?"[pin]); + snprintf(gpio->adapter.name, sizeof(gpio->adapter.name), "i915 GPIO%c", +"?BACDE?F"[pin]); gpio->adapter.owner = THIS_MODULE; gpio->adapter.algo_data = &gpio->algo; gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev; @@ -349,7 +350,7 @@ int intel_setup_gmbus(struct drm_device "panel", "dpc", "dpb", - "reserved" + "reserved", "dpd", }; struct drm_i915_private *dev_priv = dev->dev_private; @@ -366,8 +367,8 @@ int intel_setup_gmbus(struct drm_device bus->adapter.owner = THIS_MODULE; bus->adapter.class = I2C_CLASS_DDC; snprintf(bus->adapter.name, -I2C_NAME_SIZE, -"gmbus %s", + sizeof(bus->adapter.name), +"i915 gmbus %s", names[i]); bus->adapter.dev.parent = &dev->pdev->dev; -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/kms: register an i2c adapter name for the dp aux bus
Hi Alex, On Wed, 17 Nov 2010 17:56:49 -0500, Alex Deucher wrote: > This causes the connector to not be added since i2c init fails > for the adapter. Fixes: > https://bugs.freedesktop.org/show_bug.cgi?id=31688 Good catch. It may seem to be a little harsh but I'm glad I added the check to i2c-core. This will avoid future bug reports which we don't know how to process because we have no idea what the underlying faulty I2C adapter is. > Noticed by Ari Savolainen. > > Signed-off-by: Alex Deucher > Cc: Ari Savolainen > Cc: Jean Delvare > Cc: sta...@kernel.org > --- > drivers/gpu/drm/radeon/radeon_i2c.c |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c > b/drivers/gpu/drm/radeon/radeon_i2c.c > index 0cfbba0..d263bd1 100644 > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > @@ -946,6 +946,7 @@ struct radeon_i2c_chan *radeon_i2c_create_dp(struct > drm_device *dev, > i2c->rec = *rec; > i2c->adapter.owner = THIS_MODULE; > i2c->dev = dev; > + sprintf(i2c->adapter.name, "Radeon aux bus %s", name); I would much prefer snprintf(), it's safer: snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "Radeon aux bus %s", name); And snprintf() should be used for the other i2c adapter types in the radeon driver, too. > i2c_set_adapdata(&i2c->adapter, i2c); > i2c->adapter.algo_data = &i2c->algo.dp; > i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch; -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/kms: i2c s/sprintf/snprintf/g for safety
On Thu, 18 Nov 2010 11:37:18 -0500, Alex Deucher wrote:
> As per advice from Jean Delvare.
>
> Signed-off-by: Alex Deucher
> Cc: Jean Delvare
Thanks Alex.
Acked-by: Jean Delvare
> ---
> drivers/gpu/drm/radeon/radeon_i2c.c |9 ++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> b/drivers/gpu/drm/radeon/radeon_i2c.c
> index d263bd1..ded2a45 100644
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -896,7 +896,8 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
> drm_device *dev,
>((rdev->family <= CHIP_RS480) ||
> ((rdev->family >= CHIP_RV515) && (rdev->family <= CHIP_R580) {
> /* set the radeon hw i2c adapter */
> - sprintf(i2c->adapter.name, "Radeon i2c hw bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon i2c hw bus %s", name);
> i2c->adapter.algo = &radeon_i2c_algo;
> ret = i2c_add_adapter(&i2c->adapter);
> if (ret) {
> @@ -905,7 +906,8 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
> drm_device *dev,
> }
> } else {
> /* set the radeon bit adapter */
> - sprintf(i2c->adapter.name, "Radeon i2c bit bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon i2c bit bus %s", name);
> i2c->adapter.algo_data = &i2c->algo.bit;
> i2c->algo.bit.pre_xfer = pre_xfer;
> i2c->algo.bit.post_xfer = post_xfer;
> @@ -946,7 +948,8 @@ struct radeon_i2c_chan *radeon_i2c_create_dp(struct
> drm_device *dev,
> i2c->rec = *rec;
> i2c->adapter.owner = THIS_MODULE;
> i2c->dev = dev;
> - sprintf(i2c->adapter.name, "Radeon aux bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon aux bus %s", name);
> i2c_set_adapdata(&i2c->adapter, i2c);
> i2c->adapter.algo_data = &i2c->algo.dp;
> i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch;
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
2.6.37-rc2-git7 regression: wine fails to start
Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the following error in the kernel logs: radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too big radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! Hardware is: 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD 4350] 07:00.1 Audio device: ATI Technologies Inc RV710/730 X.org version is 7.5. With kernel 2.6.36, wine works just fine on the same system. Does it ring a bell to anyone? Any clue how to investigate this bug? If not, I can start a bisection. I can also provide any additional data you need for investigation. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, Thanks for your fast answer. On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 9:38 AM, Jean Delvare wrote: > > Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the > > following error in the kernel logs: > > > > radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too > > big > > radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 > > [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! > > > > Hardware is: > > 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD > > 4350] > > 07:00.1 Audio device: ATI Technologies Inc RV710/730 > > > > X.org version is 7.5. > > > > With kernel 2.6.36, wine works just fine on the same system. Does it > > ring a bell to anyone? Any clue how to investigate this bug? If not, I > > can start a bisection. > > > > I can also provide any additional data you need for investigation. > > It's probably this patch: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 Your guess was correct: after applying the above patch to kernel 2.6.36, I get the same problem. > Which exposes a bug in the 3D driver in certain cases. For some > reason we get a cb height of 8192 or greater in some cases in the 3D > driver. I haven't had time to look into why. You might try r600g for > comparison. Not sure how to "try r600g"? -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, On Sun, 21 Nov 2010 19:57:23 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 1:51 PM, Jean Delvare wrote: > > On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > >> It's probably this patch: > >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 > > > > Your guess was correct: after applying the above patch to kernel > > 2.6.36, I get the same problem. > > > >> Which exposes a bug in the 3D driver in certain cases. For some > >> reason we get a cb height of 8192 or greater in some cases in the 3D > >> driver. I haven't had time to look into why. You might try r600g for > >> comparison. > > > > Not sure how to "try r600g"? > > It's a newer driver for r6xx+ hardware. Configure mesa > (http://cgit.freedesktop.org/mesa/mesa) with --enable-gallium-r600. Unfortunately the thing doesn't seem to be willing to build on my system :( It won't find talloc even though it is installed. I don't have more time to spend on this. > I actually talked to Dave earlier today, and the problem is more > likely a command buffer getting flushed at the wrong time leading to a > buffer without cb info in it. I think I'll just wait for you to come up with a fix :) I am in no hurry, I only use wine once in a while. Let me know if I can help with testing. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, David, On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 9:38 AM, Jean Delvare wrote: > > Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the > > following error in the kernel logs: > > > > radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too > > big > > radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 > > [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! > > > > Hardware is: > > 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD > > 4350] > > 07:00.1 Audio device: ATI Technologies Inc RV710/730 > > > > X.org version is 7.5. > > > > With kernel 2.6.36, wine works just fine on the same system. Does it > > ring a bell to anyone? Any clue how to investigate this bug? If not, I > > can start a bisection. > > > > I can also provide any additional data you need for investigation. > > It's probably this patch: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 > > Which exposes a bug in the 3D driver in certain cases. For some > reason we get a cb height of 8192 or greater in some cases in the 3D > driver. I haven't had time to look into why. You might try r600g for > comparison. Any progress on this? Linus' latest kernel (2.6.37-rc5-git3) still has the problem. And this is a regression, so I don't think you can just ignore it. Is there a bug opened to track this bug already? I couldn't find one on bugzilla.kernel.org, but maybe I missed it. If there is none, I'll create one. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, On Sun, 12 Dec 2010 23:33:42 -0500, Alex Deucher wrote: > On Sun, Dec 12, 2010 at 7:31 PM, Alex Deucher wrote: > > On Fri, Dec 10, 2010 at 7:01 AM, Jean Delvare wrote: > >> Any progress on this? Linus' latest kernel (2.6.37-rc5-git3) still has > >> the problem. And this is a regression, so I don't think you can just > >> ignore it. > >> > >> Is there a bug opened to track this bug already? I couldn't find one on > >> bugzilla.kernel.org, but maybe I missed it. If there is none, I'll > >> create one. > > > > Was it not fixed by this commit? > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a235e4c9302509ac5956bbbffa22eb5ed9fcdc54 Not, it wasn't. > > If not, open a bug and attach the dmesg with the error messages. > > Does the attached patch fix it? Yes it does, thanks! If you consider that user-space applications failing these tests are broken, maybe you could use printk_once() to let them know? Otherwise I suspect they will never get fixed. I think there's a way to get the name of the faulty process and include it in the message. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 2.6.37-rc2-git7 regression: wine fails to start
On Mon, 13 Dec 2010 19:37:27 +1000, Dave Airlie wrote: > No its a problem with developing GPU drivers, you write some code in > userspace that seems correct, it gets rolled out, you later add a new > feature like tiling, find a bug in the kernel checker code, realise > the userspace code is wrong, and give up and become a farmer. I wish you success in you upcoming endeavour! :D I know what you mean. The kernel/user-space interface is sometimes very difficult to live with. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] i2c-algo-bit: make sure to call pre/post_xfer for bit_test
Hi Alex,
On Thu, 14 Apr 2011 19:47:06 -0400, Alex Deucher wrote:
> Apparently some distros set i2c-algo-bit.bit_test to 1 by
> default. In some cases this causes i2c_bit_add_bus
> to fail and prevents the i2c bus from being added. In the
> radeon case, we fail to add the ddc i2c buses which prevents
> the driver from being able to detect attached monitors.
> The i2c bus works fine even if bit_test fails. This is likely
> due to gpio switching that is required and handled in the
> pre/post_xfer hooks, so call the pre/post_xfer hooks in the
> bit test as well.
>
> Fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=36221
>
> Cc: Jean Delvare
> Signed-off-by: Alex Deucher
Good catch, applied, thanks. I'll also push this to the stable kernel
trees (from .38 down to .34.)
> ---
> drivers/i2c/algos/i2c-algo-bit.c | 21 ++---
> 1 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 38319a6..e2740e6 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -232,9 +232,16 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
> * Sanity check for the adapter hardware - check the reaction of
> * the bus lines only if it seems to be idle.
> */
> -static int test_bus(struct i2c_algo_bit_data *adap, char *name)
> +static int test_bus(struct i2c_adapter *i2c_adap, char *name)
> {
> - int scl, sda;
> + struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> + int scl, sda, ret;
> +
> + if (adap->pre_xfer) {
> + ret = adap->pre_xfer(i2c_adap);
> + if (ret < 0)
> + return -ENODEV;
> + }
>
> if (adap->getscl == NULL)
> pr_info("%s: Testing SDA only, SCL is not readable\n", name);
> @@ -297,11 +304,19 @@ static int test_bus(struct i2c_algo_bit_data *adap,
> char *name)
> "while pulling SCL high!\n", name);
> goto bailout;
> }
> +
> + if (adap->post_xfer)
> + adap->post_xfer(i2c_adap);
> +
> pr_info("%s: Test OK\n", name);
> return 0;
> bailout:
> sdahi(adap);
> sclhi(adap);
> +
> + if (adap->post_xfer)
> + adap->post_xfer(i2c_adap);
> +
> return -ENODEV;
> }
>
> @@ -607,7 +622,7 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap,
> int ret;
>
> if (bit_test) {
> - ret = test_bus(bit_adap, adap->name);
> + ret = test_bus(adap, adap->name);
> if (ret < 0)
> return -ENODEV;
> }
--
Jean Delvare
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/radeon/kms: set i2c adapter class to I2C_CLASS_DDC
On Tue, 3 May 2011 13:32:36 -0400, Alex Deucher wrote: > The most common use of the radeon i2c buses is for ddc. > > Signed-off-by: Alex Deucher > Cc: Jean Delvare Acked-by: Jean Delvare > --- > drivers/gpu/drm/radeon/radeon_i2c.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c > b/drivers/gpu/drm/radeon/radeon_i2c.c > index 983cbac..781196d 100644 > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > @@ -888,6 +888,7 @@ struct radeon_i2c_chan *radeon_i2c_create(struct > drm_device *dev, > > i2c->rec = *rec; > i2c->adapter.owner = THIS_MODULE; > + i2c->adapter.class = I2C_CLASS_DDC; > i2c->dev = dev; > i2c_set_adapdata(&i2c->adapter, i2c); > if (rec->mm_i2c || > @@ -947,6 +948,7 @@ struct radeon_i2c_chan *radeon_i2c_create_dp(struct > drm_device *dev, > > i2c->rec = *rec; > i2c->adapter.owner = THIS_MODULE; > + i2c->adapter.class = I2C_CLASS_DDC; > i2c->dev = dev; > snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), >"Radeon aux bus %s", name); -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 and boot freeze
Hi Yermandu, Please pay attention when writing and proofread yourself before sending. You certainly meant i915, not i195, in the subject line. I've fixed it. This is important if you want to get the attention of the relevant maintainers and developers. On Mon, 23 May 2011 19:33:15 -0300, Yermandu Patapitafious wrote: > since kernel .39 i can not boot, when kernel go to gmbus start the issue. > I try capture a dump with kexec, but no success, so i have the idea to > make video booting, > kernel version 2.6.39-02745-g0b26d47 and 2.6.39 vanilla same problem. What is the last kernel that works for you? Are you running self-built or distribution kernels? Can we please see the kernel configuration file? You seem to have verbose debugging options enabled (CONFIG_DEBUG_KOBJECT at least.) it makes the logs harder to read. > http://www.vimeo.com/24138372 Unfortunately the beginning is pretty hard to read. It would be great if you were able to get us the kernel logs in a text form, using some form of serial console. I see references to i2c_for_each_dev, a new function which only i2c-dev and i2c-core are using at the moment. So if you have CONFIG_I2C_CHARDEV set, and this is a self-built kernel, try again without it just to get it out of the picture. Note that logs from a pure 2.6.39 kernel would probably be more useful than from a later git snapshot. The 2.6.40 development branch is very young and could have other problems. Alternatively, if you are familiar with git, a git bisection would help too. -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 and boot freeze
On Wed, 25 May 2011 10:38:59 -0300, Yermandu Patapitafious wrote: > Hello Jean, > > Thanks for answer, i will take more atention in subjects, > > > What is the last kernel that works for you? > > > > Are you running self-built or distribution kernels? > > Can we please see the kernel configuration file? > > My Last kernel working from git repository was 2.6.38-rc8+, the stable > line .38.X work fines. OK, this is good, it means the bisection domain isn't too big. Can you remember if you made any kernel configuration change between 2.6.38 and 2.6.39? > I use the git version from kernel.org site pull and build manually be > me. The Latest Stable Kernel is from gentoo-sources, and i always > compile manually too. > I have attached .config kernel in mail. I see you have many debug options enabled. I suggest you try building a kernel without these and see if it makes a difference. > > It would be great > > if you were able to get us the kernel logs in a text form, using some > > form of serial console. > > Removing the CONFIG_I2C_CHARDEV has not effect, the panic continues > happing I looking for some way to send panic log in text, but no > success yet. If you can't get a serial console to capture the complete error log, the other approach is to bisect between kernels 2.6.38 and 2.6.39, to find the faulty commit. If you're used to building your kernel yourself, it shouldn't be too difficult. Just clone Linus' kernel tree and follow the git bisect manual page: http://www.kernel.org/pub/software/scm/git/docs/git-bisect.html -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"
Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a hang when loading the eeprom driver (see bug #35572.) GMBUS will be re-enabled later, differently. Signed-off-by: Jean Delvare Reported-by: Marek Otahal Tested-by: Yermandu Patapitafious Tested-by: Andrew Lutomirski Acked-by: Chris Wilson Cc: David Airlie --- drivers/gpu/drm/i915/intel_i2c.c |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- linux-2.6.39.orig/drivers/gpu/drm/i915/intel_i2c.c 2011-05-20 10:42:40.0 +0200 +++ linux-2.6.39/drivers/gpu/drm/i915/intel_i2c.c 2011-06-02 16:26:56.0 +0200 @@ -401,8 +401,7 @@ int intel_setup_gmbus(struct drm_device bus->reg0 = i | GMBUS_RATE_100KHZ; /* XXX force bit banging until GMBUS is fully debugged */ - if (IS_GEN2(dev)) - bus->force_bit = intel_gpio_create(dev_priv, i); + bus->force_bit = intel_gpio_create(dev_priv, i); } intel_i2c_reset(dev_priv->dev); -- Jean Delvare ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/radeon: simplify driver data retrieval
You can get the driver data from struct device directly, there's no
need to get the PCI device first.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
drivers/gpu/drm/radeon/radeon_pm.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
--- linux-3.11-rc7.orig/drivers/gpu/drm/radeon/radeon_pm.c 2013-09-02
22:06:28.569606289 +0200
+++ linux-3.11-rc7/drivers/gpu/drm/radeon/radeon_pm.c 2013-09-03
09:20:41.850518596 +0200
@@ -333,7 +333,7 @@ static ssize_t radeon_get_pm_profile(str
struct device_attribute *attr,
char *buf)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
int cp = rdev->pm.profile;
@@ -349,7 +349,7 @@ static ssize_t radeon_set_pm_profile(str
const char *buf,
size_t count)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
mutex_lock(&rdev->pm.mutex);
@@ -383,7 +383,7 @@ static ssize_t radeon_get_pm_method(stru
struct device_attribute *attr,
char *buf)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
int pm = rdev->pm.pm_method;
@@ -397,7 +397,7 @@ static ssize_t radeon_set_pm_method(stru
const char *buf,
size_t count)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
/* we don't support the legacy modes with dpm */
@@ -433,7 +433,7 @@ static ssize_t radeon_get_dpm_state(stru
struct device_attribute *attr,
char *buf)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
enum radeon_pm_state_type pm = rdev->pm.dpm.user_state;
@@ -447,7 +447,7 @@ static ssize_t radeon_set_dpm_state(stru
const char *buf,
size_t count)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
mutex_lock(&rdev->pm.mutex);
@@ -472,7 +472,7 @@ static ssize_t radeon_get_dpm_forced_per
struct device_attribute
*attr,
char *buf)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
enum radeon_dpm_forced_level level = rdev->pm.dpm.forced_level;
@@ -486,7 +486,7 @@ static ssize_t radeon_set_dpm_forced_per
const char *buf,
size_t count)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
enum radeon_dpm_forced_level level;
int ret = 0;
@@ -524,7 +524,7 @@ static ssize_t radeon_hwmon_show_temp(st
struct device_attribute *attr,
char *buf)
{
- struct drm_device *ddev = pci_get_drvdata(to_pci_dev(dev));
+ struct drm_device *ddev = dev_get_drvdata(dev);
struct radeon_device *rdev = ddev->dev_private;
int temp;
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/radeon: expose DPM thermal thresholds through sysfs
The hwmon sysfs interface allows exposing temperature limits. The "max"
and "min" thresholds will be exposed as a critical high limit and its
hysteresis value, respectively. This gives the user a better idea of how
well cooling is doing and whether it is sufficient.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
drivers/gpu/drm/radeon/radeon_pm.c | 38 +
1 file changed, 38 insertions(+)
--- linux-3.11-rc7.orig/drivers/gpu/drm/radeon/radeon_pm.c 2013-09-03
09:20:41.850518596 +0200
+++ linux-3.11-rc7/drivers/gpu/drm/radeon/radeon_pm.c 2013-09-03
09:26:03.630852919 +0200
@@ -536,6 +536,23 @@ static ssize_t radeon_hwmon_show_temp(st
return snprintf(buf, PAGE_SIZE, "%d\n", temp);
}
+static ssize_t radeon_hwmon_show_temp_thresh(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct radeon_device *rdev = ddev->dev_private;
+ int hyst = to_sensor_dev_attr(attr)->index;
+ int temp;
+
+ if (hyst)
+ temp = rdev->pm.dpm.thermal.min_temp;
+ else
+ temp = rdev->pm.dpm.thermal.max_temp;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
static ssize_t radeon_hwmon_show_name(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -544,16 +561,37 @@ static ssize_t radeon_hwmon_show_name(st
}
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, radeon_hwmon_show_temp, NULL,
0);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, radeon_hwmon_show_temp_thresh,
NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO,
radeon_hwmon_show_temp_thresh, NULL, 1);
static SENSOR_DEVICE_ATTR(name, S_IRUGO, radeon_hwmon_show_name, NULL, 0);
static struct attribute *hwmon_attributes[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
+ &sensor_dev_attr_temp1_crit.dev_attr.attr,
+ &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
&sensor_dev_attr_name.dev_attr.attr,
NULL
};
+static umode_t hwmon_attributes_visible(struct kobject *kobj,
+ struct attribute *attr, int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct radeon_device *rdev = ddev->dev_private;
+
+ /* Skip limit attributes if DPM is not enabled */
+ if (rdev->pm.pm_method != PM_METHOD_DPM &&
+ (attr == &sensor_dev_attr_temp1_crit.dev_attr.attr ||
+attr == &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr))
+ return 0;
+
+ return attr->mode;
+}
+
static const struct attribute_group hwmon_attrgroup = {
.attrs = hwmon_attributes,
+ .is_visible = hwmon_attributes_visible,
};
static int radeon_hwmon_init(struct radeon_device *rdev)
--
Jean Delvare
Suse L3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Fix I2C adapter registration
Fix many small bugs in I2C adapter registration: * Properly reject unsupported GPIO pin. * Fix improper use of I2C_NAME_SIZE (which is the size of i2c_client.name, not i2c_adapter.name.) * Prefix adapter names with "i915" so that the user knows what the I2C channel is connected to. * Fix swapped characters in the string used to name the GPIO-based adapter. * Add missing comma in gmbus name table. Signed-off-by: Jean Delvare Tested-by: Randy Dunlap --- Chris, what's the status of this patch? I think it should go to Linus ASAP. drivers/gpu/drm/i915/intel_i2c.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) --- linux-2.6.37-rc1.orig/drivers/gpu/drm/i915/intel_i2c.c 2010-11-02 09:19:35.0 +0100 +++ linux-2.6.37-rc1/drivers/gpu/drm/i915/intel_i2c.c 2010-11-05 18:38:15.0 +0100 @@ -160,7 +160,7 @@ intel_gpio_create(struct drm_i915_privat }; struct intel_gpio *gpio; - if (pin < 1 || pin > 7) + if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) return NULL; gpio = kzalloc(sizeof(struct intel_gpio), GFP_KERNEL); @@ -172,7 +172,8 @@ intel_gpio_create(struct drm_i915_privat gpio->reg += PCH_GPIOA - GPIOA; gpio->dev_priv = dev_priv; - snprintf(gpio->adapter.name, I2C_NAME_SIZE, "GPIO%c", "?BACDEF?"[pin]); + snprintf(gpio->adapter.name, sizeof(gpio->adapter.name), "i915 GPIO%c", +"?BACDE?F"[pin]); gpio->adapter.owner = THIS_MODULE; gpio->adapter.algo_data = &gpio->algo; gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev; @@ -349,7 +350,7 @@ int intel_setup_gmbus(struct drm_device "panel", "dpc", "dpb", - "reserved" + "reserved", "dpd", }; struct drm_i915_private *dev_priv = dev->dev_private; @@ -366,8 +367,8 @@ int intel_setup_gmbus(struct drm_device bus->adapter.owner = THIS_MODULE; bus->adapter.class = I2C_CLASS_DDC; snprintf(bus->adapter.name, -I2C_NAME_SIZE, -"gmbus %s", + sizeof(bus->adapter.name), +"i915 gmbus %s", names[i]); bus->adapter.dev.parent = &dev->pdev->dev; -- Jean Delvare
[PATCH] drm/radeon/kms: register an i2c adapter name for the dp aux bus
Hi Alex, On Wed, 17 Nov 2010 17:56:49 -0500, Alex Deucher wrote: > This causes the connector to not be added since i2c init fails > for the adapter. Fixes: > https://bugs.freedesktop.org/show_bug.cgi?id=31688 Good catch. It may seem to be a little harsh but I'm glad I added the check to i2c-core. This will avoid future bug reports which we don't know how to process because we have no idea what the underlying faulty I2C adapter is. > Noticed by Ari Savolainen. > > Signed-off-by: Alex Deucher > Cc: Ari Savolainen > Cc: Jean Delvare > Cc: stable at kernel.org > --- > drivers/gpu/drm/radeon/radeon_i2c.c |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c > b/drivers/gpu/drm/radeon/radeon_i2c.c > index 0cfbba0..d263bd1 100644 > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > @@ -946,6 +946,7 @@ struct radeon_i2c_chan *radeon_i2c_create_dp(struct > drm_device *dev, > i2c->rec = *rec; > i2c->adapter.owner = THIS_MODULE; > i2c->dev = dev; > + sprintf(i2c->adapter.name, "Radeon aux bus %s", name); I would much prefer snprintf(), it's safer: snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "Radeon aux bus %s", name); And snprintf() should be used for the other i2c adapter types in the radeon driver, too. > i2c_set_adapdata(&i2c->adapter, i2c); > i2c->adapter.algo_data = &i2c->algo.dp; > i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch; -- Jean Delvare
[PATCH] drm/radeon/kms: i2c s/sprintf/snprintf/g for safety
On Thu, 18 Nov 2010 11:37:18 -0500, Alex Deucher wrote:
> As per advice from Jean Delvare.
>
> Signed-off-by: Alex Deucher
> Cc: Jean Delvare
Thanks Alex.
Acked-by: Jean Delvare
> ---
> drivers/gpu/drm/radeon/radeon_i2c.c |9 ++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> b/drivers/gpu/drm/radeon/radeon_i2c.c
> index d263bd1..ded2a45 100644
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -896,7 +896,8 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
> drm_device *dev,
>((rdev->family <= CHIP_RS480) ||
> ((rdev->family >= CHIP_RV515) && (rdev->family <= CHIP_R580) {
> /* set the radeon hw i2c adapter */
> - sprintf(i2c->adapter.name, "Radeon i2c hw bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon i2c hw bus %s", name);
> i2c->adapter.algo = &radeon_i2c_algo;
> ret = i2c_add_adapter(&i2c->adapter);
> if (ret) {
> @@ -905,7 +906,8 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
> drm_device *dev,
> }
> } else {
> /* set the radeon bit adapter */
> - sprintf(i2c->adapter.name, "Radeon i2c bit bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon i2c bit bus %s", name);
> i2c->adapter.algo_data = &i2c->algo.bit;
> i2c->algo.bit.pre_xfer = pre_xfer;
> i2c->algo.bit.post_xfer = post_xfer;
> @@ -946,7 +948,8 @@ struct radeon_i2c_chan *radeon_i2c_create_dp(struct
> drm_device *dev,
> i2c->rec = *rec;
> i2c->adapter.owner = THIS_MODULE;
> i2c->dev = dev;
> - sprintf(i2c->adapter.name, "Radeon aux bus %s", name);
> + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> + "Radeon aux bus %s", name);
> i2c_set_adapdata(&i2c->adapter, i2c);
> i2c->adapter.algo_data = &i2c->algo.dp;
> i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch;
--
Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the following error in the kernel logs: radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too big radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! Hardware is: 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD 4350] 07:00.1 Audio device: ATI Technologies Inc RV710/730 X.org version is 7.5. With kernel 2.6.36, wine works just fine on the same system. Does it ring a bell to anyone? Any clue how to investigate this bug? If not, I can start a bisection. I can also provide any additional data you need for investigation. -- Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, Thanks for your fast answer. On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 9:38 AM, Jean Delvare wrote: > > Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the > > following error in the kernel logs: > > > > radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too > > big > > radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 > > [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! > > > > Hardware is: > > 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD > > 4350] > > 07:00.1 Audio device: ATI Technologies Inc RV710/730 > > > > X.org version is 7.5. > > > > With kernel 2.6.36, wine works just fine on the same system. Does it > > ring a bell to anyone? Any clue how to investigate this bug? If not, I > > can start a bisection. > > > > I can also provide any additional data you need for investigation. > > It's probably this patch: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 Your guess was correct: after applying the above patch to kernel 2.6.36, I get the same problem. > Which exposes a bug in the 3D driver in certain cases. For some > reason we get a cb height of 8192 or greater in some cases in the 3D > driver. I haven't had time to look into why. You might try r600g for > comparison. Not sure how to "try r600g"? -- Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, On Sun, 21 Nov 2010 19:57:23 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 1:51 PM, Jean Delvare wrote: > > On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > >> It's probably this patch: > >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 > > > > Your guess was correct: after applying the above patch to kernel > > 2.6.36, I get the same problem. > > > >> Which exposes a bug in the 3D driver in certain cases. ?For some > >> reason we get a cb height of 8192 or greater in some cases in the 3D > >> driver. ?I haven't had time to look into why. ?You might try r600g for > >> comparison. > > > > Not sure how to "try r600g"? > > It's a newer driver for r6xx+ hardware. Configure mesa > (http://cgit.freedesktop.org/mesa/mesa) with --enable-gallium-r600. Unfortunately the thing doesn't seem to be willing to build on my system :( It won't find talloc even though it is installed. I don't have more time to spend on this. > I actually talked to Dave earlier today, and the problem is more > likely a command buffer getting flushed at the wrong time leading to a > buffer without cb info in it. I think I'll just wait for you to come up with a fix :) I am in no hurry, I only use wine once in a while. Let me know if I can help with testing. -- Jean Delvare
Error message on RV710: reserve failed for wait
On Thu, 30 Sep 2010 15:18:27 +0200, Jean Delvare wrote: > I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see > the following error messages in the logs: > > Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: > 88007c334000 reserve failed for wait > Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: > 88007c334000 reserve failed for wait For completeness, I also saw these error messages when running kernels 2.6.34.4+ and 2.6.35.5+, so this isn't something new in 2.6.36. -- Jean Delvare
Error message on RV710: reserve failed for wait
Hi again, On Thu, 30 Sep 2010 15:18:27 +0200, Jean Delvare wrote: > I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see > the following error messages in the logs: > > Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: > 88007c334000 reserve failed for wait > Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: > 88007c334000 reserve failed for wait > > Sometimes that's just one or two of these, sometimes a bunch of them. I > didn't notice any major problem so far. What kind of issue may result > from this error? I see the message comes from function radeon_bo_wait() > but I have no idea what this function does. Problem still present in 2.6.36-rc7. I have investigated it myself, and here's what I found. The error code returned by ttm_bo_reserve() to radeon_bo_wait() when I get this error message is EBUSY. This can only happen when ttm_bo_reserve() isn't allowed to wait (no_wait=true). The only caller passing no_wait=true is radeon_gem_busy_ioctl(). So, as a summary, I see an error message in the kernel log whenever radeon_gem_busy_ioctl() tries to process some ioctl but can't get access to some kernel data it needs. Sorry for being vague here but I'm not familiar with the code at all, so that's the best I can say. I don't know who exactly is calling radeon_gem_busy_ioctl(), as it can only be reached through a function pointer. I presume this is the X11 radeon driver, in user-space? Jerome, you wrote that part of the code, maybe you could suggest a course of action? Is it OK for the radeon kernel driver to return -EBUSY to user-space in that case? If it is OK, then the error message shouldn't be printed, or maybe it should be degraded to a debugging message. If it isn't OK, what can be done to avoid it? Thanks, -- Jean Delvare
[PATCH] drm/radeon/kms: Silent spurious error message
I see the following error message in my kernel log from time to time:
radeon :07:00.0: 88007c334000 reserve failed for wait
radeon :07:00.0: 88007c334000 reserve failed for wait
After investigation, it turns out that there's nothing to be afraid of
and everything works as intended. So remove the spurious log message.
Signed-off-by: Jean Delvare
Cc: Jerome Glisse
---
drivers/gpu/drm/radeon/radeon_object.h |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
--- linux-2.6.36-rc7.orig/drivers/gpu/drm/radeon/radeon_object.h
2010-10-08 13:32:46.0 +0200
+++ linux-2.6.36-rc7/drivers/gpu/drm/radeon/radeon_object.h 2010-10-08
13:33:05.0 +0200
@@ -124,11 +124,8 @@ static inline int radeon_bo_wait(struct
int r;
r = ttm_bo_reserve(&bo->tbo, true, no_wait, false, 0);
- if (unlikely(r != 0)) {
- if (r != -ERESTARTSYS)
- dev_err(bo->rdev->dev, "%p reserve failed for wait\n",
bo);
+ if (unlikely(r != 0))
return r;
- }
spin_lock(&bo->tbo.lock);
if (mem_type)
*mem_type = bo->tbo.mem.mem_type;
--
Jean Delvare
Inlined functions in radeon_object.h
Hi Jerome, What was the rationale for inlining functions radeon_bo_reserve() and radeon_bo_wait()? These functions are called many times (especially radeon_bo_reserve, called 47 times) and they aren't particularly small (especially radeon_bo_wait). I found that un-inlining radeon_bo_wait() saves 187 bytes of code on radeon.o on x86-64: add/remove: 1/0 grow/shrink: 0/3 up/down: 178/-365 (-187) function old new delta radeon_bo_wait - 178+178 radeon_gem_wait_idle_ioctl 257 157-100 radeon_gem_busy_ioctl298 187-111 radeon_gem_set_domain251 97-154 And un-inlining radeon_bo_reserve() saves 2644 bytes of code on radeon.o on x86-64 (2947 if you include the string section): add/remove: 1/0 grow/shrink: 0/38 up/down: 100/-2744 (-2644) function old new delta radeon_bo_reserve - 100+100 rs600_gart_disable 178 143 -35 radeon_gart_table_vram_free 142 94 -48 r600_wb_enable 657 609 -48 r600_ih_ring_fini164 116 -48 radeon_ring_fini 189 139 -50 radeon_ib_pool_fini 221 171 -50 r600_suspend 231 181 -50 rv770_fini 262 210 -52 r600_blit_fini 146 94 -52 r600_wb_disable 194 141 -53 radeon_gem_get_tiling_ioctl 263 209 -54 radeon_bo_set_tiling_flags 151 97 -54 rv770_suspend231 175 -56 radeon_suspend_kms 540 481 -59 radeon_fb_find_or_create_single 15971538 -59 rv770_pcie_gart_disable 958 898 -60 rv370_pcie_gart_disable 947 887 -60 radeon_ring_init 382 322 -60 r600_pcie_gart_disable 14051345 -60 evergreen_pcie_gart_disable 1020 960 -60 radeon_gem_object_unpin 115 54 -61 radeon_ttm_fini 283 220 -63 radeon_ttm_init 997 929 -68 radeon_gem_object_pin158 90 -68 radeon_gart_table_vram_pin 243 174 -69 rv770_startup 11065 10995 -70 radeon_bo_list_reserve 126 56 -70 r600_blit_init 850 779 -71 radeonfb_destroy_pinned_object 178 106 -72 r600_startup32353163 -72 radeon_ib_pool_init 546 470 -76 r600_irq_init 26482572 -76 r100_wb_fini 281 197 -84 r100_wb_init 580 488 -92 radeon_test_moves 15241429 -95 radeon_crtc_set_base17741668-106 atombios_crtc_set_base 44774248-229 radeon_benchmark_move 1208 974-234 Would you take a patch un-inlining either or both of these functions? For radeon_bo_reserve(), an alternative would be to remove the error message. After all, we just decided that the same error message was needless in radeon_bo_wait(), maybe the same reasoning applies to radeon_bo_reserve(), in which case the function would become a one-liner which we can legitimately keep inlined. The binary size benefit is slightly smaller (-2294 bytes on x86-64) but the code would be slightly faster (one function call saved.) What do you think? -- Jean Delvare
[PATCH] drm/ttm: Simplify ttm_bo_wait_unreserved
Function ttm_bo_wait_unreserved can be slightly simplified.
Signed-off-by: Jean Delvare
Cc: Thomas Hellstrom
Cc: Jerome Glisse
---
drivers/gpu/drm/ttm/ttm_bo.c |9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
--- linux-2.6.36-rc7.orig/drivers/gpu/drm/ttm/ttm_bo.c 2010-10-09
13:38:39.0 +0200
+++ linux-2.6.36-rc7/drivers/gpu/drm/ttm/ttm_bo.c 2010-10-09
14:23:07.0 +0200
@@ -169,18 +169,13 @@ static void ttm_bo_release_list(struct k
int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo, bool interruptible)
{
-
if (interruptible) {
- int ret = 0;
-
- ret = wait_event_interruptible(bo->event_queue,
+ return wait_event_interruptible(bo->event_queue,
atomic_read(&bo->reserved) == 0);
- if (unlikely(ret != 0))
- return ret;
} else {
wait_event(bo->event_queue, atomic_read(&bo->reserved) == 0);
+ return 0;
}
- return 0;
}
EXPORT_SYMBOL(ttm_bo_wait_unreserved);
--
Jean Delvare
Error message on RV710: reserve failed for wait
Hi all, I am running kernel 2.6.36-rc6 on a Radeon HD4350 (RV710), and I see the following error messages in the logs: Sep 30 14:09:27 endymion kernel: [21556.560593] radeon :07:00.0: 88007c334000 reserve failed for wait Sep 30 14:09:29 endymion kernel: [21558.253859] radeon :07:00.0: 88007c334000 reserve failed for wait Sometimes that's just one or two of these, sometimes a bunch of them. I didn't notice any major problem so far. What kind of issue may result from this error? I see the message comes from function radeon_bo_wait() but I have no idea what this function does. Here are the messages related to my Radeon card as found in the kernel boot log: [drm] radeon defaulting to kernel modesetting. [drm] radeon kernel modesetting enabled. alloc irq_desc for 30 on node -1 alloc kstat_irqs on node -1 radeon :07:00.0: PCI INT A -> GSI 30 (level, low) -> IRQ 30 radeon :07:00.0: setting latency timer to 64 [drm] initializing kernel modesetting (RV710 0x1002:0x954F). [drm] register mmio base: 0xFBEE [drm] register mmio size: 65536 ATOM BIOS: 113 radeon :07:00.0: VRAM: 512M 0x - 0x1FFF (512M used) radeon :07:00.0: GTT: 512M 0x2000 - 0x3FFF [drm] Detected VRAM RAM=512M, BAR=256M [drm] RAM width 64bits DDR [TTM] Zone kernel: Available graphics memory: 1024276 kiB. [TTM] Initializing pool allocator. [drm] radeon: 512M of VRAM memory ready [drm] radeon: 512M of GTT memory ready. alloc irq_desc for 64 on node -1 alloc kstat_irqs on node -1 radeon :07:00.0: irq 64 for MSI/MSI-X radeon :07:00.0: radeon: using MSI. [drm] radeon: irq initialized. [drm] GART: num cpu pages 131072, num gpu pages 131072 [drm] Loading RV710 Microcode [drm] ring test succeeded in 1 usecs [drm] radeon: ib pool ready. [drm] ib test succeeded in 0 usecs [drm] Enabling audio support failed to evaluate ATIF got AE_BAD_PARAMETER [drm] Default TV standard: NTSC [drm] Default TV standard: NTSC [drm] Radeon Display Connectors [drm] Connector 0: [drm] VGA [drm] DDC: 0x7e50 0x7e50 0x7e54 0x7e54 0x7e58 0x7e58 0x7e5c 0x7e5c [drm] Encoders: [drm] CRT2: INTERNAL_KLDSCP_DAC2 [drm] Connector 1: [drm] HDMI-A [drm] HPD1 [drm] DDC: 0x7e40 0x7e40 0x7e44 0x7e44 0x7e48 0x7e48 0x7e4c 0x7e4c [drm] Encoders: [drm] DFP1: INTERNAL_UNIPHY [drm] Connector 2: [drm] DVI-I [drm] HPD4 [drm] DDC: 0x7f10 0x7f10 0x7f14 0x7f14 0x7f18 0x7f18 0x7f1c 0x7f1c [drm] Encoders: [drm] CRT1: INTERNAL_KLDSCP_DAC1 [drm] DFP2: INTERNAL_UNIPHY2 [drm] Internal thermal controller with fan control [drm] radeon: power management initialized [drm] fb mappable at 0xD0142000 [drm] vram apper at 0xD000 [drm] size 8294400 [drm] fb depth is 24 [drm]pitch is 7680 checking generic (d000 100) vs hw (d000 2000) fb: conflicting fb hw usage radeondrmfb vs VESA VGA - removing generic driver Console: switching to colour dummy device 80x25 Console: switching to colour frame buffer device 240x67 fb0: radeondrmfb frame buffer device drm: registered panic notifier [drm] Initialized radeon 2.6.0 20080528 for :07:00.0 on minor 0 The only thing out of the ordinary I could spot is the "failed to evaluate ATIF" message, but apparently it is only a debug message, so I doubt it is related (but it would certainly be nice to solve that problem too.) Let me know if I can provide more information, or help in any way with debugging this issue. Thanks, -- Jean Delvare
[PATCH] i2c-algo-bit: make sure to call pre/post_xfer for bit_test
Hi Alex,
On Thu, 14 Apr 2011 19:47:06 -0400, Alex Deucher wrote:
> Apparently some distros set i2c-algo-bit.bit_test to 1 by
> default. In some cases this causes i2c_bit_add_bus
> to fail and prevents the i2c bus from being added. In the
> radeon case, we fail to add the ddc i2c buses which prevents
> the driver from being able to detect attached monitors.
> The i2c bus works fine even if bit_test fails. This is likely
> due to gpio switching that is required and handled in the
> pre/post_xfer hooks, so call the pre/post_xfer hooks in the
> bit test as well.
>
> Fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=36221
>
> Cc: Jean Delvare
> Signed-off-by: Alex Deucher
Good catch, applied, thanks. I'll also push this to the stable kernel
trees (from .38 down to .34.)
> ---
> drivers/i2c/algos/i2c-algo-bit.c | 21 ++---
> 1 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c
> b/drivers/i2c/algos/i2c-algo-bit.c
> index 38319a6..e2740e6 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -232,9 +232,16 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
> * Sanity check for the adapter hardware - check the reaction of
> * the bus lines only if it seems to be idle.
> */
> -static int test_bus(struct i2c_algo_bit_data *adap, char *name)
> +static int test_bus(struct i2c_adapter *i2c_adap, char *name)
> {
> - int scl, sda;
> + struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> + int scl, sda, ret;
> +
> + if (adap->pre_xfer) {
> + ret = adap->pre_xfer(i2c_adap);
> + if (ret < 0)
> + return -ENODEV;
> + }
>
> if (adap->getscl == NULL)
> pr_info("%s: Testing SDA only, SCL is not readable\n", name);
> @@ -297,11 +304,19 @@ static int test_bus(struct i2c_algo_bit_data *adap,
> char *name)
> "while pulling SCL high!\n", name);
> goto bailout;
> }
> +
> + if (adap->post_xfer)
> + adap->post_xfer(i2c_adap);
> +
> pr_info("%s: Test OK\n", name);
> return 0;
> bailout:
> sdahi(adap);
> sclhi(adap);
> +
> + if (adap->post_xfer)
> + adap->post_xfer(i2c_adap);
> +
> return -ENODEV;
> }
>
> @@ -607,7 +622,7 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap,
> int ret;
>
> if (bit_test) {
> - ret = test_bus(bit_adap, adap->name);
> + ret = test_bus(adap, adap->name);
> if (ret < 0)
> return -ENODEV;
> }
--
Jean Delvare
[PATCH] drm/radeon: Fix stack data leak
Always zero-init a structure on the stack which is returned by a
function. Otherwise you may leak random stack data from previous
function calls.
This fixes the following warning I was seeing:
CC [M] drivers/gpu/drm/radeon/radeon_atombios.o
drivers/gpu/drm/radeon/radeon_atombios.c: In function
"radeon_atom_get_hpd_info_from_gpio":
drivers/gpu/drm/radeon/radeon_atombios.c:261: warning: "hpd.plugged_state" is
used uninitialized in this function
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Alex Deucher
---
As a side, note, passing whole structures on the stack this way seems
suboptimal. Passing structures by address would be faster (and safer,
as it stands.)
drivers/gpu/drm/radeon/radeon_atombios.c |2 ++
1 file changed, 2 insertions(+)
--- linux-2.6.36-rc0.orig/drivers/gpu/drm/radeon/radeon_atombios.c
2010-08-15 10:24:49.0 +0200
+++ linux-2.6.36-rc0/drivers/gpu/drm/radeon/radeon_atombios.c 2010-08-15
13:52:31.0 +0200
@@ -226,6 +226,8 @@ static struct radeon_hpd radeon_atom_get
struct radeon_hpd hpd;
u32 reg;
+ memset(&hpd, 0, sizeof(struct radeon_hpd));
+
if (ASIC_IS_DCE4(rdev))
reg = EVERGREEN_DC_GPIO_HPD_A;
else
--
Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, David, On Sun, 21 Nov 2010 11:12:32 -0500, Alex Deucher wrote: > On Sun, Nov 21, 2010 at 9:38 AM, Jean Delvare wrote: > > Running 2.6.37-rc2-git7 on x86_64, wine fails to start. I get the > > following error in the kernel logs: > > > > radeon :07:00.0: r600_cs_track_validate_cb offset[0] 0 2095360 4096 too > > big > > radeon :07:00.0: r600_packet3_check:1331 invalid cmd stream 484 > > [drm:radeon_cs_ioctl] *ERROR* Invalid command stream ! > > > > Hardware is: > > 07:00.0 VGA compatible controller: ATI Technologies Inc RV710 [Radeon HD > > 4350] > > 07:00.1 Audio device: ATI Technologies Inc RV710/730 > > > > X.org version is 7.5. > > > > With kernel 2.6.36, wine works just fine on the same system. Does it > > ring a bell to anyone? Any clue how to investigate this bug? If not, I > > can start a bisection. > > > > I can also provide any additional data you need for investigation. > > It's probably this patch: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f30df2fad0c901e74ac9a52a488a54c69a373a41 > > Which exposes a bug in the 3D driver in certain cases. For some > reason we get a cb height of 8192 or greater in some cases in the 3D > driver. I haven't had time to look into why. You might try r600g for > comparison. Any progress on this? Linus' latest kernel (2.6.37-rc5-git3) still has the problem. And this is a regression, so I don't think you can just ignore it. Is there a bug opened to track this bug already? I couldn't find one on bugzilla.kernel.org, but maybe I missed it. If there is none, I'll create one. -- Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
Hi Alex, On Sun, 12 Dec 2010 23:33:42 -0500, Alex Deucher wrote: > On Sun, Dec 12, 2010 at 7:31 PM, Alex Deucher > wrote: > > On Fri, Dec 10, 2010 at 7:01 AM, Jean Delvare wrote: > >> Any progress on this? Linus' latest kernel (2.6.37-rc5-git3) still has > >> the problem. And this is a regression, so I don't think you can just > >> ignore it. > >> > >> Is there a bug opened to track this bug already? I couldn't find one on > >> bugzilla.kernel.org, but maybe I missed it. If there is none, I'll > >> create one. > > > > Was it not fixed by this commit? > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a235e4c9302509ac5956bbbffa22eb5ed9fcdc54 Not, it wasn't. > > If not, open a bug and attach the dmesg with the error messages. > > Does the attached patch fix it? Yes it does, thanks! If you consider that user-space applications failing these tests are broken, maybe you could use printk_once() to let them know? Otherwise I suspect they will never get fixed. I think there's a way to get the name of the faulty process and include it in the message. -- Jean Delvare
2.6.37-rc2-git7 regression: wine fails to start
On Mon, 13 Dec 2010 19:37:27 +1000, Dave Airlie wrote: > No its a problem with developing GPU drivers, you write some code in > userspace that seems correct, it gets rolled out, you later add a new > feature like tiling, find a bug in the kernel checker code, realise > the userspace code is wrong, and give up and become a farmer. I wish you success in you upcoming endeavour! :D I know what you mean. The kernel/user-space interface is sometimes very difficult to live with. -- Jean Delvare
[PATCH 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
Hi Justin, On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: > could be a right solution, could be wrong > here is the warning: > CC drivers/i2c/i2c-core.o > drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': > drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used > > Signed-off-by: Justin P. Mattock > > --- > drivers/i2c/i2c-core.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 1cca263..79c6c26 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > mutex_lock(&core_lock); > dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >__process_new_adapter); > + if(!dummy) > + dummy = 0; One word: scripts/checkpatch.pl In other news, the above is just plain wrong. First we force people to read the result of bus_for_each_drv() and then when they do and don't need the value, gcc complains, so we add one more layer of useless code, which developers and possibly tools will later wonder and complain about? I can easily imagine that a static code analyzer would spot the above code as being a potential bug. Let's stop this madness now please. Either __must_check goes away from bus_for_each_drv() and from every other function which raises this problem, or we must disable that new type of warning gcc 4.6.0 generates. Depends which warnings we value more, as we can't sanely have both. > mutex_unlock(&core_lock); > > return 0; -- Jean Delvare
[PATCH 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used
On Tue, 15 Jun 2010 09:20:39 -0700, David Daney wrote: > On 06/15/2010 04:40 AM, Jean Delvare wrote: > > __process_new_adapter() calls i2c_do_add_adapter() which always returns > > 0. Why should I check the return value of bus_for_each_drv() when I > > know it will always be 0 by construction? > > > > Also note that the same function is also called through > > bus_for_each_dev() somewhere else in i2c-core, and there is no warning > > there because bus_for_each_dev() is not marked __must_check. How > > consistent is this? If bus_for_each_dev() is OK without __must_check, > > then I can't see why bus_for_each_drv() wouldn't be. > > Well, I would advocate removing the __must_check then. I have just sent a patch to LKML doing exactly this. -- Jean Delvare
