Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
> Add host1x, the driver for host1x and its client unit 2D.

Maybe this could be a bit more verbose. Perhaps describe what host1x is.

> diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
[...]
> @@ -0,0 +1,6 @@
> +config TEGRA_HOST1X
> + tristate "Tegra host1x driver"
> + help
> +   Driver for the Tegra host1x hardware.

Maybe s/Tegra/NVIDIA Tegra/?

> +
> +   Required for enabling tegradrm.

This should probably be dropped. Either encode such knowledge as
explicit dependencies or in this case just remove it altogether since we
will probably merge both drivers anyway.

> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
[...]
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "dev.h"

Maybe add a blank line between the previous two lines to visually
separate standard Linux includes from driver-specific ones.

> +#include "hw/host1x01.h"
> +
> +#define CREATE_TRACE_POINTS
> +#include 
> +
> +#define DRIVER_NAME  "tegra-host1x"

You only ever use this once, so maybe it can just be dropped?

> +static struct host1x_device_info host1x_info = {

Perhaps this should be host1x01_info in order to match the hardware
revision? That'll avoid it having to be renamed later on when other
revisions start to appear.

> +static int host1x_probe(struct platform_device *dev)
> +{
[...]
> + syncpt_irq = platform_get_irq(dev, 0);
> + if (IS_ERR_VALUE(syncpt_irq)) {

This is somewhat unusual. It should be fine to just do:

if (syncpt_irq < 0)

but IS_ERR_VALUE() should work fine too.

> + memcpy(&host->info, devid->data, sizeof(struct host1x_device_info));

Why not make the .info field a pointer to struct host1x_device_info
instead? That way you don't have to keep separate copies of the same
information.

> + /* set common host1x device data */
> + platform_set_drvdata(dev, host);
> +
> + host->regs = devm_request_and_ioremap(&dev->dev, regs);
> + if (!host->regs) {
> + dev_err(&dev->dev, "failed to remap host registers\n");
> + return -ENXIO;
> + }

This should probably be rewritten as:

host->regs = devm_ioremap_resource(&dev->dev, regs);
if (IS_ERR(host->regs))
return PTR_ERR(host->regs);

Though that function will only be available in 3.9-rc1.

> + err = host1x_syncpt_init(host);
> + if (err)
> + return err;
[...]
> + host1x_syncpt_reset(host);

Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
it might be useful to have host1x_syncpt_reset() as a separate function
but couldn't it be called as part of host1x_syncpt_init()?

> + dev_info(&dev->dev, "initialized\n");

I don't think this is very useful. We should make sure to tell people
when things fail. When everything goes as planned we don't need to brag
about it =)

> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
> +struct host1x_syncpt_ops {
[...]
> + const char * (*name)(struct host1x_syncpt *);

Why do we need this? Could we not refer to the syncpt name directly
instead of going through this wrapper? I'd expect the name to be static.

> +struct host1x_device_info {

Maybe this should be called simply host1x_info? _device seems redundant.

> + int nb_channels;/* host1x: num channels supported */
> + int nb_pts; /* host1x: num syncpoints supported */
> + int nb_bases;   /* host1x: num syncpoints supported */
> + int nb_mlocks;  /* host1x: number of mlocks */
> + int (*init)(struct host1x *); /* initialize per SoC ops */
> + int sync_offset;
> +};

While this isn't public API, maybe it would still be useful to turn the
comments into proper kerneldoc? That's what people are used to.

> +struct host1x {
> + void __iomem *regs;
> + struct host1x_syncpt *syncpt;
> + struct platform_device *dev;
> + struct host1x_device_info info;
> + struct clk *clk;
> +
> + struct host1x_syncpt_ops syncpt_op;

Maybe make this a struct host1x_syncpt_ops * instead so you don't have
separate copies? While at it, maybe this should be const as well.

> + struct dentry *debugfs;

This doesn't seem to be used anywhere.

> +static inline
> +struct host1x *host1x_get_host(struct platform_device *_dev)
> +{
> + struct platform_device *pdev;
> +
> + if (_dev->dev.parent) {
> + pdev = to_platform_device(_dev->dev.parent);
> + return platform_get_drvdata(pdev);
> + } else
> + return platform_get_drvdata(_dev);
> +}

There is a lot of needless casting in here. Why not pass in a struct
device * and use dev_get_drvdata() instead?

> diff --git a/drivers/gpu/host1x/hw/host1x01.c 
> b/drivers/gpu/host1x/hw/host1x01.c
[...]
> +#include "hw/host1x01.h"
> +#include "dev.h"
> +#include "hw/host1x0

[Bug 59899] Diagonal rendering artifacts on xbmc

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=59899

--- Comment #6 from aeriks...@fastmail.fm ---
Tried mesa git as of yesterday. No improvement.
I've also tried to flip all USE flags on/off for mesa 9.0.1 with no
improvement.

Doing some surfing, I noticed the z buffer issues being a common problem, so I
decided to try the car racing game "trigger". It does show the same effect as
I've seen in screenshots on youtube (flickering triangular misrenderings on
objects). The dynamic behavior seems to be the same. However, I see the same
effect on the fist screen in Trigger, where the three helmets are shown. I'm
not sure if they are supposed to be animated or static. The zbuffer issues I
saw in other reports were all related to movements (Stuff move back/front, when
I move a little). I also have zbuffer issues on the terrain once the game
starts.

If xbmc makes use of depth to paint its screen with a bunch of objects, I can
see how this might be z related. However, xbmc should show a static screen (I
think), whereas common z reports are all related to movement of the observer (I
think).

Any ideas how one could test this theory?

Another crazy thing is that every once in a while it renders perfectly ok.
While "bisecting" through the USEflags, I had a run of "good, good, good,..."
and each test was a restart of Xserver, starting xbmc. After going back to the
known bad set of USE flags (which now rendered good!!), I did some cursing and
warm rebooted the box, and it suddenly rendered bad again :-(. This kind of
suggests an uninitialized HW register to me...

Any ideas how one can make xbmc not exercise the (gl?) related areas of the HW?
I'm lacking a solid known-good state at the moment.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Linaro-mm-sig] [PATCH 6/7] reservation: cross-device reservation support

[Maarten Lankhorst]

Op 04-02-13 08:06, Inki Dae schreef:
>> +/**
>> + * ticket_commit - commit a reservation with a new fence
>> + * @ticket:[in]the reservation_ticket returned by
>> + * ticket_reserve
>> + * @entries:   [in]a linked list of struct reservation_entry
>> + * @fence: [in]the fence that indicates completion
>> + *
>> + * This function will call reservation_ticket_fini, no need
>> + * to do it manually.
>> + *
>> + * This function should be called after a hardware command submission is
>> + * completed succesfully. The fence is used to indicate completion of
>> + * those commands.
>> + */
>> +void
>> +ticket_commit(struct reservation_ticket *ticket,
>> + struct list_head *entries, struct fence *fence)
>> +{
>> +   struct list_head *cur;
>> +
>> +   if (list_empty(entries))
>> +   return;
>> +
>> +   if (WARN_ON(!fence)) {
>> +   ticket_backoff(ticket, entries);
>> +   return;
>> +   }
>> +
>> +   list_for_each(cur, entries) {
>> +   struct reservation_object *bo;
>> +   bool shared;
>> +
>> +   reservation_entry_get(cur, &bo, &shared);
>> +
>> +   if (!shared) {
>> +   int i;
>> +   for (i = 0; i < bo->fence_shared_count; ++i) {
>> +   fence_put(bo->fence_shared[i]);
>> +   bo->fence_shared[i] = NULL;
>> +   }
>> +   bo->fence_shared_count = 0;
>> +   if (bo->fence_excl)
>> +   fence_put(bo->fence_excl);
>> +
>> +   bo->fence_excl = fence;
>> +   } else {
>> +   if (WARN_ON(bo->fence_shared_count >=
>> +   ARRAY_SIZE(bo->fence_shared))) {
>> +   mutex_unreserve_unlock(&bo->lock);
>> +   continue;
>> +   }
>> +
>> +   bo->fence_shared[bo->fence_shared_count++] = fence;
>> +   }
> Hi,
>
> I got some questions to fence_excl and fence_shared. At the above
> code, if bo->fence_excl is not NULL then it puts bo->fence_excl and
> sets a new fence to it. This seems like that someone that committed a
> new fence, wants to access the given dmabuf exclusively even if
> someone is accessing the given dmabuf.
Yes, if there were shared fences, they had to issue a wait for the previous 
exclusive fence, so if you add
(possibly hardware) wait ops on those shared fences to your command stream, it 
follows that you also
waited for the previous exclusive fence implicitly.

If there is only an exclusive fence, you have to issue some explicit wait op  
on it before you start the rest
of the commands.
> On the other hand, in case of fence_shared, someone wants to access
> that dmabuf non-exclusively. So this case seems like that the given
> dmabuf could be accessed by two more devices. So I guess that the
> fence_excl could be used for write access(may need buffer sync like
> blocking) and read access for the fence_shared(may not need buffer
> sync). I'm not sure that I understand these two things correctly so
> could you please give me more comments for them?
Correct, if you create a shared fence, you still have to emit a wait op for the 
exclusive fence.
> Thanks,
> Inki Dae
>
>> +   fence_get(fence);
>> +
>> +   mutex_unreserve_unlock(&bo->lock);
>> +   }
>> +   reservation_ticket_fini(ticket);
>> +}
>> +EXPORT_SYMBOL(ticket_commit);

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

Re: [RFC v2 0/5] Common Display Framework

[Marcus Lorentzon]

On 02/02/2013 12:35 AM, Laurent Pinchart wrote:

Hi Marcus,

On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:

On 01/08/2013 05:36 PM, Tomasz Figa wrote:

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

[...]

But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
   enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
   u8 cmd;
   int dataLen;
   u8 *data;
}

struct dsi_ops {
   int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
   ...
}

Do you have DSI IP(s) that can handle a list of commands ? Or would all DSI
transmitter drivers need to iterate over the commands manually ? In the later
case a lower-level API might be easier to implement in DSI transmitter
drivers. Helper functions could provide the higher-level API you proposed.


The HW has a FIFO, so it can handle a few. Currently we use the low 
level type of call with one call per command. But we have found DSI 
command mode panels that don't accept any commands during the "update" 
(write start+continues). And so we must use a mutex/state machine to 
exclude any async calls to send DSI commands during update. But if you 
need to send more than one command per frame this will be hard (like 
CABC and backlight commands). It will be a ping pong between update and 
command calls. One option is to expose the mutex to the caller so it can 
make many calls before the next update grabs the mutex again.
So maybe we could create a helper that handle the op for list of 
commands and another op for single command that you actually have to 
implement.

Yes, this should be flexible enough to cover most of (or even whole) DSI
specification.

However I'm not sure whether the dsi_write name would be appropriate,
since DSI packet types include also read and special transactions. So,
according to DSI terminology, maybe dsi_transaction would be better?

Or dsi_transfer or dsi_xfer ? Does the DSI bus have a concept of transactions
?


No transactions. And I don't want to mix reads and writes. It should be 
similar to I2C and other stream control busses. So one read and one 
write should be fine. And then a bunch of helpers on top for callers to 
use, like one per major DSI packet type.

I think read should still be separate. At least on my HW read and write
are quite different. But all "transactions" are very much the same in HW
setup. The ... was dsi_write etc ;) Like set_max_packet_size should
maybe be an ops. Since only the implementer of the "video source" will
know what the max read return packet size for that DSI IP is. The panels
don't know that. Maybe another ops to retrieve some info about the caps
of the video source would help that. Then a helper could call that and
then the dsi_write one.

If panels (or helper functions) need information about the DSI transmitter
capabilities, sure, we can add an op.


Yes, a "video source" op for getting caps would be ok too. But so far 
the only limits I have found is the read/write sizes. But if anyone else 
has other limits, please list them so we could add them to this struct 
dsi_host_caps.

And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached
to the panel stuff with the find_video_source call.

The Linux driver framework is based on control busses. DSI usually handles
both control and video transfer, but the control and data busses can also be
separate (think DPI + SPI/I2C for instance). In that case the panel will be a
child of its control bus master, and will need a separate interface to access
video data operations. As a separate video interface is thus needed, I think
we should use it for DSI as well.

My initial proposal included a DBI bus (as I don't have any DSI hardware - DBI
and DSI can be used interchangeably in this discussions, they both share the
caracteristic of having a common control + data bus), and panels were children
of the DBI bus master. The DBI bus API was only used for control, not for data
transfers. Tomi then removed the DBI bus and moved the control operations to
the video source, turning the DBI panels into platform devices. I still favor
my initial approach, but I can agree to drop the DBI bus if there's a
consensus on that. Video bus operations will be separate in any case.


As discussed at FOSDEM I will give DSI bus with full feature set a try.

BTW. Who got the action to ask Greg about devices with multiple 
parents/buses?



Also, as discussed in previous posts, some panels might use DSI only for
video data and another interface (I2

[Bug 60224] Radeon hd6770 graphics artifacts in team fortress 2 with 3.8 kernel

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60224

--- Comment #1 from Michel Dänzer  ---
Could be related to bug 60236. If that's not it, can you bisect which kernel
change introduced the problem?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 2/8] gpu: host1x: Add syncpoint wait and interrupts

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:43:58PM +0200, Terje Bergstrom wrote:
[...]
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
[...]
> @@ -95,7 +96,6 @@ static int host1x_probe(struct platform_device *dev)
>  
>   /* set common host1x device data */
>   platform_set_drvdata(dev, host);
> -
>   host->regs = devm_request_and_ioremap(&dev->dev, regs);
>   if (!host->regs) {
>   dev_err(&dev->dev, "failed to remap host registers\n");

This seems an unrelated (and actually undesirable) change.

> @@ -109,28 +109,40 @@ static int host1x_probe(struct platform_device *dev)
>   }
>  
>   err = host1x_syncpt_init(host);
> - if (err)
> + if (err) {
> + dev_err(&dev->dev, "failed to init sync points");
>   return err;
> + }

This error message should probably have gone in the previous patch as
well.

> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> index d8f5979..8376092 100644
> --- a/drivers/gpu/host1x/dev.h
> +++ b/drivers/gpu/host1x/dev.h
> @@ -17,11 +17,12 @@
>  #ifndef HOST1X_DEV_H
>  #define HOST1X_DEV_H
>  
> +#include 
>  #include "syncpt.h"
> +#include "intr.h"
>  
>  struct host1x;
>  struct host1x_syncpt;
> -struct platform_device;

Why include platform_device.h here?

> @@ -34,6 +35,18 @@ struct host1x_syncpt_ops {
>   const char * (*name)(struct host1x_syncpt *);
>  };
>  
> +struct host1x_intr_ops {
> + void (*init_host_sync)(struct host1x_intr *);
> + void (*set_host_clocks_per_usec)(
> + struct host1x_intr *, u32 clocks);

Could the above two not be combined? The only reason to keep them
separate would be if the host1x clock was dynamically changed, but I
don't think we support that, right?

> + void (*set_syncpt_threshold)(
> + struct host1x_intr *, u32 id, u32 thresh);
> + void (*enable_syncpt_intr)(struct host1x_intr *, u32 id);
> + void (*disable_syncpt_intr)(struct host1x_intr *, u32 id);
> + void (*disable_all_syncpt_intrs)(struct host1x_intr *);

Can disable_all_syncpt_intrs() not be implemented generically using the
number of syncpoints as exposed by host1x_device_info and the
.disable_syncpt_intr() function?

> @@ -46,11 +59,13 @@ struct host1x_device_info {
>  struct host1x {
>   void __iomem *regs;
>   struct host1x_syncpt *syncpt;
> + struct host1x_intr intr;
>   struct platform_device *dev;
>   struct host1x_device_info info;
>   struct clk *clk;
>  
>   struct host1x_syncpt_ops syncpt_op;
> + struct host1x_intr_ops intr_op;

I think carrying a const pointer to the interrupt operations structure
is a better option here.

> diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
[...]
> +static void host1x_intr_syncpt_thresh_isr(struct host1x_intr_syncpt *syncpt);

Can we avoid this forward declaration?

> +static void syncpt_thresh_cascade_fn(struct work_struct *work)

syncpt_thresh_work()?

> +{
> + struct host1x_intr_syncpt *sp =
> + container_of(work, struct host1x_intr_syncpt, work);
> + host1x_syncpt_thresh_fn(sp);

Couldn't we inline the host1x_syncpt_thresh_fn() implementation here?
Why do we need to go through an external function declaration?

> +static irqreturn_t syncpt_thresh_cascade_isr(int irq, void *dev_id)
> +{
> + struct host1x *host1x = dev_id;
> + struct host1x_intr *intr = &host1x->intr;
> + unsigned long reg;
> + int i, id;
> +
> + for (i = 0; i < host1x->info.nb_pts / BITS_PER_LONG; i++) {
> + reg = host1x_sync_readl(host1x,
> + HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS +
> + i * REGISTER_STRIDE);
> + for_each_set_bit(id, ®, BITS_PER_LONG) {
> + struct host1x_intr_syncpt *sp =
> + intr->syncpt + (i * BITS_PER_LONG + id);
> + host1x_intr_syncpt_thresh_isr(sp);

Have you considered mimicking the IRQ API and name this something like
host1x_intr_syncpt_thresh_handle() and name the actual ISR just
syncpt_thresh_isr()? Not so important but it makes things a bit clearer
in my opinion.

> + queue_work(intr->wq, &sp->work);

Should the call to queue_work() perhaps be moved into
host1x_intr_syncpt_thresh_isr().

> +static void host1x_intr_init_host_sync(struct host1x_intr *intr)
> +{
> + struct host1x *host1x = intr_to_host1x(intr);
> + int i, err;
> +
> + host1x_sync_writel(host1x, 0xUL,
> + HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE);
> + host1x_sync_writel(host1x, 0xUL,
> + HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS);
> +
> + for (i = 0; i < host1x->info.nb_pts; i++)
> + INIT_WORK(&intr->syncpt[i].work, syncpt_thresh_cascade_fn);
> +
> + err = devm_request_irq(&host1x->dev->dev, intr->syncpt_irq,
> + syncpt_thresh_cascade_isr,
> + IRQF_SHAR

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #4 from Michel Dänzer  ---
The attached log ends abruptly. Is there more information in the kdm log file?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 4/8] gpu: host1x: Add debug support

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:00PM +0200, Terje Bergstrom wrote:
> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
[...]
> +static pid_t host1x_debug_null_kickoff_pid;
> +unsigned int host1x_debug_trace_cmdbuf;
> +
> +static pid_t host1x_debug_force_timeout_pid;
> +static u32 host1x_debug_force_timeout_val;
> +static u32 host1x_debug_force_timeout_channel;

Please group static and non-static variables.

> diff --git a/drivers/gpu/host1x/debug.h b/drivers/gpu/host1x/debug.h
[...]
> +struct output {
> + void (*fn)(void *ctx, const char *str, size_t len);
> + void *ctx;
> + char buf[256];
> +};

Do we really need this kind of abstraction? There really should be only
one location where debug information is obtained, so I don't see a need
for this.

> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
>  struct host1x_syncpt_ops {
>   void (*reset)(struct host1x_syncpt *);
>   void (*reset_wait_base)(struct host1x_syncpt *);
> @@ -117,6 +133,7 @@ struct host1x {
>   struct host1x_channel_ops channel_op;
>   struct host1x_cdma_ops cdma_op;
>   struct host1x_pushbuffer_ops cdma_pb_op;
> + struct host1x_debug_ops debug_op;
>   struct host1x_syncpt_ops syncpt_op;
>   struct host1x_intr_ops intr_op;

Again, better to pass in a const pointer to the ops structure.

> diff --git a/drivers/gpu/host1x/hw/debug_hw.c 
> b/drivers/gpu/host1x/hw/debug_hw.c

> +static int show_channel_command(struct output *o, u32 addr, u32 val, int 
> *count)
> +{
> + unsigned mask;
> + unsigned subop;
> +
> + switch (val >> 28) {
> + case 0x0:

These can easily be derived by looking at the debug output, but it may
still make sense to assign symbolic names to them.

> +static void show_channel_word(struct output *o, int *state, int *count,
> + u32 addr, u32 val, struct host1x_cdma *cdma)
> +{
> + static int start_count, dont_print;

What if two processes read debug information at the same time?

> +static void do_show_channel_gather(struct output *o,
> + phys_addr_t phys_addr,
> + u32 words, struct host1x_cdma *cdma,
> + phys_addr_t pin_addr, u32 *map_addr)
> +{
> + /* Map dmaget cursor to corresponding mem handle */
> + u32 offset;
> + int state, count, i;
> +
> + offset = phys_addr - pin_addr;
> + /*
> +  * Sometimes we're given different hardware address to the same
> +  * page - in these cases the offset will get an invalid number and
> +  * we just have to bail out.
> +  */

Why's that?

> + map_addr = host1x_memmgr_mmap(mem);
> + if (!map_addr) {
> + host1x_debug_output(o, "[could not mmap]\n");
> + return;
> + }
> +
> + /* Get base address from mem */
> + sgt = host1x_memmgr_pin(mem);
> + if (IS_ERR(sgt)) {
> + host1x_debug_output(o, "[couldn't pin]\n");
> + host1x_memmgr_munmap(mem, map_addr);
> + return;
> + }

Maybe you should stick with one of "could not" or "couldn't". Makes it
easier to search for.

> +static void show_channel_gathers(struct output *o, struct host1x_cdma *cdma)
> +{
> + struct host1x_job *job;
> +
> + list_for_each_entry(job, &cdma->sync_queue, list) {
> + int i;
> + host1x_debug_output(o,
> + "\n%p: JOB, syncpt_id=%d, syncpt_val=%d,"
> + " first_get=%08x, timeout=%d"
> + " num_slots=%d, num_handles=%d\n",
> + job,
> + job->syncpt_id,
> + job->syncpt_end,
> + job->first_get,
> + job->timeout,
> + job->num_slots,
> + job->num_unpins);

This could go on fewer lines.

> +static void host1x_debug_show_channel_cdma(struct host1x *m,
> + struct host1x_channel *ch, struct output *o, int chid)
> +{
[...]
> + switch (cbstat) {
> + case 0x00010008:

Again, symbolic names would be nice.

Thierry


pgpGajaAIYmJ3.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 5/8] drm: tegra: Move drm to live under host1x

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:01PM +0200, Terje Bergstrom wrote:
[...]
> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
> index 697d49a..ffc8bf1 100644
> --- a/drivers/gpu/host1x/Makefile
> +++ b/drivers/gpu/host1x/Makefile
> @@ -12,4 +12,10 @@ host1x-y = \
>   hw/host1x01.o
>  
>  host1x-$(CONFIG_TEGRA_HOST1X_CMA) += cma.o
> +
> +ccflags-y += -Iinclude/drm
> +ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
> +
> +host1x-$(CONFIG_DRM_TEGRA) += drm/drm.o drm/fb.o drm/dc.o drm/host1x.o
> +host1x-$(CONFIG_DRM_TEGRA) += drm/output.o drm/rgb.o drm/hdmi.o
>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o

Can this be moved into a separate Makefile in the drm subdirectory?

> diff --git a/drivers/gpu/host1x/host1x_client.h 
> b/drivers/gpu/host1x/host1x_client.h
[...]
> new file mode 100644
> index 000..fdd2920
> --- /dev/null
> +++ b/drivers/gpu/host1x/host1x_client.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#ifndef HOST1X_CLIENT_H
> +#define HOST1X_CLIENT_H
> +
> +struct platform_device;
> +
> +void host1x_set_drm_data(struct platform_device *pdev, void *data);
> +void *host1x_get_drm_data(struct platform_device *pdev);
> +
> +#endif

These aren't defined or used yet.

Thierry


pgpbALJiJRAV0.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60254] kernel Oops when provoking GPU lock.

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60254

--- Comment #2 from Andy Furniss  ---
(In reply to comment #1)
> (In reply to comment #0)
> > The GPU lock with Rv670 and openarena is nothing new - it seems to have been
> > a feature for almost a year (I haven't used rv670 for most of that time).
> > 
> > On noticing the new gpu reset code in drm-next-3.9-wip I decided to provoke
> > it on my AGP box and got -
> 
> Hmm I just managed to get the same running drm-fixes so it's not wip maybe
> it's because I am now using llvm. In the (no llvm) past with other kernels
> this GPU lock normally went quite well - the game often just continued for a
> while, until it hit another one.

It's nothing to do with llvm seems like it's a feature of more recent kernels.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 6/8] gpu: host1x: Remove second host1x driver

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:02PM +0200, Terje Bergstrom wrote:
[...]
> +void host1x_set_drm_data(struct platform_device *pdev, void *data)
> +{
> + struct host1x *host1x = platform_get_drvdata(pdev);
> + host1x->drm_data = data;
> +}
> +
> +void *host1x_get_drm_data(struct platform_device *pdev)
> +{
> + struct host1x *host1x = platform_get_drvdata(pdev);
> + return host1x->drm_data;
> +}

Passing around struct device * should be enough and avoids the need for
the explicit cast to struct platform_device.

It is a bit unfortunate that we have now have two structures called
host1x, but I think we can live with it for now. We can clean that up
once the code has been merged.

Thierry


pgpaq_7pjgm9S.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 7/8] ARM: tegra: Add board data and 2D clocks

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:03PM +0200, Terje Bergstrom wrote:
> Add a driver alias gr2d for Tegra 2D device, and assign a duplicate
> of 2D clock to that driver alias.
> 
> Signed-off-by: Terje Bergstrom 
> ---
>  arch/arm/mach-tegra/board-dt-tegra20.c|1 +
>  arch/arm/mach-tegra/board-dt-tegra30.c|1 +
>  arch/arm/mach-tegra/tegra20_clocks_data.c |2 +-
>  arch/arm/mach-tegra/tegra30_clocks_data.c |1 +
>  4 files changed, 4 insertions(+), 1 deletion(-)

With Prashant's clock rework patches now merged this patch can be
dropped.

Thierry


pgpfIzUU9bzOo.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60224] Radeon hd6770 graphics artifacts in team fortress 2 with 3.8 kernel

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60224

--- Comment #2 from Iaroslav  ---
I know that the problem appeared after upgrading to 3.8-rc1-drm-next, and
continues to exist in the 3.8-rc5-vanilla.
async dma is a likely cause.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 8/8] drm: tegra: Add gr2d device

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:04PM +0200, Terje Bergstrom wrote:
[...]
> diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
> @@ -270,7 +274,29 @@ static int tegra_drm_unload(struct drm_device *drm)
>  
>  static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp)
>  {
> - return 0;
> + struct host1x_drm_fpriv *fpriv;
> + int err = 0;

Can be dropped.

> +
> + fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
> + if (!fpriv)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&fpriv->contexts);
> + filp->driver_priv = fpriv;
> +
> + return err;

return 0;

> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
> +{
> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(filp);
> + struct host1x_drm_context *context, *tmp;
> +
> + list_for_each_entry_safe(context, tmp, &fpriv->contexts, list) {
> + context->client->ops->close_channel(context);
> + kfree(context);
> + }
> + kfree(fpriv);
>  }

Maybe you should add host1x_drm_context_free() to wrap the loop
contents?

> @@ -280,7 +306,204 @@ static void tegra_drm_lastclose(struct drm_device *drm)
>   drm_fbdev_cma_restore_mode(host1x->fbdev);
>  }
>  
> +static int
> +tegra_drm_ioctl_syncpt_read(struct drm_device *drm, void *data,
> +  struct drm_file *file_priv)

static int and function name on one line, please.

> +{
> + struct host1x *host1x = drm->dev_private;
> + struct tegra_drm_syncpt_read_args *args = data;
> + struct host1x_syncpt *sp =
> + host1x_syncpt_get_bydev(host1x->dev, args->id);

I don't know if we need this, except maybe to work around the problem
that we have two different structures named host1x. The _bydev() suffix
is misleading because all you really do here is obtain the syncpt from
the host1x.

> +static int
> +tegra_drm_ioctl_open_channel(struct drm_device *drm, void *data,
> +  struct drm_file *file_priv)
> +{
> + struct tegra_drm_open_channel_args *args = data;
> + struct host1x_client *client;
> + struct host1x_drm_context *context;
> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
> + struct host1x *host1x = drm->dev_private;
> + int err = 0;

err = -ENODEV; (see below)

> +
> + context = kzalloc(sizeof(*context), GFP_KERNEL);
> + if (!context)
> + return -ENOMEM;
> +
> + list_for_each_entry(client, &host1x->clients, list) {
> + if (client->class == args->class) {
> + context->client = client;
> + err = client->ops->open_channel(client, context);
> + if (err)
> + goto out;
> +
> + list_add(&context->list, &fpriv->contexts);
> + args->context = (uintptr_t)context;

Perhaps cast this to __u64 directly instead? There's little sense in
taking the detour via uintptr_t.

> + goto out;

return 0;

> + }
> + }
> + err = -ENODEV;
> +
> +out:
> + if (err)
> + kfree(context);
> +
> + return err;
> +}

Then this simply becomes:

kfree(context);
return err;

> +static int
> +tegra_drm_ioctl_close_channel(struct drm_device *drm, void *data,
> +  struct drm_file *file_priv)
> +{
> + struct tegra_drm_open_channel_args *args = data;
> + struct host1x_drm_context *context, *tmp;
> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
> + int err = 0;
> +
> + list_for_each_entry_safe(context, tmp, &fpriv->contexts, list) {
> + if ((uintptr_t)context == args->context) {
> + context->client->ops->close_channel(context);
> + list_del(&context->list);
> + kfree(context);
> + goto out;
> + }
> + }
> + err = -EINVAL;
> +
> +out:
> + return err;
> +}

Same comments as for tegra_drm_ioctl_open_channel().

> +static int
> +tegra_drm_ioctl_get_syncpoint(struct drm_device *drm, void *data,
> +  struct drm_file *file_priv)
> +{
> + struct tegra_drm_get_channel_param_args *args = data;
> + struct host1x_drm_context *context;
> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
> + int err = 0;
> +
> + list_for_each_entry(context, &fpriv->contexts, list) {
> + if ((uintptr_t)context == args->context) {
> + args->value =
> + context->client->ops->get_syncpoint(context,
> + args->param);
> + goto out;
> + }
> + }
> + err = -ENODEV;
> +
> +out:
> + return err;
> +}

Same comments as well. Also you may want to factor out the context
lookup into a separate function so you don't have to repeat the same
code over and over again.

I wonder 

Re: [Linaro-mm-sig] [PATCH 6/7] reservation: cross-device reservation support

[Daniel Vetter]

On Mon, Feb 04, 2013 at 10:57:22AM +0100, Maarten Lankhorst wrote:
> Op 04-02-13 08:06, Inki Dae schreef:
> >> +/**
> >> + * ticket_commit - commit a reservation with a new fence
> >> + * @ticket:[in]the reservation_ticket returned by
> >> + * ticket_reserve
> >> + * @entries:   [in]a linked list of struct reservation_entry
> >> + * @fence: [in]the fence that indicates completion
> >> + *
> >> + * This function will call reservation_ticket_fini, no need
> >> + * to do it manually.
> >> + *
> >> + * This function should be called after a hardware command submission is
> >> + * completed succesfully. The fence is used to indicate completion of
> >> + * those commands.
> >> + */
> >> +void
> >> +ticket_commit(struct reservation_ticket *ticket,
> >> + struct list_head *entries, struct fence *fence)
> >> +{
> >> +   struct list_head *cur;
> >> +
> >> +   if (list_empty(entries))
> >> +   return;
> >> +
> >> +   if (WARN_ON(!fence)) {
> >> +   ticket_backoff(ticket, entries);
> >> +   return;
> >> +   }
> >> +
> >> +   list_for_each(cur, entries) {
> >> +   struct reservation_object *bo;
> >> +   bool shared;
> >> +
> >> +   reservation_entry_get(cur, &bo, &shared);
> >> +
> >> +   if (!shared) {
> >> +   int i;
> >> +   for (i = 0; i < bo->fence_shared_count; ++i) {
> >> +   fence_put(bo->fence_shared[i]);
> >> +   bo->fence_shared[i] = NULL;
> >> +   }
> >> +   bo->fence_shared_count = 0;
> >> +   if (bo->fence_excl)
> >> +   fence_put(bo->fence_excl);
> >> +
> >> +   bo->fence_excl = fence;
> >> +   } else {
> >> +   if (WARN_ON(bo->fence_shared_count >=
> >> +   ARRAY_SIZE(bo->fence_shared))) {
> >> +   mutex_unreserve_unlock(&bo->lock);
> >> +   continue;
> >> +   }
> >> +
> >> +   bo->fence_shared[bo->fence_shared_count++] = fence;
> >> +   }
> > Hi,
> >
> > I got some questions to fence_excl and fence_shared. At the above
> > code, if bo->fence_excl is not NULL then it puts bo->fence_excl and
> > sets a new fence to it. This seems like that someone that committed a
> > new fence, wants to access the given dmabuf exclusively even if
> > someone is accessing the given dmabuf.
> Yes, if there were shared fences, they had to issue a wait for the previous 
> exclusive fence, so if you add
> (possibly hardware) wait ops on those shared fences to your command stream, 
> it follows that you also
> waited for the previous exclusive fence implicitly.
> 
> If there is only an exclusive fence, you have to issue some explicit wait op  
> on it before you start the rest
> of the commands.
> > On the other hand, in case of fence_shared, someone wants to access
> > that dmabuf non-exclusively. So this case seems like that the given
> > dmabuf could be accessed by two more devices. So I guess that the
> > fence_excl could be used for write access(may need buffer sync like
> > blocking) and read access for the fence_shared(may not need buffer
> > sync). I'm not sure that I understand these two things correctly so
> > could you please give me more comments for them?
> Correct, if you create a shared fence, you still have to emit a wait op for 
> the exclusive fence.

Just a quick note: We limit the number of non-exclusive fences to avoid
reallocating memory, which would be a bit a pain. Otoh if we support some
form of fence timeline concept, the fence core code could overwrite
redundant fences. Which would reasonable limit the total attached fence
count. Still we'd need to thread potential -ENOMEM failures through all
callers.

Do you see a use-case for more than just a few non-exclusive fences?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/62] drm/i915: convert to idr_alloc()

[Daniel Vetter]

On Sat, Feb 02, 2013 at 05:20:18PM -0800, Tejun Heo wrote:
> Convert to the much saner new idr interface.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: dri-devel@lists.freedesktop.org
> ---
> This patch depends on an earlier idr changes and I think it would be
> best to route these together through -mm.  Please holler if there's
> any objection.  Thanks.

Indeed, this looks nice.

Acked-by: Daniel Vetter 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Use C8 instead of RGB332 when determining the format from depth/bpp

[Daniel Vetter]

On Thu, Jan 31, 2013 at 07:43:38PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Support for real RGB332 is a rarity, most hardware only really support
> C8. So use C8 instead of RGB332 when determining the format based on
> depth/bpp.
> 
> This fixes 8bpp fbcon on i915, since i915 will only accept C8 and not
> RGB332.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59572
> Signed-off-by: Ville Syrjälä 

Tested-by: mlsemo...@gmail.com

Dave, can you please consider including these two patches into -fixes? The
fix a black screen regression when users opt for 8bpp console ...
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ff7344c..826a5ca 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2253,7 +2253,7 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, 
> uint32_t depth)
>  
>   switch (bpp) {
>   case 8:
> - fmt = DRM_FORMAT_RGB332;
> + fmt = DRM_FORMAT_C8;
>   break;
>   case 16:
>   if (depth == 15)
> -- 
> 1.7.12.4
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined reference to `acpi_video_get_edid'

[Borislav Petkov]

Hi,

I'm guessing someone has already triggered this on latest Linus' tree
and has a fix?

drivers/built-in.o: In function `nouveau_acpi_edid':
/w/kernel/linux/drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined reference 
to `acpi_video_get_edid'
make: *** [vmlinux] Error 1

Btw, I got CONFIG_ACPI_VIDEO=m while CONFIG_DRM_NOUVEAU=y and this is
probably the reason for the vmlinux link error.

Thanks.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/nouveau: always select ACPI_VIDEO if ACPI is enabled.

[Maarten Lankhorst]

Hey,

Op 04-02-13 16:23, Borislav Petkov schreef:
> Hi,
>
> I'm guessing someone has already triggered this on latest Linus' tree
> and has a fix?
>
> drivers/built-in.o: In function `nouveau_acpi_edid':
> /w/kernel/linux/drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined 
> reference to `acpi_video_get_edid'
> make: *** [vmlinux] Error 1
>
> Btw, I got CONFIG_ACPI_VIDEO=m while CONFIG_DRM_NOUVEAU=y and this is
> probably the reason for the vmlinux link error.
>
> Thanks.
>
Does this fix things?

-->8
Having nouveau builtin would still allow ACPI_VIDEO to be used as external 
module if some of the deps for acpi_video
have not been met, which would result in a linking failure. Solve this by only 
requiring ACPI && X86 to select ACPI_VIDEO.

Signed-off-by: Maarten Lankhorst 

---
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 8a55bee..f08b9b6 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -10,7 +10,7 @@ config DRM_NOUVEAU
select FB
select FRAMEBUFFER_CONSOLE if !EXPERT
select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
-   select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && 
VIDEO_OUTPUT_CONTROL && INPUT
+   select ACPI_VIDEO if ACPI && X86
select ACPI_WMI if ACPI
select MXM_WMI if ACPI
select POWER_SUPPLY

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

Re: DMAR faults from unrelated device when vfio is used

[Alex Williamson]

On Mon, 2013-02-04 at 11:10 +0100, David Gstir wrote:
> Hi!
> 
> I get the following error messages over and over again when using vfio
> in qemu-kvm:
> 
> [ 1692.021403] dmar: DMAR:[DMA Read] Request device [00:02.0] fault addr 
> 1a45aa9000 
> [ 1692.021403] DMAR:[fault reason 12] non-zero reserved fields in PTE
> [ 1692.021416] dmar: DRHD: handling fault status reg 2
> 
> This pci device is the graphics card, which I did not assign to qemu!
> I did assign the following devices:
> 00:1a.0, 00:1b.0, 00:1c.0, 00:1c.6, 00:1d.0, 03:00.0.

Piecing together your logs:

iommu_group 5
00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04)
iommu_group 6
00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset 
Family High Definition Audio Controller [8086:1c20] (rev 04)
iommu_group 7
00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 1 [8086:1c10] (rev b4)
00:1c.6 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 7 [8086:1c1c] (rev b4)
03:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host 
Controller [1033:0194] (rev ff)
iommu_group 8
00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04)

Can you clarify what you mean by assign?  Are you actually assigning the
root ports to the qemu guest (1c.0 & 1c.6)?  vfio will require they be
owned by vfio-pci to make use of 3:00.0, but assigning them to the guest
is not recommended.  Can you provided your qemu command line?  We need
to re-visit how to handle pcieport devices with vfio-pci, perhaps
white-listing it as a vfio "compatible" driver, but this still should
not interfere with devices external to the group.

The DMAR fault address looks pretty bogus unless you happen to have
100GB+ of ram in the system.

> The error occurs at random and is not reproducible every time. It
> happens about every third reboot. 
> I'm running qemu-kvm 1.3.0 (kvm-1.3.0-187.3), kernel 3.8.0-rc5 and
> windows 7 as guest OS. The hardware uses an Intel IOMMU. See
> attachments for output of lspci, and details on iommu groups
> 
> I'm not sure if this problem originates from qemu, kvm, vfio or the
> GPU driver.
> Do you have any hints how to debug this further?

vfio makes use of the IOMMU API for programming DMA translations, so an
reserved fields would have to be programmed by intel-iommu itself.  We
could of course be passing some kind of bogus data that intel-iommu
isn't catching.  If you're assigning the root ports to the guest, I'd
start with that, don't do it.  Attach them to vfio, but don't give them
to the guest.  Maybe that'll give us a hint.  I also notice that your
USB 3 controller is dead:

03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller (rev 
ff) (prog-if ff)
!!! Unknown header type 7f

We only see unknown header type 7f when the read from the device returns
-1.  This might have something to do with the root port above it (1c.6)
being in state D3.  Windows likes to put unused devices in D3, which
leads me to suspect you are giving it to the guest.  Let's see what
happens without that.  Thanks,

Alex

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

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #5 from runetmem...@gmail.com ---
> Is there more information in the kdm log file?
I use LightDM-KDE. I'll look into LightDM log next time.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Radeon RV100 init fails with specific card+mobo - IRQ problem

[Meelis Roos]

> >> > Adding some printks reveals it calls
> >> > r100_init() -> radeon_irq_kms_init() -> drm_irq_install() ->
> >> > drm_dev_to_irq() and that fails. So no IRQ, no KMS.
> >> >
> >> > lspci does not show Interrupt like on some other PCI devices.
> >> > /proc/interrupts obviously does not contain the irq since we do not
> >> > register any yet.
> >> >
> >> > There is no BIOS option of "Enable IRQ for VGA" or similar (BIOS is
> >> > latest). Could this be the problem?
> >> >
> >> > What can I do to get KMS working on this computer?
> >> >
> >> > --
> >> > Meelis Roos (mr...@linux.ee)
> >>
> >> Try booting with radeon.agp_mode=-1
> >
> > radeon: `-1' invalid for parameter `agp_mode'
> 
> radeon.agpmode=-1
> 
> However, I think there are a number of places where we require
> interrupts that would need to be worked around (fences, display
> hotplug, pageflipping) if not interrupt is available.

I did dome more testing with different cards an got a surprise. The 
machine works fine with other AGP cards - one MX440 with nouvea, another 
RV100 QY and a R200. They do have interrupt in lspci -vvv output.

Why does this specific card not have its IRQ routed - card ROM problem?

-- 
Meelis Roos (mr...@linux.ee)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/ttm: avoid allocation memory while spinlock is held

[j . glisse]

From: Jerome Glisse 

We need to take reference on the sync object while holding the
fence spinlock but at the same time we don't want to allocate
memory while holding the spinlock. This patch make sure we
enforce both of this constraint.

Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296

Signed-off-by: Jerome Glisse 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |   16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 44420fc..77799a5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -413,6 +413,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  * @bo: A pointer to a struct ttm_buffer_object.
  * @new_obj: A pointer to a pointer to a newly created ttm_buffer_object,
  * holding the data of @bo with the old placement.
+ * @sync_obj: the sync object caller is responsible to take a reference on
+ * behalf of this function
  *
  * This is a utility function that may be called after an accelerated move
  * has been scheduled. A new buffer object is created as a placeholder for
@@ -423,7 +425,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  */
 
 static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
- struct ttm_buffer_object **new_obj)
+ struct ttm_buffer_object **new_obj,
+ void sync_obj)
 {
struct ttm_buffer_object *fbo;
struct ttm_bo_device *bdev = bo->bdev;
@@ -448,7 +451,8 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
fbo->vm_node = NULL;
atomic_set(&fbo->cpu_writers, 0);
 
-   fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
+   /* reference on sync obj is taken by the caller of this function */
+   fbo->sync_obj = sync_obj;
kref_init(&fbo->list_kref);
kref_init(&fbo->kref);
fbo->destroy = &ttm_transfered_destroy;
@@ -652,6 +656,8 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
}
ttm_bo_free_old_node(bo);
} else {
+   void *sync_obj;
+
/**
 * This should help pipeline ordinary buffer moves.
 *
@@ -662,12 +668,14 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object 
*bo,
 
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);
 
-   /* ttm_buffer_object_transfer accesses bo->sync_obj */
-   ret = ttm_buffer_object_transfer(bo, &ghost_obj);
+   /* take the ref on the sync object before releasing the 
spinlock */
+   sync_obj = driver->sync_obj_ref(bo->sync_obj);
spin_unlock(&bdev->fence_lock);
+
if (tmp_obj)
driver->sync_obj_unref(&tmp_obj);
 
+   ret = ttm_buffer_object_transfer(bo, &ghost_obj, sync_obj);
if (ret)
return ret;
 
-- 
1.7.10.4

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

Re: Radeon RV100 init fails with specific card+mobo - IRQ problem

[Alex Deucher]

On Mon, Feb 4, 2013 at 11:32 AM, Meelis Roos  wrote:
>> >> > Adding some printks reveals it calls
>> >> > r100_init() -> radeon_irq_kms_init() -> drm_irq_install() ->
>> >> > drm_dev_to_irq() and that fails. So no IRQ, no KMS.
>> >> >
>> >> > lspci does not show Interrupt like on some other PCI devices.
>> >> > /proc/interrupts obviously does not contain the irq since we do not
>> >> > register any yet.
>> >> >
>> >> > There is no BIOS option of "Enable IRQ for VGA" or similar (BIOS is
>> >> > latest). Could this be the problem?
>> >> >
>> >> > What can I do to get KMS working on this computer?
>> >> >
>> >> > --
>> >> > Meelis Roos (mr...@linux.ee)
>> >>
>> >> Try booting with radeon.agp_mode=-1
>> >
>> > radeon: `-1' invalid for parameter `agp_mode'
>>
>> radeon.agpmode=-1
>>
>> However, I think there are a number of places where we require
>> interrupts that would need to be worked around (fences, display
>> hotplug, pageflipping) if not interrupt is available.
>
> I did dome more testing with different cards an got a surprise. The
> machine works fine with other AGP cards - one MX440 with nouvea, another
> RV100 QY and a R200. They do have interrupt in lspci -vvv output.
>
> Why does this specific card not have its IRQ routed - card ROM problem?

For some reason the system bios on your box doesn't assign it one.
The video bios isn't really involved.  I'm not sure if there is a way
to force assign an irq in Linux or not.

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

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #6 from Ilija Hadzic  ---
Created attachment 74183
  --> https://bugs.freedesktop.org/attachment.cgi?id=74183&action=edit
Possible "Band-Aid" to get you going.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #7 from Ilija Hadzic  ---
I have just posted a patch that may help at least prevent the crash. I have
seen this bug on my end too, but I though that I locally introduced it with my
own hacks. The patch does not solve the root cause (i.e. buffer double-free),
but if it prevents the crash, maybe we can apply it to get people going until
we find the real solution.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5,RESEND 7/8] ARM: tegra: Add board data and 2D clocks

[Stephen Warren]

On 02/04/2013 04:26 AM, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:44:03PM +0200, Terje Bergstrom wrote:
>> Add a driver alias gr2d for Tegra 2D device, and assign a
>> duplicate of 2D clock to that driver alias.
>> 
>> Signed-off-by: Terje Bergstrom  --- 
>> arch/arm/mach-tegra/board-dt-tegra20.c|1 + 
>> arch/arm/mach-tegra/board-dt-tegra30.c|1 + 
>> arch/arm/mach-tegra/tegra20_clocks_data.c |2 +- 
>> arch/arm/mach-tegra/tegra30_clocks_data.c |1 + 4 files
>> changed, 4 insertions(+), 1 deletion(-)
> 
> With Prashant's clock rework patches now merged this patch can be 
> dropped.

Assuming this series is applied for 3.10 and not earlier, yes. I'd
certainly recommend applying for 3.10 not 3.9; the dependencies to
apply this for 3.9 given the AUXDATA/... requirements would be too
painful.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #8 from runetmem...@gmail.com ---
So since you find root case, LightDM log is not necessary now, I am correct?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #9 from Ilija Hadzic  ---
(In reply to comment #8)
> So since you find root case, LightDM log is not necessary now, I am correct?

No, I didn't find the root cause. I am only assuming (based on the last message
in your Xorg.0.log file) that you are seeing the same kind of a crash that I
have been seeing recently.

So I would like you to test if the patch that I posted fixes the problem for
you. If it does, then there are two open issues that remain: a) should we push
my patch upstream (i.e. to at least prevent the crash of affected users like
you) and b) what really causes the double-free. The latter is what we have no
idea about yet.

Any additional logs that you can produce in the meantime would be useful.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #10 from runetmem...@gmail.com ---
> So I would like you to test if the patch that I posted fixes the problem for 
> you. 
Unfortunately, I am not developer, I can't check this patch.

> Any additional logs that you can produce in the meantime would be useful.
Ok.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Fix RC6VIDS encode/devoce

[Daniel Vetter]

On Fri, Feb 01, 2013 at 04:41:14PM -0800, Ben Widawsky wrote:
> The RC6 VIDS has a linear ramp starting at 250mv, which means any values
> below 250 are invalid. The old buggy macros tried to adjust for this to
> be more flexible, but there is no need. As Dan pointed out the ENCODE
> only ever has one value. The only invalid value for decode is an input
> of 0 which means something is really wonky, and the cases where DECODE
> are used either don't matter (debug values), or would be implicitly
> correct (the check for less than 450).
> 
> This patch makes simpler, easier to read macros which are actually
> correct. Maybe this patch can actually fix some bugs now.
> 
> Thanks to Dan for catching this. /me hides
> 
> Cc: sta...@kernel.org
> Reported-by: Dan Carpenter 
> Signed-off-by: Ben Widawsky 

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[j . glisse]

From: Jerome Glisse 

We need to take reference on the sync object while holding the
fence spinlock but at the same time we don't want to allocate
memory while holding the spinlock. This patch make sure we
enforce both of this constraint.

v2: actually test build it

Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296

Signed-off-by: Jerome Glisse 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |   17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 44420fc..f4b7acd 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -413,6 +413,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  * @bo: A pointer to a struct ttm_buffer_object.
  * @new_obj: A pointer to a pointer to a newly created ttm_buffer_object,
  * holding the data of @bo with the old placement.
+ * @sync_obj: the sync object caller is responsible to take a reference on
+ * behalf of this function
  *
  * This is a utility function that may be called after an accelerated move
  * has been scheduled. A new buffer object is created as a placeholder for
@@ -423,11 +425,11 @@ static void ttm_transfered_destroy(struct 
ttm_buffer_object *bo)
  */
 
 static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
- struct ttm_buffer_object **new_obj)
+ struct ttm_buffer_object **new_obj,
+ void *sync_obj)
 {
struct ttm_buffer_object *fbo;
struct ttm_bo_device *bdev = bo->bdev;
-   struct ttm_bo_driver *driver = bdev->driver;
 
fbo = kzalloc(sizeof(*fbo), GFP_KERNEL);
if (!fbo)
@@ -448,7 +450,8 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
fbo->vm_node = NULL;
atomic_set(&fbo->cpu_writers, 0);
 
-   fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
+   /* reference on sync obj is taken by the caller of this function */
+   fbo->sync_obj = sync_obj;
kref_init(&fbo->list_kref);
kref_init(&fbo->kref);
fbo->destroy = &ttm_transfered_destroy;
@@ -652,6 +655,8 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
}
ttm_bo_free_old_node(bo);
} else {
+   void *sync_obj;
+
/**
 * This should help pipeline ordinary buffer moves.
 *
@@ -662,12 +667,14 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object 
*bo,
 
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);
 
-   /* ttm_buffer_object_transfer accesses bo->sync_obj */
-   ret = ttm_buffer_object_transfer(bo, &ghost_obj);
+   /* take the ref on the sync object before releasing the 
spinlock */
+   sync_obj = driver->sync_obj_ref(bo->sync_obj);
spin_unlock(&bdev->fence_lock);
+
if (tmp_obj)
driver->sync_obj_unref(&tmp_obj);
 
+   ret = ttm_buffer_object_transfer(bo, &ghost_obj, sync_obj);
if (ret)
return ret;
 
-- 
1.7.10.4

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

Re: [PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Daniel Vetter]

On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
> From: Jerome Glisse 
>
> We need to take reference on the sync object while holding the
> fence spinlock but at the same time we don't want to allocate
> memory while holding the spinlock. This patch make sure we
> enforce both of this constraint.
>
> v2: actually test build it
>
> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>
> Signed-off-by: Jerome Glisse 

Isn't that just another iteration of
https://patchwork.kernel.org/patch/1972071/ which somehow never
reached -fixes?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Jerome Glisse]

On Mon, Feb 4, 2013 at 2:21 PM, Daniel Vetter  wrote:
> On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
>> From: Jerome Glisse 
>>
>> We need to take reference on the sync object while holding the
>> fence spinlock but at the same time we don't want to allocate
>> memory while holding the spinlock. This patch make sure we
>> enforce both of this constraint.
>>
>> v2: actually test build it
>>
>> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>>
>> Signed-off-by: Jerome Glisse 
>
> Isn't that just another iteration of
> https://patchwork.kernel.org/patch/1972071/ which somehow never
> reached -fixes?
> -Daniel

Yes but my version doesn't drop the lock before taking the ref, iirc
there might be a race if droping the lock and then taking it again.
Another process might race to unref the sync obj but i haven't
tortured too much my brain on how likely if at all this is possible.

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

Re: [PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Daniel Vetter]

On Mon, Feb 4, 2013 at 8:36 PM, Jerome Glisse  wrote:
> On Mon, Feb 4, 2013 at 2:21 PM, Daniel Vetter  wrote:
>> On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
>>> From: Jerome Glisse 
>>>
>>> We need to take reference on the sync object while holding the
>>> fence spinlock but at the same time we don't want to allocate
>>> memory while holding the spinlock. This patch make sure we
>>> enforce both of this constraint.
>>>
>>> v2: actually test build it
>>>
>>> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>>>
>>> Signed-off-by: Jerome Glisse 
>>
>> Isn't that just another iteration of
>> https://patchwork.kernel.org/patch/1972071/ which somehow never
>> reached -fixes?
>> -Daniel
>
> Yes but my version doesn't drop the lock before taking the ref, iirc
> there might be a race if droping the lock and then taking it again.
> Another process might race to unref the sync obj but i haven't
> tortured too much my brain on how likely if at all this is possible.

Hm, mine rechecks whether the sync object disappeared (spotted by
Maarten) before grabbing a reference. So should be ok if the fence
signals. Ofc, if we don't hold a reservation on bo someone else might
sneak and add a new one. But since we're trying to move the bo that'd
be a pretty bug already.

In any case yours is a bit nicer since it only grabs the fence_lock
once. My poke was more a stab at Dave, since he originally prodded me
on irc for breaking this, and then it seems to have fallen by the
wayside ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/nouveau: add lockdep annotations

[Marcin Slusarz]

1) Lockdep thinks all nouveau subdevs belong to the same class and can be
locked in arbitrary order, which is not true (at least in general case).
Tell it to distinguish subdevs by (o)class type.
2) DRM client can be locked under user client lock - tell lockdep to put DRM
client lock in a separate class.

Reported-by: Arend van Spriel 
Reported-by: Peter Hurley 
Reported-by: Maarten Lankhorst 
Reported-by: Daniel J Blueman 
Signed-off-by: Marcin Slusarz 
Cc: sta...@vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
---
Lightly tested, only on NV4B and NVC1.
---
 drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
 drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c  | 3 +++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c 
b/drivers/gpu/drm/nouveau/core/core/subdev.c
index f74c30a..48f0637 100644
--- a/drivers/gpu/drm/nouveau/core/core/subdev.c
+++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
@@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
if (ret)
return ret;
 
-   mutex_init(&subdev->mutex);
+   __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
subdev->name = subname;
 
if (parent) {
diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h 
b/drivers/gpu/drm/nouveau/core/include/core/object.h
index 6a90267..62e68ba 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/object.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
@@ -50,10 +50,13 @@ int  nouveau_object_fini(struct nouveau_object *, bool 
suspend);
 
 extern struct nouveau_ofuncs nouveau_object_ofuncs;
 
+/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
+ * ".data". */
 struct nouveau_oclass {
u32 handle;
-   struct nouveau_ofuncs *ofuncs;
-   struct nouveau_omthds *omthds;
+   struct nouveau_ofuncs * const ofuncs;
+   struct nouveau_omthds * const omthds;
+   struct lock_class_key lock_class_key;
 };
 
 #define nv_oclass(o)nv_object(o)->oclass
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ef1ad21..bc00587 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
return 0;
 }
 
+static struct lock_class_key drm_client_lock_class_key;
+
 static int
 nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 {
@@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long 
flags)
ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
if (ret)
return ret;
+   lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
 
dev->dev_private = drm;
drm->dev = dev;
-- 
1.8.1

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

Re: [PATCH] drm/nouveau: add lockdep annotations

[Maarten Lankhorst]

Op 04-02-13 22:30, Marcin Slusarz schreef:
> 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> locked in arbitrary order, which is not true (at least in general case).
> Tell it to distinguish subdevs by (o)class type.
Apart from this specific case, is there any other reason why we require being 
able to nest 2 subdev locks?

Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return 
-EBUSY if there is any existing object. Problem solved, and lockdep is still 
generic.

> 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> client lock in a separate class.
Can you copy paste a typical lockdep splat for this? Also this should be a 
separate patch. :-)


> Reported-by: Arend van Spriel 
> Reported-by: Peter Hurley 
> Reported-by: Maarten Lankhorst 
> Reported-by: Daniel J Blueman 
> Signed-off-by: Marcin Slusarz 
> Cc: sta...@vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
> ---
> Lightly tested, only on NV4B and NVC1.
> ---
>  drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
>  drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +--
>  drivers/gpu/drm/nouveau/nouveau_drm.c  | 3 +++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c 
> b/drivers/gpu/drm/nouveau/core/core/subdev.c
> index f74c30a..48f0637 100644
> --- a/drivers/gpu/drm/nouveau/core/core/subdev.c
> +++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
> @@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
>   if (ret)
>   return ret;
>  
> - mutex_init(&subdev->mutex);
> + __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
>   subdev->name = subname;
>  
>   if (parent) {
> diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h 
> b/drivers/gpu/drm/nouveau/core/include/core/object.h
> index 6a90267..62e68ba 100644
> --- a/drivers/gpu/drm/nouveau/core/include/core/object.h
> +++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
> @@ -50,10 +50,13 @@ int  nouveau_object_fini(struct nouveau_object *, bool 
> suspend);
>  
>  extern struct nouveau_ofuncs nouveau_object_ofuncs;
>  
> +/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
> + * ".data". */
>  struct nouveau_oclass {
>   u32 handle;
> - struct nouveau_ofuncs *ofuncs;
> - struct nouveau_omthds *omthds;
> + struct nouveau_ofuncs * const ofuncs;
> + struct nouveau_omthds * const omthds;
> + struct lock_class_key lock_class_key;
>  };
>  
>  #define nv_oclass(o)nv_object(o)->oclass
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ef1ad21..bc00587 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>   return 0;
>  }
>  
> +static struct lock_class_key drm_client_lock_class_key;
> +
>  static int
>  nouveau_drm_load(struct drm_device *dev, unsigned long flags)
>  {
> @@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long 
> flags)
>   ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
>   if (ret)
>   return ret;
> + lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
>  
>   dev->dev_private = drm;
>   drm->dev = dev;

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

[Bug 60034] rv790 etqw regression since r600g: add async for staging buffer upload v2

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60034

--- Comment #11 from Jerome Glisse  ---
Created attachment 74200
  --> https://bugs.freedesktop.org/attachment.cgi?id=74200&action=edit
work around

Can you check that this work around fix the rendering ?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 60034] rv790 etqw regression since r600g: add async for staging buffer upload v2

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60034

--- Comment #12 from Andy Furniss  ---
(In reply to comment #11)
> Created attachment 74200 [details] [review]
> work around
> 
> Can you check that this work around fix the rendering ?

Yes, this fixes etqw for me.

I see someone else reported an issue with this commit and xonotic.

I haven't checked if xonotic was OK before, but I can see the issue and this
patch does not help it.

https://bugs.freedesktop.org/show_bug.cgi?id=60236

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 59672] Problems initializing Radeon driver: lockup during IB test

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=59672

Florian Mickler  changed:

   What|Removed |Added

 CC||flor...@mickler.org

--- Comment #11 from Florian Mickler  ---
A patch referencing this bug report has been merged in Linux v3.8-rc6:

commit b3dfcb207e550dffb8680cab7afaf6b4fb6eae33
Author: Michel Dänzer 
Date:   Thu Jan 24 19:02:01 2013 +0100

drm/radeon: Enable DMA_IB_SWAP_ENABLE on big endian hosts.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 59672] Problems initializing Radeon driver: lockup during IB test

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=59672

Alex Deucher  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D

[Inki Dae]

2013/2/4 Sachin Kamat :
> On 1 February 2013 18:28, Inki Dae  wrote:
>>
>>
>>
>>
>> 2013. 2. 1. 오후 8:52 Inki Dae  작성:
>>
>>>
>>>
 -Original Message-
 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Sachin Kamat
 Sent: Friday, February 01, 2013 8:40 PM
 To: Inki Dae
 Cc: Sylwester Nawrocki; Kukjin Kim; Sylwester Nawrocki; linux-
 me...@vger.kernel.org; dri-devel@lists.freedesktop.org; devicetree-
 disc...@lists.ozlabs.org; patc...@linaro.org
 Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery
 support for G2D

 On 1 February 2013 17:02, Inki Dae  wrote:
>
> How about using like below?
>Compatible = ""samsung,exynos4x12-fimg-2d" /* for Exynos4212,
> Exynos4412  */
> It looks odd to use "samsung,exynos4212-fimg-2d" saying that this ip is
 for
> exynos4212 and exynos4412.

 AFAIK, compatible strings are not supposed to have any wildcard
>>> characters.
 Compatible string should suggest the first SoC that contained this IP.
 Hence IMO 4212 is OK.

>>
>> Oops, one more thing. AFAIK Exynos4210 also has fimg-2d ip. In this case, we 
>> should use "samsung,exynos4210-fimg-2d" as comparible string and add it to 
>> exynos4210.dtsi?
>
> Exynos4210 has same g2d IP (v3.0) as C110 or V210; so the same
> comptible string will be used for this one too.
>
>> And please check if exynos4212 and 4412 SoCs have same fimg-2d ip. If it's 
>> different, we might need to add ip version property or compatible string to 
>> each dtsi file to identify the ip version.
>
> AFAIK, they both have the same IP (v4.1).
>

Ok, let's use the below,

For exynos4210 SoC,
compatible = "samsung,exynos4210-g2d"

For exynos4x12 SoCs,
compatible = "samsung,exynos4212-g2d"

For exynos5250, 5410 (In case of Exynos5440, I'm not sure that the SoC
has same ip)
compatible = "samsung,exynos5250-g2d"

To other guys,
The device tree is used by not only v4l2 side but also drm side so we
should reach an arrangement. So please give me ack if you agree to my
opinion. Otherwise please, give me your opinions.

Thanks,
Inki Dae


>>
>> Sorry but give me your opinions.
>>
>> Thanks,
>> Inki Dae
>>
>>
>>>
>>> Got it. Please post it again.
>>>

 --
 With warm regards,
 Sachin
 --
 To unsubscribe from this list: send the line "unsubscribe linux-media" in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> ___
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> With warm regards,
> Sachin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Use C8 instead of RGB332 when determining the format from depth/bpp

[Dave Airlie]

On Tue, Feb 5, 2013 at 12:55 AM, Daniel Vetter  wrote:
> On Thu, Jan 31, 2013 at 07:43:38PM +0200, ville.syrj...@linux.intel.com wrote:
>> From: Ville Syrjälä 
>>
>> Support for real RGB332 is a rarity, most hardware only really support
>> C8. So use C8 instead of RGB332 when determining the format based on
>> depth/bpp.
>>
>> This fixes 8bpp fbcon on i915, since i915 will only accept C8 and not
>> RGB332.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59572
>> Signed-off-by: Ville Syrjälä 
>
> Tested-by: mlsemo...@gmail.com
>
> Dave, can you please consider including these two patches into -fixes? The
> fix a black screen regression when users opt for 8bpp console ...

These are for next, considering what Linus said about -fixes not
unless they are big.

so I'll stick a stable cc onto it in next.

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

Re: [PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

[Terje Bergström]

On 04.02.2013 01:09, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
>> Add host1x, the driver for host1x and its client unit 2D.
> 
> Maybe this could be a bit more verbose. Perhaps describe what host1x is.

Sure. I could just steal the paragraph from Stephen:

The Tegra host1x module is the DMA engine for register access to Tegra's
graphics- and multimedia-related modules. The modules served by host1x
are referred to as clients. host1x includes some other  functionality,
such as synchronization.

>> diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
> [...]
>> @@ -0,0 +1,6 @@
>> +config TEGRA_HOST1X
>> + tristate "Tegra host1x driver"
>> + help
>> +   Driver for the Tegra host1x hardware.
> 
> Maybe s/Tegra/NVIDIA Tegra/?

Sounds good.

>> +
>> +   Required for enabling tegradrm.
> 
> This should probably be dropped. Either encode such knowledge as
> explicit dependencies or in this case just remove it altogether since we
> will probably merge both drivers anyway.

I think this was left from previous versions. Now it just doesn't make
sense. I'll just drop it.

> 
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> [...]
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "dev.h"
> 
> Maybe add a blank line between the previous two lines to visually
> separate standard Linux includes from driver-specific ones.

Ok. You commented in quite few places in a similar way. I'll fix all of
them to first include system includes, then driver's own includes, and
add a blank line in between.

> 
>> +#include "hw/host1x01.h"
>> +
>> +#define CREATE_TRACE_POINTS
>> +#include 
>> +
>> +#define DRIVER_NAME  "tegra-host1x"
> 
> You only ever use this once, so maybe it can just be dropped?

Yes.

> 
>> +static struct host1x_device_info host1x_info = {
> 
> Perhaps this should be host1x01_info in order to match the hardware
> revision? That'll avoid it having to be renamed later on when other
> revisions start to appear.

Ok, will do. I thought it'd be awkward being alone until the second
version appears, but I'll add it.

> 
>> +static int host1x_probe(struct platform_device *dev)
>> +{
> [...]
>> + syncpt_irq = platform_get_irq(dev, 0);
>> + if (IS_ERR_VALUE(syncpt_irq)) {
> 
> This is somewhat unusual. It should be fine to just do:
> 
> if (syncpt_irq < 0)
> 
> but IS_ERR_VALUE() should work fine too.

I'll use the simpler version.

> 
>> + memcpy(&host->info, devid->data, sizeof(struct host1x_device_info));
> 
> Why not make the .info field a pointer to struct host1x_device_info
> instead? That way you don't have to keep separate copies of the same
> information.

This had something to do with __init data and non-init data. But, we're
not really putting this data into __init, so we should be able to use
just a pointer.

> 
>> + /* set common host1x device data */
>> + platform_set_drvdata(dev, host);
>> +
>> + host->regs = devm_request_and_ioremap(&dev->dev, regs);
>> + if (!host->regs) {
>> + dev_err(&dev->dev, "failed to remap host registers\n");
>> + return -ENXIO;
>> + }
> 
> This should probably be rewritten as:
> 
> host->regs = devm_ioremap_resource(&dev->dev, regs);
> if (IS_ERR(host->regs))
> return PTR_ERR(host->regs);
> 
> Though that function will only be available in 3.9-rc1.

Ok, 3.9-rc1 is fine as a basis.

>> + err = host1x_syncpt_init(host);
>> + if (err)
>> + return err;
> [...]
>> + host1x_syncpt_reset(host);
> 
> Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
> it might be useful to have host1x_syncpt_reset() as a separate function
> but couldn't it be called as part of host1x_syncpt_init()?

host1x_syncpt_init() is used for initializing the syncpt structures, and
is called in probe. host1x_syncpt_reset() should be called whenever we
think hardware state is lost, for example if VDD_CORE was rail gated due
to system suspend.

> 
>> + dev_info(&dev->dev, "initialized\n");
> 
> I don't think this is very useful. We should make sure to tell people
> when things fail. When everything goes as planned we don't need to brag
> about it =)

True. I wish other kernel drivers followed that same philosophy. :-)
I'll remove that. It's mainly useful as debug help, but it's as easy to
check from sysfs the state.

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> [...]
>> +struct host1x_syncpt_ops {
> [...]
>> + const char * (*name)(struct host1x_syncpt *);
> 
> Why do we need this? Could we not refer to the syncpt name directly
> instead of going through this wrapper? I'd expect the name to be static.

This must be a relic. I'll remove the wrapper.

> 
>> +struct host1x_device_info {
> 
> Maybe this should be called simply host1x_info? _device seems redundant.

Sure.

> 
>> + int  

[Bug 60302] New: piglit failures r600g radeon hd 6850

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=60302

  Priority: medium
Bug ID: 60302
  Assignee: dri-devel@lists.freedesktop.org
   Summary: piglit failures r600g radeon hd 6850
  Severity: normal
Classification: Unclassified
OS: Linux (All)
  Reporter: co...@octayn.net
  Hardware: x86-64 (AMD64)
Status: NEW
   Version: git
 Component: Drivers/Gallium/r600
   Product: Mesa

Ran piglet (d2250db) on the 9.1 branch (1003652). Kernel 3.8-rc5

Results available at: http://octayn.net/9.1-piglit

Any additional information I can provide, let me know (should I run it on 9.0
stable to provide baseline maybe?)

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D

[Kyungmin Park]

On Tue, Feb 5, 2013 at 12:03 PM, Inki Dae  wrote:
> 2013/2/4 Sachin Kamat :
>> On 1 February 2013 18:28, Inki Dae  wrote:
>>>
>>>
>>>
>>>
>>> 2013. 2. 1. 오후 8:52 Inki Dae  작성:
>>>


> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sachin Kamat
> Sent: Friday, February 01, 2013 8:40 PM
> To: Inki Dae
> Cc: Sylwester Nawrocki; Kukjin Kim; Sylwester Nawrocki; linux-
> me...@vger.kernel.org; dri-devel@lists.freedesktop.org; devicetree-
> disc...@lists.ozlabs.org; patc...@linaro.org
> Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery
> support for G2D
>
> On 1 February 2013 17:02, Inki Dae  wrote:
>>
>> How about using like below?
>>Compatible = ""samsung,exynos4x12-fimg-2d" /* for Exynos4212,
>> Exynos4412  */
>> It looks odd to use "samsung,exynos4212-fimg-2d" saying that this ip is
> for
>> exynos4212 and exynos4412.
>
> AFAIK, compatible strings are not supposed to have any wildcard
 characters.
> Compatible string should suggest the first SoC that contained this IP.
> Hence IMO 4212 is OK.
>
>>>
>>> Oops, one more thing. AFAIK Exynos4210 also has fimg-2d ip. In this case, 
>>> we should use "samsung,exynos4210-fimg-2d" as comparible string and add it 
>>> to exynos4210.dtsi?
>>
>> Exynos4210 has same g2d IP (v3.0) as C110 or V210; so the same
>> comptible string will be used for this one too.
>>
>>> And please check if exynos4212 and 4412 SoCs have same fimg-2d ip. If it's 
>>> different, we might need to add ip version property or compatible string to 
>>> each dtsi file to identify the ip version.
>>
>> AFAIK, they both have the same IP (v4.1).
>>
>
> Ok, let's use the below,
>
> For exynos4210 SoC,
> compatible = "samsung,exynos4210-g2d"
>
> For exynos4x12 SoCs,
> compatible = "samsung,exynos4212-g2d"
Even though 4212 is exist, I can't see 4212 board support at mainline.
so I prefer exynos4412-g2d instead of 4212-g2d.
>
> For exynos5250, 5410 (In case of Exynos5440, I'm not sure that the SoC
> has same ip)
> compatible = "samsung,exynos5250-g2d"
Acked-by: Kyungmin Park 
>
> To other guys,
> The device tree is used by not only v4l2 side but also drm side so we
> should reach an arrangement. So please give me ack if you agree to my
> opinion. Otherwise please, give me your opinions.
>
> Thanks,
> Inki Dae
>
>
>>>
>>> Sorry but give me your opinions.
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>

 Got it. Please post it again.

>
> --
> With warm regards,
> Sachin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>> --
>> With warm regards,
>> Sachin
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv5, RESEND 2/8] gpu: host1x: Add syncpoint wait and interrupts

[Terje Bergström]

On 04.02.2013 02:30, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:43:58PM +0200, Terje Bergstrom wrote:
> [...]
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> [...]
>> @@ -95,7 +96,6 @@ static int host1x_probe(struct platform_device *dev)
>>
>>   /* set common host1x device data */
>>   platform_set_drvdata(dev, host);
>> -
>>   host->regs = devm_request_and_ioremap(&dev->dev, regs);
>>   if (!host->regs) {
>>   dev_err(&dev->dev, "failed to remap host registers\n");
> 
> This seems an unrelated (and actually undesirable) change.
> 
>> @@ -109,28 +109,40 @@ static int host1x_probe(struct platform_device *dev)
>>   }
>>
>>   err = host1x_syncpt_init(host);
>> - if (err)
>> + if (err) {
>> + dev_err(&dev->dev, "failed to init sync points");
>>   return err;
>> + }
> 
> This error message should probably have gone in the previous patch as
> well.

Oops, will move these to previous patch. I'm pretty sure I already fixed
this once. :-(

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
>> index d8f5979..8376092 100644
>> --- a/drivers/gpu/host1x/dev.h
>> +++ b/drivers/gpu/host1x/dev.h
>> @@ -17,11 +17,12 @@
>>  #ifndef HOST1X_DEV_H
>>  #define HOST1X_DEV_H
>>
>> +#include 
>>  #include "syncpt.h"
>> +#include "intr.h"
>>
>>  struct host1x;
>>  struct host1x_syncpt;
>> -struct platform_device;
> 
> Why include platform_device.h here?

host1x_get_host() actually needs that, so this #include should've also
been in previous patch.

> 
>> @@ -34,6 +35,18 @@ struct host1x_syncpt_ops {
>>   const char * (*name)(struct host1x_syncpt *);
>>  };
>>
>> +struct host1x_intr_ops {
>> + void (*init_host_sync)(struct host1x_intr *);
>> + void (*set_host_clocks_per_usec)(
>> + struct host1x_intr *, u32 clocks);
> 
> Could the above two not be combined? The only reason to keep them
> separate would be if the host1x clock was dynamically changed, but I
> don't think we support that, right?

I've left this as a placeholder to at some point start supporting host1x
clock scaling. But I don't think we're going to do that for a while, so
I could merge them.

> 
>> + void (*set_syncpt_threshold)(
>> + struct host1x_intr *, u32 id, u32 thresh);
>> + void (*enable_syncpt_intr)(struct host1x_intr *, u32 id);
>> + void (*disable_syncpt_intr)(struct host1x_intr *, u32 id);
>> + void (*disable_all_syncpt_intrs)(struct host1x_intr *);
> 
> Can disable_all_syncpt_intrs() not be implemented generically using the
> number of syncpoints as exposed by host1x_device_info and the
> .disable_syncpt_intr() function?

disable_all_syncpt_intrs() disables all interrupts in one write (or one
per 32 sync points), so it's more efficient.

> 
>> @@ -46,11 +59,13 @@ struct host1x_device_info {
>>  struct host1x {
>>   void __iomem *regs;
>>   struct host1x_syncpt *syncpt;
>> + struct host1x_intr intr;
>>   struct platform_device *dev;
>>   struct host1x_device_info info;
>>   struct clk *clk;
>>
>>   struct host1x_syncpt_ops syncpt_op;
>> + struct host1x_intr_ops intr_op;
> 
> I think carrying a const pointer to the interrupt operations structure
> is a better option here.

Ok.

> 
>> diff --git a/drivers/gpu/host1x/hw/intr_hw.c 
>> b/drivers/gpu/host1x/hw/intr_hw.c
> [...]
>> +static void host1x_intr_syncpt_thresh_isr(struct host1x_intr_syncpt 
>> *syncpt);
> 
> Can we avoid this forward declaration?

I think we can, if I just move the isr to top of file.

> 
>> +static void syncpt_thresh_cascade_fn(struct work_struct *work)
> 
> syncpt_thresh_work()?

Sounds good.

> 
>> +{
>> + struct host1x_intr_syncpt *sp =
>> + container_of(work, struct host1x_intr_syncpt, work);
>> + host1x_syncpt_thresh_fn(sp);
> 
> Couldn't we inline the host1x_syncpt_thresh_fn() implementation here?
> Why do we need to go through an external function declaration?

If I move syncpt_thresh_work() to intr.c from intr_hw.c, I could do
that. That'd simplify the interrupt path.

> 
>> +static irqreturn_t syncpt_thresh_cascade_isr(int irq, void *dev_id)
>> +{
>> + struct host1x *host1x = dev_id;
>> + struct host1x_intr *intr = &host1x->intr;
>> + unsigned long reg;
>> + int i, id;
>> +
>> + for (i = 0; i < host1x->info.nb_pts / BITS_PER_LONG; i++) {
>> + reg = host1x_sync_readl(host1x,
>> + HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS +
>> + i * REGISTER_STRIDE);
>> + for_each_set_bit(id, ®, BITS_PER_LONG) {
>> + struct host1x_intr_syncpt *sp =
>> + intr->syncpt + (i * BITS_PER_LONG + id);
>> + host1x_intr_syncpt_thresh_isr(sp);
> 
> Have you considered mimicking the IRQ API and name this something like
> host1x_intr_syncpt_thresh_handle() and name the actual ISR just
> syncpt_thresh_isr()? Not so importan

Re: [PATCHv5,RESEND 4/8] gpu: host1x: Add debug support

[Terje Bergström]

On 04.02.2013 03:03, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 15, 2013 at 01:44:00PM +0200, Terje Bergstrom wrote:
>> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
> [...]
>> +static pid_t host1x_debug_null_kickoff_pid;
>> +unsigned int host1x_debug_trace_cmdbuf;
>> +
>> +static pid_t host1x_debug_force_timeout_pid;
>> +static u32 host1x_debug_force_timeout_val;
>> +static u32 host1x_debug_force_timeout_channel;
> 
> Please group static and non-static variables.

Will do.

> 
>> diff --git a/drivers/gpu/host1x/debug.h b/drivers/gpu/host1x/debug.h
> [...]
>> +struct output {
>> +void (*fn)(void *ctx, const char *str, size_t len);
>> +void *ctx;
>> +char buf[256];
>> +};
> 
> Do we really need this kind of abstraction? There really should be only
> one location where debug information is obtained, so I don't see a need
> for this.

This is used by debugfs code to direct to debugfs, and
nvhost_debug_dump() to send via printk.

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> [...]
>>  struct host1x_syncpt_ops {
>>  void (*reset)(struct host1x_syncpt *);
>>  void (*reset_wait_base)(struct host1x_syncpt *);
>> @@ -117,6 +133,7 @@ struct host1x {
>>  struct host1x_channel_ops channel_op;
>>  struct host1x_cdma_ops cdma_op;
>>  struct host1x_pushbuffer_ops cdma_pb_op;
>> +struct host1x_debug_ops debug_op;
>>  struct host1x_syncpt_ops syncpt_op;
>>  struct host1x_intr_ops intr_op;
> 
> Again, better to pass in a const pointer to the ops structure.

Ok.

> 
>> diff --git a/drivers/gpu/host1x/hw/debug_hw.c 
>> b/drivers/gpu/host1x/hw/debug_hw.c
> 
>> +static int show_channel_command(struct output *o, u32 addr, u32 val, int 
>> *count)
>> +{
>> +unsigned mask;
>> +unsigned subop;
>> +
>> +switch (val >> 28) {
>> +case 0x0:
> 
> These can easily be derived by looking at the debug output, but it may
> still make sense to assign symbolic names to them.

I have another suggestion. In downstream I removed the decoding part and
I just print out a string of hex. That removes quite a bit bunch of code
from kernel. It makes the debug output also more compact.

It's much easier to write a user space program to decode than maintain
it in kernel.

> 
>> +static void show_channel_word(struct output *o, int *state, int *count,
>> +u32 addr, u32 val, struct host1x_cdma *cdma)
>> +{
>> +static int start_count, dont_print;
> 
> What if two processes read debug information at the same time?

show_channels() acquires cdma.lock, so that shouldn't happen.

> 
>> +static void do_show_channel_gather(struct output *o,
>> +phys_addr_t phys_addr,
>> +u32 words, struct host1x_cdma *cdma,
>> +phys_addr_t pin_addr, u32 *map_addr)
>> +{
>> +/* Map dmaget cursor to corresponding mem handle */
>> +u32 offset;
>> +int state, count, i;
>> +
>> +offset = phys_addr - pin_addr;
>> +/*
>> + * Sometimes we're given different hardware address to the same
>> + * page - in these cases the offset will get an invalid number and
>> + * we just have to bail out.
>> + */
> 
> Why's that?

Because of a race - memory might've been unpinned and unmapped from
IOMMU and when we re-map (pin), we are given a new address.

But, I think this comment is a bit stale - we used to dump also old
gathers. The latest code only dumps jobs in sync queue, so the race
shouldn't happen.

> 
>> +map_addr = host1x_memmgr_mmap(mem);
>> +if (!map_addr) {
>> +host1x_debug_output(o, "[could not mmap]\n");
>> +return;
>> +}
>> +
>> +/* Get base address from mem */
>> +sgt = host1x_memmgr_pin(mem);
>> +if (IS_ERR(sgt)) {
>> +host1x_debug_output(o, "[couldn't pin]\n");
>> +host1x_memmgr_munmap(mem, map_addr);
>> +return;
>> +}
> 
> Maybe you should stick with one of "could not" or "couldn't". Makes it
> easier to search for.

I prefer "could not", so I'll use that.

> 
>> +static void show_channel_gathers(struct output *o, struct host1x_cdma *cdma)
>> +{
>> +struct host1x_job *job;
>> +
>> +list_for_each_entry(job, &cdma->sync_queue, list) {
>> +int i;
>> +host1x_debug_output(o,
>> +"\n%p: JOB, syncpt_id=%d, syncpt_val=%d,"
>> +" first_get=%08x, timeout=%d"
>> +" num_slots=%d, num_handles=%d\n",
>> +job,
>> +job->syncpt_id,
>> +job->syncpt_end,
>> +job->first_get,
>> +job->timeout,
>> +job->num_slots,
>> +job->num_unpins);
> 
> This could go on fewer lines.

Yes, will merge.

> 
>> +static void host1x_debug_show_channel_cdma(struct host1x *m,
>> +struct host1x_channel *ch, s

Re: [PATCHv5,RESEND 5/8] drm: tegra: Move drm to live under host1x

[Terje Bergström]

On 04.02.2013 03:08, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 15, 2013 at 01:44:01PM +0200, Terje Bergstrom wrote:
> [...]
>> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
>> index 697d49a..ffc8bf1 100644
>> --- a/drivers/gpu/host1x/Makefile
>> +++ b/drivers/gpu/host1x/Makefile
>> @@ -12,4 +12,10 @@ host1x-y = \
>>  hw/host1x01.o
>>  
>>  host1x-$(CONFIG_TEGRA_HOST1X_CMA) += cma.o
>> +
>> +ccflags-y += -Iinclude/drm
>> +ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
>> +
>> +host1x-$(CONFIG_DRM_TEGRA) += drm/drm.o drm/fb.o drm/dc.o drm/host1x.o
>> +host1x-$(CONFIG_DRM_TEGRA) += drm/output.o drm/rgb.o drm/hdmi.o
>>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
> 
> Can this be moved into a separate Makefile in the drm subdirectory?

I tried, and kernel build helpfully created two .ko files. As having
cyclic dependencies between two modules isn't nice, I merged them to
same module and that seemed to force merging Makefile.

If anybody has an idea on how to do it otherwise, I'd be happy to keep
the Makefiles separate.

> 
>> diff --git a/drivers/gpu/host1x/host1x_client.h 
>> b/drivers/gpu/host1x/host1x_client.h
> [...]
>> new file mode 100644
>> index 000..fdd2920
>> --- /dev/null
>> +++ b/drivers/gpu/host1x/host1x_client.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Copyright (c) 2013, NVIDIA Corporation.
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#ifndef HOST1X_CLIENT_H
>> +#define HOST1X_CLIENT_H
>> +
>> +struct platform_device;
>> +
>> +void host1x_set_drm_data(struct platform_device *pdev, void *data);
>> +void *host1x_get_drm_data(struct platform_device *pdev);
>> +
>> +#endif
> 
> These aren't defined or used yet.

Hmm, right, they would go to patch 7.

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

Re: [PATCHv5,RESEND 7/8] ARM: tegra: Add board data and 2D clocks

[Terje Bergström]

On 04.02.2013 03:26, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 15, 2013 at 01:44:03PM +0200, Terje Bergstrom wrote:
>> Add a driver alias gr2d for Tegra 2D device, and assign a duplicate
>> of 2D clock to that driver alias.
>>
>> Signed-off-by: Terje Bergstrom 
>> ---
>>  arch/arm/mach-tegra/board-dt-tegra20.c|1 +
>>  arch/arm/mach-tegra/board-dt-tegra30.c|1 +
>>  arch/arm/mach-tegra/tegra20_clocks_data.c |2 +-
>>  arch/arm/mach-tegra/tegra30_clocks_data.c |1 +
>>  4 files changed, 4 insertions(+), 1 deletion(-)
> 
> With Prashant's clock rework patches now merged this patch can be
> dropped.

Yes, and I'll also need to start calling 2D clock with name NULL in gr2d.c.

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

Re: [PATCHv5,RESEND 8/8] drm: tegra: Add gr2d device

[Terje Bergström]

On 04.02.2013 04:56, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 15, 2013 at 01:44:04PM +0200, Terje Bergstrom wrote:
> [...]
>> diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
>> @@ -270,7 +274,29 @@ static int tegra_drm_unload(struct drm_device *drm)
>>
>>  static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp)
>>  {
>> - return 0;
>> + struct host1x_drm_fpriv *fpriv;
>> + int err = 0;
> 
> Can be dropped.
> 
>> +
>> + fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
>> + if (!fpriv)
>> + return -ENOMEM;
>> +
>> + INIT_LIST_HEAD(&fpriv->contexts);
>> + filp->driver_priv = fpriv;
>> +
>> + return err;
> 
> return 0;

Ok.

> 
>> +static void tegra_drm_close(struct drm_device *drm, struct drm_file *filp)
>> +{
>> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(filp);
>> + struct host1x_drm_context *context, *tmp;
>> +
>> + list_for_each_entry_safe(context, tmp, &fpriv->contexts, list) {
>> + context->client->ops->close_channel(context);
>> + kfree(context);
>> + }
>> + kfree(fpriv);
>>  }
> 
> Maybe you should add host1x_drm_context_free() to wrap the loop
> contents?

Makes sense. Will do.

> 
>> @@ -280,7 +306,204 @@ static void tegra_drm_lastclose(struct drm_device *drm)
>>   drm_fbdev_cma_restore_mode(host1x->fbdev);
>>  }
>>
>> +static int
>> +tegra_drm_ioctl_syncpt_read(struct drm_device *drm, void *data,
>> +  struct drm_file *file_priv)
> 
> static int and function name on one line, please.

Ok, will re-split the lines.

> 
>> +{
>> + struct host1x *host1x = drm->dev_private;
>> + struct tegra_drm_syncpt_read_args *args = data;
>> + struct host1x_syncpt *sp =
>> + host1x_syncpt_get_bydev(host1x->dev, args->id);
> 
> I don't know if we need this, except maybe to work around the problem
> that we have two different structures named host1x. The _bydev() suffix
> is misleading because all you really do here is obtain the syncpt from
> the host1x.

Yeah, it's actually working around the host1x duplicate naming.
host1x_syncpt_get takes struct host1x as parameter, but that's different
host1x than in this code.

> 
>> +static int
>> +tegra_drm_ioctl_open_channel(struct drm_device *drm, void *data,
>> +  struct drm_file *file_priv)
>> +{
>> + struct tegra_drm_open_channel_args *args = data;
>> + struct host1x_client *client;
>> + struct host1x_drm_context *context;
>> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
>> + struct host1x *host1x = drm->dev_private;
>> + int err = 0;
> 
> err = -ENODEV; (see below)

Ok, makes sense.

> 
>> +
>> + context = kzalloc(sizeof(*context), GFP_KERNEL);
>> + if (!context)
>> + return -ENOMEM;
>> +
>> + list_for_each_entry(client, &host1x->clients, list) {
>> + if (client->class == args->class) {
>> + context->client = client;
>> + err = client->ops->open_channel(client, context);
>> + if (err)
>> + goto out;
>> +
>> + list_add(&context->list, &fpriv->contexts);
>> + args->context = (uintptr_t)context;
> 
> Perhaps cast this to __u64 directly instead? There's little sense in
> taking the detour via uintptr_t.

I think compiler complained about a direct cast to __u64, but I'll try
again.

> 
>> + goto out;
> 
> return 0;
> 
>> + }
>> + }
>> + err = -ENODEV;
>> +
>> +out:
>> + if (err)
>> + kfree(context);
>> +
>> + return err;
>> +}
> 
> Then this simply becomes:
> 
> kfree(context);
> return err;

Sounds good.

> 
>> +static int
>> +tegra_drm_ioctl_close_channel(struct drm_device *drm, void *data,
>> +  struct drm_file *file_priv)
>> +{
>> + struct tegra_drm_open_channel_args *args = data;
>> + struct host1x_drm_context *context, *tmp;
>> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
>> + int err = 0;
>> +
>> + list_for_each_entry_safe(context, tmp, &fpriv->contexts, list) {
>> + if ((uintptr_t)context == args->context) {
>> + context->client->ops->close_channel(context);
>> + list_del(&context->list);
>> + kfree(context);
>> + goto out;
>> + }
>> + }
>> + err = -EINVAL;
>> +
>> +out:
>> + return err;
>> +}
> 
> Same comments as for tegra_drm_ioctl_open_channel().

Ok, will apply.

> 
>> +static int
>> +tegra_drm_ioctl_get_syncpoint(struct drm_device *drm, void *data,
>> +  struct drm_file *file_priv)
>> +{
>> + struct tegra_drm_get_channel_param_args *args = data;
>> + struct host1x_drm_context *context;
>> + struct host1x_drm_fpriv *fpriv = host1x_drm_fpriv(file_priv);
>>

Re: radeon modeset crashes on A4-3400 HD6410D

[Mikko Tiihonen]

I had time to test the patches during weekend. The first div-by-zero patch
allows the machine to boot with modeset enabled. The second patch you
created actually makes X stable (with only my patch it used to halt for 5
seconds every 1 second). So both patches look excellent.

Thank you for making my machine usable and hope to see the patches reach
official kernel soon.

-Mikko


On Wed, Jan 30, 2013 at 9:14 PM, Alex Deucher  wrote:

> On Tue, Jan 29, 2013 at 1:09 PM, Mikko Tiihonen 
> wrote:
> > Hi,
> >
> > I have a A4-3400 CPU that I bought half a year ago. I've tried once
> month,
> > but  Fedora has never succeeded in booting in graphical mode.
> >
> > Some time ago a finally built some custom kernels to figure out why. I
> > created a bug to Fedora bug, but it has not progressed.
> >
> > Analysis:
> > The radeon module startup fails with division by zero in
> > r6xx_remap_render_backend because the first RB is not enabled and the
> code
> > assuming that enabled RBs must be all the first ones of the valid range.
> >
> >
> > Longer version:
> >
> > In r600.c:r6xx_remap_render_backend
> >
> > The function contains only one divide:
> >
> > u32 r6xx_remap_render_backend(struct radeon_device *rdev,
> >   u32 tiling_pipe_num,
> >   u32 max_rb_num,
> >   u32 total_max_rb_num,
> >   u32 disabled_rb_mask)
> > {
> > u32 rendering_pipe_num, rb_num_width, req_rb_num;
> > ...
> > /* mask out the RBs that don't exist on that asic */
> > disabled_rb_mask |= (0xff << max_rb_num) & 0xff;
> >
> > rendering_pipe_num = 1 << tiling_pipe_num;
> > req_rb_num = total_max_rb_num -
> > r600_count_pipe_bits(disabled_rb_mask);
> > BUG_ON(rendering_pipe_num < req_rb_num);
> >
> > pipe_rb_ratio = rendering_pipe_num / req_rb_num;
> >
> > I added a printk to see what actual parameters are passed in:
> >
> > tiling_pipe_num=2, max_rb_num=1, total_max_rb_num=8, disabled_rb_mask=253
> >
> > Using those to calculate the divide by zero comes from:
> >
> > disabled_rb_mask |= 254; -> 255
> > req_rb_num = 8 - 8;
> >
> >
> > Quick fix for stable kernel series:
> >
> > The attached fix activates only in the division by zero case and instead
> of
> > failing uses the given disabled_rb_mask. This is guaranteed to be
> regression
> > free and actually allows me to load the radeon driver successfully. It
> does
> > help with cards that currently work, but for which some usable RBs are
> > disabled due to the eager masking.
> >
> >
> > Proper fix would propably be to find out think why the disabled_rb_mask
> is
> > masked the way it is, were there bugs in other places for which it is a
> > workaround. I can see two possible modifictions:
> > 1) only mask bits exceeding total_max_rb_num instead of the max_rb_num
> since
> > the one enabled RB on A4-3400 can propably be any one of the 8 possible
> > values.
> > 2) or activate the current code only if there are more than max_rb_num
> zero
> > bits in the given disabled_rb_mask
>
> Looks good.  I've gone ahead and applied your patch for safety sake
> and I also added a patch to set up the RBs correctly on your board.
> Please test the attached patches.
>
> Thanks!
>
> Alex
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

DMAR faults from unrelated device when vfio is used

[David Gstir]

Hi!

I get the following error messages over and over again when using vfio in 
qemu-kvm:

[ 1692.021403] dmar: DMAR:[DMA Read] Request device [00:02.0] fault addr 
1a45aa9000 
[ 1692.021403] DMAR:[fault reason 12] non-zero reserved fields in PTE
[ 1692.021416] dmar: DRHD: handling fault status reg 2

This pci device is the graphics card, which I did not assign to qemu! I did 
assign the following devices:
00:1a.0, 00:1b.0, 00:1c.0, 00:1c.6, 00:1d.0, 03:00.0.

The error occurs at random and is not reproducible every time. It happens about 
every third reboot. 
I'm running qemu-kvm 1.3.0 (kvm-1.3.0-187.3), kernel 3.8.0-rc5 and windows 7 as 
guest OS. The hardware uses an Intel IOMMU. See attachments for output of 
lspci, and details on iommu groups

I'm not sure if this problem originates from qemu, kvm, vfio or the GPU driver.
Do you have any hints how to debug this further?


Thanks,
David

PS: please cc me, I'm not subscribed.
# /sbin/lspci -nn
00:00.0 Host bridge [0600]: Intel Corporation 2nd Generation Core Processor 
Family DRAM Controller [8086:0100] (rev 09)
00:01.0 PCI bridge [0604]: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port [8086:0101] (rev 09)
00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core 
Processor Family Integrated Graphics Controller [8086:0102] (rev 09)
00:16.0 Communication controller [0780]: Intel Corporation 6 Series/C200 Series 
Chipset Family MEI Controller #1 [8086:1c3a] (rev 04)
00:16.2 IDE interface [0101]: Intel Corporation 6 Series/C200 Series Chipset 
Family IDE-r Controller [8086:1c3c] (rev 04)
00:16.3 Serial controller [0700]: Intel Corporation 6 Series/C200 Series 
Chipset Family KT Controller [8086:1c3d] (rev 04)
00:19.0 Ethernet controller [0200]: Intel Corporation 82579LM Gigabit Network 
Connection [8086:1502] (rev 04)
00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04)
00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset 
Family High Definition Audio Controller [8086:1c20] (rev 04)
00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 1 [8086:1c10] (rev b4)
00:1c.6 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 7 [8086:1c1c] (rev b4)
00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04)
00:1e.0 PCI bridge [0604]: Intel Corporation 82801 PCI Bridge [8086:244e] (rev 
a4)
00:1f.0 ISA bridge [0601]: Intel Corporation Q67 Express Chipset Family LPC 
Controller [8086:1c4e] (rev 04)
00:1f.2 SATA controller [0106]: Intel Corporation 6 Series/C200 Series Chipset 
Family SATA AHCI Controller [8086:1c02] (rev 04)
00:1f.3 SMBus [0c05]: Intel Corporation 6 Series/C200 Series Chipset Family 
SMBus Controller [8086:1c22] (rev 04)
03:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host 
Controller [1033:0194] (rev ff)




# lspci -vv
00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family 
DRAM Controller (rev 09)
Subsystem: Intel Corporation Device 2008
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- 

00:01.0 PCI bridge: Intel Corporation Xeon E3-1200/2nd Generation Core 
Processor Family PCI Express Root Port (rev 09) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- TAbort- Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [88] Subsystem: Intel Corporation Device 2008
Capabilities: [80] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Address: fee002d8  Data: 
Capabilities: [a0] Express (v2) Root Port (Slot+), MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, 
L1 <1us
ExtTag- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- 
Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 128 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- 
TransPend-
LnkCap: Port #2, Speed 5GT/s, Width x16, ASPM L0s L1, Latency 
L0 <1us, L1 <4us
ClockPM- Surprise- LLActRep- BwNot+
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
 

Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D

[Sachin Kamat]

On 1 February 2013 18:28, Inki Dae  wrote:
>
>
>
>
> 2013. 2. 1. 오후 8:52 Inki Dae  작성:
>
>>
>>
>>> -Original Message-
>>> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
>>> ow...@vger.kernel.org] On Behalf Of Sachin Kamat
>>> Sent: Friday, February 01, 2013 8:40 PM
>>> To: Inki Dae
>>> Cc: Sylwester Nawrocki; Kukjin Kim; Sylwester Nawrocki; linux-
>>> me...@vger.kernel.org; dri-devel@lists.freedesktop.org; devicetree-
>>> disc...@lists.ozlabs.org; patc...@linaro.org
>>> Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery
>>> support for G2D
>>>
>>> On 1 February 2013 17:02, Inki Dae  wrote:

 How about using like below?
Compatible = ""samsung,exynos4x12-fimg-2d" /* for Exynos4212,
 Exynos4412  */
 It looks odd to use "samsung,exynos4212-fimg-2d" saying that this ip is
>>> for
 exynos4212 and exynos4412.
>>>
>>> AFAIK, compatible strings are not supposed to have any wildcard
>> characters.
>>> Compatible string should suggest the first SoC that contained this IP.
>>> Hence IMO 4212 is OK.
>>>
>
> Oops, one more thing. AFAIK Exynos4210 also has fimg-2d ip. In this case, we 
> should use "samsung,exynos4210-fimg-2d" as comparible string and add it to 
> exynos4210.dtsi?

Exynos4210 has same g2d IP (v3.0) as C110 or V210; so the same
comptible string will be used for this one too.

> And please check if exynos4212 and 4412 SoCs have same fimg-2d ip. If it's 
> different, we might need to add ip version property or compatible string to 
> each dtsi file to identify the ip version.

AFAIK, they both have the same IP (v4.1).

>
> Sorry but give me your opinions.
>
> Thanks,
> Inki Dae
>
>
>>
>> Got it. Please post it again.
>>
>>>
>>> --
>>> With warm regards,
>>> Sachin
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
With warm regards,
Sachin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

status of [Patch] drm/ttm: fix fence locking in ttm_buffer_object_transfer, 2nd try ?

[Jerry Snitselaar]

What is the current status of this patch? I haven't seen it show up in
-next or linus's tree.

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

Re: [PATCH] drm/nouveau: add lockdep annotations

[Peter Hurley]

Hi Maarten

On Mon, 2013-02-04 at 22:59 +0100, Maarten Lankhorst wrote:
> Op 04-02-13 22:30, Marcin Slusarz schreef:
> > 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> > locked in arbitrary order, which is not true (at least in general case).
> > Tell it to distinguish subdevs by (o)class type.
> Apart from this specific case, is there any other reason why we require being 
> able to nest 2 subdev locks?
> 
> Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return 
> -EBUSY if there is any existing object. Problem solved, and lockdep is still 
> generic.
> 
> > 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> > client lock in a separate class.
> Can you copy paste a typical lockdep splat for this? Also this should be a 
> separate patch. :-)

PS - Deep call chain. Has anyone looked into max stack depth yet?

[5.995881] =
[5.995886] [ INFO: possible recursive locking detected ]
[5.995892] 3.8.0-next-20130125+ttypatch-xeon+lockdep #20130125+ttypatch Not 
tainted
[5.995898] -
[5.995904] modprobe/275 is trying to acquire lock:
[5.995909]  (&subdev->mutex){+.+.+.}, at: [] 
nouveau_instobj_create_+0x48/0x90 [nouveau]
[5.995955] 
[5.995955] but task is already holding lock:
[5.995961]  (&subdev->mutex){+.+.+.}, at: [] 
nv50_disp_data_ctor+0x65/0xd0 [nouveau]
[5.995989] 
[5.995989] other info that might help us debug this:
[5.995995]  Possible unsafe locking scenario:
[5.995995] 
[5.996001]CPU0
[5.996004]
[5.996005]   lock(&subdev->mutex);
[5.996005]   lock(&subdev->mutex);
[5.996005] 
[5.996005]  *** DEADLOCK ***
[5.996005] 
[5.996005]  May be due to missing lock nesting notation
[5.996005] 
[5.996005] 4 locks held by modprobe/275:
[5.996005]  #0:  (&__lockdep_no_validate__){..}, at: 
[] __driver_attach+0x5b/0xb0
[5.996005]  #1:  (&__lockdep_no_validate__){..}, at: 
[] __driver_attach+0x69/0xb0
[5.996005]  #2:  (drm_global_mutex){+.+.+.}, at: [] 
drm_get_pci_dev+0xc6/0x2d0 [drm]
[5.996005]  #3:  (&subdev->mutex){+.+.+.}, at: [] 
nv50_disp_data_ctor+0x65/0xd0 [nouveau]
[5.996005] 
[5.996005] stack backtrace:
[5.996005] Pid: 275, comm: modprobe Not tainted 
3.8.0-next-20130125+ttypatch-xeon+lockdep #20130125+ttypatch
[5.996005] Call Trace:
[5.996005]  [] __lock_acquire+0x687/0x1a70
[5.996005]  [] ? mark_held_locks+0x9b/0x100
[5.996005]  [] ? trace_hardirqs_on_caller+0x10d/0x1a0
[5.996005]  [] ? trace_hardirqs_on+0xd/0x10
[5.996005]  [] ? _raw_write_unlock_irqrestore+0x4a/0x90
[5.996005]  [] lock_acquire+0x98/0x150
[5.996005]  [] ? nouveau_instobj_create_+0x48/0x90 
[nouveau]
[5.996005]  [] ? nouveau_instobj_create_+0x48/0x90 
[nouveau]
[5.996005]  [] mutex_lock_nested+0x50/0x390
[5.996005]  [] ? nouveau_instobj_create_+0x48/0x90 
[nouveau]
[5.996005]  [] ? nouveau_object_ref+0x46/0xd0 [nouveau]
[5.996005]  [] ? nouveau_object_create_+0x65/0xa0 
[nouveau]
[5.996005]  [] nouveau_instobj_create_+0x48/0x90 [nouveau]
[5.996005]  [] nv50_instobj_ctor+0x51/0xf0 [nouveau]
[5.996005]  [] nouveau_object_ctor+0x38/0xc0 [nouveau]
[5.996005]  [] nv50_instmem_alloc+0x26/0x30 [nouveau]
[5.996005]  [] nouveau_gpuobj_create_+0x247/0x2f0 
[nouveau]
[5.996005]  [] ? _raw_spin_unlock_irqrestore+0x6d/0x90
[5.996005]  [] ? trace_hardirqs_on_caller+0x10d/0x1a0
[5.996005]  [] nouveau_engctx_create_+0x26c/0x2b0 
[nouveau]
[5.996005]  [] nv50_disp_data_ctor+0xc1/0xd0 [nouveau]
[5.996005]  [] nouveau_object_ctor+0x38/0xc0 [nouveau]
[5.996005]  [] nouveau_object_new+0x112/0x240 [nouveau]
[5.996005]  [] nv50_display_create+0x1a5/0x890 [nouveau]
[5.996005]  [] ? __cancel_work_timer+0x8b/0xe0
[5.996005]  [] nouveau_display_create+0x3cb/0x670 
[nouveau]
[5.996005]  [] nouveau_drm_load+0x26f/0x590 [nouveau]
[5.996005]  [] ? device_register+0x1e/0x30
[5.996005]  [] ? drm_sysfs_device_add+0x86/0xb0 [drm]
[5.996005]  [] drm_get_pci_dev+0x186/0x2d0 [drm]
[5.996005]  [] nouveau_drm_probe+0x26a/0x2c0 [nouveau]
[5.996005]  [] ? _raw_spin_unlock_irqrestore+0x4a/0x90
[5.996005]  [] local_pci_probe+0x4b/0x80
[5.996005]  [] pci_device_probe+0x111/0x120
[5.996005]  [] driver_probe_device+0x8b/0x3a0
[5.996005]  [] __driver_attach+0xab/0xb0
[5.996005]  [] ? driver_probe_device+0x3a0/0x3a0
[5.996005]  [] bus_for_each_dev+0x55/0x90
[5.996005]  [] driver_attach+0x1e/0x20
[5.996005]  [] bus_add_driver+0x121/0x2b0
[5.996005]  [] ? 0xa01acfff
[5.996005]  [] driver_register+0x77/0x170
[5.996005]  [] ? 0xa01acfff
[5.996005]  [] __pci_register_driver+0x64/0x70
[5.996005]  [] drm_pci_init+0x11a/0x130 [drm]
[5.996005]  [] ? 0xa01acfff
[5.996005]  [] ? 

[PATCH v4 0/1] Adds display-timing node parsing to exynos drm fimd

[Vikas Sajjan]

This patch adds display-timing node parsing to drm fimd, this depends on
the display helper patchset at
http://lists.freedesktop.org/archives/dri-devel/2013-January/033998.html

It also adds pinctrl support for drm fimd.

changes since v3:
- addressed comments from Sean Paul , to modify
the return values and print messages.

changes since v2:
- moved 'devm_pinctrl_get_select_default' function call under
'if (pdev->dev.of_node)', this makes NON-DT code unchanged.
(reported by: Rahul Sharma )

changes since v1:
- addressed comments from Sean Paul 

Vikas Sajjan (1):
  video: drm: exynos: Adds display-timing node parsing using video
helper function

 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   41 +++---
 1 file changed, 37 insertions(+), 4 deletions(-)

-- 
1.7.9.5

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

[PATCH v4 1/1] video: drm: exynos: Adds display-timing node parsing using video helper function

[Vikas Sajjan]

This patch adds display-timing node parsing using video helper function

Signed-off-by: Leela Krishna Amudala 
Signed-off-by: Vikas Sajjan 
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   41 +++---
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index bf0d9ba..978e866 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -905,16 +906,48 @@ static int __devinit fimd_probe(struct platform_device 
*pdev)
struct exynos_drm_subdrv *subdrv;
struct exynos_drm_fimd_pdata *pdata;
struct exynos_drm_panel_info *panel;
+   struct fb_videomode *fbmode;
+   struct pinctrl *pctrl;
struct resource *res;
int win;
int ret = -EINVAL;
 
DRM_DEBUG_KMS("%s\n", __FILE__);
 
-   pdata = pdev->dev.platform_data;
-   if (!pdata) {
-   dev_err(dev, "no platform data specified\n");
-   return -EINVAL;
+   if (pdev->dev.of_node) {
+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata) {
+   DRM_ERROR("memory allocation for pdata failed\n");
+   return -ENOMEM;
+   }
+
+   fbmode = devm_kzalloc(dev, sizeof(*fbmode), GFP_KERNEL);
+   if (!fbmode) {
+   DRM_ERROR("memory allocation for fbmode failed\n");
+   return -ENOMEM;
+   }
+
+   ret = of_get_fb_videomode(dev->of_node, fbmode, -1);
+   if (ret) {
+   DRM_ERROR("failed: of_get_fb_videomode() :"
+   "return value: %d\n", ret);
+   return ret;
+   }
+   pdata->panel.timing = (struct fb_videomode) *fbmode;
+
+   pctrl = devm_pinctrl_get_select_default(dev);
+   if (IS_ERR_OR_NULL(pctrl)) {
+   DRM_ERROR("failed: devm_pinctrl_get_select_default()"
+   "return value: %d\n", PTR_RET(pctrl));
+   return PTR_RET(pctrl);
+   }
+
+   } else {
+   pdata = pdev->dev.platform_data;
+   if (!pdata) {
+   DRM_ERROR("no platform data specified\n");
+   return -EINVAL;
+   }
}
 
panel = &pdata->panel;
-- 
1.7.9.5

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

[Linaro-mm-sig] [PATCH 6/7] reservation: cross-device reservation support

[Inki Dae]

> +/**
> + * ticket_commit - commit a reservation with a new fence
> + * @ticket:[in]the reservation_ticket returned by
> + * ticket_reserve
> + * @entries:   [in]a linked list of struct reservation_entry
> + * @fence: [in]the fence that indicates completion
> + *
> + * This function will call reservation_ticket_fini, no need
> + * to do it manually.
> + *
> + * This function should be called after a hardware command submission is
> + * completed succesfully. The fence is used to indicate completion of
> + * those commands.
> + */
> +void
> +ticket_commit(struct reservation_ticket *ticket,
> + struct list_head *entries, struct fence *fence)
> +{
> +   struct list_head *cur;
> +
> +   if (list_empty(entries))
> +   return;
> +
> +   if (WARN_ON(!fence)) {
> +   ticket_backoff(ticket, entries);
> +   return;
> +   }
> +
> +   list_for_each(cur, entries) {
> +   struct reservation_object *bo;
> +   bool shared;
> +
> +   reservation_entry_get(cur, &bo, &shared);
> +
> +   if (!shared) {
> +   int i;
> +   for (i = 0; i < bo->fence_shared_count; ++i) {
> +   fence_put(bo->fence_shared[i]);
> +   bo->fence_shared[i] = NULL;
> +   }
> +   bo->fence_shared_count = 0;
> +   if (bo->fence_excl)
> +   fence_put(bo->fence_excl);
> +
> +   bo->fence_excl = fence;
> +   } else {
> +   if (WARN_ON(bo->fence_shared_count >=
> +   ARRAY_SIZE(bo->fence_shared))) {
> +   mutex_unreserve_unlock(&bo->lock);
> +   continue;
> +   }
> +
> +   bo->fence_shared[bo->fence_shared_count++] = fence;
> +   }

Hi,

I got some questions to fence_excl and fence_shared. At the above
code, if bo->fence_excl is not NULL then it puts bo->fence_excl and
sets a new fence to it. This seems like that someone that committed a
new fence, wants to access the given dmabuf exclusively even if
someone is accessing the given dmabuf.

On the other hand, in case of fence_shared, someone wants to access
that dmabuf non-exclusively. So this case seems like that the given
dmabuf could be accessed by two more devices. So I guess that the
fence_excl could be used for write access(may need buffer sync like
blocking) and read access for the fence_shared(may not need buffer
sync). I'm not sure that I understand these two things correctly so
could you please give me more comments for them?

Thanks,
Inki Dae

> +   fence_get(fence);
> +
> +   mutex_unreserve_unlock(&bo->lock);
> +   }
> +   reservation_ticket_fini(ticket);
> +}
> +EXPORT_SYMBOL(ticket_commit);

[PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

[Thierry Reding]

yncpt.h and dev.h?

> +#define MAX_SYNCPT_LENGTH5

This doesn't seem to be used anywhere.

> +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
> + struct platform_device *pdev,
> + int client_managed);

Can't you move the actual implementation here? Also I'm not sure if
passing the platform_device is the best choice here. struct device
should work just as well.

> +/*
> + * Resets syncpoint and waitbase values to sw shadows
> + */
> +void host1x_syncpt_reset(struct host1x *dev)

Maybe host1x_syncpt_flush() would be a better name given the above
description? Reset does have this hardware reset connotation so my first
intuition had been that this would reset the syncpt value to 0.

If you decide to change the name, make sure to change it in the syncpt
ops as well.

> +/*
> + * Updates sw shadow state for client managed registers
> + */
> +void host1x_syncpt_save(struct host1x *dev)
> +{
> + struct host1x_syncpt *sp_base = dev->syncpt;
> + u32 i;
> +
> + for (i = 0; i < host1x_syncpt_nb_pts(dev); i++) {
> + if (host1x_syncpt_client_managed(sp_base + i))
> + dev->syncpt_op.load_min(sp_base + i);
> + else
> + WARN_ON(!host1x_syncpt_min_eq_max(sp_base + i));
> + }
> +
> + for (i = 0; i < host1x_syncpt_nb_bases(dev); i++)
> + dev->syncpt_op.read_wait_base(sp_base + i);
> +}

A similar comment applies here. Though I'm not so sure about a better
name. Perhaps host1x_syncpt_sync()?

I know that this must sound all pretty straightforward to you, but for
somebody who hasn't used these functions at all the names are quite
confusing. So instead of people to go read the documentation I tend to
think that making the names as descriptive as possible is essential
here.

> +/*
> + * Updates the last value read from hardware.
> + */
> +u32 host1x_syncpt_load_min(struct host1x_syncpt *sp)
> +{
> + u32 val;
> + val = sp->dev->syncpt_op.load_min(sp);
> + trace_host1x_syncpt_load_min(sp->id, val);
> +
> + return val;
> +}

I don't know I understand what this means exactly. Does it read the
value that hardware last incremented? Perhaps this will become clearer
when you add a comment to the syncpt_load_min() implementation.

> +int host1x_syncpt_init(struct host1x *host)
> +{
> + struct host1x_syncpt *syncpt, *sp;
> + int i;
> +
> + syncpt = sp = devm_kzalloc(&host->dev->dev,
> + sizeof(struct host1x_syncpt) * host->info.nb_pts,

You can make this a bit shorter by using sizeof(*sp) instead.

> + for (i = 0; i < host->info.nb_pts; ++i, ++sp) {
> + sp->id = i;
> + sp->dev = host;

Perhaps:

syncpt[i].id = i;
syncpt[i].dev = host;

To avoid the need to explicitly keep track of sp?

> +static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
> + struct platform_device *pdev,
> + int client_managed)
> +{
> + int i;
> + struct host1x_syncpt *sp = host->syncpt;
> + char *name;
> +
> + for (i = 0; i < host->info.nb_pts && sp->name; i++, sp++)
> + ;
> + if (sp->pdev)
> + return NULL;
> +
> + name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id,
> + pdev ? dev_name(&pdev->dev) : NULL);
> + if (!name)
> + return NULL;
> +
> + sp->pdev = pdev;
> + sp->name = name;
> + sp->client_managed = client_managed;
> +
> + return sp;
> +}
> +
> +struct host1x_syncpt *host1x_syncpt_alloc(struct platform_device *pdev,
> + int client_managed)
> +{
> + struct host1x *host = host1x_get_host(pdev);
> + return _host1x_syncpt_alloc(host, pdev, client_managed);
> +}

I think it's enough to keep track of the struct device here instead of
the struct platform_device.

Also the syncpoint is not actually allocated here, so maybe
host1x_syncpt_request() would be a better name. As a nice side-effect it
makes the naming more similar to the IRQ API and might be easier to work
with.

> +struct host1x_syncpt *host1x_syncpt_get(struct host1x *dev, u32 id)
> +{
> + return dev->syncpt + id;
> +}

Should this perhaps do some error checking. What if the specified syncpt
hasn't actually been requested before?

> diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
[...]
> +struct host1x_syncpt {
> + int id;
> + atomic_t min_val;
> + atomic_t max_val;
> + u32 base_val;
> + const char *name;
> + int client_managed;

Is this field actually ever used? Looking through the patches none of
the clients actually set this.

> +/*
> + * Returns true if syncpoint min == max, which means that there are no
> + * outstanding operations.
> + */
> +static inline bool host1x_syncpt_min_eq_max(struct host1x_syncpt *sp)
> +{
> + int min, max;
> + smp_rmb();
> + min = atomic_read(&sp->min_val);
> + max = atomic_read(&sp->max_val);
> + return (min == max);
> +}

Maybe call this host1x_syncpt_idle() or something similar instead?

> +{
> + return sp->id != NVSYNCPT_INVALID &&
> + sp->id < host1x_syncpt_nb_pts(sp->dev);
> +}

Is there really a need for NVSYNCPT_INVALID? Even if you want to keep
the special case you can drop the explicit check because -1 will be
larger than host1x_syncpt_nb_pts() anyway.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/84876489/attachment-0001.pgp>

[Bug 59899] Diagonal rendering artifacts on xbmc

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=59899

--- Comment #6 from aeriksson at fastmail.fm ---
Tried mesa git as of yesterday. No improvement.
I've also tried to flip all USE flags on/off for mesa 9.0.1 with no
improvement.

Doing some surfing, I noticed the z buffer issues being a common problem, so I
decided to try the car racing game "trigger". It does show the same effect as
I've seen in screenshots on youtube (flickering triangular misrenderings on
objects). The dynamic behavior seems to be the same. However, I see the same
effect on the fist screen in Trigger, where the three helmets are shown. I'm
not sure if they are supposed to be animated or static. The zbuffer issues I
saw in other reports were all related to movements (Stuff move back/front, when
I move a little). I also have zbuffer issues on the terrain once the game
starts.

If xbmc makes use of depth to paint its screen with a bunch of objects, I can
see how this might be z related. However, xbmc should show a static screen (I
think), whereas common z reports are all related to movement of the observer (I
think).

Any ideas how one could test this theory?

Another crazy thing is that every once in a while it renders perfectly ok.
While "bisecting" through the USEflags, I had a run of "good, good, good,..."
and each test was a restart of Xserver, starting xbmc. After going back to the
known bad set of USE flags (which now rendered good!!), I did some cursing and
warm rebooted the box, and it suddenly rendered bad again :-(. This kind of
suggests an uninitialized HW register to me...

Any ideas how one can make xbmc not exercise the (gl?) related areas of the HW?
I'm lacking a solid known-good state at the moment.

-- 
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/20130204/9591bb89/attachment.html>

[Linaro-mm-sig] [PATCH 6/7] reservation: cross-device reservation support

[Maarten Lankhorst]

Op 04-02-13 08:06, Inki Dae schreef:
>> +/**
>> + * ticket_commit - commit a reservation with a new fence
>> + * @ticket:[in]the reservation_ticket returned by
>> + * ticket_reserve
>> + * @entries:   [in]a linked list of struct reservation_entry
>> + * @fence: [in]the fence that indicates completion
>> + *
>> + * This function will call reservation_ticket_fini, no need
>> + * to do it manually.
>> + *
>> + * This function should be called after a hardware command submission is
>> + * completed succesfully. The fence is used to indicate completion of
>> + * those commands.
>> + */
>> +void
>> +ticket_commit(struct reservation_ticket *ticket,
>> + struct list_head *entries, struct fence *fence)
>> +{
>> +   struct list_head *cur;
>> +
>> +   if (list_empty(entries))
>> +   return;
>> +
>> +   if (WARN_ON(!fence)) {
>> +   ticket_backoff(ticket, entries);
>> +   return;
>> +   }
>> +
>> +   list_for_each(cur, entries) {
>> +   struct reservation_object *bo;
>> +   bool shared;
>> +
>> +   reservation_entry_get(cur, &bo, &shared);
>> +
>> +   if (!shared) {
>> +   int i;
>> +   for (i = 0; i < bo->fence_shared_count; ++i) {
>> +   fence_put(bo->fence_shared[i]);
>> +   bo->fence_shared[i] = NULL;
>> +   }
>> +   bo->fence_shared_count = 0;
>> +   if (bo->fence_excl)
>> +   fence_put(bo->fence_excl);
>> +
>> +   bo->fence_excl = fence;
>> +   } else {
>> +   if (WARN_ON(bo->fence_shared_count >=
>> +   ARRAY_SIZE(bo->fence_shared))) {
>> +   mutex_unreserve_unlock(&bo->lock);
>> +   continue;
>> +   }
>> +
>> +   bo->fence_shared[bo->fence_shared_count++] = fence;
>> +   }
> Hi,
>
> I got some questions to fence_excl and fence_shared. At the above
> code, if bo->fence_excl is not NULL then it puts bo->fence_excl and
> sets a new fence to it. This seems like that someone that committed a
> new fence, wants to access the given dmabuf exclusively even if
> someone is accessing the given dmabuf.
Yes, if there were shared fences, they had to issue a wait for the previous 
exclusive fence, so if you add
(possibly hardware) wait ops on those shared fences to your command stream, it 
follows that you also
waited for the previous exclusive fence implicitly.

If there is only an exclusive fence, you have to issue some explicit wait op  
on it before you start the rest
of the commands.
> On the other hand, in case of fence_shared, someone wants to access
> that dmabuf non-exclusively. So this case seems like that the given
> dmabuf could be accessed by two more devices. So I guess that the
> fence_excl could be used for write access(may need buffer sync like
> blocking) and read access for the fence_shared(may not need buffer
> sync). I'm not sure that I understand these two things correctly so
> could you please give me more comments for them?
Correct, if you create a shared fence, you still have to emit a wait op for the 
exclusive fence.
> Thanks,
> Inki Dae
>
>> +   fence_get(fence);
>> +
>> +   mutex_unreserve_unlock(&bo->lock);
>> +   }
>> +   reservation_ticket_fini(ticket);
>> +}
>> +EXPORT_SYMBOL(ticket_commit);

[RFC v2 0/5] Common Display Framework

[Marcus Lorentzon]

On 02/02/2013 12:35 AM, Laurent Pinchart wrote:
> Hi Marcus,
>
> On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:
>> On 01/08/2013 05:36 PM, Tomasz Figa wrote:
>>> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
[...]
 But it is not perfect. After a couple of products we realized that most
 panel drivers want an easy way to send a bunch of init commands in one
 go. So I think it should be an op for sending an array of commands at
 once. Something like

 struct dsi_cmd {
enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
u8 cmd;
int dataLen;
u8 *data;
 }

 struct dsi_ops {
int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
...
 }
> Do you have DSI IP(s) that can handle a list of commands ? Or would all DSI
> transmitter drivers need to iterate over the commands manually ? In the later
> case a lower-level API might be easier to implement in DSI transmitter
> drivers. Helper functions could provide the higher-level API you proposed.

The HW has a FIFO, so it can handle a few. Currently we use the low 
level type of call with one call per command. But we have found DSI 
command mode panels that don't accept any commands during the "update" 
(write start+continues). And so we must use a mutex/state machine to 
exclude any async calls to send DSI commands during update. But if you 
need to send more than one command per frame this will be hard (like 
CABC and backlight commands). It will be a ping pong between update and 
command calls. One option is to expose the mutex to the caller so it can 
make many calls before the next update grabs the mutex again.
So maybe we could create a helper that handle the op for list of 
commands and another op for single command that you actually have to 
implement.
>>> Yes, this should be flexible enough to cover most of (or even whole) DSI
>>> specification.
>>>
>>> However I'm not sure whether the dsi_write name would be appropriate,
>>> since DSI packet types include also read and special transactions. So,
>>> according to DSI terminology, maybe dsi_transaction would be better?
> Or dsi_transfer or dsi_xfer ? Does the DSI bus have a concept of transactions
> ?

No transactions. And I don't want to mix reads and writes. It should be 
similar to I2C and other stream control busses. So one read and one 
write should be fine. And then a bunch of helpers on top for callers to 
use, like one per major DSI packet type.
>> I think read should still be separate. At least on my HW read and write
>> are quite different. But all "transactions" are very much the same in HW
>> setup. The ... was dsi_write etc ;) Like set_max_packet_size should
>> maybe be an ops. Since only the implementer of the "video source" will
>> know what the max read return packet size for that DSI IP is. The panels
>> don't know that. Maybe another ops to retrieve some info about the caps
>> of the video source would help that. Then a helper could call that and
>> then the dsi_write one.
> If panels (or helper functions) need information about the DSI transmitter
> capabilities, sure, we can add an op.

Yes, a "video source" op for getting caps would be ok too. But so far 
the only limits I have found is the read/write sizes. But if anyone else 
has other limits, please list them so we could add them to this struct 
dsi_host_caps.
 And I think I still prefer the dsi_bus in favor of the abstract video
 source. It just looks like a home made bus with bus-ops ... can't you do
 something similar using the normal driver framework? enable/disable
 looks like suspend/resume, register/unregister_vid_src is like
 bus_(un)register_device, ... the video source anyway seems unattached
 to the panel stuff with the find_video_source call.
> The Linux driver framework is based on control busses. DSI usually handles
> both control and video transfer, but the control and data busses can also be
> separate (think DPI + SPI/I2C for instance). In that case the panel will be a
> child of its control bus master, and will need a separate interface to access
> video data operations. As a separate video interface is thus needed, I think
> we should use it for DSI as well.
>
> My initial proposal included a DBI bus (as I don't have any DSI hardware - DBI
> and DSI can be used interchangeably in this discussions, they both share the
> caracteristic of having a common control + data bus), and panels were children
> of the DBI bus master. The DBI bus API was only used for control, not for data
> transfers. Tomi then removed the DBI bus and moved the control operations to
> the video source, turning the DBI panels into platform devices. I still favor
> my initial approach, but I can agree to drop the DBI bus if there's a
> consensus on that. Video bus operations will be separate in any case.

As discussed at FOSDEM I will give DSI bus with full feature set a try

[Bug 60224] Radeon hd6770 graphics artifacts in team fortress 2 with 3.8 kernel

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60224

--- Comment #1 from Michel D?nzer  ---
Could be related to bug 60236. If that's not it, can you bisect which kernel
change introduced the problem?

-- 
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/20130204/50e42b04/attachment.html>

[PATCHv5,RESEND 2/8] gpu: host1x: Add syncpoint wait and interrupts

[Thierry Reding]

or
statement. Instead you could either index intr->syncpt directly or
obtain a reference within the loop. It allows the for statement to be
written much more canonically.

> diff --git a/drivers/gpu/host1x/intr.h b/drivers/gpu/host1x/intr.h
[...]
> +#define intr_syncpt_to_intr(is) (is->intr)

This one doesn't buy you anything. It actually uses up more characters
so you can just drop it.

> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
[...]
> @@ -119,6 +122,166 @@ void host1x_syncpt_incr(struct host1x_syncpt *sp)
>   host1x_syncpt_cpu_incr(sp);
>  }
>  
> +/*
> + * Updated sync point form hardware, and returns true if syncpoint is 
> expired,
> + * false if we may need to wait
> + */
> +static bool syncpt_load_min_is_expired(
> + struct host1x_syncpt *sp,
> + u32 thresh)

This can all go on one line.

> +/*
> + * Main entrypoint for syncpoint value waits.
> + */
> +int host1x_syncpt_wait(struct host1x_syncpt *sp,
> + u32 thresh, long timeout, u32 *value)
> +{
[...]
> +}
> +EXPORT_SYMBOL(host1x_syncpt_wait);

This doesn't only seem to be the main entrypoint, but it's basically the
only way to currently wait for syncpoints. One actual use-case where
this might turn out to be a problem is video capturing. The problem is
that using this API you can't very well asynchronously capture frames.
So eventually I think we need a way to allow a generic handler to be
attached to syncpoints so that you can have this handler continuously
invoked after each frame is captured and just pass the buffer back to
userspace.

> +bool host1x_syncpt_is_expired(
> + struct host1x_syncpt *sp,
> + u32 thresh)

This can go on one line.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/f6a65b77/attachment-0001.pgp>

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #4 from Michel D?nzer  ---
The attached log ends abruptly. Is there more information in the kdm log file?

-- 
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/20130204/558a621f/attachment.html>

[PATCHv5,RESEND 4/8] gpu: host1x: Add debug support

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:00PM +0200, Terje Bergstrom wrote:
> diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c
[...]
> +static pid_t host1x_debug_null_kickoff_pid;
> +unsigned int host1x_debug_trace_cmdbuf;
> +
> +static pid_t host1x_debug_force_timeout_pid;
> +static u32 host1x_debug_force_timeout_val;
> +static u32 host1x_debug_force_timeout_channel;

Please group static and non-static variables.

> diff --git a/drivers/gpu/host1x/debug.h b/drivers/gpu/host1x/debug.h
[...]
> +struct output {
> + void (*fn)(void *ctx, const char *str, size_t len);
> + void *ctx;
> + char buf[256];
> +};

Do we really need this kind of abstraction? There really should be only
one location where debug information is obtained, so I don't see a need
for this.

> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
>  struct host1x_syncpt_ops {
>   void (*reset)(struct host1x_syncpt *);
>   void (*reset_wait_base)(struct host1x_syncpt *);
> @@ -117,6 +133,7 @@ struct host1x {
>   struct host1x_channel_ops channel_op;
>   struct host1x_cdma_ops cdma_op;
>   struct host1x_pushbuffer_ops cdma_pb_op;
> + struct host1x_debug_ops debug_op;
>   struct host1x_syncpt_ops syncpt_op;
>   struct host1x_intr_ops intr_op;

Again, better to pass in a const pointer to the ops structure.

> diff --git a/drivers/gpu/host1x/hw/debug_hw.c 
> b/drivers/gpu/host1x/hw/debug_hw.c

> +static int show_channel_command(struct output *o, u32 addr, u32 val, int 
> *count)
> +{
> + unsigned mask;
> + unsigned subop;
> +
> + switch (val >> 28) {
> + case 0x0:

These can easily be derived by looking at the debug output, but it may
still make sense to assign symbolic names to them.

> +static void show_channel_word(struct output *o, int *state, int *count,
> + u32 addr, u32 val, struct host1x_cdma *cdma)
> +{
> + static int start_count, dont_print;

What if two processes read debug information at the same time?

> +static void do_show_channel_gather(struct output *o,
> + phys_addr_t phys_addr,
> + u32 words, struct host1x_cdma *cdma,
> + phys_addr_t pin_addr, u32 *map_addr)
> +{
> + /* Map dmaget cursor to corresponding mem handle */
> + u32 offset;
> + int state, count, i;
> +
> + offset = phys_addr - pin_addr;
> + /*
> +  * Sometimes we're given different hardware address to the same
> +  * page - in these cases the offset will get an invalid number and
> +  * we just have to bail out.
> +  */

Why's that?

> + map_addr = host1x_memmgr_mmap(mem);
> + if (!map_addr) {
> + host1x_debug_output(o, "[could not mmap]\n");
> + return;
> + }
> +
> + /* Get base address from mem */
> + sgt = host1x_memmgr_pin(mem);
> + if (IS_ERR(sgt)) {
> + host1x_debug_output(o, "[couldn't pin]\n");
> + host1x_memmgr_munmap(mem, map_addr);
> + return;
> + }

Maybe you should stick with one of "could not" or "couldn't". Makes it
easier to search for.

> +static void show_channel_gathers(struct output *o, struct host1x_cdma *cdma)
> +{
> + struct host1x_job *job;
> +
> + list_for_each_entry(job, &cdma->sync_queue, list) {
> + int i;
> + host1x_debug_output(o,
> + "\n%p: JOB, syncpt_id=%d, syncpt_val=%d,"
> + " first_get=%08x, timeout=%d"
> + " num_slots=%d, num_handles=%d\n",
> + job,
> + job->syncpt_id,
> + job->syncpt_end,
> + job->first_get,
> + job->timeout,
> + job->num_slots,
> + job->num_unpins);

This could go on fewer lines.

> +static void host1x_debug_show_channel_cdma(struct host1x *m,
> + struct host1x_channel *ch, struct output *o, int chid)
> +{
[...]
> + switch (cbstat) {
> + case 0x00010008:

Again, symbolic names would be nice.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/24971c1f/attachment.pgp>

[PATCHv5,RESEND 5/8] drm: tegra: Move drm to live under host1x

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:01PM +0200, Terje Bergstrom wrote:
[...]
> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
> index 697d49a..ffc8bf1 100644
> --- a/drivers/gpu/host1x/Makefile
> +++ b/drivers/gpu/host1x/Makefile
> @@ -12,4 +12,10 @@ host1x-y = \
>   hw/host1x01.o
>  
>  host1x-$(CONFIG_TEGRA_HOST1X_CMA) += cma.o
> +
> +ccflags-y += -Iinclude/drm
> +ccflags-$(CONFIG_DRM_TEGRA_DEBUG) += -DDEBUG
> +
> +host1x-$(CONFIG_DRM_TEGRA) += drm/drm.o drm/fb.o drm/dc.o drm/host1x.o
> +host1x-$(CONFIG_DRM_TEGRA) += drm/output.o drm/rgb.o drm/hdmi.o
>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o

Can this be moved into a separate Makefile in the drm subdirectory?

> diff --git a/drivers/gpu/host1x/host1x_client.h 
> b/drivers/gpu/host1x/host1x_client.h
[...]
> new file mode 100644
> index 000..fdd2920
> --- /dev/null
> +++ b/drivers/gpu/host1x/host1x_client.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HOST1X_CLIENT_H
> +#define HOST1X_CLIENT_H
> +
> +struct platform_device;
> +
> +void host1x_set_drm_data(struct platform_device *pdev, void *data);
> +void *host1x_get_drm_data(struct platform_device *pdev);
> +
> +#endif

These aren't defined or used yet.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/3e52fe2a/attachment.pgp>

[Bug 60254] kernel Oops when provoking GPU lock.

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60254

--- Comment #2 from Andy Furniss  ---
(In reply to comment #1)
> (In reply to comment #0)
> > The GPU lock with Rv670 and openarena is nothing new - it seems to have been
> > a feature for almost a year (I haven't used rv670 for most of that time).
> > 
> > On noticing the new gpu reset code in drm-next-3.9-wip I decided to provoke
> > it on my AGP box and got -
> 
> Hmm I just managed to get the same running drm-fixes so it's not wip maybe
> it's because I am now using llvm. In the (no llvm) past with other kernels
> this GPU lock normally went quite well - the game often just continued for a
> while, until it hit another one.

It's nothing to do with llvm seems like it's a feature of more recent kernels.

-- 
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/20130204/a43d8c4e/attachment.html>

[PATCHv5,RESEND 6/8] gpu: host1x: Remove second host1x driver

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:02PM +0200, Terje Bergstrom wrote:
[...]
> +void host1x_set_drm_data(struct platform_device *pdev, void *data)
> +{
> + struct host1x *host1x = platform_get_drvdata(pdev);
> + host1x->drm_data = data;
> +}
> +
> +void *host1x_get_drm_data(struct platform_device *pdev)
> +{
> + struct host1x *host1x = platform_get_drvdata(pdev);
> + return host1x->drm_data;
> +}

Passing around struct device * should be enough and avoids the need for
the explicit cast to struct platform_device.

It is a bit unfortunate that we have now have two structures called
host1x, but I think we can live with it for now. We can clean that up
once the code has been merged.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/2d9561b5/attachment.pgp>

[PATCHv5,RESEND 7/8] ARM: tegra: Add board data and 2D clocks

[Thierry Reding]

On Tue, Jan 15, 2013 at 01:44:03PM +0200, Terje Bergstrom wrote:
> Add a driver alias gr2d for Tegra 2D device, and assign a duplicate
> of 2D clock to that driver alias.
> 
> Signed-off-by: Terje Bergstrom 
> ---
>  arch/arm/mach-tegra/board-dt-tegra20.c|1 +
>  arch/arm/mach-tegra/board-dt-tegra30.c|1 +
>  arch/arm/mach-tegra/tegra20_clocks_data.c |2 +-
>  arch/arm/mach-tegra/tegra30_clocks_data.c |1 +
>  4 files changed, 4 insertions(+), 1 deletion(-)

With Prashant's clock rework patches now merged this patch can be
dropped.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/335443cc/attachment-0001.pgp>

[Bug 60224] Radeon hd6770 graphics artifacts in team fortress 2 with 3.8 kernel

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60224

--- Comment #2 from Iaroslav  ---
I know that the problem appeared after upgrading to 3.8-rc1-drm-next, and
continues to exist in the 3.8-rc5-vanilla.
async dma is a likely cause.

-- 
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/20130204/8eb7631a/attachment.html>

[PATCHv5,RESEND 8/8] drm: tegra: Add gr2d device

[Thierry Reding]

x_syncpt_free(gr2d->syncpt);
> + return 0;
> +}

Isn't this missing a host1x_channel_put() or host1x_free_channel()?

> diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
[...]
> +struct tegra_gem_create {
> + __u64 size;
> + unsigned int flags;
> + unsigned int handle;
> + unsigned int offset;
> +};

I think it's better to consistently use the explicitly sized types here.

> +struct tegra_gem_invalidate {
> + unsigned int handle;
> +};
> +
> +struct tegra_gem_flush {
> + unsigned int handle;
> +};

Where are these used?

> +struct tegra_drm_syncpt_wait_args {
> + __u32 id;
> + __u32 thresh;
> + __s32 timeout;
> + __u32 value;
> +};
> +
> +#define DRM_TEGRA_NO_TIMEOUT (-1)

Is this the only reason why timeout is signed? If so maybe a better
choice would be __u32 and DRM_TEGRA_NO_TIMEOUT 0x.

> +struct tegra_drm_get_channel_param_args {
> + __u64 context;
> + __u32 param;
> + __u32 value;
> +};

What's the reason for not calling this tegra_drm_get_syncpoint?

> +struct tegra_drm_syncpt_incr {
> + __u32 syncpt_id;
> + __u32 syncpt_incrs;
> +};

Maybe the fields would be better named id and incrs. Though I also
notice that incrs is never used. I guess that's supposed to be used in
the future to allow increments by more than a single value. If so,
perhaps value would be a better name.

Now on to the dreaded patch 3...

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130204/d8ac799a/attachment.pgp>

[Linaro-mm-sig] [PATCH 6/7] reservation: cross-device reservation support

[Daniel Vetter]

On Mon, Feb 04, 2013 at 10:57:22AM +0100, Maarten Lankhorst wrote:
> Op 04-02-13 08:06, Inki Dae schreef:
> >> +/**
> >> + * ticket_commit - commit a reservation with a new fence
> >> + * @ticket:[in]the reservation_ticket returned by
> >> + * ticket_reserve
> >> + * @entries:   [in]a linked list of struct reservation_entry
> >> + * @fence: [in]the fence that indicates completion
> >> + *
> >> + * This function will call reservation_ticket_fini, no need
> >> + * to do it manually.
> >> + *
> >> + * This function should be called after a hardware command submission is
> >> + * completed succesfully. The fence is used to indicate completion of
> >> + * those commands.
> >> + */
> >> +void
> >> +ticket_commit(struct reservation_ticket *ticket,
> >> + struct list_head *entries, struct fence *fence)
> >> +{
> >> +   struct list_head *cur;
> >> +
> >> +   if (list_empty(entries))
> >> +   return;
> >> +
> >> +   if (WARN_ON(!fence)) {
> >> +   ticket_backoff(ticket, entries);
> >> +   return;
> >> +   }
> >> +
> >> +   list_for_each(cur, entries) {
> >> +   struct reservation_object *bo;
> >> +   bool shared;
> >> +
> >> +   reservation_entry_get(cur, &bo, &shared);
> >> +
> >> +   if (!shared) {
> >> +   int i;
> >> +   for (i = 0; i < bo->fence_shared_count; ++i) {
> >> +   fence_put(bo->fence_shared[i]);
> >> +   bo->fence_shared[i] = NULL;
> >> +   }
> >> +   bo->fence_shared_count = 0;
> >> +   if (bo->fence_excl)
> >> +   fence_put(bo->fence_excl);
> >> +
> >> +   bo->fence_excl = fence;
> >> +   } else {
> >> +   if (WARN_ON(bo->fence_shared_count >=
> >> +   ARRAY_SIZE(bo->fence_shared))) {
> >> +   mutex_unreserve_unlock(&bo->lock);
> >> +   continue;
> >> +   }
> >> +
> >> +   bo->fence_shared[bo->fence_shared_count++] = fence;
> >> +   }
> > Hi,
> >
> > I got some questions to fence_excl and fence_shared. At the above
> > code, if bo->fence_excl is not NULL then it puts bo->fence_excl and
> > sets a new fence to it. This seems like that someone that committed a
> > new fence, wants to access the given dmabuf exclusively even if
> > someone is accessing the given dmabuf.
> Yes, if there were shared fences, they had to issue a wait for the previous 
> exclusive fence, so if you add
> (possibly hardware) wait ops on those shared fences to your command stream, 
> it follows that you also
> waited for the previous exclusive fence implicitly.
> 
> If there is only an exclusive fence, you have to issue some explicit wait op  
> on it before you start the rest
> of the commands.
> > On the other hand, in case of fence_shared, someone wants to access
> > that dmabuf non-exclusively. So this case seems like that the given
> > dmabuf could be accessed by two more devices. So I guess that the
> > fence_excl could be used for write access(may need buffer sync like
> > blocking) and read access for the fence_shared(may not need buffer
> > sync). I'm not sure that I understand these two things correctly so
> > could you please give me more comments for them?
> Correct, if you create a shared fence, you still have to emit a wait op for 
> the exclusive fence.

Just a quick note: We limit the number of non-exclusive fences to avoid
reallocating memory, which would be a bit a pain. Otoh if we support some
form of fence timeline concept, the fence core code could overwrite
redundant fences. Which would reasonable limit the total attached fence
count. Still we'd need to thread potential -ENOMEM failures through all
callers.

Do you see a use-case for more than just a few non-exclusive fences?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 17/62] drm/i915: convert to idr_alloc()

[Daniel Vetter]

On Sat, Feb 02, 2013 at 05:20:18PM -0800, Tejun Heo wrote:
> Convert to the much saner new idr interface.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: dri-devel at lists.freedesktop.org
> ---
> This patch depends on an earlier idr changes and I think it would be
> best to route these together through -mm.  Please holler if there's
> any objection.  Thanks.

Indeed, this looks nice.

Acked-by: Daniel Vetter 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/2] drm: Use C8 instead of RGB332 when determining the format from depth/bpp

[Daniel Vetter]

On Thu, Jan 31, 2013 at 07:43:38PM +0200, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> Support for real RGB332 is a rarity, most hardware only really support
> C8. So use C8 instead of RGB332 when determining the format based on
> depth/bpp.
> 
> This fixes 8bpp fbcon on i915, since i915 will only accept C8 and not
> RGB332.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59572
> Signed-off-by: Ville Syrj?l? 

Tested-by: mlsemon35 at gmail.com

Dave, can you please consider including these two patches into -fixes? The
fix a black screen regression when users opt for 8bpp console ...
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ff7344c..826a5ca 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2253,7 +2253,7 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, 
> uint32_t depth)
>  
>   switch (bpp) {
>   case 8:
> - fmt = DRM_FORMAT_RGB332;
> + fmt = DRM_FORMAT_C8;
>   break;
>   case 16:
>   if (depth == 15)
> -- 
> 1.7.12.4
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined reference to `acpi_video_get_edid'

[Borislav Petkov]

Hi,

I'm guessing someone has already triggered this on latest Linus' tree
and has a fix?

drivers/built-in.o: In function `nouveau_acpi_edid':
/w/kernel/linux/drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined reference 
to `acpi_video_get_edid'
make: *** [vmlinux] Error 1

Btw, I got CONFIG_ACPI_VIDEO=m while CONFIG_DRM_NOUVEAU=y and this is
probably the reason for the vmlinux link error.

Thanks.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] drm/nouveau: always select ACPI_VIDEO if ACPI is enabled.

[Maarten Lankhorst]

Hey,

Op 04-02-13 16:23, Borislav Petkov schreef:
> Hi,
>
> I'm guessing someone has already triggered this on latest Linus' tree
> and has a fix?
>
> drivers/built-in.o: In function `nouveau_acpi_edid':
> /w/kernel/linux/drivers/gpu/drm/nouveau/nouveau_acpi.c:420: undefined 
> reference to `acpi_video_get_edid'
> make: *** [vmlinux] Error 1
>
> Btw, I got CONFIG_ACPI_VIDEO=m while CONFIG_DRM_NOUVEAU=y and this is
> probably the reason for the vmlinux link error.
>
> Thanks.
>
Does this fix things?

-->8
Having nouveau builtin would still allow ACPI_VIDEO to be used as external 
module if some of the deps for acpi_video
have not been met, which would result in a linking failure. Solve this by only 
requiring ACPI && X86 to select ACPI_VIDEO.

Signed-off-by: Maarten Lankhorst 

---
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 8a55bee..f08b9b6 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -10,7 +10,7 @@ config DRM_NOUVEAU
select FB
select FRAMEBUFFER_CONSOLE if !EXPERT
select FB_BACKLIGHT if DRM_NOUVEAU_BACKLIGHT
-   select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && 
VIDEO_OUTPUT_CONTROL && INPUT
+   select ACPI_VIDEO if ACPI && X86
select ACPI_WMI if ACPI
select MXM_WMI if ACPI
select POWER_SUPPLY

DMAR faults from unrelated device when vfio is used

[Alex Williamson]

On Mon, 2013-02-04 at 11:10 +0100, David Gstir wrote:
> Hi!
> 
> I get the following error messages over and over again when using vfio
> in qemu-kvm:
> 
> [ 1692.021403] dmar: DMAR:[DMA Read] Request device [00:02.0] fault addr 
> 1a45aa9000 
> [ 1692.021403] DMAR:[fault reason 12] non-zero reserved fields in PTE
> [ 1692.021416] dmar: DRHD: handling fault status reg 2
> 
> This pci device is the graphics card, which I did not assign to qemu!
> I did assign the following devices:
> 00:1a.0, 00:1b.0, 00:1c.0, 00:1c.6, 00:1d.0, 03:00.0.

Piecing together your logs:

iommu_group 5
00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04)
iommu_group 6
00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset 
Family High Definition Audio Controller [8086:1c20] (rev 04)
iommu_group 7
00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 1 [8086:1c10] (rev b4)
00:1c.6 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset 
Family PCI Express Root Port 7 [8086:1c1c] (rev b4)
03:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host 
Controller [1033:0194] (rev ff)
iommu_group 8
00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset 
Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04)

Can you clarify what you mean by assign?  Are you actually assigning the
root ports to the qemu guest (1c.0 & 1c.6)?  vfio will require they be
owned by vfio-pci to make use of 3:00.0, but assigning them to the guest
is not recommended.  Can you provided your qemu command line?  We need
to re-visit how to handle pcieport devices with vfio-pci, perhaps
white-listing it as a vfio "compatible" driver, but this still should
not interfere with devices external to the group.

The DMAR fault address looks pretty bogus unless you happen to have
100GB+ of ram in the system.

> The error occurs at random and is not reproducible every time. It
> happens about every third reboot. 
> I'm running qemu-kvm 1.3.0 (kvm-1.3.0-187.3), kernel 3.8.0-rc5 and
> windows 7 as guest OS. The hardware uses an Intel IOMMU. See
> attachments for output of lspci, and details on iommu groups
> 
> I'm not sure if this problem originates from qemu, kvm, vfio or the
> GPU driver.
> Do you have any hints how to debug this further?

vfio makes use of the IOMMU API for programming DMA translations, so an
reserved fields would have to be programmed by intel-iommu itself.  We
could of course be passing some kind of bogus data that intel-iommu
isn't catching.  If you're assigning the root ports to the guest, I'd
start with that, don't do it.  Attach them to vfio, but don't give them
to the guest.  Maybe that'll give us a hint.  I also notice that your
USB 3 controller is dead:

03:00.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host Controller (rev 
ff) (prog-if ff)
!!! Unknown header type 7f

We only see unknown header type 7f when the read from the device returns
-1.  This might have something to do with the root port above it (1c.6)
being in state D3.  Windows likes to put unused devices in D3, which
leads me to suspect you are giving it to the guest.  Let's see what
happens without that.  Thanks,

Alex

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #5 from runetmember at gmail.com ---
> Is there more information in the kdm log file?
I use LightDM-KDE. I'll look into LightDM log next time.

-- 
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/20130204/8068779d/attachment.html>

Radeon RV100 init fails with specific card+mobo - IRQ problem

[Meelis Roos]

> >> > Adding some printks reveals it calls
> >> > r100_init() -> radeon_irq_kms_init() -> drm_irq_install() ->
> >> > drm_dev_to_irq() and that fails. So no IRQ, no KMS.
> >> >
> >> > lspci does not show Interrupt like on some other PCI devices.
> >> > /proc/interrupts obviously does not contain the irq since we do not
> >> > register any yet.
> >> >
> >> > There is no BIOS option of "Enable IRQ for VGA" or similar (BIOS is
> >> > latest). Could this be the problem?
> >> >
> >> > What can I do to get KMS working on this computer?
> >> >
> >> > --
> >> > Meelis Roos (mroos at linux.ee)
> >>
> >> Try booting with radeon.agp_mode=-1
> >
> > radeon: `-1' invalid for parameter `agp_mode'
> 
> radeon.agpmode=-1
> 
> However, I think there are a number of places where we require
> interrupts that would need to be worked around (fences, display
> hotplug, pageflipping) if not interrupt is available.

I did dome more testing with different cards an got a surprise. The 
machine works fine with other AGP cards - one MX440 with nouvea, another 
RV100 QY and a R200. They do have interrupt in lspci -vvv output.

Why does this specific card not have its IRQ routed - card ROM problem?

-- 
Meelis Roos (mroos at linux.ee)

[PATCH] drm/ttm: avoid allocation memory while spinlock is held

[j.gli...@gmail.com]

From: Jerome Glisse 

We need to take reference on the sync object while holding the
fence spinlock but at the same time we don't want to allocate
memory while holding the spinlock. This patch make sure we
enforce both of this constraint.

Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296

Signed-off-by: Jerome Glisse 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |   16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 44420fc..77799a5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -413,6 +413,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  * @bo: A pointer to a struct ttm_buffer_object.
  * @new_obj: A pointer to a pointer to a newly created ttm_buffer_object,
  * holding the data of @bo with the old placement.
+ * @sync_obj: the sync object caller is responsible to take a reference on
+ * behalf of this function
  *
  * This is a utility function that may be called after an accelerated move
  * has been scheduled. A new buffer object is created as a placeholder for
@@ -423,7 +425,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  */

 static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
- struct ttm_buffer_object **new_obj)
+ struct ttm_buffer_object **new_obj,
+ void sync_obj)
 {
struct ttm_buffer_object *fbo;
struct ttm_bo_device *bdev = bo->bdev;
@@ -448,7 +451,8 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
fbo->vm_node = NULL;
atomic_set(&fbo->cpu_writers, 0);

-   fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
+   /* reference on sync obj is taken by the caller of this function */
+   fbo->sync_obj = sync_obj;
kref_init(&fbo->list_kref);
kref_init(&fbo->kref);
fbo->destroy = &ttm_transfered_destroy;
@@ -652,6 +656,8 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
}
ttm_bo_free_old_node(bo);
} else {
+   void *sync_obj;
+
/**
 * This should help pipeline ordinary buffer moves.
 *
@@ -662,12 +668,14 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object 
*bo,

set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);

-   /* ttm_buffer_object_transfer accesses bo->sync_obj */
-   ret = ttm_buffer_object_transfer(bo, &ghost_obj);
+   /* take the ref on the sync object before releasing the 
spinlock */
+   sync_obj = driver->sync_obj_ref(bo->sync_obj);
spin_unlock(&bdev->fence_lock);
+
if (tmp_obj)
driver->sync_obj_unref(&tmp_obj);

+   ret = ttm_buffer_object_transfer(bo, &ghost_obj, sync_obj);
if (ret)
return ret;

-- 
1.7.10.4

Radeon RV100 init fails with specific card+mobo - IRQ problem

[Alex Deucher]

On Mon, Feb 4, 2013 at 11:32 AM, Meelis Roos  wrote:
>> >> > Adding some printks reveals it calls
>> >> > r100_init() -> radeon_irq_kms_init() -> drm_irq_install() ->
>> >> > drm_dev_to_irq() and that fails. So no IRQ, no KMS.
>> >> >
>> >> > lspci does not show Interrupt like on some other PCI devices.
>> >> > /proc/interrupts obviously does not contain the irq since we do not
>> >> > register any yet.
>> >> >
>> >> > There is no BIOS option of "Enable IRQ for VGA" or similar (BIOS is
>> >> > latest). Could this be the problem?
>> >> >
>> >> > What can I do to get KMS working on this computer?
>> >> >
>> >> > --
>> >> > Meelis Roos (mroos at linux.ee)
>> >>
>> >> Try booting with radeon.agp_mode=-1
>> >
>> > radeon: `-1' invalid for parameter `agp_mode'
>>
>> radeon.agpmode=-1
>>
>> However, I think there are a number of places where we require
>> interrupts that would need to be worked around (fences, display
>> hotplug, pageflipping) if not interrupt is available.
>
> I did dome more testing with different cards an got a surprise. The
> machine works fine with other AGP cards - one MX440 with nouvea, another
> RV100 QY and a R200. They do have interrupt in lspci -vvv output.
>
> Why does this specific card not have its IRQ routed - card ROM problem?

For some reason the system bios on your box doesn't assign it one.
The video bios isn't really involved.  I'm not sure if there is a way
to force assign an irq in Linux or not.

Alex

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #6 from Ilija Hadzic  ---
Created attachment 74183
  --> https://bugs.freedesktop.org/attachment.cgi?id=74183&action=edit
Possible "Band-Aid" to get you going.

-- 
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/20130204/3c7895bd/attachment.html>

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #7 from Ilija Hadzic  ---
I have just posted a patch that may help at least prevent the crash. I have
seen this bug on my end too, but I though that I locally introduced it with my
own hacks. The patch does not solve the root cause (i.e. buffer double-free),
but if it prevents the crash, maybe we can apply it to get people going until
we find the real solution.

-- 
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/20130204/309e0f05/attachment.html>

[PATCHv5,RESEND 7/8] ARM: tegra: Add board data and 2D clocks

[Stephen Warren]

On 02/04/2013 04:26 AM, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:44:03PM +0200, Terje Bergstrom wrote:
>> Add a driver alias gr2d for Tegra 2D device, and assign a
>> duplicate of 2D clock to that driver alias.
>> 
>> Signed-off-by: Terje Bergstrom  --- 
>> arch/arm/mach-tegra/board-dt-tegra20.c|1 + 
>> arch/arm/mach-tegra/board-dt-tegra30.c|1 + 
>> arch/arm/mach-tegra/tegra20_clocks_data.c |2 +- 
>> arch/arm/mach-tegra/tegra30_clocks_data.c |1 + 4 files
>> changed, 4 insertions(+), 1 deletion(-)
> 
> With Prashant's clock rework patches now merged this patch can be 
> dropped.

Assuming this series is applied for 3.10 and not earlier, yes. I'd
certainly recommend applying for 3.10 not 3.9; the dependencies to
apply this for 3.9 given the AUXDATA/... requirements would be too
painful.

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #8 from runetmember at gmail.com ---
So since you find root case, LightDM log is not necessary now, I am correct?

-- 
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/20130204/b50019a8/attachment.html>

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #9 from Ilija Hadzic  ---
(In reply to comment #8)
> So since you find root case, LightDM log is not necessary now, I am correct?

No, I didn't find the root cause. I am only assuming (based on the last message
in your Xorg.0.log file) that you are seeing the same kind of a crash that I
have been seeing recently.

So I would like you to test if the patch that I posted fixes the problem for
you. If it does, then there are two open issues that remain: a) should we push
my patch upstream (i.e. to at least prevent the crash of affected users like
you) and b) what really causes the double-free. The latter is what we have no
idea about yet.

Any additional logs that you can produce in the meantime would be useful.

-- 
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/20130204/2d3d9d22/attachment.html>

[Bug 60182] X.Org Server terminate when I close video player

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60182

--- Comment #10 from runetmember at gmail.com ---
> So I would like you to test if the patch that I posted fixes the problem for 
> you. 
Unfortunately, I am not developer, I can't check this patch.

> Any additional logs that you can produce in the meantime would be useful.
Ok.

-- 
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/20130204/112f0064/attachment.html>

[Intel-gfx] [PATCH] drm/i915: Fix RC6VIDS encode/devoce

[Daniel Vetter]

On Fri, Feb 01, 2013 at 04:41:14PM -0800, Ben Widawsky wrote:
> The RC6 VIDS has a linear ramp starting at 250mv, which means any values
> below 250 are invalid. The old buggy macros tried to adjust for this to
> be more flexible, but there is no need. As Dan pointed out the ENCODE
> only ever has one value. The only invalid value for decode is an input
> of 0 which means something is really wonky, and the cases where DECODE
> are used either don't matter (debug values), or would be implicitly
> correct (the check for less than 450).
> 
> This patch makes simpler, easier to read macros which are actually
> correct. Maybe this patch can actually fix some bugs now.
> 
> Thanks to Dan for catching this. /me hides
> 
> Cc: stable at kernel.org
> Reported-by: Dan Carpenter 
> Signed-off-by: Ben Widawsky 

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[j.gli...@gmail.com]

From: Jerome Glisse 

We need to take reference on the sync object while holding the
fence spinlock but at the same time we don't want to allocate
memory while holding the spinlock. This patch make sure we
enforce both of this constraint.

v2: actually test build it

Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296

Signed-off-by: Jerome Glisse 
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |   17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 44420fc..f4b7acd 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -413,6 +413,8 @@ static void ttm_transfered_destroy(struct ttm_buffer_object 
*bo)
  * @bo: A pointer to a struct ttm_buffer_object.
  * @new_obj: A pointer to a pointer to a newly created ttm_buffer_object,
  * holding the data of @bo with the old placement.
+ * @sync_obj: the sync object caller is responsible to take a reference on
+ * behalf of this function
  *
  * This is a utility function that may be called after an accelerated move
  * has been scheduled. A new buffer object is created as a placeholder for
@@ -423,11 +425,11 @@ static void ttm_transfered_destroy(struct 
ttm_buffer_object *bo)
  */

 static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
- struct ttm_buffer_object **new_obj)
+ struct ttm_buffer_object **new_obj,
+ void *sync_obj)
 {
struct ttm_buffer_object *fbo;
struct ttm_bo_device *bdev = bo->bdev;
-   struct ttm_bo_driver *driver = bdev->driver;

fbo = kzalloc(sizeof(*fbo), GFP_KERNEL);
if (!fbo)
@@ -448,7 +450,8 @@ static int ttm_buffer_object_transfer(struct 
ttm_buffer_object *bo,
fbo->vm_node = NULL;
atomic_set(&fbo->cpu_writers, 0);

-   fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
+   /* reference on sync obj is taken by the caller of this function */
+   fbo->sync_obj = sync_obj;
kref_init(&fbo->list_kref);
kref_init(&fbo->kref);
fbo->destroy = &ttm_transfered_destroy;
@@ -652,6 +655,8 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
}
ttm_bo_free_old_node(bo);
} else {
+   void *sync_obj;
+
/**
 * This should help pipeline ordinary buffer moves.
 *
@@ -662,12 +667,14 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object 
*bo,

set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);

-   /* ttm_buffer_object_transfer accesses bo->sync_obj */
-   ret = ttm_buffer_object_transfer(bo, &ghost_obj);
+   /* take the ref on the sync object before releasing the 
spinlock */
+   sync_obj = driver->sync_obj_ref(bo->sync_obj);
spin_unlock(&bdev->fence_lock);
+
if (tmp_obj)
driver->sync_obj_unref(&tmp_obj);

+   ret = ttm_buffer_object_transfer(bo, &ghost_obj, sync_obj);
if (ret)
return ret;

-- 
1.7.10.4

[PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Daniel Vetter]

On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
> From: Jerome Glisse 
>
> We need to take reference on the sync object while holding the
> fence spinlock but at the same time we don't want to allocate
> memory while holding the spinlock. This patch make sure we
> enforce both of this constraint.
>
> v2: actually test build it
>
> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>
> Signed-off-by: Jerome Glisse 

Isn't that just another iteration of
https://patchwork.kernel.org/patch/1972071/ which somehow never
reached -fixes?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Jerome Glisse]

On Mon, Feb 4, 2013 at 2:21 PM, Daniel Vetter  wrote:
> On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
>> From: Jerome Glisse 
>>
>> We need to take reference on the sync object while holding the
>> fence spinlock but at the same time we don't want to allocate
>> memory while holding the spinlock. This patch make sure we
>> enforce both of this constraint.
>>
>> v2: actually test build it
>>
>> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>>
>> Signed-off-by: Jerome Glisse 
>
> Isn't that just another iteration of
> https://patchwork.kernel.org/patch/1972071/ which somehow never
> reached -fixes?
> -Daniel

Yes but my version doesn't drop the lock before taking the ref, iirc
there might be a race if droping the lock and then taking it again.
Another process might race to unref the sync obj but i haven't
tortured too much my brain on how likely if at all this is possible.

Cheers,
Jerome

[PATCH] drm/ttm: avoid allocation memory while spinlock is held v2

[Daniel Vetter]

On Mon, Feb 4, 2013 at 8:36 PM, Jerome Glisse  wrote:
> On Mon, Feb 4, 2013 at 2:21 PM, Daniel Vetter  wrote:
>> On Mon, Feb 4, 2013 at 7:34 PM,   wrote:
>>> From: Jerome Glisse 
>>>
>>> We need to take reference on the sync object while holding the
>>> fence spinlock but at the same time we don't want to allocate
>>> memory while holding the spinlock. This patch make sure we
>>> enforce both of this constraint.
>>>
>>> v2: actually test build it
>>>
>>> Fix https://bugzilla.redhat.com/show_bug.cgi?id=906296
>>>
>>> Signed-off-by: Jerome Glisse 
>>
>> Isn't that just another iteration of
>> https://patchwork.kernel.org/patch/1972071/ which somehow never
>> reached -fixes?
>> -Daniel
>
> Yes but my version doesn't drop the lock before taking the ref, iirc
> there might be a race if droping the lock and then taking it again.
> Another process might race to unref the sync obj but i haven't
> tortured too much my brain on how likely if at all this is possible.

Hm, mine rechecks whether the sync object disappeared (spotted by
Maarten) before grabbing a reference. So should be ok if the fence
signals. Ofc, if we don't hold a reservation on bo someone else might
sneak and add a new one. But since we're trying to move the bo that'd
be a pretty bug already.

In any case yours is a bit nicer since it only grabs the fence_lock
once. My poke was more a stab at Dave, since he originally prodded me
on irc for breaking this, and then it seems to have fallen by the
wayside ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/nouveau: add lockdep annotations

[Marcin Slusarz]

1) Lockdep thinks all nouveau subdevs belong to the same class and can be
locked in arbitrary order, which is not true (at least in general case).
Tell it to distinguish subdevs by (o)class type.
2) DRM client can be locked under user client lock - tell lockdep to put DRM
client lock in a separate class.

Reported-by: Arend van Spriel 
Reported-by: Peter Hurley 
Reported-by: Maarten Lankhorst 
Reported-by: Daniel J Blueman 
Signed-off-by: Marcin Slusarz 
Cc: stable at vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
---
Lightly tested, only on NV4B and NVC1.
---
 drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
 drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c  | 3 +++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c 
b/drivers/gpu/drm/nouveau/core/core/subdev.c
index f74c30a..48f0637 100644
--- a/drivers/gpu/drm/nouveau/core/core/subdev.c
+++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
@@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
if (ret)
return ret;

-   mutex_init(&subdev->mutex);
+   __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
subdev->name = subname;

if (parent) {
diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h 
b/drivers/gpu/drm/nouveau/core/include/core/object.h
index 6a90267..62e68ba 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/object.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
@@ -50,10 +50,13 @@ int  nouveau_object_fini(struct nouveau_object *, bool 
suspend);

 extern struct nouveau_ofuncs nouveau_object_ofuncs;

+/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
+ * ".data". */
 struct nouveau_oclass {
u32 handle;
-   struct nouveau_ofuncs *ofuncs;
-   struct nouveau_omthds *omthds;
+   struct nouveau_ofuncs * const ofuncs;
+   struct nouveau_omthds * const omthds;
+   struct lock_class_key lock_class_key;
 };

 #define nv_oclass(o)nv_object(o)->oclass
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ef1ad21..bc00587 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
return 0;
 }

+static struct lock_class_key drm_client_lock_class_key;
+
 static int
 nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 {
@@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long 
flags)
ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
if (ret)
return ret;
+   lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);

dev->dev_private = drm;
drm->dev = dev;
-- 
1.8.1

[PATCH] drm/nouveau: add lockdep annotations

[Maarten Lankhorst]

Op 04-02-13 22:30, Marcin Slusarz schreef:
> 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> locked in arbitrary order, which is not true (at least in general case).
> Tell it to distinguish subdevs by (o)class type.
Apart from this specific case, is there any other reason why we require being 
able to nest 2 subdev locks?

Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return 
-EBUSY if there is any existing object. Problem solved, and lockdep is still 
generic.

> 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> client lock in a separate class.
Can you copy paste a typical lockdep splat for this? Also this should be a 
separate patch. :-)


> Reported-by: Arend van Spriel 
> Reported-by: Peter Hurley 
> Reported-by: Maarten Lankhorst 
> Reported-by: Daniel J Blueman 
> Signed-off-by: Marcin Slusarz 
> Cc: stable at vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
> ---
> Lightly tested, only on NV4B and NVC1.
> ---
>  drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
>  drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +--
>  drivers/gpu/drm/nouveau/nouveau_drm.c  | 3 +++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c 
> b/drivers/gpu/drm/nouveau/core/core/subdev.c
> index f74c30a..48f0637 100644
> --- a/drivers/gpu/drm/nouveau/core/core/subdev.c
> +++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
> @@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
>   if (ret)
>   return ret;
>  
> - mutex_init(&subdev->mutex);
> + __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
>   subdev->name = subname;
>  
>   if (parent) {
> diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h 
> b/drivers/gpu/drm/nouveau/core/include/core/object.h
> index 6a90267..62e68ba 100644
> --- a/drivers/gpu/drm/nouveau/core/include/core/object.h
> +++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
> @@ -50,10 +50,13 @@ int  nouveau_object_fini(struct nouveau_object *, bool 
> suspend);
>  
>  extern struct nouveau_ofuncs nouveau_object_ofuncs;
>  
> +/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
> + * ".data". */
>  struct nouveau_oclass {
>   u32 handle;
> - struct nouveau_ofuncs *ofuncs;
> - struct nouveau_omthds *omthds;
> + struct nouveau_ofuncs * const ofuncs;
> + struct nouveau_omthds * const omthds;
> + struct lock_class_key lock_class_key;
>  };
>  
>  #define nv_oclass(o)nv_object(o)->oclass
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ef1ad21..bc00587 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>   return 0;
>  }
>  
> +static struct lock_class_key drm_client_lock_class_key;
> +
>  static int
>  nouveau_drm_load(struct drm_device *dev, unsigned long flags)
>  {
> @@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long 
> flags)
>   ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
>   if (ret)
>   return ret;
> + lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
>  
>   dev->dev_private = drm;
>   drm->dev = dev;

[Bug 60034] rv790 etqw regression since r600g: add async for staging buffer upload v2

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60034

--- Comment #11 from Jerome Glisse  ---
Created attachment 74200
  --> https://bugs.freedesktop.org/attachment.cgi?id=74200&action=edit
work around

Can you check that this work around fix the rendering ?

-- 
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/20130204/5fff9737/attachment.html>

[Bug 60034] rv790 etqw regression since r600g: add async for staging buffer upload v2

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=60034

--- Comment #12 from Andy Furniss  ---
(In reply to comment #11)
> Created attachment 74200 [details] [review]
> work around
> 
> Can you check that this work around fix the rendering ?

Yes, this fixes etqw for me.

I see someone else reported an issue with this commit and xonotic.

I haven't checked if xonotic was OK before, but I can see the issue and this
patch does not help it.

https://bugs.freedesktop.org/show_bug.cgi?id=60236

-- 
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/20130204/e5acad3e/attachment.html>

[PATCHv5,RESEND 1/8] gpu: host1x: Add host1x driver

[Terje Bergström]

On 04.02.2013 01:09, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:43:57PM +0200, Terje Bergstrom wrote:
>> Add host1x, the driver for host1x and its client unit 2D.
> 
> Maybe this could be a bit more verbose. Perhaps describe what host1x is.

Sure. I could just steal the paragraph from Stephen:

The Tegra host1x module is the DMA engine for register access to Tegra's
graphics- and multimedia-related modules. The modules served by host1x
are referred to as clients. host1x includes some other  functionality,
such as synchronization.

>> diff --git a/drivers/gpu/host1x/Kconfig b/drivers/gpu/host1x/Kconfig
> [...]
>> @@ -0,0 +1,6 @@
>> +config TEGRA_HOST1X
>> + tristate "Tegra host1x driver"
>> + help
>> +   Driver for the Tegra host1x hardware.
> 
> Maybe s/Tegra/NVIDIA Tegra/?

Sounds good.

>> +
>> +   Required for enabling tegradrm.
> 
> This should probably be dropped. Either encode such knowledge as
> explicit dependencies or in this case just remove it altogether since we
> will probably merge both drivers anyway.

I think this was left from previous versions. Now it just doesn't make
sense. I'll just drop it.

> 
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> [...]
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include "dev.h"
> 
> Maybe add a blank line between the previous two lines to visually
> separate standard Linux includes from driver-specific ones.

Ok. You commented in quite few places in a similar way. I'll fix all of
them to first include system includes, then driver's own includes, and
add a blank line in between.

> 
>> +#include "hw/host1x01.h"
>> +
>> +#define CREATE_TRACE_POINTS
>> +#include 
>> +
>> +#define DRIVER_NAME  "tegra-host1x"
> 
> You only ever use this once, so maybe it can just be dropped?

Yes.

> 
>> +static struct host1x_device_info host1x_info = {
> 
> Perhaps this should be host1x01_info in order to match the hardware
> revision? That'll avoid it having to be renamed later on when other
> revisions start to appear.

Ok, will do. I thought it'd be awkward being alone until the second
version appears, but I'll add it.

> 
>> +static int host1x_probe(struct platform_device *dev)
>> +{
> [...]
>> + syncpt_irq = platform_get_irq(dev, 0);
>> + if (IS_ERR_VALUE(syncpt_irq)) {
> 
> This is somewhat unusual. It should be fine to just do:
> 
> if (syncpt_irq < 0)
> 
> but IS_ERR_VALUE() should work fine too.

I'll use the simpler version.

> 
>> + memcpy(&host->info, devid->data, sizeof(struct host1x_device_info));
> 
> Why not make the .info field a pointer to struct host1x_device_info
> instead? That way you don't have to keep separate copies of the same
> information.

This had something to do with __init data and non-init data. But, we're
not really putting this data into __init, so we should be able to use
just a pointer.

> 
>> + /* set common host1x device data */
>> + platform_set_drvdata(dev, host);
>> +
>> + host->regs = devm_request_and_ioremap(&dev->dev, regs);
>> + if (!host->regs) {
>> + dev_err(&dev->dev, "failed to remap host registers\n");
>> + return -ENXIO;
>> + }
> 
> This should probably be rewritten as:
> 
> host->regs = devm_ioremap_resource(&dev->dev, regs);
> if (IS_ERR(host->regs))
> return PTR_ERR(host->regs);
> 
> Though that function will only be available in 3.9-rc1.

Ok, 3.9-rc1 is fine as a basis.

>> + err = host1x_syncpt_init(host);
>> + if (err)
>> + return err;
> [...]
>> + host1x_syncpt_reset(host);
> 
> Why separate host1x_syncpt_reset() from host1x_syncpt_init()? I see why
> it might be useful to have host1x_syncpt_reset() as a separate function
> but couldn't it be called as part of host1x_syncpt_init()?

host1x_syncpt_init() is used for initializing the syncpt structures, and
is called in probe. host1x_syncpt_reset() should be called whenever we
think hardware state is lost, for example if VDD_CORE was rail gated due
to system suspend.

> 
>> + dev_info(&dev->dev, "initialized\n");
> 
> I don't think this is very useful. We should make sure to tell people
> when things fail. When everything goes as planned we don't need to brag
> about it =)

True. I wish other kernel drivers followed that same philosophy. :-)
I'll remove that. It's mainly useful as debug help, but it's as easy to
check from sysfs the state.

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
> [...]
>> +struct host1x_syncpt_ops {
> [...]
>> + const char * (*name)(struct host1x_syncpt *);
> 
> Why do we need this? Could we not refer to the syncpt name directly
> instead of going through this wrapper? I'd expect the name to be static.

This must be a relic. I'll remove the wrapper.

> 
>> +struct host1x_device_info {
> 
> Maybe this should be called simply host1x_info? _device seems redundant.

Sure.

> 
>> + int  

[PATCHv5, RESEND 2/8] gpu: host1x: Add syncpoint wait and interrupts

[Terje Bergström]

On 04.02.2013 02:30, Thierry Reding wrote:
> On Tue, Jan 15, 2013 at 01:43:58PM +0200, Terje Bergstrom wrote:
> [...]
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> [...]
>> @@ -95,7 +96,6 @@ static int host1x_probe(struct platform_device *dev)
>>
>>   /* set common host1x device data */
>>   platform_set_drvdata(dev, host);
>> -
>>   host->regs = devm_request_and_ioremap(&dev->dev, regs);
>>   if (!host->regs) {
>>   dev_err(&dev->dev, "failed to remap host registers\n");
> 
> This seems an unrelated (and actually undesirable) change.
> 
>> @@ -109,28 +109,40 @@ static int host1x_probe(struct platform_device *dev)
>>   }
>>
>>   err = host1x_syncpt_init(host);
>> - if (err)
>> + if (err) {
>> + dev_err(&dev->dev, "failed to init sync points");
>>   return err;
>> + }
> 
> This error message should probably have gone in the previous patch as
> well.

Oops, will move these to previous patch. I'm pretty sure I already fixed
this once. :-(

> 
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
>> index d8f5979..8376092 100644
>> --- a/drivers/gpu/host1x/dev.h
>> +++ b/drivers/gpu/host1x/dev.h
>> @@ -17,11 +17,12 @@
>>  #ifndef HOST1X_DEV_H
>>  #define HOST1X_DEV_H
>>
>> +#include 
>>  #include "syncpt.h"
>> +#include "intr.h"
>>
>>  struct host1x;
>>  struct host1x_syncpt;
>> -struct platform_device;
> 
> Why include platform_device.h here?

host1x_get_host() actually needs that, so this #include should've also
been in previous patch.

> 
>> @@ -34,6 +35,18 @@ struct host1x_syncpt_ops {
>>   const char * (*name)(struct host1x_syncpt *);
>>  };
>>
>> +struct host1x_intr_ops {
>> + void (*init_host_sync)(struct host1x_intr *);
>> + void (*set_host_clocks_per_usec)(
>> + struct host1x_intr *, u32 clocks);
> 
> Could the above two not be combined? The only reason to keep them
> separate would be if the host1x clock was dynamically changed, but I
> don't think we support that, right?

I've left this as a placeholder to at some point start supporting host1x
clock scaling. But I don't think we're going to do that for a while, so
I could merge them.

> 
>> + void (*set_syncpt_threshold)(
>> + struct host1x_intr *, u32 id, u32 thresh);
>> + void (*enable_syncpt_intr)(struct host1x_intr *, u32 id);
>> + void (*disable_syncpt_intr)(struct host1x_intr *, u32 id);
>> + void (*disable_all_syncpt_intrs)(struct host1x_intr *);
> 
> Can disable_all_syncpt_intrs() not be implemented generically using the
> number of syncpoints as exposed by host1x_device_info and the
> .disable_syncpt_intr() function?

disable_all_syncpt_intrs() disables all interrupts in one write (or one
per 32 sync points), so it's more efficient.

> 
>> @@ -46,11 +59,13 @@ struct host1x_device_info {
>>  struct host1x {
>>   void __iomem *regs;
>>   struct host1x_syncpt *syncpt;
>> + struct host1x_intr intr;
>>   struct platform_device *dev;
>>   struct host1x_device_info info;
>>   struct clk *clk;
>>
>>   struct host1x_syncpt_ops syncpt_op;
>> + struct host1x_intr_ops intr_op;
> 
> I think carrying a const pointer to the interrupt operations structure
> is a better option here.

Ok.

> 
>> diff --git a/drivers/gpu/host1x/hw/intr_hw.c 
>> b/drivers/gpu/host1x/hw/intr_hw.c
> [...]
>> +static void host1x_intr_syncpt_thresh_isr(struct host1x_intr_syncpt 
>> *syncpt);
> 
> Can we avoid this forward declaration?

I think we can, if I just move the isr to top of file.

> 
>> +static void syncpt_thresh_cascade_fn(struct work_struct *work)
> 
> syncpt_thresh_work()?

Sounds good.

> 
>> +{
>> + struct host1x_intr_syncpt *sp =
>> + container_of(work, struct host1x_intr_syncpt, work);
>> + host1x_syncpt_thresh_fn(sp);
> 
> Couldn't we inline the host1x_syncpt_thresh_fn() implementation here?
> Why do we need to go through an external function declaration?

If I move syncpt_thresh_work() to intr.c from intr_hw.c, I could do
that. That'd simplify the interrupt path.

> 
>> +static irqreturn_t syncpt_thresh_cascade_isr(int irq, void *dev_id)
>> +{
>> + struct host1x *host1x = dev_id;
>> + struct host1x_intr *intr = &host1x->intr;
>> + unsigned long reg;
>> + int i, id;
>> +
>> + for (i = 0; i < host1x->info.nb_pts / BITS_PER_LONG; i++) {
>> + reg = host1x_sync_readl(host1x,
>> + HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS +
>> + i * REGISTER_STRIDE);
>> + for_each_set_bit(id, ®, BITS_PER_LONG) {
>> + struct host1x_intr_syncpt *sp =
>> + intr->syncpt + (i * BITS_PER_LONG + id);
>> + host1x_intr_syncpt_thresh_isr(sp);
> 
> Have you considered mimicking the IRQ API and name this something like
> host1x_intr_syncpt_thresh_handle() and name the actual ISR just
> syncpt_thresh_isr()? Not so importan