From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754259AbdEHKNr (ORCPT ); Mon, 8 May 2017 06:13:47 -0400 Received: from smtprelay4.synopsys.com ([198.182.47.9]:51982 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751610AbdEHKNp (ORCPT ); Mon, 8 May 2017 06:13:45 -0400 Subject: Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper References: <20170508075041.mj3z6572k67iv7nx@phenom.ffwll.local> CC: Jose Abreu , , , Carlos Palminha , Alexey Brodkin , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Dave Airlie , Andrzej Hajda , Archit Taneja To: Daniel Vetter From: Jose Abreu Message-ID: Date: Mon, 8 May 2017 11:13:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170508075041.mj3z6572k67iv7nx@phenom.ffwll.local> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.107.19.84] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, On 08-05-2017 08:50, Daniel Vetter wrote: > On Thu, May 04, 2017 at 03:11:39PM +0100, 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 > A few comments below. > -Daniel Thanks for the review! > >> --- >> drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++--- >> 1 file changed, 67 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c >> index 1b0c14a..bf19f82 100644 >> --- a/drivers/gpu/drm/drm_probe_helper.c >> +++ b/drivers/gpu/drm/drm_probe_helper.c >> @@ -80,6 +80,67 @@ >> return MODE_OK; >> } >> >> +static enum drm_mode_status >> +drm_mode_validate_connector(struct drm_connector *connector, >> + struct drm_display_mode *mode) >> +{ >> + const struct drm_connector_helper_funcs *connector_funcs = >> + connector->helper_private; >> + 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 */ >> + if (connector_funcs && connector_funcs->mode_valid) >> + ret = connector_funcs->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]); >> + const struct drm_encoder_helper_funcs *encoder_funcs; >> + struct drm_crtc *crtc; >> + >> + if (!encoder) >> + continue; >> + >> + encoder_funcs = encoder->helper_private; >> + if (encoder_funcs && encoder_funcs->mode_valid) >> + ret = encoder_funcs->mode_valid(encoder, mode); >> + else >> + ret = MODE_OK; /* encoder accepts everything */ >> + >> + /* 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. */ >> + if (ret != MODE_OK) >> + continue; > I think we should validate encoders against connector->possible_ids here. > Might be that not many drivers fill this out correctly, and in case fixing > that is much work, perhaps as a follow-up. But would be good to at least > help look down that part of uapi a bit more. I'm sorry but I'm not following you here (I think I need more coffee :)). What do you mean by connector->possible_ids ? Is this something new ? Because I didn't find anything in my tree and I'm based on today's drm-next from Dave. > > This isn't checked within atomic and legacy helpers since we assume that > ->best_encoder respectively ->atomic_best_encoder gives us a valid encoder > ... > >> + >> + drm_for_each_crtc(crtc, dev) { >> + const struct drm_crtc_helper_funcs *crtc_funcs; >> + >> + if (!drm_encoder_crtc_ok(encoder, crtc)) >> + continue; > We check this one here already in both atomic and legacy helpers, so all > drivers should get this right. But drm_for_each_crtc() iterates over all crtc from a device and some crtcs may only be used by specific encoders (by setting encoder->possible_crtcs), right ? We need to iterate only over the encoder's crtc we want to validate. > >> + >> + crtc_funcs = crtc->helper_private; >> + if (!crtc_funcs || !crtc_funcs->mode_valid) >> + return MODE_OK; /* crtc accepts everything */ >> + >> + ret = crtc_funcs->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; >> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) >> * (if specified) >> * - 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 >> + * - the optional &drm_connector_helper_funcs.mode_valid, >> + * &drm_crtc_helper_funcs.mode_valid and >> + * &drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or >> + * hardware specific checks > I'd leave 2 bullets for connector->mode_valid (sink specific checks) and > everything else (source checks, which are also enforced by the > modeset/atomic helpers). Ok, thanks! Best regards, Jose Miguel Abreu >> * >> * 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 +491,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); >> } >> >> -- >> 1.9.1 >> >>