Re: [PATCH 0/8] Cleanup CRTC .enable()/.disable() cargo-cult

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	Liviu Dudau <liviu.dudau@arm.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Boris Brezillon <boris.brezillon@free-electrons.com>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Alexey Brodkin <abrodkin@synopsys.com>,
	Xinliang Liu <z.liuxinliang@hisilicon.com>,
	Xinwei Kong <kong.kongxinwei@hisilicon.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Mali DP Maintainers <malidp@foss.arm.com>,
	Dave Airlie <airlied@redhat.com>,
	Chen Feng <puck.chen@hisilicon.com>, Jyri Sarha <jsarha@ti.com>,
	Vincent Abriou <vincent.abriou@st.com>,
	VMware Graphics <linux-graphics-maintainer@vmware.com>,
	Alison Wang <alison.wang@freescale.com>,
	Seung-Woo Kim <sw0312.kim@sams>
Subject: Re: [PATCH 0/8] Cleanup CRTC .enable()/.disable() cargo-cult
Date: Wed, 28 Jun 2017 16:10:14 +0300	[thread overview]
Message-ID: <1541474.We3gYspHHt@avalon> (raw)
In-Reply-To: <CACvgo5332oa0LMzVyPAo_LSbPss8stF2ck7EhMvgahsi9yvM6g@mail.gmail.com>

Hi Emil,

On Wednesday 28 Jun 2017 12:27:06 Emil Velikov wrote:
> On 27 June 2017 at 21:38, Laurent Pinchart wrote:
> > Hello,
> > 
> > The atomic helpers favour the .enable() and .atomic_disable() CRTC helper
> > operations, but still support the legacy .prepare(), .commit(), .disable()
> > and .dpms() operations. Some drivers have been updated, but most still
> > use various combination of new and legacy operations, leading to
> > confusion among new developers when they read the code.
> 
> Just had a crazy idea:
> 
> Would it make sense for the function(s) that takes the
> driver_foo_funcs as an argument to cross-check for such mistakes?
> Since many of the hooks are optional, one could check for the
> 'forbidden' combinations:
> Atomic drivers should not have any legacy hooks set, while legacy ones
> should not have any of the atomic ones.

I think it's a good idea, although transition to atomic modesetting (following 
the procedure outlined in Daniel's blog) might mix legacy and atomic 
operations.

At the moment the only atomic driver that still uses legacy CRTC helper 
operations is vmwgfx. The driver implements both the .prepare() and 
.atomic_disable() helper operations, with .prepare() being empty. The CRTC 
helpers call those operations from disable_outputs():

                /* Right function depends upon target state. */
                if (new_crtc_state->enable && funcs->prepare)
                        funcs->prepare(crtc);
                else if (funcs->atomic_disable)
                        funcs->atomic_disable(crtc, old_crtc_state);
                else if (funcs->disable)
                        funcs->disable(crtc);
                else
                        funcs->dpms(crtc, DRM_MODE_DPMS_OFF);

.prepare() and .atomic_disable() are thus not completely identical.

I believe this could be addressed by adding

        if (crtc->state->enable)
                return;

to the .atomic_disable() handler in the vmwgfx driver. I'll submit a patch for 
that.

> Of course the idea is not restricted to the CRTC helpers -
> drm_{crtc,encoder,connector,plane,mode_config}_{helper_,}funcs should
> all be applicable.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel