lkml.org 
[lkml]   [2012]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC,v2,3/8] video: tegra: host: Add channel and client support
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


\
 
 \ /
  Last update: 2012-11-29 12:21    [W:0.909 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site