[PATCH] drm/amd/display: reject both non-zero src_x and src_y only for DCN1x

[Paul Menzel]

[Which address should be used: sshankar at amd.com or shirish.s at amd.com?]

Dear Shirish,


Am 08.11.21 um 12:11 schrieb S, Shirish:

> On 11/8/2021 2:25 PM, Paul Menzel wrote:

>> Am 08.11.21 um 09:15 schrieb Shirish S:
>>> limit the MPO rejection only for DCN1x as its not required on later
>>
>> it’s
>>
>>> versions.
>>
>> Where is it documented, that it’s not required for later versions?
> 
> This is a workaround to avoid system hang & I've verified its not 
> required DCN2.0.

Please extend the commit message with that information, and also add how 
you verified, that it’s not required for DCN2.0 exactly. (Just test one 
system?)

> We generally don't have documentation for WA's.

WA is workaround?


Kind regards,

Paul


>> Shortly describing the implementation is also useful. Something like: 
>> Require `fill_dc_scaling_info()` to receive the device to be able to 
>> check the version.
>>
>>> Fixes: d89f6048bdcb ("drm/amd/display: Reject non-zero src_y and 
>>> src_x for video planes")
>>>
>>
>> I’d remove the blank line.
>>
>>> Signed-off-by: Shirish S <shirish.s at amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++++---------
>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> 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 1e26d9be8993..26b29d561919 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -4572,7 +4572,8 @@ static void get_min_max_dc_plane_scaling(struct 
>>> drm_device *dev,
>>>   }
>>>     -static int fill_dc_scaling_info(const struct drm_plane_state 
>>> *state,
>>> +static int fill_dc_scaling_info(struct amdgpu_device *adev,
>>> +                const struct drm_plane_state *state,
>>>                   struct dc_scaling_info *scaling_info)
>>>   {
>>>       int scale_w, scale_h, min_downscale, max_upscale;
>>> @@ -4586,7 +4587,8 @@ static int fill_dc_scaling_info(const struct 
>>> drm_plane_state *state,
>>>       /*
>>>        * 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)
>>> +     * system hang on DCN1x.
>>> +     * To avoid hangs (and maybe be overly cautious)
>>
>> I’d remove the added line break.
>>
>>>        * let's reject both non-zero src_x and src_y.
>>>        *
>>>        * We currently know of only one use-case to reproduce a
>>> @@ -4594,10 +4596,10 @@ static int fill_dc_scaling_info(const struct 
>>> drm_plane_state *state,
>>>        * 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))
>>> +    if (((adev->ip_versions[DCE_HWIP][0] == IP_VERSION(1, 0, 0)) ||
>>> +        (adev->ip_versions[DCE_HWIP][0] == IP_VERSION(1, 0, 1))) &&
>>> +        (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;
>>> @@ -5503,7 +5505,7 @@ static int fill_dc_plane_attributes(struct 
>>> amdgpu_device *adev,
>>>       int ret;
>>>       bool force_disable_dcc = false;
>>>   -    ret = fill_dc_scaling_info(plane_state, &scaling_info);
>>> +    ret = fill_dc_scaling_info(adev, plane_state, &scaling_info);
>>>       if (ret)
>>>           return ret;
>>>   @@ -7566,7 +7568,7 @@ static int dm_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>       if (ret)
>>>           return ret;
>>>   -    ret = fill_dc_scaling_info(new_plane_state, &scaling_info);
>>> +    ret = fill_dc_scaling_info(adev, new_plane_state, &scaling_info);
>>>       if (ret)
>>>           return ret;
>>>   @@ -9014,7 +9016,7 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>> bundle->surface_updates[planes_count].gamut_remap_matrix = 
>>> &dc_plane->gamut_remap_matrix;
>>>           }
>>>   -        fill_dc_scaling_info(new_plane_state,
>>> +        fill_dc_scaling_info(dm->adev, new_plane_state,
>>> &bundle->scaling_infos[planes_count]);
>>> bundle->surface_updates[planes_count].scaling_info =
>>>