Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965135AbcDLTaA (ORCPT ); Tue, 12 Apr 2016 15:30:00 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35977 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756401AbcDLT36 (ORCPT ); Tue, 12 Apr 2016 15:29:58 -0400 Date: Tue, 12 Apr 2016 21:30:14 +0200 From: Daniel Vetter To: Liviu Dudau Cc: Dave Airlie , Daniel Stone , David Brown , Brian Starkey , devicetree@vger.kernel.org, LKML , DRI devel Subject: Re: [RFC][PATCH 2/2] drm/arm: Add support for Mali Display Processors Message-ID: <20160412193014.GN2510@phenom.ffwll.local> Mail-Followup-To: Liviu Dudau , Dave Airlie , Daniel Stone , David Brown , Brian Starkey , devicetree@vger.kernel.org, LKML , DRI devel References: <1459527712-9488-1-git-send-email-Liviu.Dudau@arm.com> <1459527712-9488-3-git-send-email-Liviu.Dudau@arm.com> <20160412154757.GG2510@phenom.ffwll.local> <20160412171349.GK16063@e106497-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160412171349.GK16063@e106497-lin.cambridge.arm.com> X-Operating-System: Linux phenom 4.4.0-1-amd64 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 Content-Length: 2656 Lines: 64 On Tue, Apr 12, 2016 at 06:13:49PM +0100, Liviu Dudau wrote: > On Tue, Apr 12, 2016 at 05:47:57PM +0200, Daniel Vetter wrote: > > On Fri, Apr 01, 2016 at 05:21:52PM +0100, Liviu Dudau wrote: > > > +static int malidp_enable_vblank(struct drm_device *drm, unsigned int crtc) > > > +{ > > > + return 0; > > > +} > > > + > > > +static void malidp_disable_vblank(struct drm_device *drm, unsigned int pipe) > > > +{ > > > +} > > > > Might be worth it to create a patch for drm_irq.c to make > > enable/disable_vblank functions optional. Otoh does your chip really keep > > on generating vblank irqs all the time, with no way to shut it up? That > > would be terrible for power consumption ... Especially since you have no > > hw counter either. > > Initially I had code here that was turning off the vblank irq, then I've read > the comment in drmP.h that the routine should be a no-op when hardware counters > are missing, hence this version. As for the display processor: it will generate > an interrupt for every finished scanout cycle, but it has support for variable > vsync. Interrupt can be disabled, but I've read in the drmP.h that it is required > for timestamping support when one doesn't have hw counters. > > I'm OK with fixing drm_irq.c to not require enable/disable_vblank but then the > comments in drmP.h will also have to change? Nah, you bring up a good point actually - you really can't disable vblank if there's no hw counter. At least not right now. I think dummy functions in drm_irq.c like drm_vblank_get/set_no_hw_counter to make this clear would be nice. Or maybe just a comment here. The other option would be to finally fake this using high-precision timestamps, since a lot of mobile hw seems to have forgotten to add a proper vblank counter. But that has issues (it can drift), and probably better done separately. > > > +static void malidp_de_plane_disable(struct drm_plane *plane, > > > + struct drm_plane_state *state) > > > +{ > > > + struct malidp_plane *mp = to_malidp_plane(plane); > > > + > > > + /* ToDo: figure out the attached framebuffer lifecycle */ > > > > You don't need to figure this out, atomic helpers will take care of the fb > > for you. > > It is more in line with un/pinning the framebuffer and making sure that the > framebuffer has been scanned out before unref-ing it. That should be taken care of by the vblank wait the helpers do for you. Again happans all automatically (except you need to keep that in mind for the async work). > Thanks again for finding time to review the code. No problem. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch