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@xxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel