From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751261AbcEJG7h (ORCPT ); Tue, 10 May 2016 02:59:37 -0400 Received: from mail-wm0-f44.google.com ([74.125.82.44]:36509 "EHLO mail-wm0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750979AbcEJG7g (ORCPT ); Tue, 10 May 2016 02:59:36 -0400 Date: Tue, 10 May 2016 08:59:34 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: Daniel Vetter , dri-devel@lists.freedesktop.org, treding@nvidia.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] drm: Add helper for simple display pipeline Message-ID: <20160510065934.GG27098@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, treding@nvidia.com, linux-kernel@vger.kernel.org References: <1462454674-2246-1-git-send-email-noralf@tronnes.org> <1462454674-2246-4-git-send-email-noralf@tronnes.org> <20160509144648.GA27098@phenom.ffwll.local> <8b885635-4df5-e902-a755-3f32cab75131@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8b885635-4df5-e902-a755-3f32cab75131@tronnes.org> 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 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