[Intel-gfx] [PATCH 0/4] drm/i915: remove is_cpu_edp()

[Daniel Vetter]

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