From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v4 3/3] drm: Add helper for simple display pipeline Date: Tue, 17 May 2016 15:12:08 +0300 Message-ID: <20160517121208.GO4329@intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Daniel Vetter , jsarha@ti.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On Tue, May 17, 2016 at 02:00:45PM +0200, Noralf Tr=F8nnes wrote: >=20 > Den 17.05.2016 09:59, skrev Daniel Vetter: > > On Tue, May 17, 2016 at 10:46:51AM +0300, Ville Syrj=E4l=E4 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=E4l=E4 wrote= : > >>>> On Thu, May 12, 2016 at 08:25:23PM +0200, Noralf Tr=F8nnes wrote= : > >>>>> Provides helper functions for drivers that have a simple displa= y > >>>>> pipeline. Plane, crtc and encoder are collapsed into one entity= =2E > >>>>> > >>>>> Cc: jsarha@ti.com > >>>>> Signed-off-by: Noralf Tr=F8nnes > >>>>> --- > >>>>> > >>>>> 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/Doc= Book/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 *d= ev) > >>>>> !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/Makefil= e > >>>>> index 2bd3e5a..31b85df5 100644 > >>>>> --- a/drivers/gpu/drm/Makefile > >>>>> +++ b/drivers/gpu/drm/Makefile > >>>>> @@ -23,7 +23,7 @@ drm-$(CONFIG_AGP) +=3D drm_agpsupport.o > >>>>> > >>>>> drm_kms_helper-y :=3D drm_crtc_helper.o drm_dp_helper.o drm_p= robe_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) +=3D drm_edid= _load.o > >>>>> drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) +=3D drm_fb_help= er.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=F8nnes > >>>>> + * > >>>>> + * This program is free software; you can redistribute it and/= or modify > >>>>> + * it under the terms of the GNU General Public License as pub= lished by > >>>>> + * the Free Software Foundation; either version 2 of the Licen= se, 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 o= utput. 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 &dr= m_bridge > >>>>> + * encoder drivers. > >>>>> + */ > >>>>> + > >>>>> +static const struct drm_encoder_funcs drm_simple_kms_encoder_f= uncs =3D { > >>>>> + .destroy =3D drm_encoder_cleanup, > >>>>> +}; > >>>>> + > >>>>> +static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc) > >>>>> +{ > >>>>> + struct drm_simple_display_pipe *pipe; > >>>>> + > >>>>> + pipe =3D container_of(crtc, struct drm_simple_display_pipe, c= rtc); > >>>>> + 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 =3D container_of(crtc, struct drm_simple_display_pipe, c= rtc); > >>>>> + if (!pipe->funcs || !pipe->funcs->disable) > >>>>> + return; > >>>>> + > >>>>> + pipe->funcs->disable(pipe); > >>>>> +} > >>>>> + > >>>>> +static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_= helper_funcs =3D { > >>>>> + .disable =3D drm_simple_kms_crtc_disable, > >>>>> + .enable =3D drm_simple_kms_crtc_enable, > >>>>> +}; > >>>>> + > >>>>> +static const struct drm_crtc_funcs drm_simple_kms_crtc_funcs =3D= { > >>>>> + .reset =3D drm_atomic_helper_crtc_reset, > >>>>> + .destroy =3D drm_crtc_cleanup, > >>>>> + .set_config =3D drm_atomic_helper_set_config, > >>>>> + .page_flip =3D drm_atomic_helper_page_flip, > >>>>> + .atomic_duplicate_state =3D drm_atomic_helper_crtc_duplicate_= state, > >>>>> + .atomic_destroy_state =3D drm_atomic_helper_crtc_destroy_stat= e, > >>>>> +}; > >>>>> + > >>>>> +static int drm_simple_kms_plane_atomic_check(struct drm_plane = *plane, > >>>>> + struct drm_plane_state *plane_state) > >>>>> +{ > >>>>> + struct drm_rect src =3D { > >>>>> + .x1 =3D plane_state->src_x, > >>>>> + .y1 =3D plane_state->src_y, > >>>>> + .x2 =3D plane_state->src_x + plane_state->src_w, > >>>>> + .y2 =3D plane_state->src_y + plane_state->src_h, > >>>>> + }; > >>>>> + struct drm_rect dest =3D { > >>>>> + .x1 =3D plane_state->crtc_x, > >>>>> + .y1 =3D plane_state->crtc_y, > >>>>> + .x2 =3D plane_state->crtc_x + plane_state->crtc_w, > >>>>> + .y2 =3D plane_state->crtc_y + plane_state->crtc_h, > >>>>> + }; > >>>>> + struct drm_rect clip =3D { 0 }; > >>>>> + struct drm_simple_display_pipe *pipe; > >>>>> + struct drm_crtc_state *crtc_state; > >>>>> + bool visible; > >>>>> + int ret; > >>>>> + > >>>>> + pipe =3D container_of(plane, struct drm_simple_display_pipe, = plane); > >>>>> + crtc_state =3D drm_atomic_get_existing_crtc_state(plane_state= ->state, > >>>>> + &pipe->crtc); > >>>>> + if (crtc_state->enable !=3D !!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 =3D crtc_state->adjusted_mode.hdisplay; > >>>>> + clip.y2 =3D crtc_state->adjusted_mode.vdisplay; > >>>>> + ret =3D 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 coordina= tes > >>>> aren't even passed/stored anywhere? > >>> It disallows positioning and scaling, so shouldn't ever need to h= ave 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 rect= angle. > > Probably best if we add clipped plane coordinates to drm_plane_stat= e. >=20 > Both src and crtc or only src? >=20 > if (!visible) > return -EINVAL; > + > + plane_state->src_x =3D src.x1; > + plane_state->src_y =3D src.y1; > + plane_state->src_w =3D drm_rect_width(&src); > + plane_state->src_h =3D drm_rect_height(&src); > + > + plane_state->crtc_x =3D dest.x1; > + plane_state->crtc_y =3D dest.y1; > + plane_state->crtc_w =3D drm_rect_width(&dest); > + plane_state->crtc_h =3D 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. >=20 > if (!pipe->funcs || !pipe->funcs->check) > return 0; >=20 > Noralf. --=20 Ville Syrj=E4l=E4 Intel OTC