subject:"\[PATCH v2 5\/8\] drm\: Use new mode_valid\(\) helpers in connector probe helper"

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

2017-05-16 Thread Jose Abreu

Hi Laurent,


On 15-05-2017 08:05, Laurent Pinchart wrote:
> On Monday 15 May 2017 08:47:49 Daniel Vetter wrote:
>> On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
>>> On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
 On 12-05-2017 10:35, Laurent Pinchart wrote:
> On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
>> +if (mode->status == MODE_OK)
>> +mode->status = 
> drm_mode_validate_connector(connector,
>> 
> mode);
> I would reverse the arguments order to match the style of the other
> validation functions.
 Hmm, I think it makes more sense to pass connector first and then
 mode ...
>>> I disagree, as this function validates a mode against a pipeline, the same
>>> way the other validation functions validate a mode against other
>>> parameters, but it's your patch :-)
>> Call it drm_connector_validate_mode, because the first argument is
>> generally the object we operate on :-)
> But the function doesn't validate a mode for a connector, it validates a mode 
> for a complete pipeline...
>

Hmm, but note that in the same function there is
drm_mode_validate_size() and drm_mode_validate_flag() calls,
which take as first argument the mode and then the object to
validate (I hadn't seen this). So, maybe leave it as
drm_mode_validate_connector() as it takes a connector as argument
or change to drm_mode_validate_pipeline() as you said, or even
drm_mode_validate_datapath(), drm_mode_validate_videopath(),
drm_mode_validate_components() ?

Best regards,
Jose Miguel Abreu
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2017-05-15 Thread Laurent Pinchart

On Monday 15 May 2017 08:47:49 Daniel Vetter wrote:
> On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
> > On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> >> On 12-05-2017 10:35, Laurent Pinchart wrote:
> >>> On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
>  +if (mode->status == MODE_OK)
>  +mode->status = 
drm_mode_validate_connector(connector,
>  
>  
mode);
> >>> 
> >>> I would reverse the arguments order to match the style of the other
> >>> validation functions.
> >> 
> >> Hmm, I think it makes more sense to pass connector first and then
> >> mode ...
> > 
> > I disagree, as this function validates a mode against a pipeline, the same
> > way the other validation functions validate a mode against other
> > parameters, but it's your patch :-)
> 
> Call it drm_connector_validate_mode, because the first argument is
> generally the object we operate on :-)

But the function doesn't validate a mode for a connector, it validates a mode 
for a complete pipeline...

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2017-05-15 Thread Daniel Vetter

On Sun, May 14, 2017 at 02:04:24PM +0300, Laurent Pinchart wrote:
> On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> > On 12-05-2017 10:35, Laurent Pinchart wrote:
> > > On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:
> > >> +if (mode->status == MODE_OK)
> > >> +mode->status = 
> > >> drm_mode_validate_connector(connector,
> > >> 
> > >> 
> > >> mode);
> > > 
> > > I would reverse the arguments order to match the style of the other
> > > validation functions.
> > 
> > Hmm, I think it makes more sense to pass connector first and then
> > mode ...
> 
> I disagree, as this function validates a mode against a pipeline, the same 
> way 
> the other validation functions validate a mode against other parameters, but 
> it's your patch :-)

Call it drm_connector_validate_mode, because the first argument is
generally the object we operate on :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2017-05-14 Thread Laurent Pinchart

Hi Jose,

On Friday 12 May 2017 17:06:14 Jose Abreu wrote:
> On 12-05-2017 10:35, Laurent Pinchart wrote:
> > On Tuesday 09 May 2017 18:00:12 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 
> >> ---
> >> 
> >> Changes v1->v2:
> >>- Use new helpers suggested by Ville
> >>- Change documentation (Daniel)
> >>
> >>  drivers/gpu/drm/drm_probe_helper.c | 60 ++--
> >>  1 file changed, 57 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> >> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c

[snip]

> >> +static enum drm_mode_status
> >> +drm_mode_validate_connector(struct drm_connector *connector,
> >> +  struct drm_display_mode *mode)
> > 
> > This does more than validating the mode against the connector, it
> > validates it against the whole pipeline. I would call the function
> > drm_mode_validate_pipeline() (or any other similar name).
> 
> Yeah, in previous version I had something similar but I changed
> in order to address review comments. I can change again though...

Sorry, I haven't seen v1. I think it makes more sense to reflect in its name 
the fact that the function validates the mode against the whole pipeline, but 
I'll let others disagree.

> >> +{
> >> +  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 */
> >> +  ret = drm_connector_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]);
> >> +  struct drm_crtc *crtc;
> >> +
> >> +  if (!encoder)
> >> +  continue;
> >> +
> >> +  ret = drm_encoder_mode_valid(encoder, mode);
> >> +  if (ret != MODE_OK) {
> >> +  /* 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. */
> >> +  continue;
> >> +  }
> >> +
> >> +  drm_for_each_crtc(crtc, dev) {
> >> +  if (!drm_encoder_crtc_ok(encoder, crtc))
> >> +  continue;
> >> +
> >> +  ret = drm_crtc_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;
> >> +}

[snip]

> >> @@ -428,8 +482,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);
> > 
> > I would reverse the arguments order to match the style of the other
> > validation functions.
> 
> Hmm, I think it makes more sense to pass connector first and then
> mode ...

I disagree, as this function validates a mode against a pipeline, the same way 
the other validation functions validate a mode against other parameters, but 
it's your patch :-)

-- 
Regards,

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

2017-05-12 Thread Jose Abreu

Hi Laurent,


On 12-05-2017 10:35, Laurent Pinchart wrote:
> Hi Jose,
>
> Thank you for the patch.
>
> On Tuesday 09 May 2017 18:00:12 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 
>> ---
>>
>> Changes v1->v2:
>>  - Use new helpers suggested by Ville
>>  - Change documentation (Daniel)
>>
>>  drivers/gpu/drm/drm_probe_helper.c | 60 +--
>>  1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -39,6 +39,8 @@
>>  #include 
>>  #include 
>>
>> +#include "drm_crtc_internal.h"
>> +
>>  /**
>>   * DOC: output probing helper overview
>>   *
>> @@ -80,6 +82,54 @@
>>  return MODE_OK;
>>  }
>>
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +struct drm_display_mode *mode)
> This does more than validating the mode against the connector, it validates 
> it 
> against the whole pipeline. I would call the function 
> drm_mode_validate_pipeline() (or any other similar name).

Yeah, in previous version I had something similar but I changed
in order to address review comments. I can change again though...

>
>> +{
>> +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 */
>> +ret = drm_connector_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]);
>> +struct drm_crtc *crtc;
>> +
>> +if (!encoder)
>> +continue;
>> +
>> +ret = drm_encoder_mode_valid(encoder, mode);
>> +if (ret != MODE_OK) {
>> +/* 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. */
>> +continue;
>> +}
>> +
>> +drm_for_each_crtc(crtc, dev) {
>> +if (!drm_encoder_crtc_ok(encoder, crtc))
>> +continue;
>> +
>> +ret = drm_crtc_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;
>> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *- 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
>> + *  driver and/or sink specific checks
>> + *- the optional _crtc_helper_funcs.mode_valid and
>> + *  _encoder_helper_funcs.mode_valid helpers can perform driver
>> and/or
>> + *  source specific 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 +482,8 @@ int
>> drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>  if (mode->status ==

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

2017-05-12 Thread Laurent Pinchart

Hi Jose,

Thank you for the patch.

On Tuesday 09 May 2017 18:00:12 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 
> ---
> 
> Changes v1->v2:
>   - Use new helpers suggested by Ville
>   - Change documentation (Daniel)
> 
>  drivers/gpu/drm/drm_probe_helper.c | 60 +--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -39,6 +39,8 @@
>  #include 
>  #include 
> 
> +#include "drm_crtc_internal.h"
> +
>  /**
>   * DOC: output probing helper overview
>   *
> @@ -80,6 +82,54 @@
>   return MODE_OK;
>  }
> 
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> + struct drm_display_mode *mode)

This does more than validating the mode against the connector, it validates it 
against the whole pipeline. I would call the function 
drm_mode_validate_pipeline() (or any other similar name).

> +{
> + 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 */
> + ret = drm_connector_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]);
> + struct drm_crtc *crtc;
> +
> + if (!encoder)
> + continue;
> +
> + ret = drm_encoder_mode_valid(encoder, mode);
> + if (ret != MODE_OK) {
> + /* 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. */
> + continue;
> + }
> +
> + drm_for_each_crtc(crtc, dev) {
> + if (!drm_encoder_crtc_ok(encoder, crtc))
> + continue;
> +
> + ret = drm_crtc_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;
> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *- 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
> + *  driver and/or sink specific checks
> + *- the optional _crtc_helper_funcs.mode_valid and
> + *  _encoder_helper_funcs.mode_valid helpers can perform driver
> and/or
> + *  source specific 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 +482,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 ==

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

2017-05-10 Thread Jose Abreu

Hi Daniel,


On 10-05-2017 09:01, Daniel Vetter wrote:
> On Tue, May 09, 2017 at 06:00:12PM +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 
>> ---
>>
>> Changes v1->v2:
>>  - Use new helpers suggested by Ville
>>  - Change documentation (Daniel)
>>
>>  drivers/gpu/drm/drm_probe_helper.c | 60 
>> --
>>  1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c 
>> b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..de47413 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -39,6 +39,8 @@
>>  #include 
>>  #include 
>>  
>> +#include "drm_crtc_internal.h"
>> +
>>  /**
>>   * DOC: output probing helper overview
>>   *
>> @@ -80,6 +82,54 @@
>>  return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +struct drm_display_mode *mode)
>> +{
>> +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 */
>> +ret = drm_connector_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]);
>> +struct drm_crtc *crtc;
>> +
>> +if (!encoder)
>> +continue;
>> +
>> +ret = drm_encoder_mode_valid(encoder, mode);
>> +if (ret != MODE_OK) {
>> +/* 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. */
>> +continue;
>> +}
> One thing I've forgotten the last time around: Please also check
> bridge->mode_valid here. The encoder->bridge mapping is fixed.

Ok, will add in next version.

Best regards,
Jose Miguel Abreu

>
> Otherwise I think this looks good.
> -Daniel
>
>> +
>> +drm_for_each_crtc(crtc, dev) {
>> +if (!drm_encoder_crtc_ok(encoder, crtc))
>> +continue;
>> +
>> +ret = drm_crtc_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;
>> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *- 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
>> + *  driver and/or sink specific checks
>> + *- the optional _crtc_helper_funcs.mode_valid and
>> + *  _encoder_helper_funcs.mode_valid helpers can perform driver 
>> and/or
>> + *  source specific 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 +482,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

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

2017-05-10 Thread Daniel Vetter

On Tue, May 09, 2017 at 06:00:12PM +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 
> ---
> 
> Changes v1->v2:
>   - Use new helpers suggested by Ville
>   - Change documentation (Daniel)
> 
>  drivers/gpu/drm/drm_probe_helper.c | 60 
> --
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c 
> b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..de47413 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -39,6 +39,8 @@
>  #include 
>  #include 
>  
> +#include "drm_crtc_internal.h"
> +
>  /**
>   * DOC: output probing helper overview
>   *
> @@ -80,6 +82,54 @@
>   return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + 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 */
> + ret = drm_connector_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]);
> + struct drm_crtc *crtc;
> +
> + if (!encoder)
> + continue;
> +
> + ret = drm_encoder_mode_valid(encoder, mode);
> + if (ret != MODE_OK) {
> + /* 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. */
> + continue;
> + }

One thing I've forgotten the last time around: Please also check
bridge->mode_valid here. The encoder->bridge mapping is fixed.

Otherwise I think this looks good.
-Daniel

> +
> + drm_for_each_crtc(crtc, dev) {
> + if (!drm_encoder_crtc_ok(encoder, crtc))
> + continue;
> +
> + ret = drm_crtc_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;
> @@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *- 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
> + *  driver and/or sink specific checks
> + *- the optional _crtc_helper_funcs.mode_valid and
> + *  _encoder_helper_funcs.mode_valid helpers can perform driver 
> and/or
> + *  source specific 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 +482,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)
> +

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

2017-05-09 Thread 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 
---

Changes v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

 drivers/gpu/drm/drm_probe_helper.c | 60 --
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 1b0c14a..de47413 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -39,6 +39,8 @@
 #include 
 #include 
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *
@@ -80,6 +82,54 @@
return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+   struct drm_display_mode *mode)
+{
+   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 */
+   ret = drm_connector_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]);
+   struct drm_crtc *crtc;
+
+   if (!encoder)
+   continue;
+
+   ret = drm_encoder_mode_valid(encoder, mode);
+   if (ret != MODE_OK) {
+   /* 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. */
+   continue;
+   }
+
+   drm_for_each_crtc(crtc, dev) {
+   if (!drm_encoder_crtc_ok(encoder, crtc))
+   continue;
+
+   ret = drm_crtc_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;
@@ -284,7 +334,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *- 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
+ *  driver and/or sink specific checks
+ *- the optional _crtc_helper_funcs.mode_valid and
+ *  _encoder_helper_funcs.mode_valid helpers can perform driver and/or
+ *  source specific 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 +482,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


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

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

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

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

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

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

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

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

9 matches

Site Navigation

Mail list logo

Footer information