| Message ID | 1462454674-2246-5-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote: > Add function to create a simple connector for a panel. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Like in the previous patch please also add a new section for the panel helpers to gpu.tmpl. I don't think this needs an overview section, it's so simple. But adding some cross references from the drm_panel.c kerneldoc to this and back would be real good. > --- > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/panel/Kconfig | 7 +++ > include/drm/drm_panel_helper.h | 20 +++++++ > 4 files changed, 145 insertions(+) > create mode 100644 drivers/gpu/drm/drm_panel_helper.c > create mode 100644 include/drm/drm_panel_helper.h > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 7e99923..3f3696c 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o > drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o > drm-$(CONFIG_PCI) += ati_pcigart.o > drm-$(CONFIG_DRM_PANEL) += drm_panel.o > +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o > drm-$(CONFIG_OF) += drm_of.o > drm-$(CONFIG_AGP) += drm_agpsupport.o > > diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c > new file mode 100644 > index 0000000..5d8b623 > --- /dev/null > +++ b/drivers/gpu/drm/drm_panel_helper.c > @@ -0,0 +1,117 @@ > +/* > + * Copyright (C) 2016 Noralf Trønnes > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/module.h> > +#include <linux/slab.h> > + > +#include <drm/drmP.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_panel.h> > + > +struct drm_panel_connector { > + struct drm_connector base; > + struct drm_panel *panel; > +}; > + > +static inline struct drm_panel_connector * > +to_drm_panel_connector(struct drm_connector *connector) > +{ > + return container_of(connector, struct drm_panel_connector, base); > +} > + > +static int drm_panel_connector_get_modes(struct drm_connector *connector) > +{ > + return drm_panel_get_modes(to_drm_panel_connector(connector)->panel); > +} > + > +static struct drm_encoder * > +drm_panel_connector_best_encoder(struct drm_connector *connector) > +{ > + return drm_encoder_find(connector->dev, connector->encoder_ids[0]); > +} As mentioned in my previous review, this function would be extremely useful to rid tons of drivers of boilerplate - most drm_connector only support exactly 1 encoder, statically determined at driver init time. Please put this helper into the atomic helper library, and maybe call it something like drm_atomic_helper_best_encoder. To make sure it's never abused by accident please also add a WARN_ON(connector->encoder_ids[1]); to make sure that there's really only just one encoder for this connector. If you're bored you can also go through drivers and use that everywhere it fits, it would result in a _lot_ of deleted code. But also needs quite a bit of audit work ... Otherwise lgtm, but please ask Thierry for an ack as the panel maintainer. -Daniel > + > +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = { > + .get_modes = drm_panel_connector_get_modes, > + .best_encoder = drm_panel_connector_best_encoder, > +}; > + > +static enum drm_connector_status > +drm_panel_connector_detect(struct drm_connector *connector, bool force) > +{ > + if (drm_device_is_unplugged(connector->dev)) > + return connector_status_disconnected; > + > + return connector->status; > +} > + > +static void drm_panel_connector_destroy(struct drm_connector *connector) > +{ > + struct drm_panel_connector *panel_connector; > + > + panel_connector = to_drm_panel_connector(connector); > + drm_panel_detach(panel_connector->panel); > + drm_panel_remove(panel_connector->panel); > + drm_connector_unregister(connector); > + drm_connector_cleanup(connector); > + kfree(panel_connector); > +} > + > +static const struct drm_connector_funcs drm_panel_connector_funcs = { > + .dpms = drm_atomic_helper_connector_dpms, > + .reset = drm_atomic_helper_connector_reset, > + .detect = drm_panel_connector_detect, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = drm_panel_connector_destroy, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +/** > + * drm_panel_connector_create - Create simple connector for panel > + * @dev: DRM device > + * @panel: DRM panel > + * @connector_type: user visible type of the connector > + * > + * Creates a simple connector for a panel. > + * The panel needs to provide a get_modes() function. > + * > + * Returns: > + * Pointer to new connector or ERR_PTR on failure. > + */ > +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, > + struct drm_panel *panel, > + int connector_type) > +{ > + struct drm_panel_connector *panel_connector; > + struct drm_connector *connector; > + int ret; > + > + panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL); > + if (!panel_connector) > + return ERR_PTR(-ENOMEM); > + > + panel_connector->panel = panel; > + connector = &panel_connector->base; > + drm_connector_helper_add(connector, &drm_panel_connector_hfuncs); > + ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs, > + connector_type); > + if (ret) { > + kfree(panel_connector); > + return ERR_PTR(ret); > + } > + > + connector->status = connector_status_connected; > + drm_panel_init(panel); > + drm_panel_add(panel); > + drm_panel_attach(panel, connector); > + > + return connector; > +} > +EXPORT_SYMBOL(drm_panel_connector_create); > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 1500ab9..87264a3 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -4,6 +4,13 @@ config DRM_PANEL > help > Panel registration and lookup framework. > > +config DRM_PANEL_HELPER > + bool > + depends on DRM > + select DRM_PANEL > + help > + Panel helper. > + > menu "Display Panels" > depends on DRM && DRM_PANEL > > diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h > new file mode 100644 > index 0000000..c3355e3 > --- /dev/null > +++ b/include/drm/drm_panel_helper.h > @@ -0,0 +1,20 @@ > +/* > + * Copyright (C) 2016 Noralf Trønnes > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#ifndef __DRM_PANEL_HELPER_H__ > +#define __DRM_PANEL_HELPER_H__ > + > +struct drm_device; > +struct drm_panel; > + > +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, > + struct drm_panel *panel, > + int connector_type); > + > +#endif > -- > 2.2.2 >
Den 05.05.2016 19:03, skrev Daniel Vetter: > On Thu, May 05, 2016 at 03:24:34PM +0200, Noralf Trønnes wrote: >> Add function to create a simple connector for a panel. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Like in the previous patch please also add a new section for the panel > helpers to gpu.tmpl. I don't think this needs an overview section, it's so > simple. But adding some cross references from the drm_panel.c kerneldoc to > this and back would be real good. drm_panel.c doesn't have any documentation and the header file has only the drm_panel_funcs struct documented, not hooked up to gpu.tmpl. I can make a patch documenting the functions, it looks fairly straight forward, but I have no idea what to put in the DOC: section, except an xref to this helper :-) Noralf. >> --- >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/panel/Kconfig | 7 +++ >> include/drm/drm_panel_helper.h | 20 +++++++ >> 4 files changed, 145 insertions(+) >> create mode 100644 drivers/gpu/drm/drm_panel_helper.c >> create mode 100644 include/drm/drm_panel_helper.h >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 7e99923..3f3696c 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o >> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o >> drm-$(CONFIG_PCI) += ati_pcigart.o >> drm-$(CONFIG_DRM_PANEL) += drm_panel.o >> +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o >> drm-$(CONFIG_OF) += drm_of.o >> drm-$(CONFIG_AGP) += drm_agpsupport.o >> >> diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c >> new file mode 100644 >> index 0000000..5d8b623 >> --- /dev/null >> +++ b/drivers/gpu/drm/drm_panel_helper.c >> @@ -0,0 +1,117 @@ >> +/* >> + * Copyright (C) 2016 Noralf Trønnes >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/slab.h> >> + >> +#include <drm/drmP.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_crtc_helper.h> >> +#include <drm/drm_panel.h> >> + >> +struct drm_panel_connector { >> + struct drm_connector base; >> + struct drm_panel *panel; >> +}; >> + >> +static inline struct drm_panel_connector * >> +to_drm_panel_connector(struct drm_connector *connector) >> +{ >> + return container_of(connector, struct drm_panel_connector, base); >> +} >> + >> +static int drm_panel_connector_get_modes(struct drm_connector *connector) >> +{ >> + return drm_panel_get_modes(to_drm_panel_connector(connector)->panel); >> +} >> + >> +static struct drm_encoder * >> +drm_panel_connector_best_encoder(struct drm_connector *connector) >> +{ >> + return drm_encoder_find(connector->dev, connector->encoder_ids[0]); >> +} > As mentioned in my previous review, this function would be extremely > useful to rid tons of drivers of boilerplate - most drm_connector only > support exactly 1 encoder, statically determined at driver init time. > > Please put this helper into the atomic helper library, and maybe call it > something like > > drm_atomic_helper_best_encoder. > > To make sure it's never abused by accident please also add a > > WARN_ON(connector->encoder_ids[1]); > > to make sure that there's really only just one encoder for this connector. > > If you're bored you can also go through drivers and use that everywhere it > fits, it would result in a _lot_ of deleted code. But also needs quite a > bit of audit work ... > > Otherwise lgtm, but please ask Thierry for an ack as the panel maintainer. > -Daniel > > >> + >> +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = { >> + .get_modes = drm_panel_connector_get_modes, >> + .best_encoder = drm_panel_connector_best_encoder, >> +}; >> + >> +static enum drm_connector_status >> +drm_panel_connector_detect(struct drm_connector *connector, bool force) >> +{ >> + if (drm_device_is_unplugged(connector->dev)) >> + return connector_status_disconnected; >> + >> + return connector->status; >> +} >> + >> +static void drm_panel_connector_destroy(struct drm_connector *connector) >> +{ >> + struct drm_panel_connector *panel_connector; >> + >> + panel_connector = to_drm_panel_connector(connector); >> + drm_panel_detach(panel_connector->panel); >> + drm_panel_remove(panel_connector->panel); >> + drm_connector_unregister(connector); >> + drm_connector_cleanup(connector); >> + kfree(panel_connector); >> +} >> + >> +static const struct drm_connector_funcs drm_panel_connector_funcs = { >> + .dpms = drm_atomic_helper_connector_dpms, >> + .reset = drm_atomic_helper_connector_reset, >> + .detect = drm_panel_connector_detect, >> + .fill_modes = drm_helper_probe_single_connector_modes, >> + .destroy = drm_panel_connector_destroy, >> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> +}; >> + >> +/** >> + * drm_panel_connector_create - Create simple connector for panel >> + * @dev: DRM device >> + * @panel: DRM panel >> + * @connector_type: user visible type of the connector >> + * >> + * Creates a simple connector for a panel. >> + * The panel needs to provide a get_modes() function. >> + * >> + * Returns: >> + * Pointer to new connector or ERR_PTR on failure. >> + */ >> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, >> + struct drm_panel *panel, >> + int connector_type) >> +{ >> + struct drm_panel_connector *panel_connector; >> + struct drm_connector *connector; >> + int ret; >> + >> + panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL); >> + if (!panel_connector) >> + return ERR_PTR(-ENOMEM); >> + >> + panel_connector->panel = panel; >> + connector = &panel_connector->base; >> + drm_connector_helper_add(connector, &drm_panel_connector_hfuncs); >> + ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs, >> + connector_type); >> + if (ret) { >> + kfree(panel_connector); >> + return ERR_PTR(ret); >> + } >> + >> + connector->status = connector_status_connected; >> + drm_panel_init(panel); >> + drm_panel_add(panel); >> + drm_panel_attach(panel, connector); >> + >> + return connector; >> +} >> +EXPORT_SYMBOL(drm_panel_connector_create); >> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig >> index 1500ab9..87264a3 100644 >> --- a/drivers/gpu/drm/panel/Kconfig >> +++ b/drivers/gpu/drm/panel/Kconfig >> @@ -4,6 +4,13 @@ config DRM_PANEL >> help >> Panel registration and lookup framework. >> >> +config DRM_PANEL_HELPER >> + bool >> + depends on DRM >> + select DRM_PANEL >> + help >> + Panel helper. >> + >> menu "Display Panels" >> depends on DRM && DRM_PANEL >> >> diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h >> new file mode 100644 >> index 0000000..c3355e3 >> --- /dev/null >> +++ b/include/drm/drm_panel_helper.h >> @@ -0,0 +1,20 @@ >> +/* >> + * Copyright (C) 2016 Noralf Trønnes >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#ifndef __DRM_PANEL_HELPER_H__ >> +#define __DRM_PANEL_HELPER_H__ >> + >> +struct drm_device; >> +struct drm_panel; >> + >> +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, >> + struct drm_panel *panel, >> + int connector_type); >> + >> +#endif >> -- >> 2.2.2 >>
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?
Thierry
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. -Daniel
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. Thierry
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. Noralf.
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); It's imo even more impressive when you show the entire driver, including all the hooks. Because there's just 4 of them iirc, all optional. And besides the tinydrm panel drivers in it's various editions we could also use this for simpledrm (the boot-up firmware kms driver from David Herrmann) and probably a few other super-simple display drivers. Essentially with this I think drm is now better suited for dumb&simple hardware than fbdev. -Daniel
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. Thierry
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.
On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > 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); Tbh I really like that simple_display_pipe is split after the encoder. This allows us to easily splice in a drm_bridge (which will register the drm_connector itself) with very little work. And even if there's not a full-blown bridge you might have different connectors and things. > 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); Yeah, this makes more sense to me, but then we're kinda reinventing something like simple-panel.c with this. Otoh right now with smple_display_pipe it's not possible to wire up the panel callbacks easily, so maybe it should be a drm_bridge. Or we just leave this code in tinydrm and extract it when someone else has a need for it? -Daniel
Den 07.05.2016 11:59, skrev Daniel Vetter: > On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@tronnes.org> wrote: >> 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); > Tbh I really like that simple_display_pipe is split after the encoder. > This allows us to easily splice in a drm_bridge (which will register > the drm_connector itself) with very little work. And even if there's > not a full-blown bridge you might have different connectors and > things. > >> 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); > Yeah, this makes more sense to me, but then we're kinda reinventing > something like simple-panel.c with this. Otoh right now with > smple_display_pipe it's not possible to wire up the panel callbacks > easily, so maybe it should be a drm_bridge. Or we just leave this code > in tinydrm and extract it when someone else has a need for it? Yes, since there's no obvious use for such a simple controller I'll just put it in tinydrm. Noralf.
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 7e99923..3f3696c 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o drm-$(CONFIG_DRM_PANEL) += drm_panel.o +drm-$(CONFIG_DRM_PANEL_HELPER) += drm_panel_helper.o drm-$(CONFIG_OF) += drm_of.o drm-$(CONFIG_AGP) += drm_agpsupport.o diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c new file mode 100644 index 0000000..5d8b623 --- /dev/null +++ b/drivers/gpu/drm/drm_panel_helper.c @@ -0,0 +1,117 @@ +/* + * Copyright (C) 2016 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/module.h> +#include <linux/slab.h> + +#include <drm/drmP.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_panel.h> + +struct drm_panel_connector { + struct drm_connector base; + struct drm_panel *panel; +}; + +static inline struct drm_panel_connector * +to_drm_panel_connector(struct drm_connector *connector) +{ + return container_of(connector, struct drm_panel_connector, base); +} + +static int drm_panel_connector_get_modes(struct drm_connector *connector) +{ + return drm_panel_get_modes(to_drm_panel_connector(connector)->panel); +} + +static struct drm_encoder * +drm_panel_connector_best_encoder(struct drm_connector *connector) +{ + return drm_encoder_find(connector->dev, connector->encoder_ids[0]); +} + +static const struct drm_connector_helper_funcs drm_panel_connector_hfuncs = { + .get_modes = drm_panel_connector_get_modes, + .best_encoder = drm_panel_connector_best_encoder, +}; + +static enum drm_connector_status +drm_panel_connector_detect(struct drm_connector *connector, bool force) +{ + if (drm_device_is_unplugged(connector->dev)) + return connector_status_disconnected; + + return connector->status; +} + +static void drm_panel_connector_destroy(struct drm_connector *connector) +{ + struct drm_panel_connector *panel_connector; + + panel_connector = to_drm_panel_connector(connector); + drm_panel_detach(panel_connector->panel); + drm_panel_remove(panel_connector->panel); + drm_connector_unregister(connector); + drm_connector_cleanup(connector); + kfree(panel_connector); +} + +static const struct drm_connector_funcs drm_panel_connector_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .reset = drm_atomic_helper_connector_reset, + .detect = drm_panel_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = drm_panel_connector_destroy, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +/** + * drm_panel_connector_create - Create simple connector for panel + * @dev: DRM device + * @panel: DRM panel + * @connector_type: user visible type of the connector + * + * Creates a simple connector for a panel. + * The panel needs to provide a get_modes() function. + * + * Returns: + * Pointer to new connector or ERR_PTR on failure. + */ +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, + struct drm_panel *panel, + int connector_type) +{ + struct drm_panel_connector *panel_connector; + struct drm_connector *connector; + int ret; + + panel_connector = kzalloc(sizeof(*panel_connector), GFP_KERNEL); + if (!panel_connector) + return ERR_PTR(-ENOMEM); + + panel_connector->panel = panel; + connector = &panel_connector->base; + drm_connector_helper_add(connector, &drm_panel_connector_hfuncs); + ret = drm_connector_init(dev, connector, &drm_panel_connector_funcs, + connector_type); + if (ret) { + kfree(panel_connector); + return ERR_PTR(ret); + } + + connector->status = connector_status_connected; + drm_panel_init(panel); + drm_panel_add(panel); + drm_panel_attach(panel, connector); + + return connector; +} +EXPORT_SYMBOL(drm_panel_connector_create); diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 1500ab9..87264a3 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -4,6 +4,13 @@ config DRM_PANEL help Panel registration and lookup framework. +config DRM_PANEL_HELPER + bool + depends on DRM + select DRM_PANEL + help + Panel helper. + menu "Display Panels" depends on DRM && DRM_PANEL diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h new file mode 100644 index 0000000..c3355e3 --- /dev/null +++ b/include/drm/drm_panel_helper.h @@ -0,0 +1,20 @@ +/* + * Copyright (C) 2016 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __DRM_PANEL_HELPER_H__ +#define __DRM_PANEL_HELPER_H__ + +struct drm_device; +struct drm_panel; + +struct drm_connector *drm_panel_connector_create(struct drm_device *dev, + struct drm_panel *panel, + int connector_type); + +#endif
Add function to create a simple connector for a panel. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_panel_helper.c | 117 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/panel/Kconfig | 7 +++ include/drm/drm_panel_helper.h | 20 +++++++ 4 files changed, 145 insertions(+) create mode 100644 drivers/gpu/drm/drm_panel_helper.c create mode 100644 include/drm/drm_panel_helper.h