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 <joab...@synopsys.com>
> >> Cc: Carlos Palminha <palmi...@synopsys.com>
> >> Cc: Alexey Brodkin <abrod...@synopsys.com>
> >> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> >> Cc: Dave Airlie <airl...@linux.ie>
> >> Cc: Andrzej Hajda <a.ha...@samsung.com>
> >> Cc: Archit Taneja <arch...@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
