From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751340AbcEISho (ORCPT ); Mon, 9 May 2016 14:37:44 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:44884 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750919AbcEIShn (ORCPT ); Mon, 9 May 2016 14:37:43 -0400 Subject: Re: [PATCH 3/4] drm: Add helper for simple display pipeline To: Daniel Vetter References: <1462454674-2246-1-git-send-email-noralf@tronnes.org> <1462454674-2246-4-git-send-email-noralf@tronnes.org> <20160509144648.GA27098@phenom.ffwll.local> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Cc: dri-devel@lists.freedesktop.org, treding@nvidia.com, linux-kernel@vger.kernel.org Message-ID: <8b885635-4df5-e902-a755-3f32cab75131@tronnes.org> Date: Mon, 9 May 2016 20:37:39 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160509144648.GA27098@phenom.ffwll.local> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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