| Message ID | 1462982962-10530-7-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: > Provides helper functions for drivers that have a simple display > pipeline. Plane, crtc and encoder are collapsed into one entity. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> Looks really nice, just a few suggestions for the kerneldoc. Would be awesome if we could get an ack on this from Jyri for tilcdc, but even without that I think I'll just pull in the next iteration. Still please cc him. Thanks, Daniel > --- > > Changes since v1: > - Add DOC header and add to gpu.tmpl > - Fix docs: @funcs is optional, "negative error code", > "This hook is optional." > - Add checks to drm_simple_kms_plane_atomic_check() > > Documentation/DocBook/gpu.tmpl | 6 + > drivers/gpu/drm/Kconfig | 7 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++ > include/drm/drm_simple_kms_helper.h | 92 +++++++++++++++ > 5 files changed, 306 insertions(+) > create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c > create mode 100644 include/drm/drm_simple_kms_helper.h > > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl > index 4a0c599..cf3f5a8 100644 > --- a/Documentation/DocBook/gpu.tmpl > +++ b/Documentation/DocBook/gpu.tmpl > @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev) > !Edrivers/gpu/drm/drm_panel.c > !Pdrivers/gpu/drm/drm_panel.c drm panel > </sect2> > + <sect2> > + <title>Simple KMS Helper Reference</title> > +!Iinclude/drm/drm_simple_kms_helper.h > +!Edrivers/gpu/drm/drm_simple_kms_helper.c > +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview > + </sect2> > </sect1> > > <!-- Internals: kms properties --> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 16e4c21..ff9f480 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -39,6 +39,13 @@ config DRM_KMS_HELPER > help > CRTC helpers for KMS drivers. > > +config DRM_SIMPLE_KMS_HELPER > + tristate > + depends on DRM > + select DRM_KMS_HELPER > + help > + Helpers for very simple KMS drivers. Personally not sold on piles of Kconfig knobs for tiny amounts of code like this one. I'd just drop it. For a more elaborate argument see http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html Note that almost all the other helpers do not have a Kconfig option, the only real exception is the fbdev helpers. And those have a good reason: They'd drag in all of fbdev, and that is actually a pile of code. > + > config DRM_KMS_FB_HELPER > bool > depends on DRM_KMS_HELPER > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 43c2abf..7e99923 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o > +obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o > > CFLAGS_drm_trace_points.o := -I$(src) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > new file mode 100644 > index 0000000..64bf0f2 > --- /dev/null > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -0,0 +1,200 @@ > +/* > + * 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 <drm/drmP.h> > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_plane_helper.h> > +#include <drm/drm_simple_kms_helper.h> > +#include <linux/slab.h> > + > +/** > + * DOC: overview > + * > + * This helper library provides helpers for simple drivers. "drivers for simple display hardware" is imo clearer. > + * > + * drm_simple_display_pipe_init() initializes a simple display pipeline > + * consisting of a plane-crtc-encoder pipe coupled with a connector. Hm, I think a bit more text here would be useful. "drm_simple_display_pipe_init() initializes a simple display pipeline which has only one full-screen scanout buffer feeding one output. The pipeline is represented by struct &drm_simple_display_pipe and binds together &drm_plane, &drm_crtc and &drm_encoder structures into one fixed entity. Some flexibility for code reuse is provided through a separately allocated &drm_connector object and supporting optional &drm_bridge encoder drivers. > + */ > + > +static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { > + .destroy = drm_encoder_cleanup, > +}; > + > +static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc) > +{ > + struct drm_simple_display_pipe *pipe; > + > + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); > + if (!pipe->funcs || !pipe->funcs->enable) > + return; > + > + pipe->funcs->enable(pipe, crtc->state); > +} > + > +static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc) > +{ > + struct drm_simple_display_pipe *pipe; > + > + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); > + if (!pipe->funcs || !pipe->funcs->disable) > + return; > + > + pipe->funcs->disable(pipe); > +} > + > +static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = { > + .disable = drm_simple_kms_crtc_disable, > + .enable = drm_simple_kms_crtc_enable, > +}; > + > +static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = { > + .reset = drm_atomic_helper_crtc_reset, > + .destroy = drm_crtc_cleanup, > + .set_config = drm_atomic_helper_set_config, > + .page_flip = drm_atomic_helper_page_flip, > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > +}; > + > +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane, > + struct drm_plane_state *plane_state) > +{ > + struct drm_rect src = { > + .x1 = plane_state->src_x, > + .y1 = plane_state->src_y, > + .x2 = plane_state->src_x + plane_state->src_w, > + .y2 = plane_state->src_y + plane_state->src_h, > + }; > + struct drm_rect dest = { > + .x1 = plane_state->crtc_x, > + .y1 = plane_state->crtc_y, > + .x2 = plane_state->crtc_x + plane_state->crtc_w, > + .y2 = plane_state->crtc_y + plane_state->crtc_h, > + }; > + struct drm_rect clip = { 0 }; > + struct drm_simple_display_pipe *pipe; > + struct drm_crtc_state *crtc_state; > + bool visible; > + int ret; > + > + pipe = container_of(plane, struct drm_simple_display_pipe, plane); > + crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state, > + &pipe->crtc); > + if (crtc_state->enable != !!plane_state->crtc) > + return -EINVAL; /* plane must match crtc enable state */ > + > + if (!crtc_state->enable) > + return 0; /* nothing to check when disabling or disabled */ > + > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > + ret = drm_plane_helper_check_update(plane, &pipe->crtc, > + plane_state->fb, > + &src, &dest, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + false, true, &visible); > + if (ret) > + return ret; > + > + if (!visible) > + return -EINVAL; I think the logic above looks correct now, but might be worth checking with your driver that it doesn't let something silly through. I.e. a small test app that tries to reposition the primary plane, or tries to disable it while the crtc is on. We should have that somewhere in libdrm I think. > + > + if (!pipe->funcs || !pipe->funcs->check) > + return 0; > + > + return pipe->funcs->check(pipe, plane_state, crtc_state); > +} > + > +static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane, > + struct drm_plane_state *pstate) > +{ > + struct drm_simple_display_pipe *pipe; > + > + pipe = container_of(plane, struct drm_simple_display_pipe, plane); > + if (!pipe->funcs || !pipe->funcs->update) > + return; > + > + pipe->funcs->update(pipe, pstate); > +} > + > +static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = { > + .atomic_check = drm_simple_kms_plane_atomic_check, > + .atomic_update = drm_simple_kms_plane_atomic_update, > +}; > + > +static const struct drm_plane_funcs drm_simple_kms_plane_funcs = { > + .update_plane = drm_atomic_helper_update_plane, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = drm_plane_cleanup, > + .reset = drm_atomic_helper_plane_reset, > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > +}; > + > +/** > + * drm_simple_display_pipe_init - Initialize a simple display pipeline > + * @dev: DRM device > + * @pipe: simple display pipe object to initialize > + * @funcs: callbacks for the display pipe (optional) > + * @formats: array of supported formats (%DRM_FORMAT_*) > + * @format_count: number of elements in @formats > + * @connector: connector to attach and register > + * > + * Sets up a display pipeline which consist of a really simple > + * plane-crtc-encoder pipe coupled with the provided connector. How are drivers supposed to release this stuff again? Maybe add: "Teardown of a simple display pipe is all handled automatically by the drm core through calling drm_mode_config_cleanup()." > + * > + * Returns: > + * Zero on success, negative error code on failure. > + */ > +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) > +{ > + struct drm_encoder *encoder = &pipe->encoder; > + struct drm_plane *plane = &pipe->plane; > + struct drm_crtc *crtc = &pipe->crtc; > + int ret; > + > + pipe->funcs = funcs; > + > + drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs); > + ret = drm_universal_plane_init(dev, plane, 0, > + &drm_simple_kms_plane_funcs, > + formats, format_count, > + DRM_PLANE_TYPE_PRIMARY, NULL); > + if (ret) > + return ret; > + > + drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs); > + ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL, > + &drm_simple_kms_crtc_funcs, NULL); > + if (ret) > + return ret; > + > + encoder->possible_crtcs = 1 << drm_crtc_index(crtc); > + ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs, > + DRM_MODE_ENCODER_NONE, NULL); > + if (ret) > + return ret; > + > + ret = drm_mode_connector_attach_encoder(connector, encoder); > + if (ret) > + return ret; > + > + return drm_connector_register(connector); > +} > +EXPORT_SYMBOL(drm_simple_display_pipe_init); > + > +MODULE_LICENSE("GPL"); > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h > new file mode 100644 > index 0000000..ef578f4 > --- /dev/null > +++ b/include/drm/drm_simple_kms_helper.h > @@ -0,0 +1,92 @@ > +/* > + * 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 __LINUX_DRM_SIMPLE_KMS_HELPER_H > +#define __LINUX_DRM_SIMPLE_KMS_HELPER_H > + > +struct drm_simple_display_pipe; > + > +/** > + * struct drm_simple_display_pipe_funcs - helper operations for a simple > + * display pipeline > + */ > +struct drm_simple_display_pipe_funcs { > + /** > + * @enable: > + * > + * This function should be used to enable the pipeline. > + * It is called when the underlying crtc is enabled. > + * This hook is optional. > + */ > + void (*enable)(struct drm_simple_display_pipe *pipe, > + struct drm_crtc_state *crtc_state); > + /** > + * @disable: > + * > + * This function should be used to disable the pipeline. > + * It is called when the underlying crtc is disabled. > + * This hook is optional. > + */ > + void (*disable)(struct drm_simple_display_pipe *pipe); > + > + /** > + * @check: > + * > + * This function is called in the check phase of an atomic update, > + * specifically when the underlying plane is checked. I think we need to clarify a bit more what the helpers check already: "The simple display pipeline helpers already check that the plane is not scaled, fills the entire visible area and is always enabled when the crtc is also enabled." > + * This hook is optional. > + * > + * RETURNS: > + * > + * 0 on success, -EINVAL if the state or the transition can't be > + * supported, -ENOMEM on memory allocation failure and -EDEADLK if an > + * attempt to obtain another state object ran into a &drm_modeset_lock > + * deadlock. > + */ > + int (*check)(struct drm_simple_display_pipe *pipe, > + struct drm_plane_state *plane_state, > + struct drm_crtc_state *crtc_state); > + /** > + * @update: > + * > + * This function is called when the underlying plane state is updated. > + * This hook is optional. > + */ > + void (*update)(struct drm_simple_display_pipe *pipe, > + struct drm_plane_state *plane_state); > +}; > + > +/** > + * struct drm_simple_display_pipe - simple display pipeline > + * @crtc: CRTC control structure > + * @plane: Plane control structure > + * @encoder: Encoder control structure > + * @connector: Connector control structure > + * @funcs: Pipeline control functions (optional) > + * > + * Simple display pipeline with plane, crtc and encoder collapsed into one > + * entity. Maybe add: "It should be initialized by calling drm_simple_display_pipe_init()". Imo never hurts to have a few too many cross references ;-) > + */ > +struct drm_simple_display_pipe { > + struct drm_crtc crtc; > + struct drm_plane plane; > + struct drm_encoder encoder; > + struct drm_connector *connector; > + > + struct drm_simple_display_pipe_funcs *funcs; > +}; > + > +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); > + > +#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */ > -- > 2.8.2 >
On wo, 2016-05-11 at 19:09 +0200, Daniel Vetter wrote: > On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > +config DRM_SIMPLE_KMS_HELPER > > + tristate > > + depends on DRM > > + select DRM_KMS_HELPER > > + help > > + Helpers for very simple KMS drivers. > > Personally not sold on piles of Kconfig knobs for tiny amounts of code > like this one. I'd just drop it. For a more elaborate argument see > http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html > > Note that almost all the other helpers do not have a Kconfig option, the > only real exception is the fbdev helpers. And those have a good reason: > They'd drag in all of fbdev, and that is actually a pile of code. Moreover, this adds a kconfig entry without a prompt. The entry also doesn't have a "default". And I didn't spot a patch adding a select for this symbol. So, based on just this series, I think that DRM_SIMPLE_KMS_HELPER can't actually be set. I didn't test this, so perhaps I missed something. Did I, Noralf? Paul Bolle
Den 11.05.2016 19:09, skrev Daniel Vetter: > On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: >> Provides helper functions for drivers that have a simple display >> pipeline. Plane, crtc and encoder are collapsed into one entity. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Looks really nice, just a few suggestions for the kerneldoc. Would be > awesome if we could get an ack on this from Jyri for tilcdc, but even > without that I think I'll just pull in the next iteration. Still please cc > him. > > Thanks, Daniel Thanks for helping me with the docs. [...] >> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane, >> + struct drm_plane_state *plane_state) >> +{ >> + struct drm_rect src = { >> + .x1 = plane_state->src_x, >> + .y1 = plane_state->src_y, >> + .x2 = plane_state->src_x + plane_state->src_w, >> + .y2 = plane_state->src_y + plane_state->src_h, >> + }; >> + struct drm_rect dest = { >> + .x1 = plane_state->crtc_x, >> + .y1 = plane_state->crtc_y, >> + .x2 = plane_state->crtc_x + plane_state->crtc_w, >> + .y2 = plane_state->crtc_y + plane_state->crtc_h, >> + }; >> + struct drm_rect clip = { 0 }; >> + struct drm_simple_display_pipe *pipe; >> + struct drm_crtc_state *crtc_state; >> + bool visible; >> + int ret; >> + >> + pipe = container_of(plane, struct drm_simple_display_pipe, plane); >> + crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state, >> + &pipe->crtc); >> + if (crtc_state->enable != !!plane_state->crtc) >> + return -EINVAL; /* plane must match crtc enable state */ >> + >> + if (!crtc_state->enable) >> + return 0; /* nothing to check when disabling or disabled */ >> + >> + clip.x2 = crtc_state->adjusted_mode.hdisplay; >> + clip.y2 = crtc_state->adjusted_mode.vdisplay; >> + ret = drm_plane_helper_check_update(plane, &pipe->crtc, >> + plane_state->fb, >> + &src, &dest, &clip, >> + DRM_PLANE_HELPER_NO_SCALING, >> + DRM_PLANE_HELPER_NO_SCALING, >> + false, true, &visible); >> + if (ret) >> + return ret; >> + >> + if (!visible) >> + return -EINVAL; > I think the logic above looks correct now, but might be worth checking > with your driver that it doesn't let something silly through. I.e. a small > test app that tries to reposition the primary plane, or tries to disable > it while the crtc is on. We should have that somewhere in libdrm I think. I hacked libdrm tests/kms/kms-universal-planes to position the plane at (1,1) which failed (I added some debug output to the driver): [ 885.906697] [drm:drm_atomic_set_fb_for_plane] Set [FB:25] for plane state b84aec40 [ 885.906707] [drm:drm_atomic_check_only] checking b84aeec0 [ 885.906738] [drm:drm_plane_helper_check_update] Plane must cover entire CRTC [ 885.906748] [drm:drm_rect_debug_print] dst: 319x239+1+1 [ 885.906757] [drm:drm_rect_debug_print] clip: 320x240+0+0 [ 885.906765] drm_simple_kms_plane_atomic_check: ret=-22, visible=1 [ 885.906775] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0] atomic driver check failed Then I changed the test to pass 0 for fb id which also failed: [ 3144.599790] [drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane state b87805c0 [ 3144.599799] [drm:drm_atomic_check_only] checking b8780d80 [ 3144.599816] [drm:drm_atomic_helper_check_planes] [PLANE:19:plane-0] atomic driver check failed Noralf.
Den 11.05.2016 21:10, skrev Paul Bolle: > On wo, 2016-05-11 at 19:09 +0200, Daniel Vetter wrote: >> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: >>> --- a/drivers/gpu/drm/Kconfig >>> +++ b/drivers/gpu/drm/Kconfig >>> +config DRM_SIMPLE_KMS_HELPER >>> + tristate >>> + depends on DRM >>> + select DRM_KMS_HELPER >>> + help >>> + Helpers for very simple KMS drivers. >> Personally not sold on piles of Kconfig knobs for tiny amounts of code >> like this one. I'd just drop it. For a more elaborate argument see >> http://sietch-tagr.blogspot.ch/2016/04/display-panels-are-not-special.html >> >> Note that almost all the other helpers do not have a Kconfig option, the >> only real exception is the fbdev helpers. And those have a good reason: >> They'd drag in all of fbdev, and that is actually a pile of code. > Moreover, this adds a kconfig entry without a prompt. The entry also > doesn't have a "default". And I didn't spot a patch adding a select for > this symbol. So, based on just this series, I think that > DRM_SIMPLE_KMS_HELPER can't actually be set. > > I didn't test this, so perhaps I missed something. Did I, Noralf? It would have been selected in the tinydrm follow-up patchset, but I'll do as Daniel suggests and add it to drm_kms_helper-y. Noralf.
On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote: > On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: > > +/** > > + * drm_simple_display_pipe_init - Initialize a simple display pipeline > > + * @dev: DRM device > > + * @pipe: simple display pipe object to initialize > > + * @funcs: callbacks for the display pipe (optional) > > + * @formats: array of supported formats (%DRM_FORMAT_*) > > + * @format_count: number of elements in @formats > > + * @connector: connector to attach and register > > + * > > + * Sets up a display pipeline which consist of a really simple > > + * plane-crtc-encoder pipe coupled with the provided connector. > > How are drivers supposed to release this stuff again? Maybe add: > > "Teardown of a simple display pipe is all handled automatically by the drm > core through calling drm_mode_config_cleanup()." Thought a bit more about this, maybe we should also add "Drivers afterwards need to release the memory for the structure themselves." Btw one other thing I realized is that there's no atomic_commit for this. How do you plane to implement async commit? No need to address this right away, we can discuss it when you've rebased tinydrm and submit that for review. -Daniel
Den 12.05.2016 10:11, skrev Daniel Vetter: > On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote: >> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: >>> +/** >>> + * drm_simple_display_pipe_init - Initialize a simple display pipeline >>> + * @dev: DRM device >>> + * @pipe: simple display pipe object to initialize >>> + * @funcs: callbacks for the display pipe (optional) >>> + * @formats: array of supported formats (%DRM_FORMAT_*) >>> + * @format_count: number of elements in @formats >>> + * @connector: connector to attach and register >>> + * >>> + * Sets up a display pipeline which consist of a really simple >>> + * plane-crtc-encoder pipe coupled with the provided connector. >> How are drivers supposed to release this stuff again? Maybe add: >> >> "Teardown of a simple display pipe is all handled automatically by the drm >> core through calling drm_mode_config_cleanup()." > Thought a bit more about this, maybe we should also add "Drivers > afterwards need to release the memory for the structure themselves." > > Btw one other thing I realized is that there's no atomic_commit for this. > How do you plane to implement async commit? No need to address this right > away, we can discuss it when you've rebased tinydrm and submit that for > review. > -Daniel I don't follow you here. Isn't this atomic_commit: drm_atomic_helper_commit => drm_atomic_helper_commit_planes => drm_simple_kms_plane_atomic_update => pipe->funcs->update Noralf.
On Thu, May 12, 2016 at 12:18 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > Den 12.05.2016 10:11, skrev Daniel Vetter: >> >> On Wed, May 11, 2016 at 07:09:10PM +0200, Daniel Vetter wrote: >>> >>> On Wed, May 11, 2016 at 06:09:22PM +0200, Noralf Trønnes wrote: >>>> >>>> +/** >>>> + * drm_simple_display_pipe_init - Initialize a simple display pipeline >>>> + * @dev: DRM device >>>> + * @pipe: simple display pipe object to initialize >>>> + * @funcs: callbacks for the display pipe (optional) >>>> + * @formats: array of supported formats (%DRM_FORMAT_*) >>>> + * @format_count: number of elements in @formats >>>> + * @connector: connector to attach and register >>>> + * >>>> + * Sets up a display pipeline which consist of a really simple >>>> + * plane-crtc-encoder pipe coupled with the provided connector. >>> >>> How are drivers supposed to release this stuff again? Maybe add: >>> >>> "Teardown of a simple display pipe is all handled automatically by the >>> drm >>> core through calling drm_mode_config_cleanup()." >> >> Thought a bit more about this, maybe we should also add "Drivers >> afterwards need to release the memory for the structure themselves." >> >> Btw one other thing I realized is that there's no atomic_commit for this. >> How do you plane to implement async commit? No need to address this right >> away, we can discuss it when you've rebased tinydrm and submit that for >> review. >> -Daniel > > > I don't follow you here. Isn't this atomic_commit: > > drm_atomic_helper_commit => drm_atomic_helper_commit_planes => > drm_simple_kms_plane_atomic_update => pipe->funcs->update drm_atomic_helper_commit does not implement nonblocking commits, because that's a bit tricky when there's more than 1 crtc. But if you only have 1 crtc it's easy. And without nonblocking commit the legacy pageflip stuff will also not work. -Daniel
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 4a0c599..cf3f5a8 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -1693,6 +1693,12 @@ void intel_crt_init(struct drm_device *dev) !Edrivers/gpu/drm/drm_panel.c !Pdrivers/gpu/drm/drm_panel.c drm panel </sect2> + <sect2> + <title>Simple KMS Helper Reference</title> +!Iinclude/drm/drm_simple_kms_helper.h +!Edrivers/gpu/drm/drm_simple_kms_helper.c +!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview + </sect2> </sect1> <!-- Internals: kms properties --> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 16e4c21..ff9f480 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -39,6 +39,13 @@ config DRM_KMS_HELPER help CRTC helpers for KMS drivers. +config DRM_SIMPLE_KMS_HELPER + tristate + depends on DRM + select DRM_KMS_HELPER + help + Helpers for very simple KMS drivers. + config DRM_KMS_FB_HELPER bool depends on DRM_KMS_HELPER diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 43c2abf..7e99923 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -31,6 +31,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o +obj-$(CONFIG_DRM_SIMPLE_KMS_HELPER) += drm_simple_kms_helper.o CFLAGS_drm_trace_points.o := -I$(src) diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c new file mode 100644 index 0000000..64bf0f2 --- /dev/null +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -0,0 +1,200 @@ +/* + * 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 <drm/drmP.h> +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_plane_helper.h> +#include <drm/drm_simple_kms_helper.h> +#include <linux/slab.h> + +/** + * DOC: overview + * + * This helper library provides helpers for simple drivers. + * + * drm_simple_display_pipe_init() initializes a simple display pipeline + * consisting of a plane-crtc-encoder pipe coupled with a connector. + */ + +static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + +static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); + if (!pipe->funcs || !pipe->funcs->enable) + return; + + pipe->funcs->enable(pipe, crtc->state); +} + +static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc); + if (!pipe->funcs || !pipe->funcs->disable) + return; + + pipe->funcs->disable(pipe); +} + +static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = { + .disable = drm_simple_kms_crtc_disable, + .enable = drm_simple_kms_crtc_enable, +}; + +static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs = { + .reset = drm_atomic_helper_crtc_reset, + .destroy = drm_crtc_cleanup, + .set_config = drm_atomic_helper_set_config, + .page_flip = drm_atomic_helper_page_flip, + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, +}; + +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane, + struct drm_plane_state *plane_state) +{ + struct drm_rect src = { + .x1 = plane_state->src_x, + .y1 = plane_state->src_y, + .x2 = plane_state->src_x + plane_state->src_w, + .y2 = plane_state->src_y + plane_state->src_h, + }; + struct drm_rect dest = { + .x1 = plane_state->crtc_x, + .y1 = plane_state->crtc_y, + .x2 = plane_state->crtc_x + plane_state->crtc_w, + .y2 = plane_state->crtc_y + plane_state->crtc_h, + }; + struct drm_rect clip = { 0 }; + struct drm_simple_display_pipe *pipe; + struct drm_crtc_state *crtc_state; + bool visible; + int ret; + + pipe = container_of(plane, struct drm_simple_display_pipe, plane); + crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state, + &pipe->crtc); + if (crtc_state->enable != !!plane_state->crtc) + return -EINVAL; /* plane must match crtc enable state */ + + if (!crtc_state->enable) + return 0; /* nothing to check when disabling or disabled */ + + clip.x2 = crtc_state->adjusted_mode.hdisplay; + clip.y2 = crtc_state->adjusted_mode.vdisplay; + ret = drm_plane_helper_check_update(plane, &pipe->crtc, + plane_state->fb, + &src, &dest, &clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + false, true, &visible); + if (ret) + return ret; + + if (!visible) + return -EINVAL; + + if (!pipe->funcs || !pipe->funcs->check) + return 0; + + return pipe->funcs->check(pipe, plane_state, crtc_state); +} + +static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane, + struct drm_plane_state *pstate) +{ + struct drm_simple_display_pipe *pipe; + + pipe = container_of(plane, struct drm_simple_display_pipe, plane); + if (!pipe->funcs || !pipe->funcs->update) + return; + + pipe->funcs->update(pipe, pstate); +} + +static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = { + .atomic_check = drm_simple_kms_plane_atomic_check, + .atomic_update = drm_simple_kms_plane_atomic_update, +}; + +static const struct drm_plane_funcs drm_simple_kms_plane_funcs = { + .update_plane = drm_atomic_helper_update_plane, + .disable_plane = drm_atomic_helper_disable_plane, + .destroy = drm_plane_cleanup, + .reset = drm_atomic_helper_plane_reset, + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, +}; + +/** + * drm_simple_display_pipe_init - Initialize a simple display pipeline + * @dev: DRM device + * @pipe: simple display pipe object to initialize + * @funcs: callbacks for the display pipe (optional) + * @formats: array of supported formats (%DRM_FORMAT_*) + * @format_count: number of elements in @formats + * @connector: connector to attach and register + * + * Sets up a display pipeline which consist of a really simple + * plane-crtc-encoder pipe coupled with the provided connector. + * + * Returns: + * Zero on success, negative error code on failure. + */ +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) +{ + struct drm_encoder *encoder = &pipe->encoder; + struct drm_plane *plane = &pipe->plane; + struct drm_crtc *crtc = &pipe->crtc; + int ret; + + pipe->funcs = funcs; + + drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs); + ret = drm_universal_plane_init(dev, plane, 0, + &drm_simple_kms_plane_funcs, + formats, format_count, + DRM_PLANE_TYPE_PRIMARY, NULL); + if (ret) + return ret; + + drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs); + ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL, + &drm_simple_kms_crtc_funcs, NULL); + if (ret) + return ret; + + encoder->possible_crtcs = 1 << drm_crtc_index(crtc); + ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs, + DRM_MODE_ENCODER_NONE, NULL); + if (ret) + return ret; + + ret = drm_mode_connector_attach_encoder(connector, encoder); + if (ret) + return ret; + + return drm_connector_register(connector); +} +EXPORT_SYMBOL(drm_simple_display_pipe_init); + +MODULE_LICENSE("GPL"); diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h new file mode 100644 index 0000000..ef578f4 --- /dev/null +++ b/include/drm/drm_simple_kms_helper.h @@ -0,0 +1,92 @@ +/* + * 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 __LINUX_DRM_SIMPLE_KMS_HELPER_H +#define __LINUX_DRM_SIMPLE_KMS_HELPER_H + +struct drm_simple_display_pipe; + +/** + * struct drm_simple_display_pipe_funcs - helper operations for a simple + * display pipeline + */ +struct drm_simple_display_pipe_funcs { + /** + * @enable: + * + * This function should be used to enable the pipeline. + * It is called when the underlying crtc is enabled. + * This hook is optional. + */ + void (*enable)(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state); + /** + * @disable: + * + * This function should be used to disable the pipeline. + * It is called when the underlying crtc is disabled. + * This hook is optional. + */ + void (*disable)(struct drm_simple_display_pipe *pipe); + + /** + * @check: + * + * This function is called in the check phase of an atomic update, + * specifically when the underlying plane is checked. + * This hook is optional. + * + * RETURNS: + * + * 0 on success, -EINVAL if the state or the transition can't be + * supported, -ENOMEM on memory allocation failure and -EDEADLK if an + * attempt to obtain another state object ran into a &drm_modeset_lock + * deadlock. + */ + int (*check)(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *plane_state, + struct drm_crtc_state *crtc_state); + /** + * @update: + * + * This function is called when the underlying plane state is updated. + * This hook is optional. + */ + void (*update)(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *plane_state); +}; + +/** + * struct drm_simple_display_pipe - simple display pipeline + * @crtc: CRTC control structure + * @plane: Plane control structure + * @encoder: Encoder control structure + * @connector: Connector control structure + * @funcs: Pipeline control functions (optional) + * + * Simple display pipeline with plane, crtc and encoder collapsed into one + * entity. + */ +struct drm_simple_display_pipe { + struct drm_crtc crtc; + struct drm_plane plane; + struct drm_encoder encoder; + struct drm_connector *connector; + + struct drm_simple_display_pipe_funcs *funcs; +}; + +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); + +#endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
Provides helper functions for drivers that have a simple display pipeline. Plane, crtc and encoder are collapsed into one entity. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- Changes since v1: - Add DOC header and add to gpu.tmpl - Fix docs: @funcs is optional, "negative error code", "This hook is optional." - Add checks to drm_simple_kms_plane_atomic_check() Documentation/DocBook/gpu.tmpl | 6 + drivers/gpu/drm/Kconfig | 7 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_simple_kms_helper.c | 200 ++++++++++++++++++++++++++++++++ include/drm/drm_simple_kms_helper.h | 92 +++++++++++++++ 5 files changed, 306 insertions(+) create mode 100644 drivers/gpu/drm/drm_simple_kms_helper.c create mode 100644 include/drm/drm_simple_kms_helper.h -- 2.8.2