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
