From patchwork Fri Jun 14 00:54:04 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2719411 Return-Path: X-Original-To: patchwork-linux-sh@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 50D149F472 for ; Fri, 14 Jun 2013 00:53:58 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 19ECC20255 for ; Fri, 14 Jun 2013 00:53:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D899E201FD for ; Fri, 14 Jun 2013 00:53:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754240Ab3FNAxz (ORCPT ); Thu, 13 Jun 2013 20:53:55 -0400 Received: from perceval.ideasonboard.com ([95.142.166.194]:53016 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753835Ab3FNAxy (ORCPT ); Thu, 13 Jun 2013 20:53:54 -0400 Received: from avalon.localnet (254.27-200-80.adsl-dyn.isp.belgacom.be [80.200.27.254]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9369435A47; Fri, 14 Jun 2013 02:53:47 +0200 (CEST) From: Laurent Pinchart To: Daniel Vetter Cc: Laurent Pinchart , dri-devel , linux-sh@vger.kernel.org Subject: Re: [PATCH v3] drm: Renesas R-Car Display Unit DRM driver Date: Fri, 14 Jun 2013 02:54:04 +0200 Message-ID: <1393060.SF8x67qbTg@avalon> User-Agent: KMail/4.10.2 (Linux/3.8.13-gentoo; KDE/4.10.2; x86_64; ; ) In-Reply-To: <20130607085055.GP7827@phenom.ffwll.local> References: <1370314420-26524-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <4571041.QYQjiyisGP@avalon> <20130607085055.GP7827@phenom.ffwll.local> MIME-Version: 1.0 Sender: linux-sh-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Daniel, On Friday 07 June 2013 10:50:55 Daniel Vetter wrote: > On Fri, Jun 07, 2013 at 09:44:45AM +0200, Laurent Pinchart wrote: > > On Wednesday 05 June 2013 10:55:05 Daniel Vetter wrote: > > > On Wed, Jun 05, 2013 at 03:51:53AM +0200, Laurent Pinchart wrote: > > > > On Tuesday 04 June 2013 20:36:20 Daniel Vetter wrote: > > > > > On Tue, Jun 4, 2013 at 8:03 PM, Laurent Pinchart wrote: > > > > > > On Tuesday 04 June 2013 16:12:36 Daniel Vetter wrote: > > > > > >> On Tue, Jun 04, 2013 at 04:53:40AM +0200, Laurent Pinchart wrote: > > > [snip] > > > > > > > > >> > +static int rcar_du_vga_connector_get_modes(struct > > > > > >> > drm_connector > > > > > >> > *connector) > > > > > >> > +{ > > > > > >> > + return drm_add_modes_noedid(connector, 1280, 768); > > > > > >> > +} > > > > > >> > > > > > >> This (and the dummy detect function below) looks a bit funny, > > > > > >> since it essentially overrides the default behaviour already > > > > > >> provided by the crtc helpers. Until rcar has at least proper > > > > > >> detect support for VGA > > > > > > > > > > > > I would add that but the DDC signals are not connected on the > > > > > > boards I have access to :-/ > > > > > > > > > > > >> I'd just kill this and use the connector force support (and > > > > > >> manually adding the right resolutions). > > > > > > > > > > > > Looks like that's a candidate for better documentation... How does > > > > > > force support work ? > > > > > > > > > > Grep for DRM_FORCE_ON, iirc it can be set on the commandline, where > > > > > you can also force a specific mode. The best I could find wrt docs > > > > > is the kerneldoc for drm_mode_parse_command_line_for_connector. With > > > > > a bit more reading it looks like it's intermingled with the fbdev > > > > > helper code, but should be fairly easy to extract and used by your > > > > > driver. > > > > > > > > It makes sense to force the connector state from command line, but I'm > > > > not sure if the same mechanism is the best solution here. As the > > > > driver has no way to know the connector state, the best we can do is > > > > guess what modes are supported. I can just return 0 in the get_modes > > > > handler, but then the core will not call drm_add_modes_noedid(), and > > > > modes will need to be added manually. > > > > > > > > Is your point that for a board on which the VGA connector state can't > > > > be detected, the user should always be responsible for adding all the > > > > modes supported by the VGA monitor on the command line ? > > > > > > My point is that we already have both an established code for connected > > > outputs without EDID to add fallback modes and means to force connectors > > > to certain states. Your code here seems to reinvent that wheel, so I > > > wonder what we should/need to improve in the common code to suit your > > > needs. > > > > The currently available code might suit my needs, it might just be that I > > fail to see how to use it properly. > > > > Regarding the "code for connected outputs without EDID to add fallback > > modes" you're referring to, is that > > > > if (count == 0 && connector->status == connector_status_connected) > > count = drm_add_modes_noedid(connector, 1024, 768); > > > > in drm_helper_probe_single_connector_modes() ? That function will only be > > called if the connector status is connector_status_connected. There are > > two ways to enforce that: > > > > - returning connector_status_connected from the connector detect() > > operation, which seems to defeat the purpose of having > > connector_status_unknown completely. > > We might want to add such a default mode also for unknown, I'm not sure. > Userspace policy is to first try to light up any connected outputs, and if > there's none try to light up any unknown outputs. Not sure whether userspace > (i.e. X) will automatically add a default mode. fbcon might also handle this > less gracefully. > > Personally I'm ok with extending this to unknown, it shouldn't really hurt > (since we already try really hard not to leak unknown anywhere visible). Do you mean something like If so I can submit a proper patch. > > - setting connector->force to DRM_FORCE_ON. Are drivers allowed to do so > > themselves at initialization time ? Once again that seems to defeat the > > purpose of connector_status_unknown. > > Atm you can set that with the kernel video= cmdline option, but only if > fbcon force this to be parsed. I think exposing ->force to userspace > somewhere in sysfs would make lots of sense. Drivers imo shouldn't ever > need to touch this. And there's a callback interface so that drivers can > intercept forced connector state, e.g. when they need to set up some stuff > which they otherwise would only do in their ->detect callback. > > > > A few ideas: > > > - Untangling the connector forcing code from the fbdev helper so that > > > you can use it. > > > > > > - Exposing the connector state forcing through sysfs so that it's > > > runtime-adjustable. > > > > My main concern here is that fbcon won't be available if we delay setting > > force mode until userspace is ready.. > > There's also a kernel option. Since we're talking about a VGA connector I > don't think we could do a hardwired quirk here. > > > > - Adding fallback modes for connectors in the unknonw state (imo too > > > much risk in breaking something else). > > > > Could you please elaborate on what you thing it could break ? > > Changed my mind ;-) Ajax recently said that X only looks at unknown > connectors if there's nothing better around, so if we stick to that policy > we should be good. > > > > Thinking about this some more I'd vote for the new sysfs file to expose > > > connector forcing at runtime. With that it'd boil down to 1024x756 vs. > > > 1280x768 for the default fallback mode. And that could be fixed with the > > > EDID quirk support. Although that looks like it would benefit from a > > > per-connector sysfs file, too ;-) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index f554516..9aae384 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -160,7 +160,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, #endif count = (*connector_funcs->get_modes)(connector); - if (count == 0 && connector->status == connector_status_connected) + if (count == 0 && (connector->status == connector_status_connected || + connector->status == connector_status_unknown)) count = drm_add_modes_noedid(connector, 1024, 768); if (count == 0) goto prune;