Re: [PATCH v2 02/22] drm: Add valid clones check

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 12/16/2024 6:27 AM, Simona Vetter wrote:
> On Sun, Dec 15, 2024 at 06:19:08PM -0800, Abhinav Kumar wrote:
>> Hi Maxime
>>
>> Gentle reminder on this one.
>>
>> We are looking for some advice on how to go about KUnit for this static
>> function.
>>
>> Please help with our question below.
>>
>> Thanks
>>
>> Abhinav
>>
>> On 12/6/2024 4:48 PM, Jessica Zhang wrote:
>>>
>>>
>>> On 9/25/2024 12:23 AM, Maxime Ripard wrote:
>>>> On Tue, Sep 24, 2024 at 03:59:18PM GMT, Jessica Zhang wrote:
>>>>> Check that all encoders attached to a given CRTC are valid
>>>>> possible_clones of each other.
>>>>>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_atomic_helper.c | 23 +++++++++++++++++++++++
>>>>>    1 file changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index 43cdf39019a4..cc4001804fdc 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -574,6 +574,25 @@ mode_valid(struct drm_atomic_state *state)
>>>>>        return 0;
>>>>>    }
>>>>> +static int drm_atomic_check_valid_clones(struct
>>>>> drm_atomic_state *state,
>>>>> +                     struct drm_crtc *crtc)
>>>>> +{
>>>>> +    struct drm_encoder *drm_enc;
>>>>> +    struct drm_crtc_state *crtc_state =
>>>>> drm_atomic_get_new_crtc_state(state,
>>>>> +                                      crtc);
>>>>> +
>>>>> +    drm_for_each_encoder_mask(drm_enc, crtc->dev,
>>>>> crtc_state->encoder_mask) {
>>>>> +        if ((crtc_state->encoder_mask & drm_enc->possible_clones) !=
>>>>> +            crtc_state->encoder_mask) {
>>>>> +            DRM_DEBUG("crtc%d failed valid clone check for mask
>>>>> 0x%x\n",
>>>>> +                  crtc->base.id, crtc_state->encoder_mask);
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * drm_atomic_helper_check_modeset - validate state object for
>>>>> modeset changes
>>>>>     * @dev: DRM device
>>>>> @@ -745,6 +764,10 @@ drm_atomic_helper_check_modeset(struct
>>>>> drm_device *dev,
>>>>>            ret = drm_atomic_add_affected_planes(state, crtc);
>>>>>            if (ret != 0)
>>>>>                return ret;
>>>>> +
>>>>> +        ret = drm_atomic_check_valid_clones(state, crtc);
>>>>> +        if (ret != 0)
>>>>> +            return ret;
>>>>>        }
>>>>
>>>> Pretty much the same comment, we should have kunit tests for this.
>>>
>>> Hey Maxime,
>>>
>>> I'm working on the kunit test for this and had a question on the design
>>> for the unit test:
>>>
>>> Since this is a static helper that returns a pretty common error code,
>>> how would you recommend going about making sure that
>>> `drm_atomic_check_valid_clones()` specifically is returning the error
>>> (and not a different part of check_modeset) when testing the
>>> check_valid_clones() failure path?
> 
> So the usual way to test very specific things of a big function is to
> first setup a driver and atomic request which does pass all checks. And
> then do a minimal change which does not pass anymore.
> 
> So what you could do here is have 3 connectors 1 crtc, but only the first
> two connectors can be cloned. Then do an atomic request with those two
> connectors and the crtc. Then the 2nd request is with one of the
> connectors replaced with the 3rd one (so it's still a clone config, but
> not an invalid one), then have a failure.
> 
> Note: I didn't check all the details, I might be getting something wrong
> here, but the idea should work.

Hey Sima,

Ack, FWIW this describes something very similar to my planned test cases 
(my current kunit tests 3 cases -- valid clone, invalid clone, and no 
clones). Will post the changes later today if there's no major 
objections to this.

Thanks,

Jessica Zhang

> 
> Cheers, Sima
> 
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>> Maxime
>>>
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch