Re: [PATCH 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa <tfiga@chromium.org>]

Hi Yong,

On Wed, May 10, 2017 at 12:27 AM, Tomasz Figa <tfiga@chromium.org> wrote:
> On Tue, May 2, 2017 at 9:00 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>> Hi Yong,
>>
>> Thanks for the patches! Some comments below.
>>
>> On Sat, Apr 29, 2017 at 06:34:36PM -0500, Yong Zhi wrote:
>>> +
>>> +/**************** FBPT operations ****************/
>>> +
>>> +static void cio2_fbpt_exit_dummy(struct cio2_device *cio2)
>>> +{
>>> +     if (cio2->dummy_lop) {
>>> +             dma_free_noncoherent(&cio2->pci_dev->dev, PAGE_SIZE,
>>> +                             cio2->dummy_lop, cio2->dummy_lop_bus_addr);
>>> +             cio2->dummy_lop = NULL;
>>> +     }
>>> +     if (cio2->dummy_page) {
>>> +             dma_free_noncoherent(&cio2->pci_dev->dev, PAGE_SIZE,
>>> +                             cio2->dummy_page, cio2->dummy_page_bus_addr);
>>> +             cio2->dummy_page = NULL;
>>> +     }
>>> +}
>>> +
>>> +static int cio2_fbpt_init_dummy(struct cio2_device *cio2)
>>> +{
>>> +     unsigned int i;
>>> +
>>> +     cio2->dummy_page = dma_alloc_noncoherent(&cio2->pci_dev->dev, PAGE_SIZE,
>>> +                                     &cio2->dummy_page_bus_addr, GFP_KERNEL);
>>> +     cio2->dummy_lop = dma_alloc_noncoherent(&cio2->pci_dev->dev, PAGE_SIZE,
>>> +                                     &cio2->dummy_lop_bus_addr, GFP_KERNEL);
>>> +     if (!cio2->dummy_page || !cio2->dummy_lop) {
>>> +             cio2_fbpt_exit_dummy(cio2);
>>> +             return -ENOMEM;
>>> +     }
>>> +     /*
>>> +      * List of Pointers(LOP) contains 1024x32b pointers to 4KB page each
>>> +      * Initialize each entry to dummy_page bus base address.
>>> +      */
>>> +     for (i = 0; i < PAGE_SIZE / sizeof(*cio2->dummy_lop); i++)
>>> +             cio2->dummy_lop[i] = cio2->dummy_page_bus_addr >> PAGE_SHIFT;
>>> +
>>> +     dma_sync_single_for_device(&cio2->pci_dev->dev, /* DMA phy addr */
>>> +                     cio2->dummy_lop_bus_addr, PAGE_SIZE, DMA_TO_DEVICE);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void cio2_fbpt_entry_enable(struct cio2_device *cio2,
>>> +                                struct cio2_fbpt_entry entry[CIO2_MAX_LOPS])
>>> +{
>>> +     dma_wmb();
>>
>> Is there a particular reason to have this?
>>
>> The documentation states that (Documentation/memory-barriers.txt):
>>
>>      These are for use with consistent memory to guarantee the ordering
>>      of writes or reads of shared memory accessible to both the CPU and a
>>      DMA capable device.
>>
>> This is while the device does not do cache coherent DMA.
>
> I think the first question we should ask is why the driver allocates
> non-consistent memory for this kind of structures. Looks like a waste
> of cachelines, given that it seems to be a primarily write-only memory
> (from CPU point of view), with rare read backs of entry[0] to check
> the flags, which can be modified by the device. Moreover, the fact
> that the memory is being cached actually defeats the barrier, as you
> first write entry[1..N], call the barrier, write entry[0] and then
> flush the cache starting from entry[0], which means that the hardware
> sees entry[0] (+/- the size of the cacheline) before further entries.
>
> Other than that, I think the barrier makes sense, as the compiler (or
> even the CPU or buses) could reorder the writes without it, regardless
> of whether the memory is consistent or not. (Of course the cache
> flushing problem above remains if the memory is non-consistent.)

Any updates on this and other comments?

Best regards,
Tomasz