On 29.11.2012 12:01, Mark Zhang wrote:
>> +fail:
>> + /* Add clean-up */
>
> Yes, add "nvhost_module_deinit" here?
Sounds good.
>> +int nvhost_client_device_suspend(struct platform_device *dev)
>> +{
>> + int ret = 0;
>> + struct nvhost_device_data *pdata = platform_get_drvdata(dev);
>> +
>> + ret = nvhost_channel_suspend(pdata->channel);
>> + dev_info(&dev->dev, "suspend status: %d\n", ret);
>> + if (ret)
>> + return ret;
>> +
>> + return ret;
>
> Minor issue: just "return ret" is OK. That "if" doesn't make sense.
Yes, must be some snafu when doing changes in code.
>> -struct nvhost_chip_support *nvhost_chip_ops;
>> +static struct nvhost_chip_support *nvhost_chip_ops;
>>
>
> All right, already fixed here. Sorry, so just ignore what I said about
> this in my reply to your patch 1.
I was wondering about this, because I thought I did make it static. But
it looks like I added that to the wrong commit. Anyway, this needs
rethinking.
>> +struct mem_handle *nvhost_dmabuf_get(u32 id, struct platform_device *dev)
>> +{
>> + struct mem_handle *h;
>> + struct dma_buf *buf;
>> +
>> + buf = dma_buf_get(to_dmabuf_fd(id));
>> + if (IS_ERR_OR_NULL(buf))
>> + return (struct mem_handle *)buf;
>> +
>> + h = (struct mem_handle *)dma_buf_attach(buf, &dev->dev);
>> + if (IS_ERR_OR_NULL(h))
>> + dma_buf_put(buf);
>
> Return an error here.
Will do.
>> + op->nvhost_dev.alloc_nvhost_channel = t20_alloc_nvhost_channel;
>> + op->nvhost_dev.free_nvhost_channel = t20_free_nvhost_channel;
>> +
>
> I recall in previous version, there is t30-related alloc_nvhost_channel
> & free_nvhost_channel. Why remove them?
I could actually refactor these all into one alloc channel. We already
store the number of channels in a data type, so a generic channel
allocator would be better than having a chip specific one.
>> +static int push_buffer_init(struct push_buffer *pb)
>> +{
>> + struct nvhost_cdma *cdma = pb_to_cdma(pb);
>> + struct nvhost_master *master = cdma_to_dev(cdma);
>> + pb->mapped = NULL;
>> + pb->phys = 0;
>> + pb->handle = NULL;
>> +
>> + cdma_pb_op().reset(pb);
>> +
>> + /* allocate and map pushbuffer memory */
>> + pb->mapped = dma_alloc_writecombine(&master->dev->dev,
>> + PUSH_BUFFER_SIZE + 4, &pb->phys, GFP_KERNEL);
>> + if (IS_ERR_OR_NULL(pb->mapped)) {
>> + pb->mapped = NULL;
>> + goto fail;
>
> Return directly here. "goto fail" makes "push_buffer_destroy" get called.
Will do.
>
>> + }
>> +
>> + /* memory for storing mem client and handles for each opcode pair */
>> + pb->handle = kzalloc(NVHOST_GATHER_QUEUE_SIZE *
>> + sizeof(struct mem_handle *),
>> + GFP_KERNEL);
>> + if (!pb->handle)
>> + goto fail;
>> +
>> + /* put the restart at the end of pushbuffer memory */
>
> Just for curious, why "pb->mapped + 1K" is the end of a 4K pushbuffer?
pb->mapped is u32 *, so compiler will take care of multiplying by
sizeof(u32).
>> +unsigned int nvhost_cdma_wait_locked(struct nvhost_cdma *cdma,
>> + enum cdma_event event)
>> +{
>> + for (;;) {
>> + unsigned int space = cdma_status_locked(cdma, event);
>> + if (space)
>> + return space;
>> +
>> + /* If somebody has managed to already start waiting, yield */
>> + if (cdma->event != CDMA_EVENT_NONE) {
>> + mutex_unlock(&cdma->lock);
>> + schedule();
>> + mutex_lock(&cdma->lock);
>> + continue;
>> + }
>> + cdma->event = event;
>> +
>> + mutex_unlock(&cdma->lock);
>> + down(&cdma->sem);
>> + mutex_lock(&cdma->lock);
>
> I'm newbie of nvhost but I feel here is very tricky, about the lock and
> unlock of this mutex: cdma->lock. Does it require this mutex is locked
> before calling this function? And do we need to unlock it before the
> code: "return space;" above? IMHO, this is not a good design and can we
> find out a better solution?
Yeah, it's not perfect and good solutions are welcome.
cdma_status_locked() must be called with a mutex. But, what we generally
wait for is for space in push buffer. The cleanup code cannot run if we
keep cdma->lock, so I release it.
The two ways to loop are because there was a race between two processes
waiting for space. One of them set cdma->event to indicate what it's
waiting for and can go to sleep, but the other has to keep spinning.
Terje
