From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758789AbcEFTpw (ORCPT ); Fri, 6 May 2016 15:45:52 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:41445 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758152AbcEFTpt (ORCPT ); Fri, 6 May 2016 15:45:49 -0400 Subject: Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector To: Thierry Reding References: <1462454674-2246-1-git-send-email-noralf@tronnes.org> <1462454674-2246-5-git-send-email-noralf@tronnes.org> <20160506140347.GB4641@ulmo.ba.sec> <20160506140816.GD27098@phenom.ffwll.local> <20160506141532.GC4641@ulmo.ba.sec> <20160506144318.GA15076@ulmo.ba.sec> Cc: treding@nvidia.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <3123f353-439d-fbdd-22c8-38c3e4eb3717@tronnes.org> Date: Fri, 6 May 2016 21:45:41 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160506144318.GA15076@ulmo.ba.sec> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Den 06.05.2016 16:43, skrev Thierry Reding: > On Fri, May 06, 2016 at 04:34:08PM +0200, Noralf Trønnes wrote: >> Den 06.05.2016 16:15, skrev Thierry Reding: >>> On Fri, May 06, 2016 at 04:08:16PM +0200, Daniel Vetter wrote: >>>> On Fri, May 06, 2016 at 04:03:47PM +0200, Thierry Reding wrote: >>>>> On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote: >>>>>> Add function to create a simple connector for a panel. >>>>> I'm not sure I see the usefulness of this. Typically you'd attach a >>>>> panel to an encoder/connector, in which case you already have the >>>>> connector. >>>>> >>>>> Perhaps it would become more obvious why we need this if you posted >>>>> patches that show where this is used? >>>> The other helpers give you a simple drm pipeline with plane, crtc & >>>> encoder all baked into on drm_simple_pipeline structure. The only thing >>>> variable you have to hook up to that is the drm_connector. And I think for >>>> dead-simple panels avoiding the basic boilerplate in that does indeed make >>>> some sense. >>> Avoiding boilerplate is good, but I have a difficult time envisioning >>> how you might want to use this. At the same time I'm asking myself how >>> we know that this helper is any good if we haven't seen it used anywhere >>> and actually see the boilerplate go away. >> I pulled out the patches from the tinydrm patchset that would go >> into drm core and helpers. I'm not very good at juggling many patches >> around in various version and getting it right. >> I'm doing development in the downstream Raspberry Pi repo >> on 4.5 to get all the pi drivers, and then apply it on linux-next... >> >> This is the tinydrm patch that will use it: >> https://lists.freedesktop.org/archives/dri-devel/2016-April/104502.html >> >> Extract: >> +int tinydrm_display_pipe_init(struct tinydrm_device *tdev, >> + const uint32_t *formats, unsigned int format_count) >> +{ >> + struct drm_device *dev = tdev->base; >> + struct drm_connector *connector; >> + int ret; >> + >> + connector = drm_simple_kms_panel_connector_create(dev, &tdev->panel, >> + DRM_MODE_CONNECTOR_VIRTUAL); >> + if (IS_ERR(connector)) >> + return PTR_ERR(connector); >> + >> + ret = drm_simple_display_pipe_init(dev, &tdev->pipe, >> + &tinydrm_display_pipe_funcs, >> + formats, format_count, >> + connector); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(tinydrm_display_pipe_init); >> >> drm_simple_kms_panel_connector_create() changed name when Daniel >> suggested it be moved to drm_panel.c or drm_panel_helper.c. > Okay, that gives me a better understanding of where things are headed. > That said, why would these devices even need to deal with drm_panel in > the first place? Sounds to me like they are devices on some sort of > control bus that you talk to directly. And they will represent > essentially the panel with its built-in display controller. > > drm_panel on the other hand was designed as an interface between display > controllers and panels, with the goal to let any display controller talk > to any panel. > > While I'm sure you can support these types of simple panels using the > drm_panel infrastructure I'm not sure it's necessary, since your driver > will always talk to the panel directly, and hence the code to deal with > the panel specifics could be part of the display pipe functions. In the discussion following the "no more fbdev drivers" call, someone pointed me to drm_panel. It had function hooks that I needed, so I built tinydrm around it. If this is misusing drm_panel, it shouldn't be too difficult to drop it. Actually I could just do this: struct tinydrm_funcs { int (*prepare)(struct tinydrm_device *tdev); int (*unprepare)(struct tinydrm_device *tdev); int (*enable)(struct tinydrm_device *tdev); int (*disable)(struct tinydrm_device *tdev); int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned flags, unsigned color, struct drm_clip_rect *clips, unsigned num_clips); }; When it comes to the simple connector, one solution could be to extend drm_simple_display_pipe to embed a connector with (optional) modes: struct drm_simple_display_pipe { - struct drm_connector *connector; + struct drm_connector connector; + const struct drm_display_mode *modes; + unsigned int num_modes; }; And maybe optional detect and get_modes hooks: struct drm_simple_display_pipe_funcs { + enum drm_connector_status detect(struct drm_connector *connector, + bool force); + int (*get_modes)(struct drm_connector *connector); }; int drm_simple_display_pipe_init(struct drm_device *dev, struct drm_simple_display_pipe *pipe, struct drm_simple_display_pipe_funcs *funcs, const uint32_t *formats, unsigned int format_count, - struct drm_connector *connector); + int connector_type); Another solution is to create a simple connector with modes: struct drm_simple_connector { struct drm_connector connector; const struct drm_display_mode *modes; unsigned int num_modes; }; struct drm_connector *drm_simple_connector_create(struct drm_device *dev, const struct drm_display_mode *modes, unsigned int num_modes, int connector_type); Noralf.