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: <23cd28ea-59ed-7687-0912-506356b4b24d@amd.com>
Date:   Mon, 31 Oct 2022 10:24:25 -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>, Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Cc:     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>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <Y117XNaSP6/8bH+3@moc6.cz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RThWUnRDcnR3TVJwaTV1TElLVkZ3UlJjNmkwbmUrM2Q0c3l2Y3c2cTZVdnJM?=
 =?utf-8?B?RmcvcDg1QmJGb0F2NDFsOXh2LzBvWlJ5dHpzUGVFaHdMSjQxenJCTlMrdWp6?=
 =?utf-8?B?czNuS0pzblpEVWRnY0dTaXJwdDFMMS9iYlQydGNnZVg4NVhOaFQzZnJyRVJ1?=
 =?utf-8?B?M2dBOTkzOUIrSnU0dTNUeXRySDYyZW5PL2lZZkMzY3dUNFhoZUFERHUzMmZn?=
 =?utf-8?B?dERJWEV3ZlhaMUtDMmw3WW9MQTlHamJXbWdMYmV3VlVPUkUvVlpIVWFFdkFj?=
 =?utf-8?B?V0U0ZHpSaGNLUnNLaGcwMllkNER2ZmI5R1pLR2QzV3AzT1kwL1gybU5WVWxX?=
 =?utf-8?B?QzN2ZkpNcWxsZFJCZ3dBTG5YZHBiQW92akpXRTVyRE9CNno2QTBRcUhIMnQv?=
 =?utf-8?B?bFpGRVlGcXlrQXhPRXRESGNWekY2MWR4M09LRkxqUGlmdU1lK0Q3STYrUFFG?=
 =?utf-8?B?SWlZWDNDUGoyekFpNzVqaUltRFZ6MzZvYlF2V2tJZng4REFwUkdUOVFSOUdh?=
 =?utf-8?B?anZaN1V4c1VScnptWU1ib0gvYkVHam1RL1Q2NkhjbDk2cEVkbUhoWlN0bWNw?=
 =?utf-8?B?blk1anVSWDRGZENhdjZqUnNkbGhLWFIxR3dpRDhyWGZNRmEzOVNtdTczVk4w?=
 =?utf-8?B?ejc3aThubll5QW9mR3ZUdk54aDNtMkVla3JqUTRLVXNWdDRSd0lKNkcxM0ly?=
 =?utf-8?B?UkEvTUNEdEtnbWRudmRqV1NjUDhzOWNZNHJXY01pcUt2ZXR3R0phMWJXSGlS?=
 =?utf-8?B?SjI4TThOejFSRGdJaDdOa0VZV0J4Q3FreVF1SFdTeTBBY3JxeEU0b21kdjd3?=
 =?utf-8?B?VWtlRTQzMVBpMnBueDRhOTNOMDJiZ1h5Uk5YSEpFbXFMenpteENzTnE5aWM3?=
 =?utf-8?B?YVBUdFd6Ull1RVpkZ2FXOG1jc0hkaWdjMi96SlhGb253RjVJaW5yUTgyODU5?=
 =?utf-8?B?ZitXbEpYaWJQSDVMS1lNQjlZejB0WEdnZDVKNGlkYWg3VFg4VzVIbHNEOFhn?=
 =?utf-8?B?UzRnWEZ0OXZZUUN2ek1BaDU5RUJSdFNIeStjbGpTbmVUZXQ5K092Rjc5MHVr?=
 =?utf-8?B?ZUZIWGR3aWtvQVZsbS9EZngralF6Qk93RGJuVitYNzhpM1hKKzJucXRnbEYy?=
 =?utf-8?B?MFQ2TTIyNk4zMlI4RVBCTWN6aVVvUWZlbXBtbk9nL1hTeEJTYmxib2NJaWlv?=
 =?utf-8?B?MWx4aExMRkhxTXlpVDloa0F4YVpPVUhlblFRbkVUdW1xTzVSWGl1Tkc1OGxX?=
 =?utf-8?B?TVJFYk4xbkxSMjdhSDFJdWFxT1FRNXhKdnlHbHVxb0Y3S25VN0pDMlNnWGpa?=
 =?utf-8?B?a2svRHNMZlhRaS9kUitQTHBkU0RKaDNlSG5OeGExOHZsaXBDS2prUWhDbVVX?=
 =?utf-8?B?TUtiVVJ3eXk5K2pRSDJveU5ydW1ULzVXNmNSLzZnbjdmemRDQzlCQVFXRmVO?=
 =?utf-8?B?OWtwRTQ2MVp3dzlzOUhLTDJyRVkwMWZBb0NaV2ZUTDlEVGplMU12bFJGcjk1?=
 =?utf-8?B?NVY0c3ZQS0xPSXkwU1pZWjBYSkhkY0l2dDJQbFV5QjhoQTRsbmJ5YmtLbGlJ?=
 =?utf-8?B?TGZlN1FXRXhibUZpQnIwcEFJMEJkYVJwT3o4Y1BCaGVodTJWQmZwRGhmeG5B?=
 =?utf-8?B?UnFaem96TDJlRmR4QVBzMlJ3TTRvejhvUW01SFhyNzk2cVExVXhVNXJ1dExM?=
 =?utf-8?B?ZFlwSkJodEN1bkpieWU3YlFkSFZxNW9TWXUvRDFTRkxYcEhFUVdwNVpMOXgr?=
 =?utf-8?B?SGhWYkptVzVLVEM3VytnWjRDR1JrVHVYUVlTaFRNd1A3V0syRGV3bG5TTlpn?=
 =?utf-8?B?UVZLSjJjWWZiMzk5WDNET0hDMlpUUklaTjNBeHltSUZpRU5IWTZwcVVtOG03?=
 =?utf-8?B?djNrQlVjL1dFdkJkSXhwaGc1Rjd0Mm9hVDhud0JwRFMyUzc4V3RNMVhZREdo?=
 =?utf-8?B?WlM1NWJLNVBWMmNWOE9wbStuNVEvTnRXb012MDZHV2dFWmtIQy9kbVBVUmdE?=
 =?utf-8?B?elpPdklueW42d0R3UHNGNlljSzVUcERRa05qUFQ1VU43M051eUtKekgyOUZL?=
 =?utf-8?B?SU0xeUx5OUdIVXk2RlBJRjdPOUcxMEZ3eVJ1NFBFSkR2UFc4R0FFY1ZqSElM?=
 =?utf-8?Q?yWfOFIPTY4hdCAfC8e76I/WVy?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3d5d0cf5-79b8-4721-1e5d-08dabb4b9fea
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Oct 2022 14:24:30.7770
 (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: gbhjbwFJLqHJZ3kdCAVajvsknebOCiG1Cxhxlksw162WhksI5pMwkRBNjhQH80VpTFvLR+C2FS0/fzueTC7CAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4879
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-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