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

[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 <joabreu@xxxxxxxxxxxx>
> >> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
> >> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> Cc: Dave Airlie <airlied@xxxxxxxx>
> >> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> >> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> >> ---
> >> 
> >> 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