Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <36b34da1-914c-898f-750a-e02790f48a17@amd.com>
Date:   Wed, 2 Nov 2022 10:57:46 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH] drm/amd/display: add parameter backlight_min
Content-Language: en-US
To:     Filip Moc <dev@moc6.cz>
Cc:     Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <Y117XNaSP6/8bH+3@moc6.cz>
 <23cd28ea-59ed-7687-0912-506356b4b24d@amd.com> <Y2E8SsisrDn/f6+c@moc6.cz>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <Y2E8SsisrDn/f6+c@moc6.cz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZUJ5QitpRFlEWHVWRTRXM3N5UHAySlVIM1RaUDJwMFpUV2Exc2hmamYzQ2py?=
 =?utf-8?B?NVozbm02Qy9CblEwcS9HOVJlRGk5cGd1dGFMOE1hY2J0dU5vaWFNa0JXeDhr?=
 =?utf-8?B?ZU1KeERLaUw3b3lHUmRTSENYRU1HTVo0U0ZMTkZQR1RZSkhCT0g3eVFTbEtB?=
 =?utf-8?B?UkhLeUhYTW4raW40V2pRYWpTQktsL09ma1BaRnQ5bFdsYTd4RzVlRUVHbW9z?=
 =?utf-8?B?V2pMZHdJTzIwV0NudFU0Z2krY3g0V1pPZVVCWVlsWGZ5U1RmRVZsQ3RHQ1Bv?=
 =?utf-8?B?N2FySzNMcTR0U1BGaGdCejlEMWNxQ1I1N1poOWtNb0FxVS93SjRPaGhQL0R0?=
 =?utf-8?B?RU9ORzAyTWpZSFdGeU10bmZmalpFc0grMnRZK3ZUaWtPODdoMVRXQ1lFc1Bt?=
 =?utf-8?B?RFFKbzlvVGZhNXZQT2ZIb0lNYStwTVUzRXdSL25ESnJ3bjZuU3FKNk1CQ3BF?=
 =?utf-8?B?ZWZxbWZKYkNlRDlndWYzSDBKTUJBdmxLT0Zhb1BHNkFJWDJTaGorSXZsRjYr?=
 =?utf-8?B?ZldNRkwvR05KWG9FSE45Z3pBSnNsS1FoZnlhRElXRFFzclZyRk9xT0ZFQkhQ?=
 =?utf-8?B?SVMrWEY2dm1SR2o0R0p2UDNZS211S1hqNVg4WFZyMWxlVWlZSzlmVDVkdzJq?=
 =?utf-8?B?Q3NLV0pxM28rTmJDcWV2dUJ1dzIwb3gvQ3pxOWxwaloxOXpqWVRjaUtBeFZq?=
 =?utf-8?B?T0JFSFpRc2xubU5HUjIzYTdjMTlaQlIwUFVzaHNZSjNOWjE0QlVCeHh1Ykhq?=
 =?utf-8?B?QjlwdVkxcWVSQU1uYzNZNEVPS1VuWTVsMlZ3VDBoSDVtUWZuVGgxZXJFdkpL?=
 =?utf-8?B?Vi8relBDMVo5aXNKVStLblk4Ykh5QmFtSmVrOW9WWGd4WlcrckNIK3M0K1Np?=
 =?utf-8?B?YVkvOGVQZDZEZVV4ZHplSVd4SFlpTGR5dmRGSHpnTm9nVGZFNHZFMDYvcVJR?=
 =?utf-8?B?UTdkUzRvL3o3N0xwTm85eVNBV1kxN3l0RXNpUko0SEVFTWZQd3ZaY3F0c2hT?=
 =?utf-8?B?ZDc5MmZSaE1ZMnF6cnMxd1FJa2JDSnVNWnMxNkc0WlFjKzk1MFVEUW9YSHpO?=
 =?utf-8?B?N1dRMGVOajJIOUk2NC8veEFKQzFsWUx6YVUzZXNoMzl3c2hQZzZaUFpzTjJS?=
 =?utf-8?B?aWdWRDJDMEk3SFFkSjNSQnV4OXBCTEJjbjB4MklkSEh3UW4yeUpvU0E3Zng0?=
 =?utf-8?B?WDRLN0VHczJqQUh3MDJsbEpRTEtUQ2lKSndzaVJRT25NbmhaLytBWkdQL0Js?=
 =?utf-8?B?VHhCbE1LVjlYaStJaWo2WisrVUZsQkh3alYzeDRGdS8rRTh4ZDJNcHpYeTdX?=
 =?utf-8?B?S3JXQ0w0d0dQQ0hSOFlrQ25YdDVUUWlIakwyOVJYOWpwMUVudnJMME5GODJS?=
 =?utf-8?B?NGFjQy9SMVpzUlRpcy9SVWpDblhwbTRGNWFuNjhvUDY2YTlNS2M1WjBQbVBJ?=
 =?utf-8?B?SytSSXFsSlBhUWxvSVlGODh4aHFTVzNVZ0NiT2JPOWxNdzhsazNXVSt5U09F?=
 =?utf-8?B?OEJtSDFEcDBrMko5K1lUSXBmU1dPVTVuN21UKzVJUjlTSkZnVEFaTitOcHBx?=
 =?utf-8?B?NytvNnVEVTVoWEFXczBmOHdUZ2xRL2pGRFVLbzNFdWE1cDI4NDFuM1JVTnFq?=
 =?utf-8?B?dHVkdmtSQXpZRGN5U2RRZVpCSFZ4cmFiN1pUQksvWlhocE1OQ05GeWwvL1F2?=
 =?utf-8?B?Q1dOYTFXdjNFcVBlQ0JTcnBVd0lEUmZoV25UU1FjM1czdVFsbmtLeHlmQkRB?=
 =?utf-8?B?WTRUWjVTZjZ1MW5ReE9xMUo3NkdqUEl3OUdRZDZUS3ZtbDE0c0dobC9kY2Fl?=
 =?utf-8?B?OVA4bGRFT0kxMVdmTi9OWVJLVDN4c3V0NWxCdGlBbjBSZ0lRSlR2SWw1dmM2?=
 =?utf-8?B?dXJNaWZsL3BEc2N2WkZFdklTOXJXQ2dvV0l0NXF6emNYTFdTWmtEYjA2bnlm?=
 =?utf-8?B?ZzdhdExCZlVkcncxQzhDTTVqVWtXcW1ZOUk2WFM0d1Y3YXJGMzByY1YvNjlS?=
 =?utf-8?B?bnU5ZzkwTFBmY2QzLy9tNW13aU9LQlJacGp2a0VlQTk5SVNvdW1oZ1BpdGIw?=
 =?utf-8?B?YmZuK3Byak5GN2hzVVRnRnBTWXNMRHNYK2JPcWZnLytkbW12WUZmZWh0Qy9k?=
 =?utf-8?Q?SIa1y0eftV1Sy3QGCgYW9AioH?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 826bb71a-7455-4515-355f-08dabce29fbd
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2022 14:57:55.7298
 (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: KZKvV9xDzfYS1dxSzLYyNR+ImhO0vzJnJkpxgj8Ayu/RZrYGs/9UGhSg4t2fM6ScGfgicERe1d4G/qQL6j3Zqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB6732
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2022-11-01 11:33, Filip Moc wrote:
> Hello Harry,
> 
> thank you for your response.
> 
>> amdgpu.backlight_min=2:-1
> 
> almost :-)
> 
> Array elements in module parameters are separated by commas not colons.
> So for cmdline it should look like this:
> amdgpu.backlight_min=2,-1
> 
> Though you can just drop the ,-1 relying on kernel leaving the rest of array
> untouched. Which I would recommend as there is no point for user to
> follow AMDGPU_DM_MAX_NUM_EDP.
> Only when you need to override some other display than display 0 then you may
> need to use -1. E.g. backlight_min=-1,2 overrides display 1 to min backlight=2
> while keeping display 0 with no override.
> 
> When amdgpu is loaded as a kernel module, backlight_min can also be passed as a
> parameter to modprobe, e.g.:
> modprobe backlight_min=2
> So in that case you probably want to add something like
> options amdgpu backlight_min=2 to /etc/modprobe.d/amdgpu.conf
> (and also run update-initramfs if amdgpu is loaded by initramfs).
> 
> I'll add some examples to commit message in v2.
> 

Awesome. Thanks.

Harry

> Filip
> 
> 
> V Mon, Oct 31, 2022 at 10:24:25AM -0400, Harry Wentland napsal(a):
>> On 2022-10-29 15:13, Filip Moc wrote:
>>> There are some devices on which amdgpu won't allow user to set brightness
>>> to sufficiently low values even though the hardware would support it just
>>> fine.
>>>
>>> This usually happens in two cases when either configuration of brightness
>>> levels via ACPI/ATIF is not available and amdgpu falls back to defaults
>>> (currently 12 for minimum level) which may be too high for some devices or
>>> even the configuration via ATIF is available but the minimum brightness
>>> level provided by the manufacturer is set to unreasonably high value.
>>>
>>> In either case user can use this new module parameter to adjust the
>>> minimum allowed backlight brightness level.
>>>
>>
>> Thanks for this patch and covering all the bases.
>>
>> It might be useful to have an example in the commit description on
>> how to set the array property. I assume it looks like this if I
>> wanted to set the first device to a minimum of 2 and leave the default
>> for the 2nd one:
>>
>> amdgpu.backlight_min=2:-1
>>
>> Either way, this patch is
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>>
>> Harry
>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203439 
>>> Signed-off-by: Filip Moc <dev@moc6.cz>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu.h               |  3 +++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c           | 15 +++++++++++++++
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++++++++++++++
>>>  3 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 0e6ddf05c23c..c5445402c49d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -200,6 +200,9 @@ extern uint amdgpu_dc_debug_mask;
>>>  extern uint amdgpu_dc_visual_confirm;
>>>  extern uint amdgpu_dm_abm_level;
>>>  extern int amdgpu_backlight;
>>> +#ifdef CONFIG_DRM_AMD_DC
>>> +extern int amdgpu_backlight_override_min[];
>>> +#endif
>>>  extern struct amdgpu_mgpu_info mgpu_info;
>>>  extern int amdgpu_ras_enable;
>>>  extern uint amdgpu_ras_mask;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 16f6a313335e..f2fb549ac52f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -43,6 +43,7 @@
>>>  #include "amdgpu_irq.h"
>>>  #include "amdgpu_dma_buf.h"
>>>  #include "amdgpu_sched.h"
>>> +#include "amdgpu_dm.h"
>>>  #include "amdgpu_fdinfo.h"
>>>  #include "amdgpu_amdkfd.h"
>>>  
>>> @@ -853,6 +854,20 @@ int amdgpu_backlight = -1;
>>>  MODULE_PARM_DESC(backlight, "Backlight control (0 = pwm, 1 = aux, -1 auto (default))");
>>>  module_param_named(backlight, amdgpu_backlight, bint, 0444);
>>>  
>>> +/**
>>> + * DOC: backlight_min (array of int)
>>> + * Override minimum allowed backlight brightness signal (per display).
>>> + * Must be less than the maximum brightness signal.
>>> + * Negative value means no override.
>>> + *
>>> + * Defaults to all -1 (no override on any display).
>>> + */
>>> +#ifdef CONFIG_DRM_AMD_DC
>>> +int amdgpu_backlight_override_min[AMDGPU_DM_MAX_NUM_EDP] = {[0 ... (AMDGPU_DM_MAX_NUM_EDP-1)] = -1};
>>> +MODULE_PARM_DESC(backlight_min, "Override minimum backlight brightness signal (0..max-1, -1 = no override (default))");
>>> +module_param_array_named(backlight_min, amdgpu_backlight_override_min, int, NULL, 0444);
>>> +#endif
>>> +
>>>  /**
>>>   * DOC: tmz (int)
>>>   * Trusted Memory Zone (TMZ) is a method to protect data being written
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index eb4ce7216104..e2c36ba93d05 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -3911,6 +3911,21 @@ static void amdgpu_dm_update_backlight_caps(struct amdgpu_display_manager *dm,
>>>  	dm->backlight_caps[bl_idx].min_input_signal = AMDGPU_DM_DEFAULT_MIN_BACKLIGHT;
>>>  	dm->backlight_caps[bl_idx].max_input_signal = AMDGPU_DM_DEFAULT_MAX_BACKLIGHT;
>>>  #endif
>>> +
>>> +	if (amdgpu_backlight_override_min[bl_idx] >= 0) {
>>> +		if (amdgpu_backlight_override_min[bl_idx] < dm->backlight_caps[bl_idx].max_input_signal) {
>>> +			DRM_INFO("amdgpu: backlight[%i]: overriding minimum brightness from %i to %i\n",
>>> +				  bl_idx,
>>> +				  dm->backlight_caps[bl_idx].min_input_signal,
>>> +				  amdgpu_backlight_override_min[bl_idx]);
>>> +			dm->backlight_caps[bl_idx].min_input_signal = amdgpu_backlight_override_min[bl_idx];
>>> +		} else {
>>> +			DRM_ERROR("amdgpu: backlight[%i]: minimum brightness override (%i) is not below maximum (%i)\n",
>>> +				  bl_idx,
>>> +				  amdgpu_backlight_override_min[bl_idx],
>>> +				  dm->backlight_caps[bl_idx].max_input_signal);
>>> +		}
>>> +	}
>>>  }
>>>  
>>>  static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
>>>
>>> base-commit: d8c03bfe146fd5e081a252cd34f3f12ca0255357
>>