Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Double application of PWM bitmask quirk and mathematical corruption on millinits. Pre-existing issues: - [High] Sleeping function called from atomic context. - [High] Potential division by zero in brightness conversion functions when processing malformed firmware capabilities. -- commit 5d73b2399ca1fd59d638eb586a83e6e63cc7a221 Author: Andrei Rusu de Castro <[email protected]> drm/amd/display: pass userspace brightness to power module The power module consumes millipercent and builds its own PWM lookup table from the ATIF brightness transfer characteristics. This patch passes the original userspace percentage to the power module on PWM panels and keeps the existing nits conversion and source-unit brightness mask for AUX panels. It also wires the brightness bypass to the power module's existing linear bypass. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > index 424b33573a734..04e810af4623c 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > @@ -218,6 +218,41 @@ u32 convert_brightness_to_user(const struct > amdgpu_dm_backlight_caps *caps, > } > EXPORT_IF_KUNIT(convert_brightness_to_user); [Severity: High] This isn't a bug introduced by this patch, but can convert_brightness_to_user() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c trigger a kernel panic via division by zero? If a connected monitor's EDID/DPCD reports identical max_luminance and min_luminance values: amdgpu_dm_update_backlight_caps() will set aux_max_input_signal == aux_min_input_signal. This causes max - min == 0 to be returned by get_brightness_range(). Subsequently, calling amdgpu_dm_backlight_get_level() will execute convert_brightness_to_user(), which evaluates DIV_ROUND_CLOSEST_ULL(..., max - min), triggering a division by zero. This could allow a Denial of Service attack against the host system by plugging in an untrusted, maliciously crafted DisplayPort monitor or dock. > +static u32 convert_brightness_to_millipercent(const struct > amdgpu_dm_backlight_caps *caps, > + u32 brightness) [ ... ] > +STATIC_IFN_KUNIT > +u32 convert_brightness_for_power_module(const struct > amdgpu_dm_backlight_caps *caps, > + u32 user_brightness) > +{ > + u32 brightness; > + > + if (!caps) > + return user_brightness; > + > + if (!caps->aux_support) > + return convert_brightness_to_millipercent(caps, > user_brightness); > + > + brightness = convert_brightness_from_user(caps, user_brightness); > + if (caps->brightness_mask) > + brightness |= caps->brightness_mask; [Severity: High] Does convert_brightness_for_power_module() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c erroneously apply the hardware bitmask quirk to a mathematical millinits value prior to handoff? For AUX panels, caps->brightness_mask is OR'ed into the mathematically scaled millinits brightness variable here. This corrupted value is then passed to mod_power_set_backlight_nits(), which converts it to a PWM value. > + > + return brightness; > +} > + > +EXPORT_IF_KUNIT(convert_brightness_for_power_module); > + > STATIC_IFN_KUNIT > struct dc_stream_state *dm_find_stream_with_link( > struct amdgpu_display_manager *dm, > @@ -262,7 +297,6 @@ void amdgpu_dm_backlight_set_level(struct > amdgpu_display_manager *dm, > bool rc = false, reallow_idle = false; > struct drm_connector *connector; > struct dc_stream_state *stream; [Severity: High] This isn't a bug introduced by this patch, but does amdgpu_dm_backlight_set_level() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c safely handle calling mutex_lock(&dm->dc_lock)? amdgpu_dm_backlight_set_level() explicitly locks the mutex, but it is reached via amdgpu_dm_atomic_commit_tail() during non-blocking display atomic commits. Calling a sleeping function from atomic context can lead to kernel warnings (lockdep splats), system instability, and potential deadlocks. > > list_for_each_entry(connector, &dm->ddev->mode_config.connector_list, > head) { > struct amdgpu_dm_connector *aconnector = > to_amdgpu_dm_connector(connector); [ ... ] > diff --git a/drivers/gpu/drm/amd/display/modules/power/power_abm.c > b/drivers/gpu/drm/amd/display/modules/power/power_abm.c > index 5e86889eaa841..4d7d80ac60c9b 100644 > --- a/drivers/gpu/drm/amd/display/modules/power/power_abm.c > +++ b/drivers/gpu/drm/amd/display/modules/power/power_abm.c > @@ -1020,6 +1020,10 @@ void fill_backlight_level_params(struct core_power > *core_power, > > if (backlight_control_type == BACKLIGHT_CONTROL_AMD_AUX && !is_hdr) > backlight_level_params->control_type = BACKLIGHT_CONTROL_PWM; > + > + if (backlight_level_params->control_type == BACKLIGHT_CONTROL_PWM) > + backlight_level_params->backlight_pwm_u16_16 |= > + bl_prop->brightness_mask; [Severity: High] For non-HDR AUX panels falling back to PWM, does fill_backlight_level_params() in drivers/gpu/drm/amd/display/modules/power/power_abm.c apply the bitmask a second time onto the resulting PWM value, leading to undefined hardware behavior? > } > > bool mod_power_set_backlight_nits(struct mod_power *mod_power, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
