[2/5] drm: Use new mode_valid() helpers in connector probe helper

Commit Message

Jose Abreu May 4, 2017, 2:11 p.m. UTC

This changes the connector probe helper function to use the new
encoder->mode_valid() and crtc->mode_valid() helper callbacks to
validate the modes.

The new callbacks are optional so the behaviour remains the same
if they are not implemented. If they are, then the code loops
through all the connector's encodersXcrtcs and calls the
callback.

If at least a valid encoderXcrtc combination is found which
accepts the mode then the function returns MODE_OK.

Signed-off-by: Jose Abreu <[e-mail]>
Cc: Carlos Palminha <[e-mail]>
Cc: Alexey Brodkin <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Cc: Andrzej Hajda <[e-mail]>
Cc: Archit Taneja <[e-mail]>
---
 drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 4 deletions(-)

Comments

Jose Abreu May 4, 2017, 2:53 p.m. UTC | #1

Hi Ville,


On 04-05-2017 15:32, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:11:39PM +0100, Jose Abreu wrote:
>> This changes the connector probe helper function to use the new
>> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
>> validate the modes.
>>
>> The new callbacks are optional so the behaviour remains the same
>> if they are not implemented. If they are, then the code loops
>> through all the connector's encodersXcrtcs and calls the
>> callback.
>>
>> If at least a valid encoderXcrtc combination is found which
>> accepts the mode then the function returns MODE_OK.
>>
>> Signed-off-by: Jose Abreu <[e-mail]>
>> Cc: Carlos Palminha <[e-mail]>
>> Cc: Alexey Brodkin <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Dave Airlie <[e-mail]>
>> Cc: Andrzej Hajda <[e-mail]>
>> Cc: Archit Taneja <[e-mail]>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..bf19f82 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -80,6 +80,67 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
>> +{
>> +	const struct drm_connector_helper_funcs *connector_funcs =
>> +		connector->helper_private;
>> +	struct drm_device *dev = connector->dev;
>> +	uint32_t *ids = connector->encoder_ids;
>> +	enum drm_mode_status ret = MODE_OK;
>> +	unsigned int i;
>> +
>> +	/* Step 1: Validate against connector */
>> +	if (connector_funcs && connector_funcs->mode_valid)
>> +		ret = connector_funcs->mode_valid(connector, mode);
>> +
>> +	if (ret != MODE_OK)
>> +		return ret;
>> +
>> +	/* Step 2: Validate against encoders and crtcs */
>> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
>> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
>> +		const struct drm_encoder_helper_funcs *encoder_funcs;
>> +		struct drm_crtc *crtc;
>> +
>> +		if (!encoder)
>> +			continue;
>> +
>> +		encoder_funcs = encoder->helper_private;
>> +		if (encoder_funcs && encoder_funcs->mode_valid)
>> +			ret = encoder_funcs->mode_valid(encoder, mode);
>> +		else
>> +			ret = MODE_OK; /* encoder accepts everything */
> Since you already added the drm_bridge_mode_valid() helper, maybe add one
> for connectors, encoders and crtcs as well. Might keep the logic in this
> function a bit more clear on account of not having to check for the
> presence of the vfunc. Right now all three cases look slightly
> different from one another.

Ok, will add in next version. Thanks!

Best regards,
Jose Miguel Abreu

>
>> +
>> +		/* No point in continuing for crtc check as this encoder will
>> +		 * not accept the mode anyway. If all encoders reject the mode
>> +		 * then, at exit, ret will not be MODE_OK. */
>> +		if (ret != MODE_OK)
>> +			continue;
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			const struct drm_crtc_helper_funcs *crtc_funcs;
>> +
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
>> +
>> +			crtc_funcs = crtc->helper_private;
>> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +				return MODE_OK; /* crtc accepts everything */
>> +
>> +			ret = crtc_funcs->mode_valid(crtc, mode);
>> +			if (ret == MODE_OK)
>> +				/* If we get to this point there is at least
>> +				 * one combination of encoder+crtc that works
>> +				 * for this mode. Lets return now. */
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  {
>>  	struct drm_cmdline_mode *cmdline_mode;
>> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *      (if specified)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>> - *      driver and/or hardware specific checks
>> + *    - the optional &drm_connector_helper_funcs.mode_valid,
>> + *    	&drm_crtc_helper_funcs.mode_valid and
>> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
>> + *    	hardware specific checks
>>   *
>>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>>   *    accompanied by a debug message indicating the reason for the mode's
>> @@ -428,8 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>  		if (mode->status == MODE_OK)
>>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>>  
>> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
>> -			mode->status = connector_funcs->mode_valid(connector,
>> +		if (mode->status == MODE_OK)
>> +			mode->status = drm_mode_validate_connector(connector,
>>  								   mode);
>>  	}
>>  
>> -- 
>> 1.9.1
>>

Daniel Vetter May 8, 2017, 7:50 a.m. UTC | #2

On Thu, May 04, 2017 at 03:11:39PM +0100, Jose Abreu wrote:
> This changes the connector probe helper function to use the new
> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
> validate the modes.
> 
> The new callbacks are optional so the behaviour remains the same
> if they are not implemented. If they are, then the code loops
> through all the connector's encodersXcrtcs and calls the
> callback.
> 
> If at least a valid encoderXcrtc combination is found which
> accepts the mode then the function returns MODE_OK.
> 
> Signed-off-by: Jose Abreu <[e-mail]>
> Cc: Carlos Palminha <[e-mail]>
> Cc: Alexey Brodkin <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: Archit Taneja <[e-mail]>

A few comments below.
-Daniel

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..bf19f82 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,67 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +	struct drm_device *dev = connector->dev;
> +	uint32_t *ids = connector->encoder_ids;
> +	enum drm_mode_status ret = MODE_OK;
> +	unsigned int i;
> +
> +	/* Step 1: Validate against connector */
> +	if (connector_funcs && connector_funcs->mode_valid)
> +		ret = connector_funcs->mode_valid(connector, mode);
> +
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	/* Step 2: Validate against encoders and crtcs */
> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
> +		const struct drm_encoder_helper_funcs *encoder_funcs;
> +		struct drm_crtc *crtc;
> +
> +		if (!encoder)
> +			continue;
> +
> +		encoder_funcs = encoder->helper_private;
> +		if (encoder_funcs && encoder_funcs->mode_valid)
> +			ret = encoder_funcs->mode_valid(encoder, mode);
> +		else
> +			ret = MODE_OK; /* encoder accepts everything */
> +
> +		/* No point in continuing for crtc check as this encoder will
> +		 * not accept the mode anyway. If all encoders reject the mode
> +		 * then, at exit, ret will not be MODE_OK. */
> +		if (ret != MODE_OK)
> +			continue;

I think we should validate encoders against connector->possible_ids here.
Might be that not many drivers fill this out correctly, and in case fixing
that is much work, perhaps as a follow-up. But would be good to at least
help look down that part of uapi a bit more.

This isn't checked within atomic and legacy helpers since we assume that
->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
...

> +
> +		drm_for_each_crtc(crtc, dev) {
> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> +
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;

We check this one here already in both atomic and legacy helpers, so all
drivers should get this right.

> +
> +			crtc_funcs = crtc->helper_private;
> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
> +				return MODE_OK; /* crtc accepts everything */
> +
> +			ret = crtc_funcs->mode_valid(crtc, mode);
> +			if (ret == MODE_OK)
> +				/* If we get to this point there is at least
> +				 * one combination of encoder+crtc that works
> +				 * for this mode. Lets return now. */
> +				return ret;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *      (if specified)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or hardware specific checks
> + *    - the optional &drm_connector_helper_funcs.mode_valid,
> + *    	&drm_crtc_helper_funcs.mode_valid and
> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
> + *    	hardware specific checks

I'd leave 2 bullets for connector->mode_valid (sink specific checks) and
everything else (source checks, which are also enforced by the
modeset/atomic helpers).
>   *
>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>   *    accompanied by a debug message indicating the reason for the mode's
> @@ -428,8 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>  
> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
> -			mode->status = connector_funcs->mode_valid(connector,
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_connector(connector,
>  								   mode);
>  	}
>  
> -- 
> 1.9.1
> 
>

Jose Abreu May 8, 2017, 10:13 a.m. UTC | #3

Hi Daniel,


On 08-05-2017 08:50, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 03:11:39PM +0100, Jose Abreu wrote:
>> This changes the connector probe helper function to use the new
>> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
>> validate the modes.
>>
>> The new callbacks are optional so the behaviour remains the same
>> if they are not implemented. If they are, then the code loops
>> through all the connector's encodersXcrtcs and calls the
>> callback.
>>
>> If at least a valid encoderXcrtc combination is found which
>> accepts the mode then the function returns MODE_OK.
>>
>> Signed-off-by: Jose Abreu <[e-mail]>
>> Cc: Carlos Palminha <[e-mail]>
>> Cc: Alexey Brodkin <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Dave Airlie <[e-mail]>
>> Cc: Andrzej Hajda <[e-mail]>
>> Cc: Archit Taneja <[e-mail]>
> A few comments below.
> -Daniel

Thanks for the review!

>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..bf19f82 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -80,6 +80,67 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
>> +{
>> +	const struct drm_connector_helper_funcs *connector_funcs =
>> +		connector->helper_private;
>> +	struct drm_device *dev = connector->dev;
>> +	uint32_t *ids = connector->encoder_ids;
>> +	enum drm_mode_status ret = MODE_OK;
>> +	unsigned int i;
>> +
>> +	/* Step 1: Validate against connector */
>> +	if (connector_funcs && connector_funcs->mode_valid)
>> +		ret = connector_funcs->mode_valid(connector, mode);
>> +
>> +	if (ret != MODE_OK)
>> +		return ret;
>> +
>> +	/* Step 2: Validate against encoders and crtcs */
>> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
>> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
>> +		const struct drm_encoder_helper_funcs *encoder_funcs;
>> +		struct drm_crtc *crtc;
>> +
>> +		if (!encoder)
>> +			continue;
>> +
>> +		encoder_funcs = encoder->helper_private;
>> +		if (encoder_funcs && encoder_funcs->mode_valid)
>> +			ret = encoder_funcs->mode_valid(encoder, mode);
>> +		else
>> +			ret = MODE_OK; /* encoder accepts everything */
>> +
>> +		/* No point in continuing for crtc check as this encoder will
>> +		 * not accept the mode anyway. If all encoders reject the mode
>> +		 * then, at exit, ret will not be MODE_OK. */
>> +		if (ret != MODE_OK)
>> +			continue;
> I think we should validate encoders against connector->possible_ids here.
> Might be that not many drivers fill this out correctly, and in case fixing
> that is much work, perhaps as a follow-up. But would be good to at least
> help look down that part of uapi a bit more.

I'm sorry but I'm not following you here (I think I need more
coffee :)).

What do you mean by connector->possible_ids ? Is this something
new ? Because I didn't find anything in my tree and I'm based on
today's drm-next from Dave.

>
> This isn't checked within atomic and legacy helpers since we assume that
> ->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
> ...
>
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			const struct drm_crtc_helper_funcs *crtc_funcs;
>> +
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
> We check this one here already in both atomic and legacy helpers, so all
> drivers should get this right.

But drm_for_each_crtc() iterates over all crtc from a device and
some crtcs may only be used by specific encoders (by setting
encoder->possible_crtcs), right ? We need to iterate only over
the encoder's crtc we want to validate.

>
>> +
>> +			crtc_funcs = crtc->helper_private;
>> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +				return MODE_OK; /* crtc accepts everything */
>> +
>> +			ret = crtc_funcs->mode_valid(crtc, mode);
>> +			if (ret == MODE_OK)
>> +				/* If we get to this point there is at least
>> +				 * one combination of encoder+crtc that works
>> +				 * for this mode. Lets return now. */
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>  {
>>  	struct drm_cmdline_mode *cmdline_mode;
>> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *      (if specified)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>> - *      driver and/or hardware specific checks
>> + *    - the optional &drm_connector_helper_funcs.mode_valid,
>> + *    	&drm_crtc_helper_funcs.mode_valid and
>> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
>> + *    	hardware specific checks
> I'd leave 2 bullets for connector->mode_valid (sink specific checks) and
> everything else (source checks, which are also enforced by the
> modeset/atomic helpers).

Ok, thanks!

Best regards,
Jose Miguel Abreu

>>   *
>>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>>   *    accompanied by a debug message indicating the reason for the mode's
>> @@ -428,8 +491,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>  		if (mode->status == MODE_OK)
>>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>>  
>> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
>> -			mode->status = connector_funcs->mode_valid(connector,
>> +		if (mode->status == MODE_OK)
>> +			mode->status = drm_mode_validate_connector(connector,
>>  								   mode);
>>  	}
>>  
>> -- 
>> 1.9.1
>>
>>

Daniel Vetter May 8, 2017, 6:28 p.m. UTC | #4

On Mon, May 08, 2017 at 11:13:37AM +0100, Jose Abreu wrote:
> Hi Daniel,
> 
> 
> On 08-05-2017 08:50, Daniel Vetter wrote:
> > On Thu, May 04, 2017 at 03:11:39PM +0100, Jose Abreu wrote:
> >> This changes the connector probe helper function to use the new
> >> encoder->mode_valid() and crtc->mode_valid() helper callbacks to
> >> validate the modes.
> >>
> >> The new callbacks are optional so the behaviour remains the same
> >> if they are not implemented. If they are, then the code loops
> >> through all the connector's encodersXcrtcs and calls the
> >> callback.
> >>
> >> If at least a valid encoderXcrtc combination is found which
> >> accepts the mode then the function returns MODE_OK.
> >>
> >> Signed-off-by: Jose Abreu <[e-mail]>
> >> Cc: Carlos Palminha <[e-mail]>
> >> Cc: Alexey Brodkin <[e-mail]>
> >> Cc: Ville Syrjälä <[e-mail]>
> >> Cc: Daniel Vetter <[e-mail]>
> >> Cc: Dave Airlie <[e-mail]>
> >> Cc: Andrzej Hajda <[e-mail]>
> >> Cc: Archit Taneja <[e-mail]>
> > A few comments below.
> > -Daniel
> 
> Thanks for the review!
> 
> >
> >> ---
> >>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 67 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> >> index 1b0c14a..bf19f82 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -80,6 +80,67 @@
> >>  	return MODE_OK;
> >>  }
> >>  
> >> +static enum drm_mode_status
> >> +drm_mode_validate_connector(struct drm_connector *connector,
> >> +			    struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_connector_helper_funcs *connector_funcs =
> >> +		connector->helper_private;
> >> +	struct drm_device *dev = connector->dev;
> >> +	uint32_t *ids = connector->encoder_ids;
> >> +	enum drm_mode_status ret = MODE_OK;
> >> +	unsigned int i;
> >> +
> >> +	/* Step 1: Validate against connector */
> >> +	if (connector_funcs && connector_funcs->mode_valid)
> >> +		ret = connector_funcs->mode_valid(connector, mode);
> >> +
> >> +	if (ret != MODE_OK)
> >> +		return ret;
> >> +
> >> +	/* Step 2: Validate against encoders and crtcs */
> >> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> >> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
> >> +		const struct drm_encoder_helper_funcs *encoder_funcs;
> >> +		struct drm_crtc *crtc;
> >> +
> >> +		if (!encoder)
> >> +			continue;
> >> +
> >> +		encoder_funcs = encoder->helper_private;
> >> +		if (encoder_funcs && encoder_funcs->mode_valid)
> >> +			ret = encoder_funcs->mode_valid(encoder, mode);
> >> +		else
> >> +			ret = MODE_OK; /* encoder accepts everything */
> >> +
> >> +		/* No point in continuing for crtc check as this encoder will
> >> +		 * not accept the mode anyway. If all encoders reject the mode
> >> +		 * then, at exit, ret will not be MODE_OK. */
> >> +		if (ret != MODE_OK)
> >> +			continue;
> > I think we should validate encoders against connector->possible_ids here.
> > Might be that not many drivers fill this out correctly, and in case fixing
> > that is much work, perhaps as a follow-up. But would be good to at least
> > help look down that part of uapi a bit more.
> 
> I'm sorry but I'm not following you here (I think I need more
> coffee :)).
> 
> What do you mean by connector->possible_ids ? Is this something
> new ? Because I didn't find anything in my tree and I'm based on
> today's drm-next from Dave.

Oops, I guess I was on an old tree or whatever by accident. I meant
drm_connector->encoder_ids, that limits the encoders you can use on a
crtc. For many drivers there's only one.

> > This isn't checked within atomic and legacy helpers since we assume that
> > ->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
> > ...
> >
> >> +
> >> +		drm_for_each_crtc(crtc, dev) {
> >> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> >> +
> >> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> >> +				continue;
> > We check this one here already in both atomic and legacy helpers, so all
> > drivers should get this right.
> 
> But drm_for_each_crtc() iterates over all crtc from a device and
> some crtcs may only be used by specific encoders (by setting
> encoder->possible_crtcs), right ? We need to iterate only over
> the encoder's crtc we want to validate.

This was a comment about ->encoder_ids, since we don't validate that in
the atomic helpers (but instead rely on ->(atomic_)best_encoder to give us
the right encoder) validating here in this function might be a problem and
uncover driver bugs. But for drm_encoder_crtc_ok there's no such risk, so
this is safe.

I was simply dumping my thoughts while reviewing, the code is all good :-)

Cheers, Daniel

Patch

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 1b0c14a..bf19f82 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -80,6 +80,67 @@ 
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+	struct drm_device *dev = connector->dev;
+	uint32_t *ids = connector->encoder_ids;
+	enum drm_mode_status ret = MODE_OK;
+	unsigned int i;
+
+	/* Step 1: Validate against connector */
+	if (connector_funcs && connector_funcs->mode_valid)
+		ret = connector_funcs->mode_valid(connector, mode);
+
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
+		const struct drm_encoder_helper_funcs *encoder_funcs;
+		struct drm_crtc *crtc;
+
+		if (!encoder)
+			continue;
+
+		encoder_funcs = encoder->helper_private;
+		if (encoder_funcs && encoder_funcs->mode_valid)
+			ret = encoder_funcs->mode_valid(encoder, mode);
+		else
+			ret = MODE_OK; /* encoder accepts everything */
+
+		/* No point in continuing for crtc check as this encoder will
+		 * not accept the mode anyway. If all encoders reject the mode
+		 * then, at exit, ret will not be MODE_OK. */
+		if (ret != MODE_OK)
+			continue;
+
+		drm_for_each_crtc(crtc, dev) {
+			const struct drm_crtc_helper_funcs *crtc_funcs;
+
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			crtc_funcs = crtc->helper_private;
+			if (!crtc_funcs || !crtc_funcs->mode_valid)
+				return MODE_OK; /* crtc accepts everything */
+
+			ret = crtc_funcs->mode_valid(crtc, mode);
+			if (ret == MODE_OK)
+				/* If we get to this point there is at least
+				 * one combination of encoder+crtc that works
+				 * for this mode. Lets return now. */
+				return ret;
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;
@@ -283,8 +344,10 @@  void drm_kms_helper_poll_enable(struct drm_device *dev)
  *      (if specified)
  *    - drm_mode_validate_flag() checks the modes against basic connector
  *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
- *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
- *      driver and/or hardware specific checks
+ *    - the optional &drm_connector_helper_funcs.mode_valid,
+ *    	&drm_crtc_helper_funcs.mode_valid and
+ *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
+ *    	hardware specific checks
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's
@@ -428,8 +491,8 @@  int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		if (mode->status == MODE_OK)
 			mode->status = drm_mode_validate_flag(mode, mode_flags);
 
-		if (mode->status == MODE_OK && connector_funcs->mode_valid)
-			mode->status = connector_funcs->mode_valid(connector,
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_connector(connector,
 								   mode);
 	}