From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760850AbdEOJv3 (ORCPT ); Mon, 15 May 2017 05:51:29 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:21992 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754024AbdEOJv1 (ORCPT ); Mon, 15 May 2017 05:51:27 -0400 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252 X-AuditID: cbfec7ef-f796a6d00000373c-d7-59197a1ec20c Subject: Re: [PATCH v3 4/6] drm: Use new mode_valid() helpers in connector probe helper To: Jose Abreu , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Carlos Palminha , Alexey Brodkin , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Dave Airlie , Archit Taneja From: Andrzej Hajda Message-id: Date: Mon, 15 May 2017 11:51:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 In-reply-to: <20170515093051.aqmgip7npjjqawyp@phenom.ffwll.local> Content-transfer-encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupnleLIzCtJLcpLzFFi42LZduzneV25KslIg3NvrCx6z51kslj39TaT RVPHW1aLWfseslpc+fqezeLenw+sFpd3zWGz+P5vIZMDh8flvl4mj+3fHrB6zDsZ6HG/+ziT x5b9nxk9Pm+SC2CL4rJJSc3JLEst0rdL4Mo42HiBtWCOYUX74v+sDYy71bsYOTkkBEwkFtza yA5hi0lcuLeerYuRi0NIYBmjxMy/R9khnM+MEs+WXmCD6bjycjIrXNWOxsPMIAleAUGJH5Pv sYDYzAIGEjOmHGaCKHrGKLHuaBdYt7BApMSp56uZQRIiAjeZJI69fA/WzSagKfF38002iEl2 Eoe2bweLswioSlztesUKYosKREhcn7OFsYuRg4NTwFHiwQVPiGXyEgevPGcBmSkhsI5dou3g fnaQGgkBWYlNB5ghTBeJq/d4IR4Qlnh1fAvUyzISlyd3Q7V2M0p86j/BDuFMYZT492EGM0SV tcTh4xdZIZbxSUzaNh1qKK9ER5sQRImHxIeTf1khbEeJ3zc+sEA8f4JJonfJO/YJjHKzkAJp FlIgzULywwJG5lWMIqmlxbnpqcWGesWJucWleel6yfm5mxiByeT0v+PvdzA+bQ45xCjAwajE w5t4WSJSiDWxrLgy9xCjBAezkghvQaFkpBBvSmJlVWpRfnxRaU5q8SFGaQ4WJXFe3lPXIoQE 0hNLUrNTUwtSi2CyTBycUg2MHaaC5y4t3bIza3/SDJNHi/Ue7U4o1jmft+z19ovpCVvLPIXP vJp0IyJsWtuFU5/iMncn8RxW8OKX15Y2WbNuwvFNfaueBbdxcR7L74+2s1taqMl7Usnkz4NM +/7g+F9cX+9zRS7dv0TGtGrXtM3lt1+trRNZfuuWOKdxrNiVf1eD/HRyJzHHK7EUZyQaajEX FScCAA8UniUiAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrLIsWRmVeSWpSXmKPExsVy+t/xq7o2VZKRBl+3aln0njvJZLHu620m i6aOt6wWs/Y9ZLW48vU9m8W9Px9YLS7vmsNm8f3fQiYHDo/Lfb1MHtu/PWD1mHcy0ON+93Em jy37PzN6fN4kF8AW5WaTkZqYklqkkJqXnJ+SmZduqxQa4qZroaSQl5ibaqsUoesbEqSkUJaY UwrkGRmgAQfnAPdgJX27BLeMg40XWAvmGFa0L/7P2sC4W72LkZNDQsBE4srLyawQtpjEhXvr 2boYuTiEBJYwSvy+95IFJMErICjxY/I9IJuDg1lAT+L+RS2ImmeMErubWsCahQUiJU49X80M khARuM4k8Wz9Z0aIqhNMEo27GphAqtgENCX+br7JBjHVTuLQ9u3MIDaLgKrE1a5XYJNEBSIk HnbuYgfZxingKPHggidImFlAXuLglecsExj5ZyG5aRbCTbOQVC1gZF7FKJJaWpybnltspFec mFtcmpeul5yfu4kRGFXbjv3csoOx613wIUYBDkYlHt6EyxKRQqyJZcWVuYcYJTiYlUR4Cwol I4V4UxIrq1KL8uOLSnNSiw8xmgKdOpFZSjQ5HxjxeSXxhiaG5paGRsYWFuZGRkrivFM/XAkX EkhPLEnNTk0tSC2C6WPi4JRqYIzotl72m3lT997pbswZxxpN95Ws4p2gkSDQJLExSN1ljYhl 4NSWn4UnJjqG7ppx+Vx6YOZO5qMz9HTmJM4RsJe/srB+VtWhFt+rEdKVapU/xHf+iSy7Wn/7 1ZQfnam7CnJ+fL8mvSd37fv35jdv3VuluEpMc8OxklVlya9anib5F8qavn/VVK3EUpyRaKjF XFScCABlsuGbwAIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170515095123eucas1p246b5eb54b7aaf7e4a64c9e0e6977b891 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRvsgrw=?= =?UTF-8?B?7ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRtTYW1z?= =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170511090650epcas5p4f40af09dff02831285d0bcef6e12451c X-RootMTR: 20170511090650epcas5p4f40af09dff02831285d0bcef6e12451c References: <46c300b3827190dbb066f1af7b9d32088f8b5542.1494492646.git.joabreu@synopsys.com> <20170515093051.aqmgip7npjjqawyp@phenom.ffwll.local> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15.05.2017 11:30, Daniel Vetter wrote: > On Mon, May 15, 2017 at 10:39:35AM +0200, Andrzej Hajda wrote: >> On 11.05.2017 11:06, Jose Abreu wrote: >>> This changes the connector probe helper function to use the new >>> encoder->mode_valid(), bridge->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 encodersXbridgesXcrtcs and calls the >>> callback. >>> >>> If at least a valid encoderXbridgeXcrtc 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 v2->v3: >>> - Call also bridge->mode_valid (Daniel) >>> Changes v1->v2: >>> - Use new helpers suggested by Ville >>> - Change documentation (Daniel) >>> >>> drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 62 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c >>> index f01abdc..84d660e 100644 >>> --- a/drivers/gpu/drm/drm_probe_helper.c >>> +++ b/drivers/gpu/drm/drm_probe_helper.c >>> @@ -83,6 +83,61 @@ >>> return MODE_OK; >>> } >>> >>> +static enum drm_mode_status >>> +drm_mode_validate_connector(struct drm_connector *connector, >>> + struct drm_display_mode *mode) >>> +{ >>> + 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; >>> + } >>> + >>> + ret = drm_bridge_mode_valid(encoder->bridge, mode); >>> + if (ret != MODE_OK) { >>> + /* There is also no point in continuing for crtc check >>> + * here. */ >>> + continue; >>> + } >> Maybe it is a bikeshedding, but wouldn't be better to call >> drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all >> bridge related stuff from corresponding encoder stuff? >> This is more question about role of encoder->bridge, should it be >> treated as encoder's extension, or as 1st class citizen in drm? >> >> Another concern is about order of calls, it is from sink to source, to >> keep it consistent bridge should be called before encoder, am I right? > For the atomic_check stuff (where we do change the passed-in mode) this > would be correct, and calling order and layering would matter. But this > just validates the mode in turn with everything, not taking any > cross-component constraint or other configuration-dependent constraints > into account. Hence it doesn't matter in which order we call stuff. > > Note that the passed-in mode is const, so you can't escape. And v3 of > patch 1 now has added wording that you're not allowed to look at anything > else dynamie either. > > Does that address your concern? Yes, I know it practically does not matter. I have mistakenly written "Beside this: R-b", it should be rather "Anyway: R-b" :) Regards Andrzej > -Daniel > >> Beside this: >> Reviewed-by: Andrzej Hajda >> >> -- >> Regards >> Andrzej >> >>> + >>> + 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; >>> +} >>> + >>> static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) >>> { >>> struct drm_cmdline_mode *cmdline_mode; >>> @@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev) >>> * - 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 >>> + * driver and/or sink specific checks >>> + * - the optional &drm_crtc_helper_funcs.mode_valid, >>> + * &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid >>> + * helpers can perform driver and/or source specific checks which are also >>> + * enforced by the modeset/atomic helpers >>> * >>> * 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 >>> @@ -466,8 +525,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); >>> } >>> >>