Re: [PATCH 05/12] drm/i915/bxt: DSI encoder support in CRTC modeset

From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: shobhit.kumar@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/12] drm/i915/bxt: DSI encoder support in CRTC	modeset
Date: Tue, 26 May 2015 10:26:13 +0200	[thread overview]
Message-ID: <20150526082613.GH12971@phenom.ffwll.local> (raw)
In-Reply-To: <87d21niz55.fsf@intel.com>

On Tue, May 26, 2015 at 10:19:50AM +0300, Jani Nikula wrote:
> On Tue, 26 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, May 25, 2015 at 01:25:56PM +0300, Jani Nikula wrote:
> >> On Fri, 22 May 2015, Uma Shankar <uma.shankar@intel.com> wrote:
> >> > +	 * but DDI interface doesn't support DSI yet, so don't do anything
> >> > +	 * for DSI encoders
> >> > +	 */
> >> > +	if (!(HAS_DDI(dev) && has_encoder_ddi(type))) {
> >> 
> >> HAS_DDI() is always true here.
> >> 
> >> Hmm. Perhaps it would be nicer if we added INVALID_PORT = -1 to enum
> >> port, and had intel_ddi_get_encoder_port() return that for DSI. Then we
> >> could leave most of the functions the same, with just
> >> 
> >> 	if (port == INVALID_PORT)
> >>         	return;
> >> 
> >> at the beginning.
> >> 
> >> Daniel, opinions?
> >
> > Layering in the ddi/hsw+ display code is a bit fumbled - a bunch of these
> > ddi enable/disable calls should be pushed down into encoder hooks.
> > Otherwise we need to sprinkle piles of if (type == foo) checks all over.
> > Well we already have them, but we'd need more :(
> >
> > Generally the split between crtc and encoder should be at the cross-bar
> > for most of the ports (pch-split is special here with fdi vs cpu ports).
> > Especially here where we already have a ddi encoder to handle all the ddi
> > common code.
> >
> > I've started with patches a while ago, but that didn't get all that far.
> > Imo the crucial bit is to get rid of intel_ddi_get_encoder_port is the
> > indicator for how much layering confusion there still is in the ddi code.
> 
> I guess the question is, what's the short term plan for DSI?

Just expressing my unhappiness about how convoluted the ddi code is. It's
full of reverse-lookups of ports and big if (type == foo) ladders, which
makes the entire thing fairly unwielding. And now we add more conditions
to it with the bolted-on dsi ports, which doesn't help. Generally big if
ladders on the type in object-oriented code means the type hierarchy ended
up cut the wrong way. Same with all the inversion around looking up the
port.

I guess we could munge on and hope for a better day. But I'd love for
someone to untangle this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx