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

[Rodrigo Vivi]

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