Re: [PATCH v2] drm/amd/display: Reject non-zero src_y and src_x for video planes

[Harry Wentland <harry.wentland@amd.com>]

On 2021-04-23 10:18 a.m., Christian König wrote:
> Good that this has been found. Just a curious guess, but have you guys 
> checked if the src_x and src_y are a multiple of 2?
> 

That was one of my first guesses but I still observed the hang after
forcing src_x and src_y to be multiples of 2.

> Might cause problems to try to access a subsampled surface if the 
> coordinates doesn't make much sense.
> 
> Anyway patch is Acked-by: Christian König <christian.koenig@amd.com>
> 

Thanks,
Harry

> Regards,
> Christian.
> 
> Am 23.04.21 um 16:09 schrieb Harry Wentland:
>> [Why]
>> This hasn't been well tested and leads to complete system hangs on DCN1
>> based systems, possibly others.
>>
>> The system hang can be reproduced by gesturing the video on the YouTube
>> Android app on ChromeOS into full screen.
>>
>> [How]
>> Reject atomic commits with non-zero drm_plane_state.src_x or src_y 
>> values.
>>
>> v2:
>>   - Add code comment describing the reason we're rejecting non-zero
>>     src_x and src_y
>>   - Drop gerrit Change-Id
>>   - Add stable CC
>>   - Based on amd-staging-drm-next
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: stable@vger.kernel.org
>> Cc: nicholas.kazlauskas@amd.com
>> Cc: amd-gfx@lists.freedesktop.org
>> Cc: alexander.deucher@amd.com
>> Cc: Roman.Li@amd.com
>> Cc: hersenxs.wu@amd.com
>> Cc: danny.wang@amd.com
>> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> 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 be1769d29742..b12469043e6b 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4089,6 +4089,23 @@ static int fill_dc_scaling_info(const struct 
>> drm_plane_state *state,
>>       scaling_info->src_rect.x = state->src_x >> 16;
>>       scaling_info->src_rect.y = state->src_y >> 16;
>> +    /*
>> +     * For reasons we don't (yet) fully understand a non-zero
>> +     * src_y coordinate into an NV12 buffer can cause a
>> +     * system hang. To avoid hangs (and maybe be overly cautious)
>> +     * let's reject both non-zero src_x and src_y.
>> +     *
>> +     * We currently know of only one use-case to reproduce a
>> +     * scenario with non-zero src_x and src_y for NV12, which
>> +     * is to gesture the YouTube Android app into full screen
>> +     * on ChromeOS.
>> +     */
>> +    if (state->fb &&
>> +        state->fb->format->format == DRM_FORMAT_NV12 &&
>> +        (scaling_info->src_rect.x != 0 ||
>> +         scaling_info->src_rect.y != 0))
>> +        return -EINVAL;
>> +
>>       scaling_info->src_rect.width = state->src_w >> 16;
>>       if (scaling_info->src_rect.width == 0)
>>           return -EINVAL;
>