Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756035AbdCTSYe (ORCPT ); Mon, 20 Mar 2017 14:24:34 -0400 Received: from anholt.net ([50.246.234.109]:59704 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753582AbdCTSYb (ORCPT ); Mon, 20 Mar 2017 14:24:31 -0400 From: Eric Anholt To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, tom.cooksey@arm.com, Russell King , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] drm/pl111: Initial drm/kms driver for pl111 In-Reply-To: <20170320083422.sd6uvisgdgajwxtr@phenom.ffwll.local> References: <20170317224742.17219-1-eric@anholt.net> <20170317224742.17219-3-eric@anholt.net> <20170320083422.sd6uvisgdgajwxtr@phenom.ffwll.local> User-Agent: Notmuch/0.22.2+1~gb0bcfaa (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 20 Mar 2017 11:23:39 -0700 Message-ID: <87shm7rems.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6550 Lines: 195 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Daniel Vetter writes: > On Fri, Mar 17, 2017 at 03:47:42PM -0700, Eric Anholt wrote: >> From: Tom Cooksey >>=20 >> This is a modesetting driver for the pl111 CLCD display controller >> found on various ARM platforms such as the Versatile Express. The >> driver has only been tested on the bcm911360_entphn platform so far, >> with PRIME-based buffer sharing between vc4 and clcd. >>=20 >> It reuses the existing devicetree binding, while not using quite as >> many of its properties as the fbdev driver does (those are left for >> future work). >>=20 >> v2: Nearly complete rewrite by anholt, cutting 2/3 of the code thanks >> to DRM core's excellent new helpers. >>=20 >> Signed-off-by: Tom Cooksey >> Signed-off-by: Eric Anholt > > Looks pretty. A few things below, but nothing big. I'd say if the "how > generic do we want this to be for now" question is resolved it's ready to > go in. > > If you want this in drm-misc (imo fine, you're already there so doesn't > really extend the scope of the experiment), then please also add a > MAINTAINERS entry with the drm-misc git repo and yourself as reviewer. Will do. >> diff --git a/drivers/gpu/drm/pl111/pl111_connector.c b/drivers/gpu/drm/p= l111/pl111_connector.c >> new file mode 100644 >> index 000000000000..9811d1eadb63 >> --- /dev/null >> +++ b/drivers/gpu/drm/pl111/pl111_connector.c >> @@ -0,0 +1,127 @@ >> +/* >> + * (C) COPYRIGHT 2012-2013 ARM Limited. All rights reserved. >> + * >> + * Parts of this file were based on sources as follows: >> + * >> + * Copyright (c) 2006-2008 Intel Corporation >> + * Copyright (c) 2007 Dave Airlie >> + * Copyright (C) 2011 Texas Instruments >> + * >> + * This program is free software and is provided to you under the terms= of the >> + * GNU General Public License version 2 as published by the Free Softwa= re >> + * Foundation, and any use by you of this program is subject to the ter= ms of >> + * such GNU licence. >> + * >> + */ >> + >> +/** >> + * pl111_drm_connector.c >> + * Implementation of the connector functions for PL111 DRM >> + */ >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "pl111_drm.h" >> + >> +static void pl111_connector_destroy(struct drm_connector *connector) >> +{ >> + struct pl111_drm_connector *pl111_connector =3D >> + to_pl111_connector(connector); >> + >> + if (pl111_connector->panel) >> + drm_panel_detach(pl111_connector->panel); >> + >> + drm_connector_unregister(connector); >> + drm_connector_cleanup(connector); >> +} >> + >> +static enum drm_connector_status pl111_connector_detect(struct drm_conn= ector >> + *connector, bool force) >> +{ >> + struct pl111_drm_connector *pl111_connector =3D >> + to_pl111_connector(connector); >> + >> + return (pl111_connector->panel ? >> + connector_status_connected : >> + connector_status_disconnected); >> +} >> + >> +static int pl111_connector_helper_get_modes(struct drm_connector *conne= ctor) >> +{ >> + struct pl111_drm_connector *pl111_connector =3D >> + to_pl111_connector(connector); >> + >> + if (!pl111_connector->panel) >> + return 0; >> + >> + return drm_panel_get_modes(pl111_connector->panel); >> +} > > Probably the umptenth time I've seen this :( > > One option I think would work well is if we have a generic "wrap a > drm_panel into a drm_bridge" driver and just glue that in with an of > helper as the last element in the enc/transcoder. Laurent has that > practically written already, but he insist in calling it the lvds bridge, > because it's for a 100% dummy lvds transcoder. > > But since it's 100% dummy it's indistinguishable from pure sw > abstraction/impendence mismatch helper. > > Anyway, just an idea, not going to ask you to do this, but if drm_panel > takes of like crazy we'll probably want this. It seems like a generalization of lvds_encoder to wrap a panel as a connector would be useful. I think I'd like to look at that as a follow-on change. >> +static int pl111_modeset_init(struct drm_device *dev) >> +{ >> + struct drm_mode_config *mode_config; >> + struct pl111_drm_dev_private *priv =3D dev->dev_private; >> + int ret =3D 0; >> + >> + if (!priv) >> + return -EINVAL; >> + >> + drm_mode_config_init(dev); >> + mode_config =3D &dev->mode_config; >> + mode_config->funcs =3D &mode_config_funcs; >> + mode_config->min_width =3D 1; >> + mode_config->max_width =3D 1024; >> + mode_config->min_height =3D 1; >> + mode_config->max_height =3D 768; >> + >> + ret =3D pl111_primary_plane_init(dev); >> + if (ret !=3D 0) { >> + dev_err(dev->dev, "Failed to init primary plane\n"); >> + goto out_config; >> + } > > I assume this display IP has a pile of planes? Otherwise the simple pipe > helpers look like a perfect fit. Only two, actually. And the other (cursor) is a 64x64 monochrome with mask, so I'm not sure it's going to make sense to ever expose it. I think I'll give the simple helper a try before resubmitting. >> +static const struct file_operations drm_fops =3D { >> + .owner =3D THIS_MODULE, >> + .open =3D drm_open, >> + .release =3D drm_release, >> + .unlocked_ioctl =3D drm_ioctl, >> + .mmap =3D drm_gem_cma_mmap, >> + .poll =3D drm_poll, >> + .read =3D drm_read, >> +}; > > Very recent, but DEFINE_DRM_GEM_CMA_FOPS. Will do. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAljQHisACgkQtdYpNtH8 nujffRAArvoFt51cWMmohLgUDaYYJyErJ3RwGkC8Bg0Fm16Q4gdPWqmzCS+tE09M MqJJbmGTgbxBHECIa2ix27ifat2Pq0tf/9NOrXVuG2Aw/JeadUa0ihq51zpv0v2Y yUPEEhoTe3sxLCwR/P51wgY57FtC21UMaXonAhNCtRaJ6KSirOLybYOTBnbJpxv4 d7MhgU4ml3oMyNdmn66M8uSKYLSoDVEmLAH/aqRSycPzqgii0X1IRb9rSrNlO4XP A+Ro7p5MKrQVrDyvS8yP75jGv4Gk7bxeJ4kob3ZMtQhbhivJCUg1aE+lPG8OcIaY R808+q15Kh1SVPGieFf9LnjHhjicJLK+DCuYV0Fg2NsPm1A+ZnKCLg4VC3Qv+TA0 oPDZwiDXpjcAaMRf9mnBxFeZ2YmQhTn+sk8GtkeZ4hm5oXDBgL3TI6TO0aYrRASx HVLvF4RLoarUUmV1xSqd54F/2ZUIoi6Bps9w1OKAITWMbN7lIh9MwRi4K7DVdCFX 5PoeXAjKG2v3fD8uWQqXLm6qBowejpepEKcKIbRK5Gnz4JYr8WxXIfPWw3I+5LQd QIy3cSibp1ggOX3ZqJXXaHuy4VyMHJHV0suf4+x/005wY/EFz9UW7ANQL+Kv648F 7KG5QZ1JNtRvXMESvli4U985D9d6EO6sv79G0oyRt5wHCrtZx5g= =hdrr -----END PGP SIGNATURE----- --=-=-=--