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

[Jose Abreu]

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 
>> Cc: Carlos Palminha 
>> Cc: Alexey Brodkin 
>> Cc: Ville Syrjälä 
>> Cc: Daniel Vetter 
>> Cc: Dave Airlie 
>> Cc: Andrzej Hajda 
>> Cc: Archit Taneja 
> 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;
>> 

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

[Daniel Vetter]

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 
> >> Cc: Carlos Palminha 
> >> Cc: Alexey Brodkin 
> >> Cc: Ville Syrjälä 
> >> Cc: Daniel Vetter 
> >> Cc: Dave Airlie 
> >> Cc: Andrzej Hajda 
> >> Cc: Archit Taneja 
> > 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 

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

[Daniel Vetter]

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 
> Cc: Carlos Palminha 
> Cc: Alexey Brodkin 
> Cc: Ville Syrjälä 
> Cc: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: Andrzej Hajda 
> Cc: Archit Taneja 

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 _connector_helper_funcs.mode_valid helper can 
> perform
> - *  driver and/or hardware specific checks
> + *- the optional _connector_helper_funcs.mode_valid,
> + *   

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

[Jose Abreu]

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 
>> Cc: Carlos Palminha 
>> Cc: Alexey Brodkin 
>> Cc: Ville Syrjälä 
>> Cc: Daniel Vetter 
>> Cc: Dave Airlie 
>> Cc: Andrzej Hajda 
>> Cc: Archit Taneja 
>> ---
>>  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 _connector_helper_funcs.mode_valid helper can 
>> perform
>> - *  driver and/or hardware specific checks
>> + *- the optional _connector_helper_funcs.mode_valid,
>> + *  _crtc_helper_funcs.mode_valid and
>> + *  

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

[Jose Abreu]

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 
Cc: Carlos Palminha 
Cc: Alexey Brodkin 
Cc: Ville Syrjälä 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: Andrzej Hajda 
Cc: Archit Taneja 
---
 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;
+
+   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 _connector_helper_funcs.mode_valid helper can perform
- *  driver and/or hardware specific checks
+ *- the optional _connector_helper_funcs.mode_valid,
+ * _crtc_helper_funcs.mode_valid and
+ * _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)
+   

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

[Ville Syrjälä]

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 
> Cc: Carlos Palminha 
> Cc: Alexey Brodkin 
> Cc: Ville Syrjälä 
> Cc: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: Andrzej Hajda 
> Cc: Archit Taneja 
> ---
>  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.

> +
> + /* 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 _connector_helper_funcs.mode_valid helper can 
> perform
> - *  driver and/or hardware specific checks
> + *- the optional _connector_helper_funcs.mode_valid,
> + *   _crtc_helper_funcs.mode_valid and
> + *   _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,
>