Re: [PATCH 0/5] RFC: Displayport Link Policy Maker Update

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 08, 2014 at 10:47:38AM -0700, Todd Previte wrote:
> This patch set lays the initial groundwork for updating the Displayport link policy maker
> in the i915 driver. These first 5 patches add modular functions that have been split out
> from the larger, monolithic ones present in the driver. The purpose of this work is the
> following:
> 	1) Improve reliability and operation of the Displayport link policy maker
> 	2) Modularize and compartmentalize functionality to enhance readability and reliability
> 	3) Allow more fine-grain control over the operation of the link policy maker
> 	4) Enable features of Displayport such as Multistream Transport and compliance testing
> 
> Note that these new functions are not yet integrated into the operational code path(s). The 
> original functions have been left in place, as is, to maintain operational status. Follow on
> patch sets will integrate these functions into the policy maker.

So ignoring the details and just commenting on the overall approach:

When we're currently at A and want to go to B then we do that with a
series of small patche refactoring the existing code step-by-step in fully
bisectable patches. Writing completely new code and then throwing the
switch to move from A to B is the exceptional case and needs exceptional
justification. Examples:

- Switching to the common dp aux helper. But even in that case Jani first
  adjusted the existing code to make it match better with the semantics of
  the new code so that the transition was as smooth as possible. Still
  blew up.

- Completely new hardware/platforms, e.g. the DDI code on hsw+. But even
  there the very first enabling code started out from intel_dp.c and then
  only after an awful lot of refactoring.

As-is I don't see a justification why the DP code qualifies. Also note
that we now have the shared dp helper library for handling all kinds of dp
sink behaviour. Dave Airlie is already working like crazy to get dp mst
(at least the discovery part) off the ground. See:

http://cgit.freedesktop.org/~airlied/linux/log/?h=i915-mst-hacks

Imo any sink related big changes should be done in the dp helper code and
not on our side. Of course all the source handling needs to still be in
our driver (i.e. in intel_dp.c and intel_ddi.c).

So overall I'm not sold on why exactly we need these patches here at all.
-Daniel

> 
> Todd Previte (5):
>   dmr/i915: Displayport - Add a function to set the training pattern
>   drm/i915: Displayport - Add function to check link status
>   drm/i915: Displayport - Add function to enable/disable scrambling on
>     the main link
>   drm/i915: Displayport - Add function for executing a single iteration
>     of clock recovery
>   drm/i915: Displayport - Add function to execute a single iteration of
>     channel equalization
> 
>  drivers/gpu/drm/i915/i915_reg.h |   8 ++
>  drivers/gpu/drm/i915/intel_dp.c | 217 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 225 insertions(+)
> 
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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