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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel