From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753476AbdEHHz5 (ORCPT ); Mon, 8 May 2017 03:55:57 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:32983 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753229AbdEHHzz (ORCPT ); Mon, 8 May 2017 03:55:55 -0400 Date: Mon, 8 May 2017 09:55:49 +0200 From: Daniel Vetter To: Jose Abreu Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Carlos Palminha , Alexey Brodkin , Daniel Vetter , Dave Airlie , Andrzej Hajda , Archit Taneja Subject: Re: [PATCH 4/5] drm: Use mode_valid() in atomic modeset Message-ID: <20170508075549.lympqv4bfby63np3@phenom.ffwll.local> Mail-Followup-To: Jose Abreu , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Carlos Palminha , Alexey Brodkin , Dave Airlie , Andrzej Hajda , Archit Taneja References: <20170504144045.GY12629@intel.com> <4c645b70-5aa7-aecd-87f7-3fef47c303b3@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4c645b70-5aa7-aecd-87f7-3fef47c303b3@synopsys.com> X-Operating-System: Linux phenom 4.9.0-2-amd64 User-Agent: NeoMutt/20170306 (1.8.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 04, 2017 at 04:13:51PM +0100, Jose Abreu wrote: > On 04-05-2017 15:40, Ville Syrjälä wrote: > > On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote: > >> + struct drm_encoder *encoder, > >> + struct drm_crtc *crtc, > >> + struct drm_display_mode *mode) > >> +{ > >> + const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > >> + const struct drm_connector_helper_funcs *connector_funcs = > >> + connector->helper_private; > >> + const struct drm_encoder_helper_funcs *encoder_funcs = > >> + encoder->helper_private; > >> + enum drm_mode_status ret = MODE_OK; > >> + > >> + if (connector_funcs && connector_funcs->mode_valid) > >> + ret = connector_funcs->mode_valid(connector, mode); > > As I mentioned earlier, this would break i915. We either can't call the > > connector hook at all here, or we have to make it somehow opt-in if some > > drivers really want to do it. > > Ok. You said you let users exceed the limits, but why doesn't it > fail in atomic_check and will fail in mode_valid? I guess I can > remove it, but this can lead to lots of confusion, i.e. with this > series the mode_valid callbacks for all components are called, if > I remove just one call the whole point will fall apart. > > Can we think about something else ? I think making it opt-in is > not ideal, if the helper is there then it should be used, but if > thats the best solution then shall we add a flag which will call > or not *all* the mode_valid callbacks in this stage > (drm_device.allow_modevalid_call, ...) ? This is a general issue, not a driver opt-in. With your new callbacks ideally we'll have: - all the source checks (clock limits, size limits) are in the crtc/encoder/bridge callbacks - only the sink limit checks (derived from edid or DP aux) are in the connector callback Letting userspace overwrite these seems like a reasonable idea that we should support in general I think. See also my comment on patch 1 for the connector's mode_valid kerneldoc. For arcpgu that might mean that you need to move part of the connector's mode_valid checks into the encoder, but since all the encoder modeset is in there already, that seems like a good cleanup of the drm modeset framework. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch