lkml.org 
[lkml]   [2011]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 08/11] ttm: Provide DMA aware TTM page pool code.
    > > +static struct dma_page *__ttm_dma_alloc_page(struct dma_pool *pool)
    > > +{
    > > + struct dma_page *d_page;
    > > +
    > > + d_page = kmalloc(sizeof(struct dma_page), GFP_KERNEL);
    > > + if (!d_page)
    > > + return NULL;
    > > +
    > > + d_page->vaddr = dma_alloc_coherent(pool->dev, pool->size,
    > > + &d_page->dma,
    > > + pool->gfp_flags);
    > > + d_page->p = virt_to_page(d_page->vaddr);
    > > + if (!d_page->vaddr) {
    > > + kfree(d_page);
    > > + d_page = NULL;
    > > + }
    >
    > Move d_page->p = virt_to_page(d_page->vaddr); after if (!d_page->vaddr)
    > block.

    Duh! Yes.

    .. snip..
    > > +#if 0
    > > + if (nr_free > 1) {
    > > + pr_debug("%s: (%s:%d) Attempting to free %d (%d) pages\n",
    > > + pool->dev_name, pool->name, current->pid,
    > > + npages_to_free, nr_free);
    > > + }
    > > +#endif

    What is your feeling on those #if 0? I was not sure to keep them - they are useful
    when debugging, but not so much during run-time? Rip them out and I can just
    keep them in my 'debug' patch queue in case things go wrong?

    Or perhas do it (rip 'em out) in 3 months time-frame?

    .. snip..
    > > +static struct dma_pool *ttm_dma_find_pool(struct device *dev,
    > > + enum pool_type type)
    > > +{
    > > + struct dma_pool *pool, *tmp, *found = NULL;
    > > +
    > > + if (type == IS_UNDEFINED)
    > > + return found;
    > > + /* NB: We iterate on the 'struct dev' which has no spinlock, but
    > > + * it does have a kref which we have taken. */
    >
    > I fail to see where we kref dev.

    Ah, I should document that more extensivly. That is done way way
    earlier. As in in the path of the initialization of the driver:

    drm_pci_init
    for non-KMS calls pci_dev_get()

    for KMS calls pci_register_driver..
    which calls 'driver_register' which called 'device_register'

    And then during teardown (so unbind on sysfs), it ends up calling the devres
    deconstructors which cleans up the 'struct device' dev_res, - in our case
    ttm_dma_pool_release. However the nice thing is at that point of time all of
    the calls to the TTM have quiseced so nobody is calling ttm for this device
    anymore.

    Let me stick this in the comment section.

    > See comment above, otherwise:
    > Reviewed-by: Jerome Glisse <jglisse@redhat.com>

    Great! Thank you!


    \
     
     \ /
      Last update: 2011-11-01 14:55    [W:4.095 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site