linux-kernel - Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160712103402.GA23520@phenom.ffwll.local>
Date:	Tue, 12 Jul 2016 12:34:02 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Emil Velikov <emil.l.velikov@...il.com>,
	Vinay Simha BN <simhavcs@...il.com>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:DRM PANEL DRIVERS" <dri-devel@...ts.freedesktop.org>,
	Archit Taneja <archit.taneja@...il.com>
Subject: Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

On Mon, Jul 11, 2016 at 02:29:37PM +0200, Thierry Reding wrote:
> On Thu, Jun 16, 2016 at 06:02:53PM +0100, Emil Velikov wrote:
> > On 16 June 2016 at 04:00, Vinay Simha BN <simhavcs@...il.com> wrote:
> [...]
> > > +static int jdi_panel_disable(struct drm_panel *panel)
> > > +{
> > > +       struct jdi_panel *jdi = to_jdi_panel(panel);
> > > +
> > > +       if (!jdi->enabled)
> > > +               return 0;
> > > +
> > Thinking out loud:
> > 
> > Thierry,
> > Shouldn't we fold 'enabled' and 'prepared' in struct drm_panel and
> > tweak the helpers respectively ? Is there any specific reason for
> > keeping these in the drivers ?
> 
> Yes, I think that would make sense eventually. It's clearly a recurring
> pattern. Ideally nothing would be calling these functions more than once
> and thereby making the checks unnecessary. In practice that may mean
> that we need to put the variables and checks into the drm/panel core
> because display drivers (as opposed to a sane core implementation) call
> these. I suppose we could encourage proper usage by adding a couple of
> WARNs here and there if expectations aren't met.
> 
> I don't think doing this is terribly urgent because it's easy to rip out
> of drivers once the drm/panel core supports it. And it's something that
> we could even leave within drivers when the core supports it, so trivial
> to remove one by one after the core patches have landed.

As long as we have non-atomic drm drivers using this multiple
enable/disable calls can happen. Atomic drivers should screw this up
(ignoring a few misguided ones that mix atomic and legacy helpers in bad
ways, but those are getting fixed).

I think a good plan would be:
1. Move this tracking into drm panel helpers, ditch it from all drivers.
2. Add WARN_ON for multiple enables/disables, but only for DRIVER_ATOMIC.

Makes sure we can remove this boilerplate, makes sure that atomic drivers
are consistent, leaves existing drivers unharmed.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch