From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754699AbcEQNEz (ORCPT ); Tue, 17 May 2016 09:04:55 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:36483 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754594AbcEQNEy (ORCPT ); Tue, 17 May 2016 09:04:54 -0400 Date: Tue, 17 May 2016 15:04:52 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Daniel Vetter , jsarha@ti.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 3/3] drm: Add helper for simple display pipeline Message-ID: <20160517130452.GL27098@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , jsarha@ti.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <1463077523-23959-1-git-send-email-noralf@tronnes.org> <1463077523-23959-4-git-send-email-noralf@tronnes.org> <20160512183614.GU4329@intel.com> <20160517070501.GJ27098@phenom.ffwll.local> <20160517074651.GM4329@intel.com> <20160517075945.GQ27098@phenom.ffwll.local> <20160517121208.GO4329@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>>>>>>--- > >>>>>>> > >>>>>>>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 > >>>>>>> > >>>>>>>+ > >>>>>>>+ Simple KMS Helper Reference > >>>>>>>+!Iinclude/drm/drm_simple_kms_helper.h > >>>>>>>+!Edrivers/gpu/drm/drm_simple_kms_helper.c > >>>>>>>+!Pdrivers/gpu/drm/drm_simple_kms_helper.c overview > >>>>>>>+ > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>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 > >>>>>>>+#include > >>>>>>>+#include > >>>>>>>+#include > >>>>>>>+#include > >>>>>>>+#include > >>>>>>>+#include > >>>>>>>+ > >>>>>>>+/** > >>>>>>>+ * 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? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch