Re: [PATCH 2/2] drm/pl111: Initial drm/kms driver for pl111

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 5659 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Mar 17, 2017 at 03:47:42PM -0700, Eric Anholt wrote:
>> From: Tom Cooksey <tom.cooksey@arm.com>
>> 
>> 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.
>> 
>> 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).
>> 
>> v2: Nearly complete rewrite by anholt, cutting 2/3 of the code thanks
>>     to DRM core's excellent new helpers.
>> 
>> Signed-off-by: Tom Cooksey <tom.cooksey@arm.com>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> 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/pl111/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 <airlied@linux.ie>
>> + * 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 Software
>> + * Foundation, and any use by you of this program is subject to the terms of
>> + * such GNU licence.
>> + *
>> + */
>> +
>> +/**
>> + * pl111_drm_connector.c
>> + * Implementation of the connector functions for PL111 DRM
>> + */
>> +#include <linux/amba/clcd-regs.h>
>> +#include <linux/version.h>
>> +#include <linux/shmem_fs.h>
>> +#include <linux/dma-buf.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>> +
>> +#include "pl111_drm.h"
>> +
>> +static void pl111_connector_destroy(struct drm_connector *connector)
>> +{
>> +	struct pl111_drm_connector *pl111_connector =
>> +		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_connector
>> +							*connector, bool force)
>> +{
>> +	struct pl111_drm_connector *pl111_connector =
>> +		to_pl111_connector(connector);
>> +
>> +	return (pl111_connector->panel ?
>> +		connector_status_connected :
>> +		connector_status_disconnected);
>> +}
>> +
>> +static int pl111_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> +	struct pl111_drm_connector *pl111_connector =
>> +		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 = dev->dev_private;
>> +	int ret = 0;
>> +
>> +	if (!priv)
>> +		return -EINVAL;
>> +
>> +	drm_mode_config_init(dev);
>> +	mode_config = &dev->mode_config;
>> +	mode_config->funcs = &mode_config_funcs;
>> +	mode_config->min_width = 1;
>> +	mode_config->max_width = 1024;
>> +	mode_config->min_height = 1;
>> +	mode_config->max_height = 768;
>> +
>> +	ret = pl111_primary_plane_init(dev);
>> +	if (ret != 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 = {
>> +	.owner = THIS_MODULE,
>> +	.open = drm_open,
>> +	.release = drm_release,
>> +	.unlocked_ioctl = drm_ioctl,
>> +	.mmap = drm_gem_cma_mmap,
>> +	.poll = drm_poll,
>> +	.read = drm_read,
>> +};
>
> Very recent, but DEFINE_DRM_GEM_CMA_FOPS.

Will do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]