From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754996AbdBNUWl (ORCPT ); Tue, 14 Feb 2017 15:22:41 -0500 Received: from mail-ot0-f195.google.com ([74.125.82.195]:34177 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751666AbdBNUWe (ORCPT ); Tue, 14 Feb 2017 15:22:34 -0500 Date: Tue, 14 Feb 2017 21:22:29 +0100 From: Daniel Vetter To: John Stultz Cc: Daniel Vetter , lkml , Chen Feng , Xinwei Kong , Xinliang Liu , dri-devel , Rongrong Zou , Daniel Vetter Subject: Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs Message-ID: <20170214202229.GG22762@dvetter-linux.ger.corp.intel.com> Mail-Followup-To: John Stultz , lkml , Chen Feng , Xinwei Kong , Xinliang Liu , dri-devel , Rongrong Zou , Daniel Vetter References: <1487100302-9445-1-git-send-email-john.stultz@linaro.org> <1487100302-9445-2-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux dvetter-linux.ger.corp.intel.com 4.8.15-200.fc24.x86_64 User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 14, 2017 at 11:45:54AM -0800, John Stultz wrote: > On Tue, Feb 14, 2017 at 11:38 AM, Daniel Vetter wrote: > > On Tue, Feb 14, 2017 at 8:25 PM, John Stultz wrote: > >> Currently, on the hikey board, we have the adv7511 bridge wired > >> up to the kirin ade drm driver. Unfortunately, the kirin ade > >> core cannot generate accurate byteclocks for all pixel clock > >> values. > >> > >> Thus if a mode clock is selected that we cannot calculate a > >> matching byteclock, the device will boot with a blank screen. > >> > >> Unfortunately, currently the only place we can properly check > >> potential modes for this issue in the connector mode_valid > >> helper. Again, hikey uses the adv7511 bridge, which is shared > >> between a number of different devices, so its improper to put > >> restrictions caused by the kirin drm driver in the adv7511 > >> logic. > >> > >> So this patch tries to correct for that, by adding some > >> infrastructure so that the drm_crtc_helper_funcs can optionally > >> implement a mode_valid check, so that the probe helpers can > >> check to make sure there are not any restrictions at the crtc > >> level as well. > >> > >> Cc: Daniel Vetter > >> Cc: Jani Nikula > >> Cc: Sean Paul > >> Cc: David Airlie > >> Cc: Rob Clark > >> Cc: Xinliang Liu > >> Cc: Xinliang Liu > >> Cc: Rongrong Zou > >> Cc: Xinwei Kong > >> Cc: Chen Feng > >> Cc: Archit Taneja > >> Cc: dri-devel@lists.freedesktop.org > >> Signed-off-by: John Stultz > > > > So I'm going to be super-annoying here and ask for a complete > > solution. This here is defacto what ever driver already does (or has > > too), but it doesn't really solve the overall issue of having entirely > > separate validation paths for probe and atomic_check paths. I think if > > we wan to solve this, we need to solve this properly, with a generic > > solution. That would mean: > > - still in helpers, to make it all opt-int > > Just to be clear, I believe what I proposed is opt-in, but I assume > you want that in addition to the following, right? > > > - covers crtc and encoders and bridges > > So you'd like similar mode_valid() calls in the crtc/encoder/bridge > helpers? right? > > > - allows you to implement the current mode_valid in terms of the new > > stuff (maybe as a default hook) > > This bit I'm not sure I'm following. The current drm_connector's > mode_valid in terms of a new mode_valid call that also looks at > crtc/encoder/bridges? Or do you mean something else? > > > - allows you to implement the current assortment of mode_fixup and/or > > atomic_check in terms of the new stuff, or at least to not have to > > duplicate logic in there > > This is over my head, so I'll have to research to better understand. > > > Docs for all this, especially updating all the warnings on how to use > > the existing hooks correctly. > > That's fair. > > > I think just pushing this specific case into the helpers, without > > rethinking the overall big picture, isn't gaining us all that much. > > For just this I'd say just put it into drivers, until we have some > > Not following here. What do you mean by "put it into drivers"? Where? In your driver callback for ->mode_valid, call into a shared function to validate CRTC limits. Which you also call from the crtc's ->mode_fixup function. In short my complain here is that this is only a partial solution of the larger problem, specific for your driver. That kind of code is better put into drivers, until we have a clear understanding to type up something complete in the helpers. Shared code is imo overrated :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch