Yeap, makes sense let them explicit then... Thanks for explanation and fell free to go ahead ;) On Mon, May 27, 2013 at 2:28 PM, Daniel Vetter <daniel at ffwll.ch> wrote: > On Mon, May 27, 2013 at 7:16 PM, Rodrigo Vivi <rodrigo.vivi at gmail.com> wrote: >> Hi Imre, >> >> I just reviewed all patches in this series and saw no issue. >> So, in this sense, you and Daniel are free to use "Reviewed-by: >> Rodrigo Vivi <rodrigo.vivi at gmail.com>". >> >> However before you move ahead I need to show my concern with the whole idea. >> is_cpu_edp is an easy abstraction and provides an easy way to add new >> things/restrictions/was/etc like eDP on port D support for HSW. >> >> In the new way code can be more polluted and bugs can be easily added >> by forgetting to add a check somewhere when hunting for places like >> port_A || vlv || to add || (HSW && port_D). >> >> Imagine in some time we have more platforms like valleyview and more >> platforms that supports edp on many other different ports. >> I really would like to see those checks together in only one place, i. >> e., is_cpu_edp(). > > My idea behind all this is that I think is_cpu_edp is horribly > confusing as an abstraction. Imo we should be really careful to > differentiate between two things which is_cpu_edp smashes toghether: > - eDP vs. non-DP: This should control stuff like enabling the > backlight and controlling the panel power sequence. Also we should use > this for things like grabbing (and caching) the fixed panel mode. In > short this is all about the sink capabilities of a DP output. > - cpu vs. pch port (on ilk/ivb/snb) or port A vs. port B-D: This is > about how some DP ports are different on the same platform, e.g. the > link training is a bit different, or we need a special transcoder, or > a special clock. But all this has nothing to do with whether the DP > sink device is a panel or an external display. The big offender here > was eDP on port D, since on the source side there's zero difference > between eDP mode and DP mode. The only exception really is that we > don't set up a HDMI output, but that's rather external to the dp code. > > With Imre's latest patches this should now be untangled: Everything > which cares about the sink only checks for is_edp, everything which > cares about the source checks for platform + port. > > So your example of adding a (HSW && port_D) check for eDP support > should never come up. > > Does this alleviate your concerns about this refactoring? > > Note that I understand that some of the checks are now pretty ugly, > but the only thing that's changed is that the ugly is now explicit. > Once the next big platform change comes around I expect that we need > to refactor intel_dp.c big time and add a vtable to abstract away a > lot of this stuff from the core dp logic. > > Yours, Daniel > >> >> Cheers, >> Rodrigo. >> >> On Thu, May 16, 2013 at 8:40 AM, Imre Deak <imre.deak at intel.com> wrote: >>> is_cpu_edp() is equivalent with a port==PORT_A check. There are two >>> exceptions (see patch 1 and 2), where we can rewrite things to handle >>> ValleyView separately as it's done at other places. With these out of >>> the way we can replace is_cpu_edp() with a simpler port check and >>> remove is_cpu_edp(). >>> >>> I tested this only on IVB, so before applying we should test it on VLV >>> at least. >>> >>> Imre Deak (4): >>> drm/i915: stop using is_cpu_edp() in intel_disable/post_disable_dp >>> drm/i915: merge VLV eDP and DP AUX clock divider calculation >>> drm/i915: replace is_cpu_edp() with a check for port A >>> drm/i915: remove unused is_cpu_edp() >>> >>> drivers/gpu/drm/i915/intel_dp.c | 78 ++++++++++++++++++--------------------- >>> 1 file changed, 35 insertions(+), 43 deletions(-) >>> >>> -- >>> 1.7.10.4 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx at lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Rodrigo Vivi >> Blog: http://blog.vivi.eng.br >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx at 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 -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
