Re: [PATCH 01/17] drm/i915: Warn if trying to register eDP on port != B/C on vlv/chv

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 17, 2014 at 12:47:31PM +0300, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Only ports B and C have the power sequencer and backlight controls,
> > so complain if we ever try to register an eDP connector on some other
> > port.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4455009..de919e9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5222,6 +5222,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  	if (type == DRM_MODE_CONNECTOR_eDP)
> >  		intel_encoder->type = INTEL_OUTPUT_EDP;
> >  
> > +	/* eDP only on port B and/or C on vlv/chv */
> > +	if (WARN_ON(IS_VALLEYVIEW(dev) && is_edp(intel_dp) &&
> > +		    port != PORT_B && port != PORT_C))
> > +		return false;
> 
> Sidetracking about WARNs here for a sec.
> 
> One takeaway from working on bugs for a month is that when you read a
> lot (I mean really a lot) of dmesgs for various kernel versions, and you
> see all those warning backtraces from WARN_ON in the logs, it gets
> really tedious to find the line emitting the warning in the source. You
> see the function and the line number, but maybe those have changed and
> maybe the warn was different a few kernel releases ago.
> 
> The WARNs with a descriptive message, on the other hand, are really
> quite helpful both while reading the backtrace and while reading the
> source. Kind of self-documenting. I'd like to discourage the use of
> WARN_ON in favor of WARN all around.
> 
> So here you have a WARN_ON with a comment that could become the WARN
> message almost verbatim. Please consider making the change.

Yeah I've come to the same conclusion myself and yet I somehow always
skip the message when adding one initially. I need to tech myself better
habits, or someone needs to kill WARN_ON() entirely :P

I'm not actually sure this WARN here is all that useful. Might be better
to sprinkle a few more of the pipe==A|B checks in some of the lower
level PPS funcs like I did in patch 14/17. Although that could use the
message too. Also maybe WARN_ONCE() would be better for those so that we
don't bog down the entire machine.

> 
> BR,
> Jani.
> 
> 
> 
> > +
> >  	DRM_DEBUG_KMS("Adding %s connector on port %c\n",
> >  			type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
> >  			port_name(port));
> > -- 
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC