[PATCH 4/4] PCI: Add a REBAR size quirk for Sapphire RX 5600 XT Pulse

[Christian König]

Am 07.01.21 um 22:32 schrieb Bjorn Helgaas:
> On Thu, Jan 07, 2021 at 06:50:17PM +0100, Nirmoy Das wrote:
>> RX 5600 XT Pulse advertises support for BAR0 being 256MB, 512MB,
>> or 1GB, but it also supports 2GB, 4GB, and 8GB. Add a rebar
>> size quirk so that CPU can fully access the BAR0.
> This isn't quite accurate.  The CPU can fully access BAR 0 no matter
> what.  I think the problem you're solving is that without this quirk,
> BAR 0 isn't big enough to cover the VRAM.
>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> Reported-by: kernel test robot <lkp at intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> IIRC, these Reported-by lines are from the "cap == 0x3f0" problem.  It
> would make sense to include these if this patch fixed that problem in
> something that had already been merged.  But this *hasn't* been
> merged, so these lines only make sense to someone who trawls through
> the mailing list to find the previous version.
>
> I don't really think it's worthwhile to include them.  It's the same
> as giving credit to reviewers, which we typically don't do except via
> a Reviewed-by tag (which I think is too strong for this case) or a
> "v2" changes note after the "---" line.  That doesn't get included in
> the git history, but is easily findable via the Link: tags as below.
>
> If you merge these via a non-PCI tree, please include the "Link:"
> lines in the PCI patches, e.g.,
>
>    Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20210107175017.15893-5-nirmoy.das%40amd.com&data=04%7C01%7Cchristian.koenig%40amd.com%7C33c14f5361e84d9d0e4908d8b353c412%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637456519678601616%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wY9qaz3DTZA069qznMC9Wvoq9SZIzyJHE0XkaVXoqAc%3D&reserved=0
>
> for this one.  Obviously the link is different for each patch and will
> change if you repost the series.
>
> I'm not sure why you put the amd patch in the middle of the series.
> Seems like it would be slightly prettier and just as safe to put it at
> the end.
>
>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
> Acked-by: Bjorn Helgaas <bhelgaas at google.com>
>
> Let me know if you want me to take the series.

I will make the suggested changes and take this through the drm subsystem.

That makes more sense since it only affects our hardware anyway.

>
>> ---
>>   drivers/pci/pci.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 16216186b51c..b061bbd4afb1 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3577,7 +3577,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
>>   		return 0;
>>   
>>   	pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
>> -	return (cap & PCI_REBAR_CAP_SIZES) >> 4;
>> +	cap = (cap & PCI_REBAR_CAP_SIZES) >> 4;
>> +
>> +	/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
>> +	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
>> +	    bar == 0 && cap == 0x700)
>> +		cap = 0x3f00;
> I think this is structured wrong.  It should be like this so it's
> easier to match with the spec:
>
>    cap &= PCI_REBAR_CAP_SIZES;
>
>    if (... && cap == 0x7000)
>      cap = 0x3f000;
>
>    return cap >> 4;

Good point.

Thanks,
Christian.

>
>> +
>> +	return cap;
>>   }
>>   EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>>   
>> -- 
>> 2.29.2
>>