Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640361 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-05-12 11:40 +0200
    Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector  probe helper Jose Abreu <Jose.Abreu@synopsys.com> - 2017-05-12 18:10 +0200
      Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-05-14 13:10 +0200
        Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector  probe helper Daniel Vetter <daniel@ffwll.ch> - 2017-05-15 08:50 +0200
          Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-05-15 09:10 +0200
            Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector  probe helper Jose Abreu <Jose.Abreu@synopsys.com> - 2017-05-16 07:20 +0200

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

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 <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> 
> 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 <drm/drm_fb_helper.h>
>  #include <drm/drm_edid.h>
> 
> +#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 &drm_connector_helper_funcs.mode_valid helper can
> perform
> - *      driver and/or hardware specific checks
> + *      driver and/or sink specific checks
> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
> + *      &drm_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);

I would reverse the arguments order to match the style of the other validation 
functions.

>  	}

-- 
Regards,

Laurent Pinchart

[toc] | [next] | [standalone]

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

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 <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>
>> 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 <drm/drm_fb_helper.h>
>>  #include <drm/drm_edid.h>
>>
>> +#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 &drm_connector_helper_funcs.mode_valid helper can
>> perform
>> - *      driver and/or hardware specific checks
>> + *      driver and/or sink specific checks
>> + *    - the optional &drm_crtc_helper_funcs.mode_valid and
>> + *      &drm_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);
> 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 ...

Best regards,
Jose Miguel Abreu

>
>>  	}

[toc] | [prev] | [next] | [standalone]

#1641071

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 <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Dave Airlie <airlied@linux.ie>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> ---
> >> 
> >> 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,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1641316

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web