On 19/12/2018 05.08, Matthias Schwarzott wrote:
> Am 18.12.18 um 23:59 schrieb Brad Love:
>> It is reported that commit 95f408bbc4e4 ("media: cx23885: Ryzen DMA
>> related RiSC engine stall fixes") caused regresssions with other CPUs.
>>
>> Ensure that the quirk will be applied only for the CPUs that
>> are known to cause problems.
>>
>> A module option is added for explicit control of the behaviour.
>>
>> Fixes: 95f408bbc4e4 ("media: cx23885: Ryzen DMA related RiSC engine stall fixes")
>>
>> Signed-off-by: Brad Love <brad@xxxxxxxxxxxxxxxx>
> Hi Brad,
> I found one issue. See below.
>
> Regards
> Matthias
Thanks for the catch Matthias, v3 submitted.
Cheers,
Brad
>
>> ---
>> Changes since v1:
>> - Added module option for three way control
>> - Removed '7' from pci id description, Ryzen 3 is the same id
>>
>> drivers/media/pci/cx23885/cx23885-core.c | 54 ++++++++++++++++++++++++++++++--
>> drivers/media/pci/cx23885/cx23885.h | 2 ++
>> 2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
>> index 39804d8..fb721c7 100644
>> --- a/drivers/media/pci/cx23885/cx23885-core.c
>> +++ b/drivers/media/pci/cx23885/cx23885-core.c
> ...
>> @@ -2058,6 +2076,36 @@ void cx23885_gpio_enable(struct cx23885_dev *dev, u32 mask, int asoutput)
>> /* TODO: 23-19 */
>> }
>>
>> +static struct {
>> + int vendor, dev;
>> +} const broken_dev_id[] = {
>> + /* According with
>> + * https://openbenchmarking.org/system/1703021-RI-AMDZEN08075/Ryzen%207%201800X/lspci,
>> + * 0x1451 is PCI ID for the IOMMU found on Ryzen
>> + */
>> + { PCI_VENDOR_ID_AMD, 0x1451 },
>> +};
>> +
>> +static bool cx23885_does_need_dma_reset(void)
>> +{
>> + int i;
>> + struct pci_dev *pdev = NULL;
>> +
>> + if (dma_reset_workaround == 0)
>> + return false;
>> + else if (dma_reset_workaround == 2)
>> + return true;
>> +
>> + for (i = 0; i < sizeof(broken_dev_id); i++) {
> This is broken. sizeof delivers the size in bytes, not in number of
> array elements. ARRAY_SIZE is what you want.
>
>> + pdev = pci_get_device(broken_dev_id[i].vendor, broken_dev_id[i].dev, NULL);
>> + if (pdev) {
>> + pci_dev_put(pdev);
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>> +
>> static int cx23885_initdev(struct pci_dev *pci_dev,
>> const struct pci_device_id *pci_id)
>> {
>> @@ -2069,6 +2117,8 @@ static int cx23885_initdev(struct pci_dev *pci_dev,
>> if (NULL == dev)
>> return -ENOMEM;
>>
>> + dev->need_dma_reset = cx23885_does_need_dma_reset();
>> +
>> err = v4l2_device_register(&pci_dev->dev, &dev->v4l2_dev);
>> if (err < 0)
>> goto fail_free;
>> diff --git a/drivers/media/pci/cx23885/cx23885.h b/drivers/media/pci/cx23885/cx23885.h
>> index d54c7ee..cf965ef 100644
>> --- a/drivers/media/pci/cx23885/cx23885.h
>> +++ b/drivers/media/pci/cx23885/cx23885.h
>> @@ -451,6 +451,8 @@ struct cx23885_dev {
>> /* Analog raw audio */
>> struct cx23885_audio_dev *audio_dev;
>>
>> + /* Does the system require periodic DMA resets? */
>> + unsigned int need_dma_reset:1;
>> };
>>
>> static inline struct cx23885_dev *to_cx23885(struct v4l2_device *v4l2_dev)
>>
