From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752625AbdENLEZ (ORCPT ); Sun, 14 May 2017 07:04:25 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:38372 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750877AbdENLEX (ORCPT ); Sun, 14 May 2017 07:04:23 -0400 From: Laurent Pinchart To: Jose Abreu Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Vetter , Alexey Brodkin , Carlos Palminha Subject: Re: [PATCH v2 5/8] drm: Use new mode_valid() helpers in connector probe helper Date: Sun, 14 May 2017 14:04:24 +0300 Message-ID: <1874234.uZ7HLpxeJ2@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.32; x86_64; ; ) In-Reply-To: <65a1928f-a5e8-b668-0925-ff233c21c9fe@synopsys.com> References: <3844793.Y2ncMspPzi@avalon> <65a1928f-a5e8-b668-0925-ff233c21c9fe@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v4EB4TqD014062 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, Laurent Pinchart