Received-SPF: pass (google.com: domain of linux-kernel+bounces-208855-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <556979e9-c3d1-45d9-aaa5-b5a17422db5f@amd.com>
Date: Mon, 10 Jun 2024 15:20:20 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/amd: force min_input_signal to 0 on Framework AMD
 13/16
To: =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>
Cc: Alex Deucher <alexander.deucher@amd.com>,
 =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 Kieran Levin <ktl@framework.net>, amd-gfx@lists.freedesktop.org,
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
 Dustin Howett <dustin@howett.net>, Matt Hartley <matt.hartley@gmail.com>
References: <20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net>
 <74f3c189-f3d3-4dca-9357-d4bc8f98da08@amd.com>
 <0759ed40-efef-4230-86fc-cdf6702843e6@t-8ch.de>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <0759ed40-efef-4230-86fc-cdf6702843e6@t-8ch.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?ZENTb2pVY2xlNzRHTlV5WU42dlRONng1c05NM0pXbnBuc3BGeGNsNGVkNGxE?=
 =?utf-8?B?ZjNVUXEyNkNjalBjbGg4a096NU9nVlRUUVV4enNkUTdMYWVFRmRVMmowRmtv?=
 =?utf-8?B?MVp6Rm84NmpzS1RhNEZmZHFWdmRxQjNQSzN5WnNIdGgvN0tnRG5VN2tVSE1U?=
 =?utf-8?B?OHhDOWZHUnM4cDZ4QlZWc0NMTnovWjlkbEtCc0VsY0h5YWpUekVIaFN6a09o?=
 =?utf-8?B?NWUvOVlsQ1lCbVFIL3ZWcUwrWmQ2RHpmaStCMlEvNnVnS2ZRL3VRVHVsd0x4?=
 =?utf-8?B?ejhxclpkVlkyN0wwSmJjOEw0TDZ5NWZ0bXlVVG5SRFR5ZmcyRlVVcWtNUElU?=
 =?utf-8?B?RTZ6RFdYd3p2MUVDUi9zNGlaR014RExvWjNQR0dDUzRWUTZqK2hRYUZaRHRn?=
 =?utf-8?B?SGxReFE4SUVkbzRpVW5LWC9BbWlwUUVEQmwzVGdMNEVzV0d0cGFYZk1YejdC?=
 =?utf-8?B?eVFSWDNoYmtDQ0NYektVK2JFdEJsWkR6OEdUa2dZSEFHYmFqM05oVmZqZzlC?=
 =?utf-8?B?TEZzTVU5QnNSTDNXMUtBT3JHL0ZXcFpXb3FzTmhEejBHSk9Md0poTGhWNjFY?=
 =?utf-8?B?MFdFOEZTYnNQQzNjZVlsbkZ5VVlTVjhLc0lKZU5Xc3JPYStrRVFpUzFVQkZD?=
 =?utf-8?B?VTdZUllxdkZvQ0NpZC9uV3VRS2VIclhLa0ZSL243b0h3a2s5b01xRUFWOEVP?=
 =?utf-8?B?aXFNUW5LdDV1ZU9UdVhBbzlHL2I3RG9lUGJNMFM5VkxHRVF3c1BNVXFnVE44?=
 =?utf-8?B?a1V3K3RpQnBDRkNtUktEYmpnYk1VenYxTDRJRTdvUlEvTXVRNEZZVno0N2Rx?=
 =?utf-8?B?ekJtZkU2c1pTdnBEREVTQ05HcmZhOVhyZmlSS3JRVkhEOU5oUEZ4WXpvcUpG?=
 =?utf-8?B?UWNDRThtczk1YWw1dm1od3JXV2tsN1lIQnlkWDlWa0E5REM4Rm5zMVNNOS9z?=
 =?utf-8?B?ejlqU21wZ0NFV2dlM0pNTjZ3VFY5K1VNK2lOL1JGWXozMXFIQ2NJYnVPcXla?=
 =?utf-8?B?RlJwTWVNdC90TElLaHNERTljOEF5TzcydEFpTUxqd0gzKzl0YmVrS1VUVG9M?=
 =?utf-8?B?N2RtVUFpN0ExeW5qMmhCQmhEeDQ1Wmd2dDA5b09mQk8rUkhiQUFUeExCQ3p5?=
 =?utf-8?B?bDhpdVBubkMvaTBTbkVtNk5BOHBtMXlSc2pqMlhPcGk1NHU3Q3dnZklXSWIz?=
 =?utf-8?B?K3daV0VLMytxN3I4N1M3dE1Yd3NPYThhWWYwWTVzQ09QOG13aVhjekJhUTdX?=
 =?utf-8?B?alJZMG5FdDhlNnRSYzhBdkJNeDdMTU5zSlg2RlZMNGwyTGlDWjZkR0pxT3lQ?=
 =?utf-8?B?d0NGcEhpRSs1TDd6NXpqVWxRNEo3ZlVDWGlDVzMzWk1DUHVWUXoxdSsvRk1x?=
 =?utf-8?B?elArNUh6bTh4UDhCOEtDcXAvYmQ2cUp3L1dqQUw3SGpmOVE0Z3k0UGNEWlI5?=
 =?utf-8?B?cllqaUszQ2ZySGxyODR2d2FNMWh3Q3VFdkRadldOK0hhekdRRE5TUUh0dkI3?=
 =?utf-8?B?RG5BdTVnTk9JREU1aG90M0x1VWl3ZG9LblZwZytHRiszOU9POVpGdmVwTUJD?=
 =?utf-8?B?cEJrYXpGcG1yb0I2b0ovK08zUFBTTStXTTd4dDIyTENKYXB1V29neUZOWVJR?=
 =?utf-8?B?RjNEeDk2TENDa3gzR2psTFZUUis3eldwOGNGNFJ2MUxUVmxGSC9rK1ZtTUkw?=
 =?utf-8?B?TE16amdwMFJQcWVSMENjNFNQNkg1cjEzNm83WjN2eWFpL08yNzRROFA3V3cx?=
 =?utf-8?B?ck9TdVdYNk5xTFYyL1VYaW42dDVSYWFoRkI2TWNjNk55Y3pFbDNGSVlhcVo3?=
 =?utf-8?B?bWlzWVRyUkpYcVRZN1Z4b3pRbkxNaDk5ekgxQ0g4dE9UVnFMdThYb0h1bU5U?=
 =?utf-8?B?K2xPWVdQcTRiVEFMTTVSTGh4bjZ3WDJsdU9WbStmdGFqTk1RU3d1YU52anRH?=
 =?utf-8?B?aUhiM2RzdkJocXJ4cmdvcmZvaWFhN0JhcHk4RmJVaHdPL1F5VDFPSVNQekdi?=
 =?utf-8?B?bExRNE40NWdkSGpuZk02SGxDdG5rZVlHOUJPcTFNMHhRV09ub2ErNmRZeWQ5?=
 =?utf-8?B?S2dSMXRwS0hjQVZqaWZMSzIxRXYrYVR3MXZYVWU3R3VEQ2lmdklMZnhhak81?=
 =?utf-8?Q?79ugpPGgYSi/qoNEur2CMEL+F?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ab10e2ca-45a6-4b9e-2ee3-08dc898ac1bf
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2024 20:20:23.0221
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PcGTI+3+i/3YmztJg+D3YSexGQKLbusfovW1wzKNqpw4U0+akBJuKNPlCr9eWPqHZFNGMH+2AHTIHr6fFXLLiw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB9100

On 6/10/2024 15:12, Thomas Weißschuh wrote:
> On 2024-06-10 14:58:02+0000, Mario Limonciello wrote:
>> +Kieran
>>
>> On 6/10/2024 14:26, Thomas Weißschuh wrote:
>>> The value of "min_input_signal" returned from ATIF on a Framework AMD 13
>>> is "12". This leads to a fairly bright minimum display backlight.
>>>
>>> Introduce a quirk to override "min_input_signal" to "0" which leads to a
>>> much lower minimum brightness, which is still readable even in daylight.
>>>
>>> Tested on a Framework AMD 13 BIOS 3.05 and Framework AMD 16.
>>>
>>> Link: https://community.frame.work/t/25711/9
>>> Link: https://community.frame.work/t/47036
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 35 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> index 7099ff9cf8c5..b481889f7491 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> @@ -25,6 +25,7 @@
>>>    #include <linux/pci.h>
>>>    #include <linux/acpi.h>
>>>    #include <linux/backlight.h>
>>> +#include <linux/dmi.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/xarray.h>
>>>    #include <linux/power_supply.h>
>>> @@ -130,6 +131,35 @@ static struct amdgpu_acpi_priv {
>>>    	struct amdgpu_atcs atcs;
>>>    } amdgpu_acpi_priv;
>>> +struct amdgpu_acpi_quirks {
>>> +	bool ignore_min_input_signal;
>>> +};
>>> +
>>> +static const struct dmi_system_id amdgpu_acpi_quirk_table[] = {
>>> +	{
>>> +		/* the Framework Laptop 13 (AMD Ryzen) and 16 (AMD Ryzen) */
>>> +		.matches = {
>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "AMD Ryzen"),
>>> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Laptop"),
>>> +		},
>>
>> Two problems I see:
>>
>> 1) This really "should" be fixed in the BIOS. I added Kieran to the thread
>> for comments if that's viable.
> 
> Agreed!
> 
>> 2) IMO this is going to match too liberally across all potential Framework
>> models.  If they introduce a refreshed motherboard for either product then
>> the quirk would apply to both products when we don't know that such a
>> deficiency would exist.
> 
> Also agreed.
> In addition to be really specific this should also match by display type
> (via EDID?).
> 
> So far this was only tested with the matte panel.
> (I forgot to mention that, sorry)

Yeah; I would expect this also matters for the new high res panel that 
they announced whether this value can work.

> 
>> You can reference drivers/platform/x86/amd/pmc/pmc-quirks.c for what we used
>> for a quirk that was matching against a single product and single BIOS.
> 
> Will do for the next revision, but let's gather some feedback first.

????

> 
>> But FWIW if that issue isn't fixed in the next BIOS I think we'll end up
>> needing to tear out the BIOS string match and match just the platform.
> 
> I'm wondering what the longterm strategy will have to be.
> Given that there are different kinds of displays, and new ones will be
> released, each new display type will require an update to the firmware.
> 
> When there are no firmware updates for a device anymore, but new,
> compatible displays are released, then the kernel will need the quirks
> again.

Yeah I think all this points to the 'best' home for this is BIOS.

Framework can test whether the 0 value works on all the displays they 
want to support and look for negative impacts for all OSes they support.

> 
>>> +		.driver_data = &(struct amdgpu_acpi_quirks) {
>>> +			.ignore_min_input_signal = true,
>>> +		},
>>> +	},
>>> +	{}
>>> +};
>>> +
>>> +static const struct amdgpu_acpi_quirks *amdgpu_acpi_get_quirks(void)
>>> +{
>>> +	const struct dmi_system_id *dmi_id;
>>> +
>>> +	dmi_id = dmi_first_match(amdgpu_acpi_quirk_table);
>>> +	if (!dmi_id)
>>> +		return NULL;
>>> +	return dmi_id->driver_data;
>>> +}
>>> +
>>>    /* Call the ATIF method
>>>     */
>>>    /**
>>> @@ -1388,6 +1418,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>>>     */
>>>    void amdgpu_acpi_detect(void)
>>>    {
>>> +	const struct amdgpu_acpi_quirks *quirks = amdgpu_acpi_get_quirks();
>>>    	struct amdgpu_atif *atif = &amdgpu_acpi_priv.atif;
>>>    	struct amdgpu_atcs *atcs = &amdgpu_acpi_priv.atcs;
>>>    	struct pci_dev *pdev = NULL;
>>> @@ -1429,6 +1460,10 @@ void amdgpu_acpi_detect(void)
>>>    					ret);
>>>    			atif->backlight_caps.caps_valid = false;
>>>    		}
>>> +		if (quirks && quirks->ignore_min_input_signal) {
>>> +			DRM_INFO("amdgpu_acpi quirk: min_input_signal=0\n");
>>> +			atif->backlight_caps.min_input_signal = 0;
>>> +		}
>>>    	} else {
>>>    		atif->backlight_caps.caps_valid = false;
>>>    	}
>>>
>>> ---
>>> base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
>>> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
>>>
>>> Best regards,
>>