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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 17, 2014 at 08:26:13PM +0300, Mika Kuoppala wrote:
> Jani Nikula <jani.nikula@linux.intel.com> writes:
> 
> > 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.
> >
> 
> Is
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index ac6232b..9b82d90 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -57,6 +57,11 @@
>  #define DRIVER_DESC            "Intel Graphics"
>  #define DRIVER_DATE            "20141003"
>  
> +#ifdef WARN_ON
> +#undef WARN_ON
> +#define WARN_ON(x)     WARN(x, "WARN_ON(" #x ")")
> +#endif
> +
> 
> too big of a hammer?

Imo this is awesome!
> 
> adds ~16k to i915.ko.

And for all those concerned about since we need options to cut away _all_
the debug strings. Well _all_ the printk output even. Wasting a bit more
space for easier debug is imo more than worth it.

So please make this a proper patch and let's get this in. For hilarity
maybe cc: lkml and ask get_maintainers who might be interested in a
generic version ... But I think we want the i915 one in asap.

Cheers, Daniel
> 
> --Mika
> 
> > So here you have a WARN_ON with a comment that could become the WARN
> > message almost verbatim. Please consider making the change.
> >
> > 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
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch