[Bug 65761] HD 7970M Hybrid - hangs and errors and rmmod causes crash
https://bugzilla.kernel.org/show_bug.cgi?id=65761 --- Comment #30 from Christoph Haag --- Created attachment 124221 --> https://bugzilla.kernel.org/attachment.cgi?id=124221=edit sysprof with 2 xterm windows and kwin I still kind of want to try the opposite direction, to find out what X does that keeps the gpu awake so I'm trying to get a good dynamic call graph. The first thing that kind of worked is sysprof... So I started recording this after starting X and two xterms and kwin on the intel gpu and just clicked a bit between the two because it seems this triggers whatever happens. The radeon gpu was not even configured as offload slave, just default X configuration. If I saw that correctly none of the applications called functions from the radeon driver which is probably good, but in X you can see this (more detail is available in the file): [/usr/bin/X]0,00% 65,99% In file [heap]0,00% 29,73% ioctl 0,18% 16,17% - - kernel - -0,00% 15,99% system_call_fastpath0,00% 15,99% sys_ioctl 0,04% 15,99% do_vfs_ioctl0,00% 15,63% radeon_drm_ioctl 0,11% 15,60% drm_ioctl 0,43% 14,63% __pm_runtime_suspend0,00% 0,57% __pm_runtime_resume 0,04% 0,14% copy_user_enhanced_fast_string 0,07% 0,07% irq_exit0,00% 0,04% _copy_from_user 0,04% 0,04% drm_ioctl 0,04% 0,04% fget_light 0,25% 0,25% radeon_drm_ioctl0,04% 0,04% fput0,04% 0,04% I have only this GPU so I don't have any comparison to one where runpm works correctly, whether it should be this active... Maybe it's interesting that stuff like __pm_runtime_suspend and __pm_runtime_resume is called? -- You are receiving this mail because: You are watching the assignee of the bug.
[PATCH] drm/gma500: remove stub .open/postclose
On Wed, Jan 29, 2014 at 2:39 PM, David Herrmann
wrote:
> These are unused and can safely be dropped. DRM core verifies they're
> non-NULL before it calls them.
>
> Cc: Patrik Jakobsson
> Signed-off-by: David Herrmann
> ---
> drivers/gpu/drm/gma500/psb_drv.c | 11 ---
> 1 file changed, 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c
> b/drivers/gpu/drm/gma500/psb_drv.c
> index 1199180..d224503 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -588,15 +588,6 @@ static int psb_stolen_memory_ioctl(struct drm_device
> *dev, void *data,
> return 0;
> }
>
> -static int psb_driver_open(struct drm_device *dev, struct drm_file *priv)
> -{
> - return 0;
> -}
> -
> -static void psb_driver_close(struct drm_device *dev, struct drm_file *priv)
> -{
> -}
> -
> static long psb_unlocked_ioctl(struct file *filp, unsigned int cmd,
>unsigned long arg)
> {
> @@ -672,9 +663,7 @@ static struct drm_driver driver = {
> .disable_vblank = psb_disable_vblank,
> .get_vblank_counter = psb_get_vblank_counter,
> .lastclose = psb_lastclose,
> - .open = psb_driver_open,
> .preclose = psb_driver_preclose,
> - .postclose = psb_driver_close,
>
> .gem_free_object = psb_gem_free_object,
> .gem_vm_ops = _gem_vm_ops,
> --
> 1.8.5.3
>
Thanks, I'll put it in gma500-next
Reviewed-by: Patrik Jakobsson
[RFC 14/16] drm/nouveau/fb: add GK20A support
On Sun, Feb 2, 2014 at 8:58 AM, Lucas Stach wrote: > Am Samstag, den 01.02.2014, 18:28 -0500 schrieb Ilia Mirkin: >> On Sat, Feb 1, 2014 at 8:40 AM, Lucas Stach wrote: >> > Am Samstag, den 01.02.2014, 12:16 +0900 schrieb Alexandre Courbot: >> >> Add a clumsy-but-working FB support for GK20A. This chip only uses system >> >> memory, so we allocate a big chunk using CMA and let the existing memory >> >> managers work on it. >> >> >> >> A better future design would be to allocate objects directly from system >> >> memory without having to suffer from the limitations of a large, >> >> contiguous pool. >> >> >> > I don't know if Tegra124 is similar to 114 in this regard [hint: get the >> > TRM out :)], but if you go for a dedicated VRAM allocator, wouldn't it >> > make sense to take a chunk of the MMIO overlaid memory for this when >> > possible, rather than carving this out of CPU accessible mem? >> >> This is probably a stupid question... what do you need VRAM for >> anyways? In _theory_ it's an abstraction to talk about memory that's >> not accessible by the CPU. This is obviously not the case here, and >> presumably the GPU can access all the memory in the system, so it can >> be all treated as "GART" memory... AFAIK all accesses are behind the >> in-GPU MMU, so contiguous physical memory isn't an issue either. In >> practice, I suspect nouveau automatically sticks certain things into >> vram (gpuobj's), but it should be feasible to make them optionally use >> GART memory when VRAM is not available. I haven't really looked at the >> details though, perhaps that's a major undertaking. >> >> -ilia >> > If it's similar to the Tegar114 there actually is memory that isn't > accessible from the CPU. About 2GB of the address space is overlaid with > MMIO for the devices, so in a 4GB system you potentially have 2GB of RAM > that's only visible for the devices. > > But yes in general nouveau should just fall back to a GART placement if > VRAM isn't available. With the limited time I spent studying it, it seems to me that Nouveau has a strong dependency on VRAM. For gpuobjects indeed (that one could be workarounded with a new instmem driver I suppose), and also for TTM: objects placed in TTM_PL_VRAM are handled by the VRAM manager, which requires a nouveau_ram instance in the FB. Actually the FB also seems to assume the presence of a dedicated video RAM. So while I agree that getting rid of VRAM altogether would be the most logical solution, I have not found a way to do so for the moment. T124's GPU actually sees the same physical address space as the CPU, so memory management should be simplified thanks to that (you could enable the SMMU and make things more interesting/complex, but for now it seems untimely to even consider doing so). Actually even the concept of a GART is not needed here: all your memory management needs could be fulfilled by getting pages with alloc_page() and arranging them using the GMMU. No GART, no BAR (at least for the purpose of mapping objects for CPU access), no PRAMIN. I really wonder how that picture would fit within Nouveau, and it is quite likely that there is an elegant solution to this problem already that my lack of understanding of Nouveau prevents me from seeing. That's why your thoughts on this matter would be greatly appreciated. Thanks, Alex.
[Bug 74420] New: EQ overflowing - recurring total X crash
x44cc92] (EE) 2: /usr/lib/xorg/modules/input/evdev_drv.so (0x7fb35282d000+0x581d) [0x7fb35283281d] (EE) 3: /usr/bin/X (0x40+0x72f68) [0x472f68] (EE) 4: /usr/bin/X (0x40+0x9b530) [0x49b530] (EE) 5: /usr/lib/libpthread.so.0 (0x7fb35c4d2000+0xf870) [0x7fb35c4e1870] (EE) 6: /usr/lib/libc.so.6 (ioctl+0x7) [0x7fb35b201e27] (EE) 7: /usr/lib/libdrm.so.2 (drmIoctl+0x34) [0x7fb35c2c99d4] (EE) 8: /usr/lib/libdrm.so.2 (drmCommandWrite+0x1e) [0x7fb35c2cbd4e] (EE) 9: /usr/lib/libdrm_radeon.so.1 (0x7fb35688+0x2179) [0x7fb356882179] (EE) 10: /usr/lib/libdrm_radeon.so.1 (0x7fb35688+0x237c) [0x7fb35688237c] (EE) 11: /usr/lib/xorg/modules/drivers/radeon_drv.so (0x7fb356a8e000+0x27d77) [0x7fb356ab5d77] (EE) 12: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x58f7) [0x7fb35666e8f7] (EE) 13: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x7f18) [0x7fb356670f18] (EE) 14: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x10d8c) [0x7fb356679d8c] (EE) 15: /usr/bin/X (0x40+0x1116cd) [0x5116cd] (EE) 16: /usr/bin/X (0x40+0x326d2) [0x4326d2] (EE) 17: /usr/bin/X (0x40+0x35d1e) [0x435d1e] (EE) 18: /usr/bin/X (0x40+0x39b2a) [0x439b2a] (EE) 19: /usr/lib/libc.so.6 (__libc_start_main+0xf5) [0x7fb35b145b05] (EE) 20: /usr/bin/X (0x40+0x250ce) [0x4250ce] (EE) (EE) [mi] EQ overflow continuing. 300 events have been dropped. (EE) (EE) Backtrace: (EE) 0: /usr/bin/X (xorg_backtrace+0x48) [0x584ae8] (EE) 1: /usr/bin/X (QueuePointerEvents+0x52) [0x44cc92] (EE) 2: /usr/lib/xorg/modules/input/evdev_drv.so (0x7fb35282d000+0x581d) [0x7fb35283281d] (EE) 3: /usr/bin/X (0x40+0x72f68) [0x472f68] (EE) 4: /usr/bin/X (0x40+0x9b530) [0x49b530] (EE) 5: /usr/lib/libpthread.so.0 (0x7fb35c4d2000+0xf870) [0x7fb35c4e1870] (EE) 6: /usr/lib/libc.so.6 (ioctl+0x7) [0x7fb35b201e27] (EE) 7: /usr/lib/libdrm.so.2 (drmIoctl+0x34) [0x7fb35c2c99d4] (EE) 8: /usr/lib/libdrm.so.2 (drmCommandWrite+0x1e) [0x7fb35c2cbd4e] (EE) 9: /usr/lib/libdrm_radeon.so.1 (0x7fb35688+0x2179) [0x7fb356882179] (EE) 10: /usr/lib/libdrm_radeon.so.1 (0x7fb35688+0x237c) [0x7fb35688237c] (EE) 11: /usr/lib/xorg/modules/drivers/radeon_drv.so (0x7fb356a8e000+0x27d77) [0x7fb356ab5d77] (EE) 12: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x58f7) [0x7fb35666e8f7] (EE) 13: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x7f18) [0x7fb356670f18] (EE) 14: /usr/lib/xorg/modules/libexa.so (0x7fb356669000+0x10d8c) [0x7fb356679d8c] (EE) 15: /usr/bin/X (0x40+0x1116cd) [0x5116cd] (EE) 16: /usr/bin/X (0x40+0x326d2) [0x4326d2] (EE) 17: /usr/bin/X (0x40+0x35d1e) [0x435d1e] (EE) 18: /usr/bin/X (0x40+0x39b2a) [0x439b2a] (EE) 19: /usr/lib/libc.so.6 (__libc_start_main+0xf5) [0x7fb35b145b05] (EE) 20: /usr/bin/X (0x40+0x250ce) [0x4250ce] (EE) [ 5081.273] [mi] Increasing EQ size to 1024 to prevent dropped events. [ 5081.274] [mi] EQ processing has resumed after 369 dropped events. [ 5081.274] [mi] This may be caused my a misbehaving driver monopolizing the server's resources. [ 5090.921] (II) AIGLX: Suspending AIGLX clients for VT switch [ 5092.404] (II) Open ACPI successful (/var/run/acpid.socket) [ 5092.404] (II) AIGLX: Resuming AIGLX clients after VT switch [ 5092.408] (II) RADEON(0): EDID vendor "LEN", prod id 16469 [ 5092.408] (II) RADEON(0): Printing DDC gathered Modelines: [ 5092.408] (II) RADEON(0): Modeline "1920x1200"x0.0 167.80 1920 1968 2000 2298 1200 1203 1206 1217 -hsync -vsync (73.0 kHz eP) [ 5092.408] (II) RADEON(0): Modeline "1920x1200"x0.0 134.28 1920 1968 2000 2208 1200 1202 1208 1220 -hsync -vsync (60.8 kHz e) [ 5092.449] setversion 1.4 failed: Permission denied [ 5095.536] (II) AIGLX: Suspending AIGLX clients for VT switch [ 5178.680] (II) Open ACPI successful (/var/run/acpid.socket) [ 5178.680] (II) AIGLX: Resuming AIGLX clients after VT switch [ 5178.684] (II) RADEON(0): EDID vendor "LEN", prod id 16469 [ 5178.685] (II) RADEON(0): Printing DDC gathered Modelines: [ 5178.685] (II) RADEON(0): Modeline "1920x1200"x0.0 167.80 1920 1968 2000 2298 1200 1203 1206 1217 -hsync -vsync (73.0 kHz eP) [ 5178.685] (II) RADEON(0): Modeline "1920x1200"x0.0 134.28 1920 1968 2000 2208 1200 1202 1208 1220 -hsync -vsync (60.8 kHz e) [ 5206.930] (II) AIGLX: Suspending AIGLX clients for VT switch[/code] -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/e1094f0c/attachment-0001.html>
[RFC 00/16] drm/nouveau: initial support for GK20A (Tegra K1)
On Sun, Feb 2, 2014 at 9:44 PM, Alexandre Courbot wrote: > One beginner question: is it appropriate to send kernel patches to the > nouveau list in addition to dri-devel? The moderation messages I receive > make me think that this list might rather be intended for general > discussion. I usually do. The main thing is to make sure that they're To: Ben, since he's the one who will be ultimately be picking them up. I think that if you're not subscribed, all the lists.freedesktop.org lists moderate you, but dri-devel is configured not to tell you about it. Also I've been getting bounce messages from nouveau@ complaining of too many cc's and so it's getting auto-moderated -- not sure who, if anyone, is an admin of the nouveau list. Hopefully someone :) -ilia
[PATCH v5 00/23]
On Sun, 2 Feb 2014 19:15:05 + Russell King - ARM Linux wrote: > In which case, it may be better to reorder the remaining patches such > that the DT changes are at the very end - which means we can still > benefit from the rest of the patches if the DT solution remains an > open question. > > We do have another option now that my generic component support is in > mainline (merged during this window), which should result in a much > cleaner solution. If we convert TDA998x to a component, armada DRM > to a component master (and same for other tda998x users) then we don't > need the drm_encoder_slave stuff - all that goes away since it's no > longer necessary. > > We also solve this problem as well - because we're then not messing > around with working out if there's a DT node present: the TDA998x > device must pre-exist. For non-DT setups, this can be done when > the I2C bus is created - devices on it would be created using the > standard mechanisms already present via the i2c_board_data array. > For DT setups, the devices are created by parsing the I2C bus node > in DT. > > Both cases result in a component being registered upon invocation of > tda998x_probe(), and removal of the component when tda998x_remove() > is called. The tda998x driver becomes a standard I2C driver. > > This is something I've been intending to look at now that the component > stuff is in place - as I said previously when the questions around DT > and Armada DRM were first posed, we need to solve these issues in a > generic way first, rather than hacking around them. Agree. I was looking in the same direction... -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[PATCH v5 00/23]
On Sun, 2 Feb 2014 18:04:34 + Russell King - ARM Linux wrote: > So, in summary, I'm pretty happy with this again - and it's all been > tested here with no apparant detrimental effects. All committed and > queued up here: > > http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=tda998x-devel > > If you want to pull it back to check: > > git://ftp.arm.linux.org.uk/~rmk/linux-cubox.git tda998x-devel > > I'm proposing to send everything up to the tda998x-fixes tag next week > for 3.13-rc kernels. Everything is OK. Thank you. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[PATCH v5 00/23]
On Sun, 2 Feb 2014 18:23:49 + Russell King - ARM Linux wrote: > On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote: > > On Sun, 2 Feb 2014 12:43:58 + > > Russell King - ARM Linux wrote: > > > > > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > > > This patch set contains various extensions to the tda998x driver: > > > > > > > > - simplify the i2c read/write > > > > - code cleanup and fix some small errors > > > > - use global constants > > > > - don't read write-only registers > > > > - add DT support > > > > - use IRQ for connection status and EDID read > > > > > > I discussed these patches with Rob Clark recently, and the conclusion > > > we came to is that I'll merge them into a git tree, test them, and > > > once I'm happy I'll send a pull request as appropriate. > > > > > > I'll go through them later today. Those patches which have been re- > > > posted without any change for the last few times (the first few) I'll > > > take into my git tree today so you don't have to keep re-posting them > > > (more importantly, I won't have to keep on looking at them either.) > > > > Thanks. > > > > BTW, I found some problems with the patch 12 'add DT support' you > > already acked: > > > > - the .of_match_table is not needed because the i2c client is created by > > the i2c subsystem from the 'reg' in the DT, > > Okay - may it be a good idea to have someone knowledgable of I2C give it > a review? Sure! There is still a lot of magic in the DT. I used the tda998x in the DT for a long time without .of_match_table and it loaded correctly. I added it in the patch just because my other modules had it. A few days ago, when I moved the tda998x audio codec description in the DT as a subnode of the tda998x i2c, the codec module was not loaded. An of_platform_populate() of the subnodes was needed in the tda998x i2c driver. I could not find why... > > - on encoder_destroy(), the function drm_i2c_encoder_destroy() > > unregisters the i2c client, so, with a DT, a second encoder_init() > > would crash. > > I think this is one of the down-sides of trying to bolt DT into this: > the drm encoder slave support is not designed to cope with an i2c client > device pre-created. > > In fact, I can't see how this stuff comes anywhere close to working in > a DT setup: in such a scenario, you declare that there's a tda998x > device in DT. I2C parses this, and creates an i2c_client itself for > the tda998x. > > When the TDA998x driver initialises, it finds this i2c client and > binds to it, calling tda998x_probe(), which does nothing. > > However, the only way to attach a slave encoder to a DRM device is via > a call to drm_i2c_encoder_init(), which unconditionally calls > i2c_new_device(). This creates a _new_ i2c_client structure, again > unconditionally, for the tda998x. This must be bound by the I2C > subsystem to a driver - hopefully the tda998x driver, which then > calls it's encoder_init function. > > None of this will happen if DT has already created an i2c_client at > the appropriate address, because DRMs i2c_new_device() will fail. > > My hypothesis is that you have other patches to I2C and/or DRM to > sort this out which you haven't been posting with this series. So, > could you please provide some hints as to how this works? I explained how to use the tda998x in a DT context in a message to Jyri Sarha: http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[Bug 69723] GPU lockups with kernel 3.11.0 / 3.12-rc1 when dpm=1 on r600g (Cayman)
https://bugs.freedesktop.org/show_bug.cgi?id=69723
--- Comment #105 from Alexandre Demers ---
(In reply to comment #104)
> (In reply to comment #103)
> > I don't remember if we've tried this recently, but does disabling power
> > containment help?
> >
> > diff --git a/drivers/gpu/drm/radeon/ni_dpm.c
> > b/drivers/gpu/drm/radeon/ni_dpm.c
> > index 22c3391..19b7c68 100644
> > --- a/drivers/gpu/drm/radeon/ni_dpm.c
> > +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> > @@ -4250,7 +4250,7 @@ int ni_dpm_init(struct radeon_device *rdev)
> > break;
> > }
> >
> > - if (ni_pi->cac_weights->enable_power_containment_by_default) {
> > + if (0/*ni_pi->cac_weights->enable_power_containment_by_default*/) {
> > ni_pi->enable_power_containment = true;
> > ni_pi->enable_cac = true;
> > ni_pi->enable_sq_ramping = true;
>
> I don't remember playing with it lately, so I'll try it either later today
> (tonight) or tomorrow. I have to complete a report first for a personal
> project.
I've been testing it since Friday night (Desktop and games). I had a single
lock, but it was while playing a video. Since I suspect this may be related to
other issues that I've seen patches for, I'll continue testing it and I'll
report in a couple of days if things seem stable (or more stable).
--
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/b5de008a/attachment-0001.html>
[PATCH v5 00/23]
On 02/02/2014 07:23 PM, Russell King - ARM Linux wrote: > On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote: >> - on encoder_destroy(), the function drm_i2c_encoder_destroy() >>unregisters the i2c client, so, with a DT, a second encoder_init() >>would crash. > > I think this is one of the down-sides of trying to bolt DT into this: > the drm encoder slave support is not designed to cope with an i2c client > device pre-created. > > In fact, I can't see how this stuff comes anywhere close to working in > a DT setup: in such a scenario, you declare that there's a tda998x > device in DT. I2C parses this, and creates an i2c_client itself for > the tda998x. > > When the TDA998x driver initialises, it finds this i2c client and > binds to it, calling tda998x_probe(), which does nothing. > > However, the only way to attach a slave encoder to a DRM device is via > a call to drm_i2c_encoder_init(), which unconditionally calls > i2c_new_device(). This creates a _new_ i2c_client structure, again > unconditionally, for the tda998x. This must be bound by the I2C > subsystem to a driver - hopefully the tda998x driver, which then > calls it's encoder_init function. > > None of this will happen if DT has already created an i2c_client at > the appropriate address, because DRMs i2c_new_device() will fail. drm_i2c_encoder_init() could look at .of_node of the i2c_board_info. If it is there, do not try to i2c_new_device as it has already been registered by DT i2c auto-probing. Sebastian
[RFC 00/16] drm/nouveau: initial support for GK20A (Tegra K1)
On Sun, Feb 2, 2014 at 6:44 PM, Alexandre Courbot wrote: > On 02/03/2014 04:10 AM, Ilia Mirkin wrote: > >> Hi Alexandre, >> >> On Fri, Jan 31, 2014 at 10:16 PM, Alexandre Courbot >> wrote: >> >>> I guess my email address might surprise some of you, so let me >>> anticipate some >>> questions you might have. :P Yes, this work is endorsed by NVIDIA. >>> Several other >>> NVIDIAns (CC'd), including core GPU experts, have provided significant >>> technical >>> guidance and will continue their involvement. Special thanks go to Terje >>> Bergstrom and Ken Adams for their invaluable GPU expertise, and Thierry >>> Reding >>> (at FOSDEM this weekend) for help with debugging and user-space testing. >>> >>> Let me also stress that although very exciting, this effort is still >>> experimental, so I would like to make sure that nobody makes excessive >>> expectations based on these few patches. The scope of this work is >>> strictly >>> limited to Tegra (although given the similarities desktop GPU support >>> will >>> certainly benefit from it indirectly), and we do not have any plan to >>> work on >>> user-space support. So do not uninstall that proprietary driver just >>> yet. ;) >>> >>> With this being clarified, we are looking forward to getting your >>> feedback and >>> working with you guys to bring and improve Tegra K1 support into >>> Nouveau! :) >>> >> >> I've sent a couple of fairly trivial comments, as you saw, and I >> suspect that others with a better understanding of the guts will have >> more substantial architectural feedback, esp after the weekend/FOSDEM. >> However, since no one's said it already -- welcome to Nouveau! >> > > Thanks! ^_^v > > One beginner question: is it appropriate to send kernel patches to the > nouveau list in addition to dri-devel? The moderation messages I receive > make me think that this list might rather be intended for general > discussion. The moderation was because there are too many CCs: on this thread. A couple of those could probably be dropped. In any case, I've increased the max. St?phane > > > From the looks of it, you could bring up a full open-source stack with >> your patches (i.e. Xorg + nouveau DDX + mesa) and use PRIME to render >> stuff (assuming the actual display hw has an X ddx). Although I >> suspect that you're going to want to use your own drivers. Still a >> little curious if you've tried the open-source stack and whether it >> worked. [Not sure what the status is of render-node support is in >> mesa, but perhaps it's enough to try running piglit tests, if you >> can't get X going with the display HW.] >> > > We are still testing things at libdrm level, but are eventually interested > in bringing up the existing open-source stack. Our guess (and hope) is that > it will work nicely almost as-is, minus the fact that the display hardware > is not handled by Nouveau and we only support render nodes (I have yet to > look at what the state of render nodes in Mesa is). > > For X, Thierry is IIUC working on the display driver, and at some point > these efforts should join to connect tegradrm and Nouveau using PRIME. We > are not quite there yet, and since we are working with limited resources it > will likely require some time, but the fact we could bring up a (seemingly) > working Nouveau kernel driver with so little code is encouraging. > > Thanks, > Alex. > > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/5db85f0d/attachment-0001.html>
[PATCH v5 00/23]
On Sun, Feb 02, 2014 at 07:54:00PM +0100, Jean-Francois Moine wrote: > I explained how to use the tda998x in a DT context in a message to Jyri > Sarha: > > http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html Okay, so there's a bunch of changes required to the DRM slave support which aren't in place yet. In which case, it may be better to reorder the remaining patches such that the DT changes are at the very end - which means we can still benefit from the rest of the patches if the DT solution remains an open question. We do have another option now that my generic component support is in mainline (merged during this window), which should result in a much cleaner solution. If we convert TDA998x to a component, armada DRM to a component master (and same for other tda998x users) then we don't need the drm_encoder_slave stuff - all that goes away since it's no longer necessary. We also solve this problem as well - because we're then not messing around with working out if there's a DT node present: the TDA998x device must pre-exist. For non-DT setups, this can be done when the I2C bus is created - devices on it would be created using the standard mechanisms already present via the i2c_board_data array. For DT setups, the devices are created by parsing the I2C bus node in DT. Both cases result in a component being registered upon invocation of tda998x_probe(), and removal of the component when tda998x_remove() is called. The tda998x driver becomes a standard I2C driver. This is something I've been intending to look at now that the component stuff is in place - as I said previously when the questions around DT and Armada DRM were first posed, we need to solve these issues in a generic way first, rather than hacking around them. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 00/23]
On Sun, 2 Feb 2014 12:43:58 + Russell King - ARM Linux wrote: > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > This patch set contains various extensions to the tda998x driver: > > > > - simplify the i2c read/write > > - code cleanup and fix some small errors > > - use global constants > > - don't read write-only registers > > - add DT support > > - use IRQ for connection status and EDID read > > I discussed these patches with Rob Clark recently, and the conclusion > we came to is that I'll merge them into a git tree, test them, and > once I'm happy I'll send a pull request as appropriate. > > I'll go through them later today. Those patches which have been re- > posted without any change for the last few times (the first few) I'll > take into my git tree today so you don't have to keep re-posting them > (more importantly, I won't have to keep on looking at them either.) Thanks. BTW, I found some problems with the patch 12 'add DT support' you already acked: - the .of_match_table is not needed because the i2c client is created by the i2c subsystem from the 'reg' in the DT, - on encoder_destroy(), the function drm_i2c_encoder_destroy() unregisters the i2c client, so, with a DT, a second encoder_init() would crash. Do you better like a new patch or a fix? -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
On Sun, 2 Feb 2014 16:23:09 + Russell King - ARM Linux wrote: > On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote: > > This patch takes care of the write-only registers of the tda998x. > > > > The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits > > cleared after reset, so, they may be fully re-written. > > > > The register MAT_CONTRL is set to > > MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) > > after reset, so, it may be fully set again to this value. > > I said in v3 of this patch, which seems to remain unaddressed: > > > /* must be last register set: */ > > - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); > > + reg_write(priv, REG_TBG_CNTRL_0, 0); > > Register changes which have a potential effect shouldn't be part of a > patch which is really only trying to avoid reading from write only > registers. > > This could be a potential functional change - and it's probably one > which Rob Clark should at least be made aware of. As I commented last > time, when you're changing register values in an otherwise innocuous > patch, you should comment about them in the patch description. According to the tda9983b documentation, the register TBG_CNTRL_0 is set to 0 at reset time. I think that it is the same for all the tda998x family. In the other hand, this register is supposed to be write only, so reading it may return any value, and the reg_clear() function may set any other bits. Then, clearing one bit is less secure than clearing the full register. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[Bug 69301] no screen on update from 3.12.0
https://bugzilla.kernel.org/show_bug.cgi?id=69301 --- Comment #12 from Jakob Kummerow --- Bisection result: commit 56684ec5b050e6a392cb3e5324eda12a13413a57 Author: Alex Deucher Date: Wed Oct 30 10:18:37 2013 -0400 drm/radeon: enable DPM by default on BTC asics Seems to be stable on them. Which makes sense in so far as I have a Barts card, but obviously this commit did not introduce the actual problem. I guess I'll have to bisect again with an explicit radeon.dpm=1 kernel parameter. FWIW, on 3.12 and earlier, I used to "echo low > /sys/class/drm/card0/device/power_profile" by means of an init script, and never had any issues with that. -- You are receiving this mail because: You are watching the assignee of the bug.
[RFC 13/16] drm/nouveau/ibus: add GK20A support
On Sun, Feb 2, 2014 at 3:35 PM, Ilia Mirkin wrote:
> Some very trivial comments below:
>
> On Fri, Jan 31, 2014 at 10:16 PM, Alexandre Courbot
> wrote:
>> Add support for initializing the priv ring of GK20A. This is done by the
>> BIOS on desktop GPUs, but needs to be done by hand on Tegra.
>>
>> Signed-off-by: Alexandre Courbot
>> ---
>> drivers/gpu/drm/nouveau/Makefile | 1 +
>> drivers/gpu/drm/nouveau/core/include/subdev/ibus.h | 1 +
>> drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c| 108
>> +
>> 3 files changed, 110 insertions(+)
>> create mode 100644 drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
>>
>> diff --git a/drivers/gpu/drm/nouveau/Makefile
>> b/drivers/gpu/drm/nouveau/Makefile
>> index 6c4b76d..3548fcd 100644
>> --- a/drivers/gpu/drm/nouveau/Makefile
>> +++ b/drivers/gpu/drm/nouveau/Makefile
>> @@ -132,6 +132,7 @@ nouveau-y += core/subdev/i2c/nv94.o
>> nouveau-y += core/subdev/i2c/nvd0.o
>> nouveau-y += core/subdev/ibus/nvc0.o
>> nouveau-y += core/subdev/ibus/nve0.o
>> +nouveau-y += core/subdev/ibus/nvea.o
>> nouveau-y += core/subdev/instmem/base.o
>> nouveau-y += core/subdev/instmem/nv04.o
>> nouveau-y += core/subdev/instmem/nv40.o
>> diff --git a/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
>> b/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
>> index 88814f1..056a42f 100644
>> --- a/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
>> +++ b/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
>> @@ -30,5 +30,6 @@ nouveau_ibus(void *obj)
>>
>> extern struct nouveau_oclass nvc0_ibus_oclass;
>> extern struct nouveau_oclass nve0_ibus_oclass;
>> +extern struct nouveau_oclass nvea_ibus_oclass;
>>
>> #endif
>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
>> b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
>> new file mode 100644
>> index 000..0bcd281
>> --- /dev/null
>> +++ b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright (c) 2014, NVIDIA Corporation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + */
>> +
>> +#include
>> +
>> +struct nvea_ibus_priv {
>> + struct nouveau_ibus base;
>> +};
>> +
>> +static void
>> +nvea_ibus_init_priv_ring(struct nvea_ibus_priv *priv)
>> +{
>> + u32 data;
>> +
>> + data = nv_rd32(priv, 0x137250);
>> + data &= (~0x3f);
>> + nv_wr32(priv, 0x137250, data);
>
> nv_mask(priv, 0x137250, 0x3f, 0) should do this, right?
>
>> +
>> + nv_mask(priv, 0x000200, 0x20, 0);
>> + udelay(20);
>> + nv_mask(priv, 0x000200, 0x20, 0x20);
>> +
>> + nv_wr32(priv, 0x12004c, 0x4);
>> + nv_wr32(priv, 0x122204, 0x2);
>> + nv_rd32(priv, 0x122204);
>> +}
>> +
>> +static void
>> +nvea_ibus_intr(struct nouveau_subdev *subdev)
>> +{
>> + struct nvea_ibus_priv *priv = (void *)subdev;
>> + u32 status0 = nv_rd32(priv, 0x120058);
>> + s32 retry = 100;
>> + u32 command;
>> +
>> + if (status0 & 0x7) {
>> + nv_debug(priv, "resetting priv ring\n");
>> + nvea_ibus_init_priv_ring(priv);
>> + }
>> +
>> + /* Acknowledge interrupt */
>> + command = nv_rd32(priv, 0x0012004c);
>> + command |= 0x2;
>> + nv_wr32(priv, 0x0012004c, command);
>
> nv_mask(priv, 0x12004c, 0x2, 0x2)
Absolutely correct for both.
>> +
>> + while (--retry >= 0) {
>> + command = nv_rd32(priv, 0x12004c) & 0x3f;
>> + if (command == 0)
>> + break;
>> + }
>> +
>> + if (retry < 0)
>> + nv_debug(priv, "timeout waiting for ringmaster ack\n");
>
> this sounds kinda bad, no? perhaps a nv_warn?
Sounds more adequate indeed.
Thanks,
Alex.
[PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
On Sun, 2 Feb 2014 16:20:58 +
Russell King - ARM Linux wrote:
> On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
> > This patch adds more error checking inn I2C I/O functions.
> > In case of I/O error, this permits to avoid writing in bad controller
> > pages, a bad chipset detection or looping when getting the EDID.
>
> I've just looked at this again, and spotted something:
>
> > -static uint8_t
> > +static int
> > reg_read(struct tda998x_priv *priv, uint16_t reg)
> > {
> > uint8_t val = 0;
> > - reg_read_range(priv, reg, , sizeof(val));
> > + int ret;
> > +
> > + ret = reg_read_range(priv, reg, , sizeof(val));
> > + if (ret < 0)
> > + return ret;
>
> So yes, this can return negative numbers.
>
> > @@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client,
> > tda998x_reset(priv);
> >
> > /* read version: */
> > - priv->rev = reg_read(priv, REG_VERSION_LSB) |
> > - reg_read(priv, REG_VERSION_MSB) << 8;
> > + ret = reg_read(priv, REG_VERSION_LSB) |
> > + (reg_read(priv, REG_VERSION_MSB) << 8);
> > + if (ret < 0)
> > + goto fail;
> > + priv->rev = ret;
>
> Two issues here:
>
> 1. The additional parens are /really/ not required.
> 2. What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
>
> If we're going to the extent of attempting to make the read/write
> functions return errors, we should at least handle errors generated
> by them properly, otherwise it's pointless making them return errors.
>
> ret = reg_read(priv, REG_VERSION_LSB);
> if (ret < 0)
> goto fail;
>
> priv->rev = ret;
>
> ret = reg_read(priv, REG_VERSION_MSB);
> if (ret < 0)
> goto fail;
>
> priv->rev |= ret << 8;
>
> If you want it to look slightly nicer:
>
> int rev_lo, rev_hi;
>
> rev_lo = reg_read(priv, REG_VERSION_LSB);
> rev_hi = reg_read(priv, REG_VERSION_MSB);
> if (rev_lo < 0 || rev_hi < 0) {
> ret = rev_lo < 0 ? rev_lo : rev_hi;
> goto fail;
> }
>
> priv->rev = rev_lo | rev_hi << 8;
>
> I'm happy to commit such a change after this patch to clean it up, or if
> you want to regenerate your patch 2 and post /just/ that incorporating
> this change.
I think that my code works correctly: when there is an error, the
result of reg_read() is minus the error code, and this error code is
always lower than 8388607 (0x7f). Then, reg_read() << 8 will always
be negative.
Otherwise, I may redo a patch about the useless parenthesis.
--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
[PATCH v5 00/23]
On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote: > On Sun, 2 Feb 2014 12:43:58 + > Russell King - ARM Linux wrote: > > > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > > This patch set contains various extensions to the tda998x driver: > > > > > > - simplify the i2c read/write > > > - code cleanup and fix some small errors > > > - use global constants > > > - don't read write-only registers > > > - add DT support > > > - use IRQ for connection status and EDID read > > > > I discussed these patches with Rob Clark recently, and the conclusion > > we came to is that I'll merge them into a git tree, test them, and > > once I'm happy I'll send a pull request as appropriate. > > > > I'll go through them later today. Those patches which have been re- > > posted without any change for the last few times (the first few) I'll > > take into my git tree today so you don't have to keep re-posting them > > (more importantly, I won't have to keep on looking at them either.) > > Thanks. > > BTW, I found some problems with the patch 12 'add DT support' you > already acked: > > - the .of_match_table is not needed because the i2c client is created by > the i2c subsystem from the 'reg' in the DT, Okay - may it be a good idea to have someone knowledgable of I2C give it a review? > - on encoder_destroy(), the function drm_i2c_encoder_destroy() > unregisters the i2c client, so, with a DT, a second encoder_init() > would crash. I think this is one of the down-sides of trying to bolt DT into this: the drm encoder slave support is not designed to cope with an i2c client device pre-created. In fact, I can't see how this stuff comes anywhere close to working in a DT setup: in such a scenario, you declare that there's a tda998x device in DT. I2C parses this, and creates an i2c_client itself for the tda998x. When the TDA998x driver initialises, it finds this i2c client and binds to it, calling tda998x_probe(), which does nothing. However, the only way to attach a slave encoder to a DRM device is via a call to drm_i2c_encoder_init(), which unconditionally calls i2c_new_device(). This creates a _new_ i2c_client structure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it's encoder_init function. None of this will happen if DT has already created an i2c_client at the appropriate address, because DRMs i2c_new_device() will fail. My hypothesis is that you have other patches to I2C and/or DRM to sort this out which you haven't been posting with this series. So, could you please provide some hints as to how this works? -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 00/23]
On Sun, Feb 02, 2014 at 12:43:58PM +, Russell King - ARM Linux wrote: > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > This patch set contains various extensions to the tda998x driver: > > > > - simplify the i2c read/write > > - code cleanup and fix some small errors > > - use global constants > > - don't read write-only registers > > - add DT support > > - use IRQ for connection status and EDID read > > I discussed these patches with Rob Clark recently, and the conclusion > we came to is that I'll merge them into a git tree, test them, and > once I'm happy I'll send a pull request as appropriate. > > I'll go through them later today. Those patches which have been re- > posted without any change for the last few times (the first few) I'll > take into my git tree today so you don't have to keep re-posting them > (more importantly, I won't have to keep on looking at them either.) Okay, out of your patches, I would like to queue up the following for submission into -rc kernels: drm/i2c: tda998x: fix bad value in the AIF drm/i2c: tda998x: check the CEC device creation drm/i2c: tda998x: free the CEC device on encoder_destroy drm/i2c: tda998x: force the page register at startup time drm/i2c: tda998x: set the PLL division factor in range 0..3 drm/i2c: tda998x: fix the ENABLE_SPACE register since these fix real bugs. Moving them to the front of the queue isn't that big a deal (I've done it here in my git tree - yes, there were a few conflicts, but nothing serious.) I'll also consider these for -rc too: drm/i2c: tda998x: use HDMI constants drm/i2c: tda998x: add the active aspect in HDMI AVI frame drm/i2c: tda998x: change the frequence in the audio channel if we have reports of display devices affected by the info frame errors. As far as the last summary line goes, I'll change it to: "Use ALSA IEC958 definitions and update audio frequency" Note that in general, bug fixes should always be towards the beginning of a patch series, so that they can be applied independently of the remainder of the development, which also makes them easier to backport to stable kernels if that's deemed necessary. As for the remainder, I think moving the DT documentation patch immediately after the addition of DT code (or even just before it) is good form. So, in summary, I'm pretty happy with this again - and it's all been tested here with no apparant detrimental effects. All committed and queued up here: http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=tda998x-devel If you want to pull it back to check: git://ftp.arm.linux.org.uk/~rmk/linux-cubox.git tda998x-devel I'm proposing to send everything up to the tda998x-fixes tag next week for 3.13-rc kernels. Rob, do you want to provide any acks for this or are you happy? -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
On Sun, Feb 02, 2014 at 06:45:12PM +0100, Jean-Francois Moine wrote: > On Sun, 2 Feb 2014 16:23:09 + > Russell King - ARM Linux wrote: > > > On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote: > > > This patch takes care of the write-only registers of the tda998x. > > > > > > The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits > > > cleared after reset, so, they may be fully re-written. > > > > > > The register MAT_CONTRL is set to > > > MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) > > > after reset, so, it may be fully set again to this value. > > > > I said in v3 of this patch, which seems to remain unaddressed: > > > > > /* must be last register set: */ > > > - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); > > > + reg_write(priv, REG_TBG_CNTRL_0, 0); > > > > Register changes which have a potential effect shouldn't be part of a > > patch which is really only trying to avoid reading from write only > > registers. > > > > This could be a potential functional change - and it's probably one > > which Rob Clark should at least be made aware of. As I commented last > > time, when you're changing register values in an otherwise innocuous > > patch, you should comment about them in the patch description. > > According to the tda9983b documentation, the register TBG_CNTRL_0 is > set to 0 at reset time. I think that it is the same for all the tda998x > family. In the other hand, this register is supposed to be write only, > so reading it may return any value, and the reg_clear() function may > set any other bits. Then, clearing one bit is less secure than clearing > the full register. Okay, in that case I'm happy with this patch. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
On Sun, Feb 02, 2014 at 06:30:00PM +0100, Jean-Francois Moine wrote:
> On Sun, 2 Feb 2014 16:20:58 +
> Russell King - ARM Linux wrote:
>
> > On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
> > > This patch adds more error checking inn I2C I/O functions.
> > > In case of I/O error, this permits to avoid writing in bad controller
> > > pages, a bad chipset detection or looping when getting the EDID.
> >
> > I've just looked at this again, and spotted something:
> >
> > > -static uint8_t
> > > +static int
> > > reg_read(struct tda998x_priv *priv, uint16_t reg)
> > > {
> > > uint8_t val = 0;
> > > - reg_read_range(priv, reg, , sizeof(val));
> > > + int ret;
> > > +
> > > + ret = reg_read_range(priv, reg, , sizeof(val));
> > > + if (ret < 0)
> > > + return ret;
> >
> > So yes, this can return negative numbers.
> >
> > > @@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client,
> > > tda998x_reset(priv);
> > >
> > > /* read version: */
> > > - priv->rev = reg_read(priv, REG_VERSION_LSB) |
> > > - reg_read(priv, REG_VERSION_MSB) << 8;
> > > + ret = reg_read(priv, REG_VERSION_LSB) |
> > > + (reg_read(priv, REG_VERSION_MSB) << 8);
> > > + if (ret < 0)
> > > + goto fail;
> > > + priv->rev = ret;
> >
> > Two issues here:
> >
> > 1. The additional parens are /really/ not required.
> > 2. What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
> >
> > If we're going to the extent of attempting to make the read/write
> > functions return errors, we should at least handle errors generated
> > by them properly, otherwise it's pointless making them return errors.
> >
> > ret = reg_read(priv, REG_VERSION_LSB);
> > if (ret < 0)
> > goto fail;
> >
> > priv->rev = ret;
> >
> > ret = reg_read(priv, REG_VERSION_MSB);
> > if (ret < 0)
> > goto fail;
> >
> > priv->rev |= ret << 8;
> >
> > If you want it to look slightly nicer:
> >
> > int rev_lo, rev_hi;
> >
> > rev_lo = reg_read(priv, REG_VERSION_LSB);
> > rev_hi = reg_read(priv, REG_VERSION_MSB);
> > if (rev_lo < 0 || rev_hi < 0) {
> > ret = rev_lo < 0 ? rev_lo : rev_hi;
> > goto fail;
> > }
> >
> > priv->rev = rev_lo | rev_hi << 8;
> >
> > I'm happy to commit such a change after this patch to clean it up, or if
> > you want to regenerate your patch 2 and post /just/ that incorporating
> > this change.
>
> I think that my code works correctly: when there is an error, the
> result of reg_read() is minus the error code, and this error code is
> always lower than 8388607 (0x7f). Then, reg_read() << 8 will always
> be negative.
The issue I'm pointing out is not whether it will be interpreted as an
error or not, it's whether the value is a correct error code. If we
don't care about the reason why an error occurs, we might as well just
return -1.
I've added my own patch which adjusts it as per above to my tree anyway,
so I'm not that worried about this.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote: > This patch takes care of the write-only registers of the tda998x. > > The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits > cleared after reset, so, they may be fully re-written. > > The register MAT_CONTRL is set to > MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) > after reset, so, it may be fully set again to this value. I said in v3 of this patch, which seems to remain unaddressed: > /* must be last register set: */ > - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); > + reg_write(priv, REG_TBG_CNTRL_0, 0); Register changes which have a potential effect shouldn't be part of a patch which is really only trying to avoid reading from write only registers. This could be a potential functional change - and it's probably one which Rob Clark should at least be made aware of. As I commented last time, when you're changing register values in an otherwise innocuous patch, you should comment about them in the patch description. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
> This patch adds more error checking inn I2C I/O functions.
> In case of I/O error, this permits to avoid writing in bad controller
> pages, a bad chipset detection or looping when getting the EDID.
I've just looked at this again, and spotted something:
> -static uint8_t
> +static int
> reg_read(struct tda998x_priv *priv, uint16_t reg)
> {
> uint8_t val = 0;
> - reg_read_range(priv, reg, , sizeof(val));
> + int ret;
> +
> + ret = reg_read_range(priv, reg, , sizeof(val));
> + if (ret < 0)
> + return ret;
So yes, this can return negative numbers.
> @@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client,
> tda998x_reset(priv);
>
> /* read version: */
> - priv->rev = reg_read(priv, REG_VERSION_LSB) |
> - reg_read(priv, REG_VERSION_MSB) << 8;
> + ret = reg_read(priv, REG_VERSION_LSB) |
> + (reg_read(priv, REG_VERSION_MSB) << 8);
> + if (ret < 0)
> + goto fail;
> + priv->rev = ret;
Two issues here:
1. The additional parens are /really/ not required.
2. What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
If we're going to the extent of attempting to make the read/write
functions return errors, we should at least handle errors generated
by them properly, otherwise it's pointless making them return errors.
ret = reg_read(priv, REG_VERSION_LSB);
if (ret < 0)
goto fail;
priv->rev = ret;
ret = reg_read(priv, REG_VERSION_MSB);
if (ret < 0)
goto fail;
priv->rev |= ret << 8;
If you want it to look slightly nicer:
int rev_lo, rev_hi;
rev_lo = reg_read(priv, REG_VERSION_LSB);
rev_hi = reg_read(priv, REG_VERSION_MSB);
if (rev_lo < 0 || rev_hi < 0) {
ret = rev_lo < 0 ? rev_lo : rev_hi;
goto fail;
}
priv->rev = rev_lo | rev_hi << 8;
I'm happy to commit such a change after this patch to clean it up, or if
you want to regenerate your patch 2 and post /just/ that incorporating
this change.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
[RFC 00/16] drm/nouveau: initial support for GK20A (Tegra K1)
Hi Alexandre, On Fri, Jan 31, 2014 at 10:16 PM, Alexandre Courbot wrote: > I guess my email address might surprise some of you, so let me anticipate some > questions you might have. :P Yes, this work is endorsed by NVIDIA. Several > other > NVIDIAns (CC'd), including core GPU experts, have provided significant > technical > guidance and will continue their involvement. Special thanks go to Terje > Bergstrom and Ken Adams for their invaluable GPU expertise, and Thierry Reding > (at FOSDEM this weekend) for help with debugging and user-space testing. > > Let me also stress that although very exciting, this effort is still > experimental, so I would like to make sure that nobody makes excessive > expectations based on these few patches. The scope of this work is strictly > limited to Tegra (although given the similarities desktop GPU support will > certainly benefit from it indirectly), and we do not have any plan to work on > user-space support. So do not uninstall that proprietary driver just yet. ;) > > With this being clarified, we are looking forward to getting your feedback and > working with you guys to bring and improve Tegra K1 support into Nouveau! :) I've sent a couple of fairly trivial comments, as you saw, and I suspect that others with a better understanding of the guts will have more substantial architectural feedback, esp after the weekend/FOSDEM. However, since no one's said it already -- welcome to Nouveau! >From the looks of it, you could bring up a full open-source stack with your patches (i.e. Xorg + nouveau DDX + mesa) and use PRIME to render stuff (assuming the actual display hw has an X ddx). Although I suspect that you're going to want to use your own drivers. Still a little curious if you've tried the open-source stack and whether it worked. [Not sure what the status is of render-node support is in mesa, but perhaps it's enough to try running piglit tests, if you can't get X going with the display HW.] -ilia
[Bug 68571] GPU lockup on AMD Radeon HD6850 with DPM=1
https://bugzilla.kernel.org/show_bug.cgi?id=68571 --- Comment #25 from kilobug at kilobug.org --- There is no overclocking on my card, reference HD6850 is 775Mhz (accoring to http://www.amd.com/us/products/desktop/graphics/amd-radeon-hd-6000/hd-6850/Pages/amd-radeon-hd-6850-overview.aspx#2) and the Sapphire has the same frequency, according to both Sapphire website (http://www.sapphiretech.com/presentation/product/?pid=497=1) and the radeon-profile utility. -- You are receiving this mail because: You are watching the assignee of the bug.
[PATCH v5 00/23]
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > This patch set contains various extensions to the tda998x driver: > > - simplify the i2c read/write > - code cleanup and fix some small errors > - use global constants > - don't read write-only registers > - add DT support > - use IRQ for connection status and EDID read I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate. I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.) -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
[Bug 74347] openra screen corruption on a HD2600
https://bugs.freedesktop.org/show_bug.cgi?id=74347 Marek Ol??k changed: What|Removed |Added Component|Drivers/DRI/Radeon |Drivers/Gallium/r600 --- Comment #1 from Marek Ol??k --- Try to run openra with this environment variable set: R600_DEBUG=noinvalrange -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/f86ea9ad/attachment.html>
[Bug 74347] openra screen corruption on a HD2600
https://bugs.freedesktop.org/show_bug.cgi?id=74347 Fabio Pedretti changed: What|Removed |Added Attachment #93204|text/plain |image/jpeg mime type|| -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/2d10041c/attachment.html>
[Bug 66331] WebGL water demo crashes LLVM
https://bugs.freedesktop.org/show_bug.cgi?id=66331 --- Comment #10 from Kertesz Laszlo --- I have llvm from git compiled a week or so and the rest of the stack yesterday and use a 8570D IGP and the demo works fine on Seamonkey 2.23 with or without llvm (Seamonkey uses the Firefox engine). I have layers.acceleration.force-enabled set to true. Older versions of llvm had some issues with this demo, crashed the browser or specular lighting was missing. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/33ae3392/attachment.html>
[RFC 13/16] drm/nouveau/ibus: add GK20A support
Some very trivial comments below:
On Fri, Jan 31, 2014 at 10:16 PM, Alexandre Courbot
wrote:
> Add support for initializing the priv ring of GK20A. This is done by the
> BIOS on desktop GPUs, but needs to be done by hand on Tegra.
>
> Signed-off-by: Alexandre Courbot
> ---
> drivers/gpu/drm/nouveau/Makefile | 1 +
> drivers/gpu/drm/nouveau/core/include/subdev/ibus.h | 1 +
> drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c| 108
> +
> 3 files changed, 110 insertions(+)
> create mode 100644 drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
>
> diff --git a/drivers/gpu/drm/nouveau/Makefile
> b/drivers/gpu/drm/nouveau/Makefile
> index 6c4b76d..3548fcd 100644
> --- a/drivers/gpu/drm/nouveau/Makefile
> +++ b/drivers/gpu/drm/nouveau/Makefile
> @@ -132,6 +132,7 @@ nouveau-y += core/subdev/i2c/nv94.o
> nouveau-y += core/subdev/i2c/nvd0.o
> nouveau-y += core/subdev/ibus/nvc0.o
> nouveau-y += core/subdev/ibus/nve0.o
> +nouveau-y += core/subdev/ibus/nvea.o
> nouveau-y += core/subdev/instmem/base.o
> nouveau-y += core/subdev/instmem/nv04.o
> nouveau-y += core/subdev/instmem/nv40.o
> diff --git a/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
> b/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
> index 88814f1..056a42f 100644
> --- a/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
> +++ b/drivers/gpu/drm/nouveau/core/include/subdev/ibus.h
> @@ -30,5 +30,6 @@ nouveau_ibus(void *obj)
>
> extern struct nouveau_oclass nvc0_ibus_oclass;
> extern struct nouveau_oclass nve0_ibus_oclass;
> +extern struct nouveau_oclass nvea_ibus_oclass;
>
> #endif
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
> b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
> new file mode 100644
> index 000..0bcd281
> --- /dev/null
> +++ b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (c) 2014, NVIDIA Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#include
> +
> +struct nvea_ibus_priv {
> + struct nouveau_ibus base;
> +};
> +
> +static void
> +nvea_ibus_init_priv_ring(struct nvea_ibus_priv *priv)
> +{
> + u32 data;
> +
> + data = nv_rd32(priv, 0x137250);
> + data &= (~0x3f);
> + nv_wr32(priv, 0x137250, data);
nv_mask(priv, 0x137250, 0x3f, 0) should do this, right?
> +
> + nv_mask(priv, 0x000200, 0x20, 0);
> + udelay(20);
> + nv_mask(priv, 0x000200, 0x20, 0x20);
> +
> + nv_wr32(priv, 0x12004c, 0x4);
> + nv_wr32(priv, 0x122204, 0x2);
> + nv_rd32(priv, 0x122204);
> +}
> +
> +static void
> +nvea_ibus_intr(struct nouveau_subdev *subdev)
> +{
> + struct nvea_ibus_priv *priv = (void *)subdev;
> + u32 status0 = nv_rd32(priv, 0x120058);
> + s32 retry = 100;
> + u32 command;
> +
> + if (status0 & 0x7) {
> + nv_debug(priv, "resetting priv ring\n");
> + nvea_ibus_init_priv_ring(priv);
> + }
> +
> + /* Acknowledge interrupt */
> + command = nv_rd32(priv, 0x0012004c);
> + command |= 0x2;
> + nv_wr32(priv, 0x0012004c, command);
nv_mask(priv, 0x12004c, 0x2, 0x2)
> +
> + while (--retry >= 0) {
> + command = nv_rd32(priv, 0x12004c) & 0x3f;
> + if (command == 0)
> + break;
> + }
> +
> + if (retry < 0)
> + nv_debug(priv, "timeout waiting for ringmaster ack\n");
this sounds kinda bad, no? perhaps a nv_warn?
> +}
> +
> +static int
> +nvea_ibus_init(struct nouveau_object *object)
> +{
> + struct nvea_ibus_priv *priv = (void *)object;
> + int ret;
> +
> + ret = _nouveau_ibus_init(object);
> + if (ret)
> + return ret;
> +
> + nvea_ibus_init_priv_ring(priv);
> +
> + return 0;
> +}
> +
> +static int
> +nvea_ibus_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
> + struct nouveau_oclass *oclass, void *data, u32 size,
> + struct nouveau_object **pobject)
> +{
> + struct nvea_ibus_priv *priv;
> + int ret;
> +
> + ret = nouveau_ibus_create(parent, engine, oclass, );
> + *pobject = nv_object(priv);
> + if (ret)
> + return ret;
> +
> + nv_subdev(priv)->intr = nvea_ibus_intr;
> + return 0;
> +}
> +
> +struct nouveau_oclass
> +nvea_ibus_oclass = {
> + .handle = NV_SUBDEV(IBUS, 0xea),
> + .ofuncs = &(struct nouveau_ofuncs) {
> + .ctor = nvea_ibus_ctor,
> +
[Bug 74347] New: openra screen corruption on a HD2600
https://bugs.freedesktop.org/show_bug.cgi?id=74347 Priority: medium Bug ID: 74347 Assignee: dri-devel at lists.freedesktop.org Summary: openra screen corruption on a HD2600 Severity: normal Classification: Unclassified OS: Linux (All) Reporter: higuita at gmx.net Hardware: Other Status: NEW Version: git Component: Drivers/DRI/Radeon Product: Mesa Created attachment 93204 --> https://bugs.freedesktop.org/attachment.cgi?id=93204=edit openra corruption i'm trying openra on a ATI HD2600 AGP, with mesa-git, libdrm 2.4.52, ati 7.3.0 and Xserver 1.14.3 on a slackware64 and i'm getting the attach corruption when trying to use Gl or Cg video renderer. openra devs point to some graphic card driver problem. the same game binary works fine on a with a ATI 4650 mobile on a ubuntu64 13.10, using the open drivers I was using mesa 9.1.7 before upgrading to mesa-git and the corruption was also there. I don't see any corruption on other games. What i can do next? -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140202/c8527e88/attachment.html>
[RFC 14/16] drm/nouveau/fb: add GK20A support
Am Samstag, den 01.02.2014, 18:28 -0500 schrieb Ilia Mirkin: > On Sat, Feb 1, 2014 at 8:40 AM, Lucas Stach wrote: > > Am Samstag, den 01.02.2014, 12:16 +0900 schrieb Alexandre Courbot: > >> Add a clumsy-but-working FB support for GK20A. This chip only uses system > >> memory, so we allocate a big chunk using CMA and let the existing memory > >> managers work on it. > >> > >> A better future design would be to allocate objects directly from system > >> memory without having to suffer from the limitations of a large, > >> contiguous pool. > >> > > I don't know if Tegra124 is similar to 114 in this regard [hint: get the > > TRM out :)], but if you go for a dedicated VRAM allocator, wouldn't it > > make sense to take a chunk of the MMIO overlaid memory for this when > > possible, rather than carving this out of CPU accessible mem? > > This is probably a stupid question... what do you need VRAM for > anyways? In _theory_ it's an abstraction to talk about memory that's > not accessible by the CPU. This is obviously not the case here, and > presumably the GPU can access all the memory in the system, so it can > be all treated as "GART" memory... AFAIK all accesses are behind the > in-GPU MMU, so contiguous physical memory isn't an issue either. In > practice, I suspect nouveau automatically sticks certain things into > vram (gpuobj's), but it should be feasible to make them optionally use > GART memory when VRAM is not available. I haven't really looked at the > details though, perhaps that's a major undertaking. > > -ilia > If it's similar to the Tegar114 there actually is memory that isn't accessible from the CPU. About 2GB of the address space is overlaid with MMIO for the devices, so in a 4GB system you potentially have 2GB of RAM that's only visible for the devices. But yes in general nouveau should just fall back to a GART placement if VRAM isn't available. > > > >> Signed-off-by: Alexandre Courbot > >> --- > >> drivers/gpu/drm/nouveau/Makefile | 2 + > >> drivers/gpu/drm/nouveau/core/include/subdev/fb.h | 1 + > >> drivers/gpu/drm/nouveau/core/subdev/fb/nvea.c| 28 ++ > >> drivers/gpu/drm/nouveau/core/subdev/fb/priv.h| 1 + > >> drivers/gpu/drm/nouveau/core/subdev/fb/ramnvea.c | 67 > >> > >> 5 files changed, 99 insertions(+) > >> create mode 100644 drivers/gpu/drm/nouveau/core/subdev/fb/nvea.c > >> create mode 100644 drivers/gpu/drm/nouveau/core/subdev/fb/ramnvea.c > >> > >> diff --git a/drivers/gpu/drm/nouveau/Makefile > >> b/drivers/gpu/drm/nouveau/Makefile > >> index 3548fcd..d9fe3e6 100644 > >> --- a/drivers/gpu/drm/nouveau/Makefile > >> +++ b/drivers/gpu/drm/nouveau/Makefile > >> @@ -100,6 +100,7 @@ nouveau-y += core/subdev/fb/nvaa.o > >> nouveau-y += core/subdev/fb/nvaf.o > >> nouveau-y += core/subdev/fb/nvc0.o > >> nouveau-y += core/subdev/fb/nve0.o > >> +nouveau-y += core/subdev/fb/nvea.o > >> nouveau-y += core/subdev/fb/ramnv04.o > >> nouveau-y += core/subdev/fb/ramnv10.o > >> nouveau-y += core/subdev/fb/ramnv1a.o > >> @@ -114,6 +115,7 @@ nouveau-y += core/subdev/fb/ramnva3.o > >> nouveau-y += core/subdev/fb/ramnvaa.o > >> nouveau-y += core/subdev/fb/ramnvc0.o > >> nouveau-y += core/subdev/fb/ramnve0.o > >> +nouveau-y += core/subdev/fb/ramnvea.o > >> nouveau-y += core/subdev/fb/sddr3.o > >> nouveau-y += core/subdev/fb/gddr5.o > >> nouveau-y += core/subdev/gpio/base.o > >> diff --git a/drivers/gpu/drm/nouveau/core/include/subdev/fb.h > >> b/drivers/gpu/drm/nouveau/core/include/subdev/fb.h > >> index d7ecafb..3905816 100644 > >> --- a/drivers/gpu/drm/nouveau/core/include/subdev/fb.h > >> +++ b/drivers/gpu/drm/nouveau/core/include/subdev/fb.h > >> @@ -105,6 +105,7 @@ extern struct nouveau_oclass *nvaa_fb_oclass; > >> extern struct nouveau_oclass *nvaf_fb_oclass; > >> extern struct nouveau_oclass *nvc0_fb_oclass; > >> extern struct nouveau_oclass *nve0_fb_oclass; > >> +extern struct nouveau_oclass *nvea_fb_oclass; > >> > >> #include > >> > >> diff --git a/drivers/gpu/drm/nouveau/core/subdev/fb/nvea.c > >> b/drivers/gpu/drm/nouveau/core/subdev/fb/nvea.c > >> new file mode 100644 > >> index 000..5ff6029 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/nouveau/core/subdev/fb/nvea.c > >> @@ -0,0 +1,28 @@ > >> +/* > >> + * Copyright (c) 2014, NVIDIA Corporation. All rights reserved. > >> + * > >> + * This program is free software; you can redistribute it and/or modify it > >> + * under the terms and conditions of the GNU General Public License, > >> + * version 2, as published by the Free Software Foundation. > >> + * > >> + * This program is distributed in the hope it will be useful, but WITHOUT > >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > >> for > >> + * more details. > >> + * > >> + */ > >> + > >> +#include "nvc0.h" > >> + > >> +struct nouveau_oclass * > >> +nvea_fb_oclass = &(struct
