[Intel-gfx] [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups

[Jani Nikula]

On Fri, 20 Sep 2019, Thierry Reding <thierry.reding at gmail.com> wrote:
> On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding at nvidia.com>
>> 
>> Hi,
>> 
>> this series of patches improves the DP helpers a bit and cleans up some
>> inconsistencies along the way.
>> 
>> v2 incorporates all review comments add collects Reviewed-bys from v1.
>> 
>> Thierry
>> 
>> Thierry Reding (21):
>>   drm/dp: Sort includes alphabetically
>>   drm/dp: Add missing kerneldoc for struct drm_dp_link
>>   drm/dp: Add drm_dp_link_reset() implementation
>>   drm/dp: Track link capabilities alongside settings
>>   drm/dp: Turn link capabilities into booleans
>>   drm/dp: Probe link using existing parsing helpers
>>   drm/dp: Read fast training capability from link
>>   drm/dp: Read TPS3 capability from sink
>>   drm/dp: Read channel coding capability from sink
>>   drm/dp: Read alternate scrambler reset capability from sink
>>   drm/dp: Read eDP version from DPCD
>>   drm/dp: Read AUX read interval from DPCD
>>   drm/dp: Do not busy-loop during link training
>>   drm/dp: Use drm_dp_aux_rd_interval()
>>   drm/dp: Add helper to get post-cursor adjustments
>>   drm/dp: Set channel coding on link configuration
>>   drm/dp: Enable alternate scrambler reset when supported
>>   drm/dp: Add drm_dp_link_choose() helper
>>   drm/dp: Add support for eDP link rates
>>   drm/dp: Remove a gratuituous blank line
>>   drm/bridge: tc358767: Use DP nomenclature
>
> Anyone interested in reviewing these?

Thierry, I don't quite know how to put this nicely, but I also don't
think it's nice to not reply at all. So I'll try to be fair but it'll be
blunt. Fair enough?

I've glanced over the series, already before you pinged for reviews. It
looks like you've put effort into it, and it all looks nice. However, it
does not look like we could use this in i915, without significant effort
both on top of this work and in i915. It does not feel like there's any
incentive for us to review this in detail.

It also feels like there's an increasing disconnect between "small" and
"big" drivers (*) when it comes to handling DP link and training. It
scares me a bit that this work is being done on the terms of the "small"
drivers, and that later in time this might be considered the One True
Way of handling DP.

One of the technical observations is that you fill the struct
drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear
that the link caps really are an intersection of the source and sink
caps. The eDP 1.4 link rates are the prime example. I think you should
have sets of source and sink rates, and you should intersect those to
find out the available link rates. The max rate is the highest number in
that set. Similarly for many things, like training pattern support. I
think it's only going to get more complicated with DP 2.0.

Another pain point is the caching of the caps as bits in
drm_dp_link_caps. How far are you going to take it? There's an insane
and growing amount of things in the DPCD that describe the link in one
way or another. Should they all be added to caps? Where do you draw the
line? Do we add both the bit and the helper for getting that bit from
the DPCD? And are you then going to add support for intersecting all
those cap bits between the source and the sink?

---

Overall I think there is value in unifying how we handle DP in drm. Even
if just by providing helpers to simplify things in drivers. It's just
that I feel this series isn't taking us closer to that goal, except for
a subset of drivers. In the big picture, it may be increasing the
divide.

If we get a confirmation from our drm overlords that drivers doing
things the way they see fit in this regard is fine, then I'm okay with
this. But I'm definitely not committing to switching to using the
drm_dp_link structures and helpers in i915, quite the opposite actually.


BR,
Jani.


(*) Please don't read too much into "small" and "big", just two groups
of drivers handling things differently.




>
> Thierry
>
>> 
>>  drivers/gpu/drm/bridge/tc358767.c      |  22 +-
>>  drivers/gpu/drm/drm_dp_helper.c        | 327 ++++++++++++++++++++++---
>>  drivers/gpu/drm/msm/edp/edp_ctrl.c     |  12 +-
>>  drivers/gpu/drm/rockchip/cdn-dp-core.c |   8 +-
>>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  |  13 +-
>>  drivers/gpu/drm/tegra/dpaux.c          |   8 +-
>>  drivers/gpu/drm/tegra/sor.c            |  32 +--
>>  include/drm/drm_dp_helper.h            | 124 +++++++++-
>>  8 files changed, 459 insertions(+), 87 deletions(-)
>> 
>> -- 
>> 2.22.0
>> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Graphics Center