[PATCH 3/4] drm: Add helper for simple display pipeline
On Tue, May 10, 2016 at 8:59 AM, Daniel Vetter wrote: >> if (ret) >> return ret; >> >> /* How to handle !visible, is it even possible? */ > > if (!visible) > return -EINVAL; > > You can't, so need to reject it. Ok, on further thought I think we need a bit more here. We not just need to make sure the plane is visible and not scaled or positioned, but also that the plane is only enabled iff the crtc is. So even before you call anything else you need to have this check: 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 already, calling check helpers and driver callbacks might only result in confusion in such a case. */ Then continue with the remaining check logic that you have already, with my coments in the earlier mail addressed. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 3/4] drm: Add helper for simple display pipeline
On Mon, May 09, 2016 at 08:37:39PM +0200, Noralf Trønnes wrote:
>
> Den 09.05.2016 16:46, skrev Daniel Vetter:
> >On Thu, May 05, 2016 at 03:24:33PM +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
> >>+static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> >>+ struct drm_plane_state *pstate)
> >>+{
> >>+ struct drm_simple_display_pipe *pipe;
> >>+ struct drm_crtc_state *cstate;
> >>+
> >>+ pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> >>+ if (!pipe->funcs || !pipe->funcs->check)
> >>+ return 0;
> >>+
> >>+ cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> >>+ &pipe->crtc);
> >>+
> >>+ return pipe->funcs->check(pipe, pstate, cstate);
> >>+}
> >Ok one thing I've missed here is that for most drivers this is way too
> >simple a check function, which means we'll end up with tons of duplicated
> >code. Things which the drm core allows, but simple pipelines all don't
> >really cope with:
> >- plane scaling
> >- disabling the plane without the crtc (i.e. scan out black)
> >- plane not sized to fill the entire hactive/vactive
> >
> >There's a helper to do most of these checks for you -
> >drm_plane_helper_check_update. I think it'd be good to place a call for
> >that in here, before we call down into the driver's ->check callback. But
> >ofc before we return 0; we want these checks always done. And catch all
> >these things so that drivers never fall over this pitfall.
>
> Does this resemble what you're after? I'm just guessing here.
>
> static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *pstate)
> {
> struct drm_rect src = {
> .x1 = pstate->src_x,
> .y1 = pstate->src_y,
> .x2 = pstate->src_x + pstate->src_w,
> .y2 = pstate->src_y + pstate->src_h,
> };
> struct drm_rect dest = {
> .x1 = pstate->crtc_x,
> .y1 = pstate->crtc_y,
> .x2 = pstate->crtc_x + pstate->crtc_w,
> .y2 = pstate->crtc_y + pstate->crtc_h,
> };
> struct drm_rect clip = { 0 };
Clip rect needs to be set to crtc_state->adjusted_mode.h/vdisplay, see
rockchip or armada. Otherwise you'll clip to nothing and always fail.
> struct drm_simple_display_pipe *pipe;
> struct drm_crtc_state *cstate;
> bool visible;
> int ret;
>
> pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> clip.x2 = pipe->crtc.mode.hdisplay;
> clip.y2 = pipe->crtc.mode.vdisplay;
> ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
> &src, &dest, &clip,
> DRM_PLANE_HELPER_NO_SCALING,
> DRM_PLANE_HELPER_NO_SCALING,
> false, false, &visible);
can_update_disabled = true should work, Only caveat is that you might get
a call to update the primary plane when the display is turned off.
Probably best though if we handle that in the simple pipe driver too. Just
call drm_atomic_helper_commit_planes with active_only = true.
> if (ret)
> return ret;
>
> /* How to handle !visible, is it even possible? */
if (!visible)
return -EINVAL;
You can't, so need to reject it.
>
> if (!pipe->funcs || !pipe->funcs->check)
> return 0;
>
> cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> &pipe->crtc);
>
> return pipe->funcs->check(pipe, pstate, cstate);
> }
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 3/4] drm: Add helper for simple display pipeline
Den 09.05.2016 16:46, skrev Daniel Vetter:
> On Thu, May 05, 2016 at 03:24:33PM +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
>> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>> +struct drm_plane_state *pstate)
>> +{
>> +struct drm_simple_display_pipe *pipe;
>> +struct drm_crtc_state *cstate;
>> +
>> +pipe = container_of(plane, struct drm_simple_display_pipe, plane);
>> +if (!pipe->funcs || !pipe->funcs->check)
>> +return 0;
>> +
>> +cstate = drm_atomic_get_existing_crtc_state(pstate->state,
>> +&pipe->crtc);
>> +
>> +return pipe->funcs->check(pipe, pstate, cstate);
>> +}
> Ok one thing I've missed here is that for most drivers this is way too
> simple a check function, which means we'll end up with tons of duplicated
> code. Things which the drm core allows, but simple pipelines all don't
> really cope with:
> - plane scaling
> - disabling the plane without the crtc (i.e. scan out black)
> - plane not sized to fill the entire hactive/vactive
>
> There's a helper to do most of these checks for you -
> drm_plane_helper_check_update. I think it'd be good to place a call for
> that in here, before we call down into the driver's ->check callback. But
> ofc before we return 0; we want these checks always done. And catch all
> these things so that drivers never fall over this pitfall.
Does this resemble what you're after? I'm just guessing here.
static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *pstate)
{
struct drm_rect src = {
.x1 = pstate->src_x,
.y1 = pstate->src_y,
.x2 = pstate->src_x + pstate->src_w,
.y2 = pstate->src_y + pstate->src_h,
};
struct drm_rect dest = {
.x1 = pstate->crtc_x,
.y1 = pstate->crtc_y,
.x2 = pstate->crtc_x + pstate->crtc_w,
.y2 = pstate->crtc_y + pstate->crtc_h,
};
struct drm_rect clip = { 0 };
struct drm_simple_display_pipe *pipe;
struct drm_crtc_state *cstate;
bool visible;
int ret;
pipe = container_of(plane, struct drm_simple_display_pipe, plane);
clip.x2 = pipe->crtc.mode.hdisplay;
clip.y2 = pipe->crtc.mode.vdisplay;
ret = drm_plane_helper_check_update(plane, &pipe->crtc, plane->fb,
&src, &dest, &clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
false, false, &visible);
if (ret)
return ret;
/* How to handle !visible, is it even possible? */
if (!pipe->funcs || !pipe->funcs->check)
return 0;
cstate = drm_atomic_get_existing_crtc_state(pstate->state,
&pipe->crtc);
return pipe->funcs->check(pipe, pstate, cstate);
}
Noralf.
> Noticed while discussing tilcdc atomic patches, since tilcdc could
> probably use drm_simple_display_pipe too.
> -Daniel
[PATCH 3/4] drm: Add helper for simple display pipeline
On Thu, May 05, 2016 at 03:24:33PM +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
> +static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
> + struct drm_plane_state *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
> + struct drm_crtc_state *cstate;
> +
> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> + if (!pipe->funcs || !pipe->funcs->check)
> + return 0;
> +
> + cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> + &pipe->crtc);
> +
> + return pipe->funcs->check(pipe, pstate, cstate);
> +}
Ok one thing I've missed here is that for most drivers this is way too
simple a check function, which means we'll end up with tons of duplicated
code. Things which the drm core allows, but simple pipelines all don't
really cope with:
- plane scaling
- disabling the plane without the crtc (i.e. scan out black)
- plane not sized to fill the entire hactive/vactive
There's a helper to do most of these checks for you -
drm_plane_helper_check_update. I think it'd be good to place a call for
that in here, before we call down into the driver's ->check callback. But
ofc before we return 0; we want these checks always done. And catch all
these things so that drivers never fall over this pitfall.
Noticed while discussing tilcdc atomic patches, since tilcdc could
probably use drm_simple_display_pipe too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 3/4] drm: Add helper for simple display pipeline
On Thu, May 05, 2016 at 03:24:33PM +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
When resubmitting a patch please always have a per-patch changelog of what
you've done. Otherwise you force reviewers to remember they looked at this
already, dig out the old mail and compare ;-)
> ---
> drivers/gpu/drm/Kconfig | 7 ++
> drivers/gpu/drm/Makefile| 1 +
> drivers/gpu/drm/drm_simple_kms_helper.c | 157
>
> include/drm/drm_simple_kms_helper.h | 88 ++
Please also add a new section (next to all the ones for the other kms
helpers) in Documentation/DocBook/gpu.tmpl and pull your kerneldoc in.
Please also add a short overview kernel doc section to the .c file (Using
the DOC: header) and also pull that into the overall gpu docs.
You can check the results using
$ make htmldocs
> 4 files changed, 253 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/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 000..c8725bb
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -0,0 +1,157 @@
> +/*
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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 *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
> + struct drm_crtc_state *cstate;
> +
> + pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> + if (!pipe->funcs || !pipe->funcs->check)
> + return 0;
> +
> + cstate = drm_atomic_get_existing_crtc_state(pstate->state,
> + &pipe->crtc);
> +
> + return pipe->funcs->check(pipe, pstate, cstate);
> +}
> +
> +static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *pstate)
> +{
> + struct drm_simple_display_pipe *pipe;
>
[PATCH 3/4] drm: Add helper for simple display pipeline
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
---
drivers/gpu/drm/Kconfig | 7 ++
drivers/gpu/drm/Makefile| 1 +
drivers/gpu/drm/drm_simple_kms_helper.c | 157
include/drm/drm_simple_kms_helper.h | 88 ++
4 files changed, 253 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/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 000..c8725bb
--- /dev/null
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -0,0 +1,157 @@
+/*
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+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 *pstate)
+{
+ struct drm_simple_display_pipe *pipe;
+ struct drm_crtc_state *cstate;
+
+ pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+ if (!pipe->funcs || !pipe->funcs->check)
+ return 0;
+
+ cstate = drm_atomic_get_existing_crtc_state(pstate->state,
+ &pipe->crtc);
+
+ return pipe->funcs->check(pipe, pstate, cstate);
+}
+
+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_p
