Re: [Intel-gfx] [PATCH] drm/i915/display: assume some pixelrate for src smaller than 1

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

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@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