Re: [PATCH v4 3/3] drm: Add helper for simple display pipeline

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 17, 2016 at 03:19:20PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 3:14 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Tue, May 17, 2016 at 03:04:52PM +0200, Daniel Vetter wrote:
> >> On Tue, May 17, 2016 at 02:22:26PM +0200, Noralf Trønnes wrote:
> >> >
> >> >
> >> > Den 17.05.2016 14:12, skrev Ville Syrjälä:
> >> > >On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Trønnes wrote:
> >> > >>Den 17.05.2016 09:59, skrev Daniel Vetter:
> >> > >>>On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrjälä wrote:
> >> > >>>>On Tue, May 17, 2016 at 09:05:01AM +0200, Daniel Vetter wrote:
> >> > >>>>>On Thu, May 12, 2016 at 09:36:14PM +0300, Ville Syrjälä wrote:
> >> > >>>>>>On Thu, May 12, 2016 at 08:25:23PM +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.
> >> > >>>>>>>
> >> > >>>>>>>Cc: jsarha@ti.com
> >> > >>>>>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> > >>>>>>>---
> >> > >>>>>>>
> >> > >>>>>>>Changes since v3:
> >> > >>>>>>>- (struct drm_simple_display_pipe *)->funcs should be const
> >> > >>>>>>>
> >> > >>>>>>>Changes since v2:
> >> > >>>>>>>- Drop Kconfig knob DRM_KMS_HELPER
> >> > >>>>>>>- Expand documentation
> >> > >>>>>>>
> >> > >>>>>>>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/Makefile                |   2 +-
> >> > >>>>>>>   drivers/gpu/drm/drm_simple_kms_helper.c | 208 ++++++++++++++++++++++++++++++++
> >> > >>>>>>>   include/drm/drm_simple_kms_helper.h     |  94 +++++++++++++++
> >> > >>>>>>>   4 files changed, 309 insertions(+), 1 deletion(-)
> >> > >>>>>>>   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/Makefile b/drivers/gpu/drm/Makefile
> >> > >>>>>>>index 2bd3e5a..31b85df5 100644
> >> > >>>>>>>--- a/drivers/gpu/drm/Makefile
> >> > >>>>>>>+++ b/drivers/gpu/drm/Makefile
> >> > >>>>>>>@@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> >> > >>>>>>>
> >> > >>>>>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >> > >>>>>>>             drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> >> > >>>>>>>-            drm_kms_helper_common.o
> >> > >>>>>>>+            drm_kms_helper_common.o drm_simple_kms_helper.o
> >> > >>>>>>>
> >> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >> > >>>>>>>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >> > >>>>>>>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..d45417a
> >> > >>>>>>>--- /dev/null
> >> > >>>>>>>+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> >> > >>>>>>>@@ -0,0 +1,208 @@
> >> > >>>>>>>+/*
> >> > >>>>>>>+ * 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 drivers for simple display
> >> > >>>>>>>+ * hardware.
> >> > >>>>>>>+ *
> >> > >>>>>>>+ * 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;
> >> > >>>>>>>+
> >> > >>>>>>>+    if (!pipe->funcs || !pipe->funcs->check)
> >> > >>>>>>>+            return 0;
> >> > >>>>>>>+
> >> > >>>>>>>+    return pipe->funcs->check(pipe, plane_state, crtc_state);
> >> > >>>>>>>+}
> >> > >>>>>>What's anyone supposed to do with this when the clipped coordinates
> >> > >>>>>>aren't even passed/stored anywhere?
> >> > >>>>>It disallows positioning and scaling, so shouldn't ever need to have the
> >> > >>>>>clipped area?
> >> > >>>>You can still configure a larger area that gets clipped to the
> >> > >>>>fullscreen dimensions.
> >> > >>>Oh right. Noralf, sounds like we need to feed back the clipped rectangle.
> >> > >>>Probably best if we add clipped plane coordinates to drm_plane_state.
> >> > >>Both src and crtc or only src?
> >> > >>
> >> > >>       if (!visible)
> >> > >>           return -EINVAL;
> >> > >>+
> >> > >>+    plane_state->src_x = src.x1;
> >> > >>+    plane_state->src_y = src.y1;
> >> > >>+    plane_state->src_w = drm_rect_width(&src);
> >> > >>+    plane_state->src_h = drm_rect_height(&src);
> >> > >>+
> >> > >>+    plane_state->crtc_x = dest.x1;
> >> > >>+    plane_state->crtc_y = dest.y1;
> >> > >>+    plane_state->crtc_w = drm_rect_width(&dest);
> >> > >>+    plane_state->crtc_h = drm_rect_height(&dest);
> >> > >You aren't allowed clobber the user provided coordinates like this.
> >> > >What you need to do is store the clipped coordinates in the plane
> >> > >state in addition to the user coordinates.
> >> >
> >> > How do I do that?
> >>
> >> Add new set of plane_state->clipped_src/dst_x/y/h/w I think, and suggest
> >> to drivers to use that if they need clipped coordinates. I think at least,
> >> all these clip rects are a bit too confusing to me. Ville?
> >
> > Basically everyone should use the clipped coords. There must be
> > something very special going on if anyone wants to use the raw user
> > coords.
> 
> Hm, maybe it would be better then to add raw_ versions, and clip to
> src/dst in the atomic helpers for everyone? Or would that break some
> drivers? This seems to get a bit out of hand ...

I suppose we might be able to do the basic stuff in some common code.
The driver may need to adjust the coordinates a bit further to deal
with hardware specific limits and whatnot.

One issue is that I originally designed the rect stuff for nice
hardware that can eg. deal with sub-pixel coordinates. But there's
lots of not so nice hardware around and the current code may not be
the best fit there.

We should also come to some kind of decisions on what kind of
tolerances we allow in deviating from the user coordinates and
whanot.

-- 
Ville Syrjälä
Intel OTC