On Fri, Oct 17, 2014 at 12:47:31PM +0300, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
