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

Jessica Zhang posted 22 patches 1 year, 4 months ago
There is a newer version of this series
[PATCH v2 02/22] drm: Add valid clones check
Posted by Jessica Zhang 1 year, 4 months ago
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;
 	}
 
 	/*

-- 
2.34.1
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Maxime Ripard 1 year, 4 months ago
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.

Maxime
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Jessica Zhang 1 year, 2 months ago

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?

Thanks,

Jessica Zhang

> 
> Maxime
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Maxime Ripard 1 year, 1 month ago
On Fri, Dec 06, 2024 at 04:48:43PM -0800, 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?

There's two ways to go about it. Either you can unit test it, prepare a
series of custom states and use
EXPORT_SYMBOL_FOR_TESTS_ONLY/EXPORT_SYMBOL_IF_KUNIT, or you can go the
integration test way and just test that drm_atomic_check is rejected for
unsafe combinations.

I guess I'd prefer the former, but the latter also makes sense and
eventually, it checks what we want: to make sure that we reject such a
state. What part of the code does or with what error code is less
important imo.

Maxime
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Jessica Zhang 1 year, 1 month ago

On 12/16/2024 6:47 AM, Maxime Ripard wrote:
> On Fri, Dec 06, 2024 at 04:48:43PM -0800, 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?
> 
> There's two ways to go about it. Either you can unit test it, prepare a
> series of custom states and use
> EXPORT_SYMBOL_FOR_TESTS_ONLY/EXPORT_SYMBOL_IF_KUNIT, or you can go the
> integration test way and just test that drm_atomic_check is rejected for
> unsafe combinations.
> 
> I guess I'd prefer the former, but the latter also makes sense and
> eventually, it checks what we want: to make sure that we reject such a
> state. What part of the code does or with what error code is less
> important imo.

Thanks for confirming! The changes I made are based on the latter 
approach. Will post the changes later today if you have no objections on 
this

Thanks,

Jessica Zhang

> 
> Maxime
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Abhinav Kumar 1 year, 1 month ago
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?
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>> Maxime
> 
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Simona Vetter 1 year, 1 month ago
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.

Cheers, Sima

> > 
> > Thanks,
> > 
> > Jessica Zhang
> > 
> > > 
> > > Maxime
> > 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v2 02/22] drm: Add valid clones check
Posted by Jessica Zhang 1 year, 1 month ago

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