Daniel Vetter <daniel@xxxxxxxx> writes:
> On Fri, Mar 17, 2017 at 03:47:42PM -0700, Eric Anholt wrote:
>> From: Tom Cooksey <tom.cooksey@xxxxxxx>
>>
>> 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@xxxxxxx>
>> Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>
>
> 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@xxxxxxxx>
>> + * 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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
