[Intel-gfx] [PATCH] drm/i915/display: assume some pixelrate for src smaller than 1
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Wed Jan 11 21:25:48 UTC 2023
On 11.1.2023 22.09, Drew Davenport wrote:
> On Wed, Jan 11, 2023 at 09:39:26PM +0200, Ville Syrjälä wrote:
>> On Wed, Jan 11, 2023 at 11:28:51AM -0700, Drew Davenport wrote:
>>> On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote:
>>>> On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote:
>>>>> intel_adjusted_rate() didn't take into account src rectangle
>>>>> can be less than 1 in width or height.
>>>>
>>>> This should not get called in those cases. What does the
>>>> backtrace look like?
>>>
>>> In my repro of this issue, the backtrace looks as follows:
>>>
>>> [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53
>>> [ 180.798336] Code: <snip long line>
>>> [ 180.798338] RSP: 0018:ffffb080ce4179b8 EFLAGS: 00010246
>>> [ 180.798341] RAX: ffffffffffffffff RBX: ffff98cd22a24000 RCX: 0000000000000a00
>>> [ 180.798343] RDX: 0000000000000000 RSI: ffff98cccbae7000 RDI: 0000000000000000
>>> [ 180.798346] RBP: ffffb080ce4179b8 R08: 0000000000087780 R09: 0000000000000002
>>> [ 180.798348] R10: 0000000000000a00 R11: 0000000000000000 R12: 0000000000000000
>>> [ 180.798350] R13: ffff98cd0e495400 R14: ffff98ccc34e0000 R15: ffff98cccbae7000
>>> [ 180.798352] FS: 00007b84119b5000(0000) GS:ffff98d02f900000(0000) knlGS:0000000000000000
>>> [ 180.798354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [ 180.798357] CR2: 00007ffc2d5e4080 CR3: 00000001042ee006 CR4: 0000000000770ee0
>>> [ 180.798359] PKRU: 55555554
>>> [ 180.798361] Call Trace:
>>> [ 180.798364] <TASK>
>>> [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea
>>> [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145
>>> [ 180.798373] intel_atomic_check_planes+0x263/0x7ce
>>> [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923
>>> [ 180.798380] intel_atomic_check+0x14e4/0x184d
>>> [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a
>>> [ 180.798386] drm_atomic_check_only+0x3ec/0x98f
>>> [ 180.798391] drm_atomic_commit+0xa2/0x105
>>> [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5
>>> [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f
>>> [ 180.798400] drm_mode_setplane+0x236/0x30c
>>> [ 180.798404] ? drm_any_plane_has_format+0x51/0x51
>>> [ 180.798407] drm_ioctl_kernel+0xda/0x14d
>>> [ 180.798411] drm_ioctl+0x27e/0x3b4
>>> [ 180.798414] ? drm_any_plane_has_format+0x51/0x51
>>> [ 180.798418] __se_sys_ioctl+0x7a/0xbc
>>> [ 180.798421] do_syscall_64+0x55/0x9d
>>> [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b
>>> [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb
>>>
>>> If this function shouldn't be called in such a case, then perhaps
>>> I should revist my original attempt at fixing this in
>>> https://patchwork.freedesktop.org/patch/516060 by rejecting such a
>>> configuration?
>>
>> I'm saying that this should be impossible already. At least
>> I can't immediately see anything that could call this with
>> an invisible plane.
>
> In my repro case, I called drmModeSetPlane with the src_h parameter set
> to 65535 (so the largest 16.16 number that's less than one). This got
> through any existing checks on the height of the src rect, resulting in
> the divide-by-zero error in intel_plane_pixel_rate.
>
> While investigating this, I tried setting src_h to 0, but this
> configuration got rejected somewhere along the line before it got
> through the intel_plane_pixel_rate.
>
Here's one of the igt tests I had come up when debugging this:
---
#include "igt.h"
igt_main
{
igt_subtest_f("testi") {
int drm_fd = drm_open_driver_master(DRIVER_INTEL);
igt_display_t display;
igt_plane_t* plane;
igt_output_t *output;
igt_fb_t fb;
kmstest_set_vt_graphics_mode();
igt_display_require(&display, drm_fd);
igt_display_require_output(&display);
output = igt_get_single_output_for_pipe(&display, PIPE_A);
igt_output_set_pipe(output, PIPE_A);
igt_display_commit_atomic(&display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
igt_create_pattern_fb(drm_fd, 256, 256,
DRM_FORMAT_XRGB8888,
DRM_FORMAT_MOD_NONE,
&fb);
plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
igt_plane_set_position(plane, 0, 0);
igt_plane_set_fb(plane, &fb);
igt_plane_set_prop_value(plane, IGT_PLANE_SRC_H, IGT_FIXED(0, 30));
igt_plane_set_size(plane, 256, 8);
igt_display_commit_atomic(&display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
}
}
---
This should give callstack ending same way as what Drew had seen. With
my patch in place this test will die with einval coming from kernel from
skl_update_scaler(..) where it doesn't pass range checks (and will note
on dmesg about it)
/Juha-Pekka
>>
>>>
>>> I'll respond to Alan on that thread.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++---
>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>> index 10e1fc9d0698..a9948e8d3543 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>>>> @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
>>>>> const struct drm_rect *dst,
>>>>> unsigned int rate)
>>>>> {
>>>>> - unsigned int src_w, src_h, dst_w, dst_h;
>>>>> + unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
>>>>>
>>>>> src_w = drm_rect_width(src) >> 16;
>>>>> src_h = drm_rect_height(src) >> 16;
>>>>> @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
>>>>> dst_w = min(src_w, dst_w);
>>>>> dst_h = min(src_h, dst_h);
>>>>>
>>>>> - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
>>>>> - dst_w * dst_h);
>>>>> + /* in case src contained only fractional part */
>>>>> + dst_wh = max(dst_w * dst_h, (unsigned) 1);
>>>>> +
>>>>> + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
>>>>> }
>>>>>
>>>>> unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
>>>>> --
>>>>> 2.37.3
>>>>
>>>> --
>>>> Ville Syrjälä
>>>> Intel
>>
>> --
>> Ville Syrjälä
>> Intel
More information about the Intel-gfx
mailing list