Re: [PATCH v3 2/5] media: hws: add shared scratch DMA arena

[Hans Verkuil]

Hi Ben,

On 27/07/2026 21:42, Ben Hoff wrote:
> Hi Hans,
> 
> The “Changes Requested” state is correct. I interpreted your July 15
> feedback as requiring a new version of the series, and I have
> additional changes coming across the patchset.
> 
> I have a new revision drafted locally, but I want to test it more
> thoroughly before sending it.

I didn't see a v4, this is still being worked on?

In the meantime I'll mark this series as 'Changes Requested' and wait for
that v4. That way it is out of our queue :-)

Regards,

	Hans

> 
> Happy to get feedback on any component of this patchset as well and I
> can fold that in as well.
> 
> Best,
> Ben
> 
> 
> On Mon, Jul 27, 2026 at 11:07 AM <hverkuil+cisco@xxxxxxxxxx> wrote:
>>
>> Hi Ben,
>>
>> On 29/06/2026 20:26, hoff.benjamin.k@xxxxxxxxx wrote:
>>> From: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
>>>
>>> Replace the probe-time seed buffer allocation with a per-channel coherent
>>> scratch arena that is acquired by stream users and released by refcount.
>>> The arena reserves video bounce space first and an audio capture window
>>> after it, and validates that the full allocation fits within the single
>>> 512 MiB BAR remap page shared by the channel.
>>>
>>> Reset cached video DMA window state when freeing a channel arena so the
>>> next stream reprograms the hardware from a clean state.
>>
>> This patch is marked as 'Changes Requested' in patchwork:
>>
>> https://patchwork.linuxtv.org/project/linux-media/patch/20260629182649.247879-3-hoff.benjamin.k@xxxxxxxxx/
>>
>> I suspect that's a mistake. Can you confirm that the patchwork state should really
>> be 'New'? Or are there really more changes coming and I just missed something?
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Signed-off-by: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
>>> ---
>>>  drivers/media/pci/hws/hws.h       |  28 +++++
>>>  drivers/media/pci/hws/hws_pci.c   | 192 ++++++++++++++++++++++++------
>>>  drivers/media/pci/hws/hws_reg.h   |   7 ++
>>>  drivers/media/pci/hws/hws_video.c |  17 +--
>>>  4 files changed, 198 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/hws/hws.h b/drivers/media/pci/hws/hws.h
>>> index 8fbe1fe27844..c0541190c047 100644
>>> --- a/drivers/media/pci/hws/hws.h
>>> +++ b/drivers/media/pci/hws/hws.h
>>> @@ -8,6 +8,7 @@
>>>  #include <linux/kthread.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/list.h>
>>> +#include <linux/mutex.h>
>>>  #include <linux/spinlock.h>
>>>  #include <linux/sizes.h>
>>>  #include <linux/atomic.h>
>>> @@ -52,6 +53,8 @@ struct hwsvideo_buffer {
>>>       int slot;
>>>  };
>>>
>>> +#define HWS_VIDEO_BOUNCE_SLOTS 2
>>> +
>>>  struct hws_video {
>>>       /* Linkage */
>>>       struct hws_pcie_dev *parent;
>>> @@ -104,6 +107,7 @@ struct hws_video {
>>>       u32 last_dma_page;
>>>       u32 last_pci_addr;
>>>       u32 last_half16;
>>> +     u8 next_bounce_slot;
>>>
>>>       /* Misc counters */
>>>       int signal_loss_cnt;
>>> @@ -130,6 +134,7 @@ struct hws_scratch_dma {
>>>       void *cpu;
>>>       dma_addr_t dma;
>>>       size_t size;
>>> +     bool owned;
>>>  };
>>>
>>>  struct hws_pcie_dev {
>>> @@ -152,6 +157,7 @@ struct hws_pcie_dev {
>>>       u32 max_hw_video_buf_sz;
>>>       u8 max_channels;
>>>       u8 cur_max_video_ch;
>>> +     u8 cur_max_audio_ch;
>>>       bool start_run;
>>>
>>>       bool buf_allocated;
>>> @@ -161,7 +167,10 @@ struct hws_pcie_dev {
>>>
>>>       /* Kernel thread */
>>>       struct task_struct *main_task;
>>> +     struct mutex scratch_lock; /* protects scratch DMA arenas and user refs */
>>> +     unsigned int scratch_users[MAX_VID_CHANNELS];
>>>       struct hws_scratch_dma scratch_vid[MAX_VID_CHANNELS];
>>> +     struct hws_scratch_dma scratch_aud[MAX_VID_CHANNELS];
>>>
>>>       bool suspended;
>>>       int irq;
>>> @@ -170,4 +179,23 @@ struct hws_pcie_dev {
>>>       int pci_lost;
>>>  };
>>>
>>> +static inline bool hws_dma_fits_remap_window(dma_addr_t dma, size_t size)
>>> +{
>>> +     dma_addr_t end;
>>> +
>>> +     if (!size)
>>> +             return false;
>>> +
>>> +     end = dma + size - 1;
>>> +     if (end < dma)
>>> +             return false;
>>> +
>>> +     return upper_32_bits(dma) == upper_32_bits(end) &&
>>> +            (lower_32_bits(dma) & PCI_E_BAR_ADD_MASK) ==
>>> +            (lower_32_bits(end) & PCI_E_BAR_ADD_MASK);
>>> +}
>>> +
>>> +int hws_alloc_channel_scratch(struct hws_pcie_dev *hws, unsigned int ch);
>>> +void hws_release_channel_scratch(struct hws_pcie_dev *hws, unsigned int ch);
>>> +
>>>  #endif
>>> diff --git a/drivers/media/pci/hws/hws_pci.c b/drivers/media/pci/hws/hws_pci.c
>>> index 10af6c30566a..589d4840e116 100644
>>> --- a/drivers/media/pci/hws/hws_pci.c
>>> +++ b/drivers/media/pci/hws/hws_pci.c
>>> @@ -11,6 +11,7 @@
>>>  #include <linux/err.h>
>>>  #include <linux/ktime.h>
>>>  #include <linux/math64.h>
>>> +#include <linux/minmax.h>
>>>  #include <linux/pm.h>
>>>  #include <linux/freezer.h>
>>>  #include <linux/pci_regs.h>
>>> @@ -101,22 +102,30 @@ static void hws_configure_hardware_capabilities(struct hws_pcie_dev *hdev)
>>>       case 0x8504:
>>>       case 0x6504:
>>>               hdev->cur_max_video_ch = 4;
>>> +             hdev->cur_max_audio_ch = 4;
>>>               break;
>>>       case 0x8532:
>>>               hdev->cur_max_video_ch = 2;
>>> +             hdev->cur_max_audio_ch = 2;
>>>               break;
>>>       case 0x8512:
>>>       case 0x6502:
>>>               hdev->cur_max_video_ch = 2;
>>> +             hdev->cur_max_audio_ch = 0;
>>>               break;
>>>       case 0x8501:
>>>               hdev->cur_max_video_ch = 1;
>>> +             hdev->cur_max_audio_ch = 0;
>>>               break;
>>>       default:
>>>               hdev->cur_max_video_ch = 4;
>>> +             hdev->cur_max_audio_ch = 0;
>>>               break;
>>>       }
>>>
>>> +     if (hdev->cur_max_audio_ch > hdev->cur_max_video_ch)
>>> +             hdev->cur_max_audio_ch = hdev->cur_max_video_ch;
>>> +
>>>       /* universal buffer capacity */
>>>       hdev->max_hw_video_buf_sz = MAX_MM_VIDEO_SIZE;
>>>
>>> @@ -255,55 +264,164 @@ static void hws_stop_kthread_action(void *data)
>>>       }
>>>  }
>>>
>>> -static int hws_alloc_seed_buffers(struct hws_pcie_dev *hws)
>>> +static size_t hws_video_scratch_bytes(void)
>>>  {
>>> -     int ch;
>>> -     /* 64 KiB is plenty for a safe dummy; hardware needs 64-byte alignment. */
>>> -     const size_t need = ALIGN(64 * 1024, 64);
>>> +     return HWS_VIDEO_BOUNCE_SLOTS * ALIGN((size_t)MAX_VIDEO_SCALER_SIZE, 64);
>>> +}
>>>
>>> -     for (ch = 0; ch < hws->cur_max_video_ch; ch++) {
>>> +static size_t hws_audio_scratch_bytes(void)
>>> +{
>>> +     return ALIGN((size_t)MAX_AUDIO_CAP_SIZE, 64);
>>> +}
>>> +
>>> +static void hws_clear_scratch(struct hws_scratch_dma *scratch)
>>> +{
>>> +     scratch->cpu = NULL;
>>> +     scratch->dma = 0;
>>> +     scratch->size = 0;
>>> +     scratch->owned = false;
>>> +}
>>> +
>>> +static void hws_free_channel_scratch_locked(struct hws_pcie_dev *hws,
>>> +                                         unsigned int ch)
>>> +{
>>> +     struct hws_scratch_dma *vid;
>>> +     struct hws_scratch_dma *aud;
>>> +
>>> +     if (!hws || ch >= MAX_VID_CHANNELS)
>>> +             return;
>>> +
>>> +     vid = &hws->scratch_vid[ch];
>>> +     aud = &hws->scratch_aud[ch];
>>> +
>>> +     if (ch < hws->cur_max_video_ch) {
>>> +             hws->video[ch].window_valid = false;
>>> +             hws->video[ch].last_dma_hi = 0;
>>> +             hws->video[ch].last_dma_page = 0;
>>> +             hws->video[ch].last_pci_addr = 0;
>>> +             hws->video[ch].last_half16 = 0;
>>> +             hws->video[ch].next_bounce_slot = 0;
>>> +     }
>>> +     hws->scratch_users[ch] = 0;
>>> +
>>> +     if (aud->cpu && aud->owned)
>>> +             dma_free_coherent(&hws->pdev->dev, aud->size, aud->cpu,
>>> +                               aud->dma);
>>> +     hws_clear_scratch(aud);
>>> +
>>> +     if (vid->cpu && vid->owned)
>>> +             dma_free_coherent(&hws->pdev->dev, vid->size, vid->cpu,
>>> +                               vid->dma);
>>> +     hws_clear_scratch(vid);
>>> +}
>>> +
>>> +int hws_alloc_channel_scratch(struct hws_pcie_dev *hws, unsigned int ch)
>>> +{
>>> +     size_t aud_off = hws_video_scratch_bytes();
>>> +     size_t arena_need = aud_off;
>>> +     bool has_audio;
>>> +
>>> +     if (!hws || ch >= max_t(unsigned int, hws->cur_max_video_ch,
>>> +                             hws->cur_max_audio_ch))
>>> +             return -EINVAL;
>>> +
>>> +     has_audio = ch < hws->cur_max_audio_ch;
>>> +     if (has_audio)
>>> +             arena_need = ALIGN(aud_off + hws_audio_scratch_bytes(), 64);
>>> +
>>> +     /*
>>> +      * One coherent per-channel arena backs both fallback video DMA and audio
>>> +      * DMA. The video bounce slots live first; the audio capture window starts
>>> +      * at aud_off. The whole arena must fit inside a single 512 MiB remap page
>>> +      * because video and audio share the channel remap slot.
>>> +      */
>>> +     mutex_lock(&hws->scratch_lock);
>>> +     if (hws->scratch_vid[ch].cpu) {
>>> +             hws->scratch_users[ch]++;
>>> +             mutex_unlock(&hws->scratch_lock);
>>> +             return 0;
>>> +     }
>>> +
>>> +     {
>>>  #if defined(CONFIG_HAS_DMA) /* normal on PCIe platforms */
>>> -             void *cpu = dma_alloc_coherent(&hws->pdev->dev, need,
>>> -                                            &hws->scratch_vid[ch].dma,
>>> -                                            GFP_KERNEL);
>>> +             dma_addr_t dma = 0;
>>> +             void *cpu = NULL;
>>> +             int attempt;
>>> +
>>> +             for (attempt = 0; attempt < 8; attempt++) {
>>> +                     cpu = dma_alloc_coherent(&hws->pdev->dev, arena_need,
>>> +                                              &dma, GFP_KERNEL);
>>> +                     if (!cpu)
>>> +                             break;
>>> +                     if (!hws_dma_fits_remap_window(dma, arena_need)) {
>>> +                             dev_dbg(&hws->pdev->dev,
>>> +                                     "scratch arena: retry ch=%u dma=%pad size=%zu crosses remap window\n",
>>> +                                     ch, &dma, arena_need);
>>> +                             dma_free_coherent(&hws->pdev->dev, arena_need,
>>> +                                               cpu, dma);
>>> +                             cpu = NULL;
>>> +                             continue;
>>> +                     }
>>> +                     break;
>>> +             }
>>>  #else
>>>               void *cpu = NULL;
>>> +             dma_addr_t dma = 0;
>>>  #endif
>>>               if (!cpu) {
>>>                       dev_warn(&hws->pdev->dev,
>>> -                              "scratch: dma_alloc_coherent failed ch=%d\n", ch);
>>> -                     /* not fatal: free earlier ones and continue without seeding */
>>> -                     while (--ch >= 0) {
>>> -                             if (hws->scratch_vid[ch].cpu)
>>> -                                     dma_free_coherent(&hws->pdev->dev,
>>> -                                                       hws->scratch_vid[ch].size,
>>> -                                                       hws->scratch_vid[ch].cpu,
>>> -                                                       hws->scratch_vid[ch].dma);
>>> -                             hws->scratch_vid[ch].cpu = NULL;
>>> -                             hws->scratch_vid[ch].size = 0;
>>> -                     }
>>> +                              "scratch arena: dma_alloc_coherent failed ch=%u\n",
>>> +                              ch);
>>> +                     mutex_unlock(&hws->scratch_lock);
>>>                       return -ENOMEM;
>>>               }
>>> -             hws->scratch_vid[ch].cpu  = cpu;
>>> -             hws->scratch_vid[ch].size = need;
>>> +
>>> +             hws->scratch_vid[ch].dma = dma;
>>> +             hws->scratch_vid[ch].cpu = cpu;
>>> +             hws->scratch_vid[ch].size = arena_need;
>>> +             hws->scratch_vid[ch].owned = true;
>>> +
>>> +             if (has_audio) {
>>> +                     hws->scratch_aud[ch].dma = dma + aud_off;
>>> +                     hws->scratch_aud[ch].cpu = (u8 *)cpu + aud_off;
>>> +                     hws->scratch_aud[ch].size = hws_audio_scratch_bytes();
>>> +                     hws->scratch_aud[ch].owned = false;
>>> +             }
>>>       }
>>> +     hws->scratch_users[ch] = 1;
>>> +
>>> +     dev_dbg(&hws->pdev->dev,
>>> +             "scratch arena: allocated ch=%u size=%zu audio=%d\n",
>>> +             ch, arena_need, has_audio);
>>> +     mutex_unlock(&hws->scratch_lock);
>>>       return 0;
>>>  }
>>>
>>> +void hws_release_channel_scratch(struct hws_pcie_dev *hws, unsigned int ch)
>>> +{
>>> +     if (!hws || ch >= MAX_VID_CHANNELS)
>>> +             return;
>>> +
>>> +     mutex_lock(&hws->scratch_lock);
>>> +     if (hws->scratch_users[ch] > 0)
>>> +             hws->scratch_users[ch]--;
>>> +     if (!hws->scratch_users[ch])
>>> +             hws_free_channel_scratch_locked(hws, ch);
>>> +     mutex_unlock(&hws->scratch_lock);
>>> +}
>>> +
>>>  static void hws_free_seed_buffers(struct hws_pcie_dev *hws)
>>>  {
>>>       int ch;
>>>
>>> -     for (ch = 0; ch < hws->cur_max_video_ch; ch++) {
>>> -             if (hws->scratch_vid[ch].cpu) {
>>> -                     dma_free_coherent(&hws->pdev->dev,
>>> -                                       hws->scratch_vid[ch].size,
>>> -                                       hws->scratch_vid[ch].cpu,
>>> -                                       hws->scratch_vid[ch].dma);
>>> -                     hws->scratch_vid[ch].cpu = NULL;
>>> -                     hws->scratch_vid[ch].size = 0;
>>> -             }
>>> -     }
>>> +     if (!hws)
>>> +             return;
>>> +
>>> +     /* Teardown-only force-free path; normal streams use release refcounts. */
>>> +     mutex_lock(&hws->scratch_lock);
>>> +     for (ch = 0; ch < MAX_VID_CHANNELS; ch++)
>>> +             hws_free_channel_scratch_locked(hws, ch);
>>> +     mutex_unlock(&hws->scratch_lock);
>>>  }
>>>
>>>  static void hws_seed_channel(struct hws_pcie_dev *hws, int ch)
>>> @@ -326,11 +444,11 @@ static void hws_seed_channel(struct hws_pcie_dev *hws, int ch)
>>>                      hws->bar0_base + CVBS_IN_BUF_BASE +
>>>                      ch * PCIE_BARADDROFSIZE);
>>>
>>> -     /* Half size: use either the current format's half or half of scratch. */
>>> +     /* Half size: use either the current format or the video arena. */
>>>       {
>>>               u32 half = hws->video[ch].pix.half_size ?
>>>                       hws->video[ch].pix.half_size :
>>> -                     (u32)(hws->scratch_vid[ch].size / 2);
>>> +                     (u32)(MAX_VIDEO_SCALER_SIZE / 2);
>>>
>>>               writel_relaxed(half / 16,
>>>                              hws->bar0_base + CVBS_IN_BUF_BASE2 +
>>> @@ -400,6 +518,7 @@ static int hws_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
>>>       hws->pdev = pdev;
>>>       hws->irq = -1;
>>>       hws->suspended = false;
>>> +     mutex_init(&hws->scratch_lock);
>>>       pci_set_drvdata(pdev, hws);
>>>
>>>       /* 1) Enable device + bus mastering (managed) */
>>> @@ -448,12 +567,7 @@ static int hws_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
>>>               }
>>>       }
>>>
>>> -     /* 6) Allocate scratch DMA and seed BAR table + channel base/half (legacy SetDMAAddress) */
>>> -     ret = hws_alloc_seed_buffers(hws);
>>> -     if (!ret)
>>> -             hws_seed_all_channels(hws);
>>> -
>>> -     /* 7) Start-run sequence. */
>>> +     /* 6) Start-run sequence. Scratch DMA is allocated on stream start. */
>>>       hws_init_video_sys(hws, false);
>>>
>>>       /* A) Force legacy INTx; legacy used request_irq(pdev->irq, ..., IRQF_SHARED) */
>>> diff --git a/drivers/media/pci/hws/hws_reg.h b/drivers/media/pci/hws/hws_reg.h
>>> index 344cb8d011a9..c8d6715fe0c2 100644
>>> --- a/drivers/media/pci/hws/hws_reg.h
>>> +++ b/drivers/media/pci/hws/hws_reg.h
>>> @@ -33,6 +33,13 @@
>>>  #define PCI_E_BAR_ADD_MASK 0xE0000000
>>>  #define PCI_E_BAR_ADD_LOWMASK 0x1FFFFFFF
>>>
>>> +/*
>>> + * The legacy driver reserved a 10 KiB hardware capture window per audio
>>> + * channel even though the delivered packet size is smaller. Keep that headroom
>>> + * for the split-buffer DMA engine.
>>> + */
>>> +#define MAX_AUDIO_CAP_SIZE         (10U * 1024U)
>>> +
>>>  #define MAX_VID_CHANNELS            4
>>>
>>>  #define MAX_MM_VIDEO_SIZE            SZ_4M
>>> diff --git a/drivers/media/pci/hws/hws_video.c b/drivers/media/pci/hws/hws_video.c
>>> index b1af81d1368a..1531d703f6d8 100644
>>> --- a/drivers/media/pci/hws/hws_video.c
>>> +++ b/drivers/media/pci/hws/hws_video.c
>>> @@ -306,6 +306,7 @@ int hws_video_init_channel(struct hws_pcie_dev *pdev, int ch)
>>>
>>>       vid->queued_count = 0;
>>>       vid->window_valid = false;
>>> +     vid->next_bounce_slot = 0;
>>>
>>>       /* Default format. */
>>>       vid->pix.width = 1920;
>>> @@ -556,17 +557,18 @@ static void hws_seed_dma_windows(struct hws_pcie_dev *hws)
>>>                                      hws->bar0_base + CVBS_IN_BUF_BASE +
>>>                                      ch * PCIE_BARADDROFSIZE);
>>>
>>> -                     /* Half-frame length in /16 units.
>>> -                      * Prefer the current channel's computed half_size if available.
>>> -                      * Fall back to half of the probe-owned scratch buffer.
>>> +                     /*
>>> +                      * Half-frame length in /16 units. Prefer the current
>>> +                      * format and fall back to the video bounce window,
>>> +                      * not the full per-channel arena that also contains audio.
>>>                        */
>>>                       {
>>>                               u32 half_bytes = hws->video[ch].pix.half_size ?
>>> -                                 hws->video[ch].pix.half_size :
>>> -                                 (u32)(hws->scratch_vid[ch].size / 2);
>>> +                                     hws->video[ch].pix.half_size :
>>> +                                     (u32)(MAX_VIDEO_SCALER_SIZE / 2);
>>> +
>>>                               writel_relaxed(half_bytes / 16,
>>> -                                            hws->bar0_base +
>>> -                                            CVBS_IN_BUF_BASE2 +
>>> +                                            hws->bar0_base + CVBS_IN_BUF_BASE2 +
>>>                                              ch * PCIE_BARADDROFSIZE);
>>>                       }
>>>               }
>>> @@ -1166,6 +1168,7 @@ static int hws_start_streaming(struct vb2_queue *q, unsigned int count)
>>>               }
>>>               return ret;
>>>       }
>>> +
>>>       (void)hws_read_active_state(hws, v->channel_index,
>>>                                      &v->pix.interlaced);
>>>
>>
>