[Intel-gfx] [PATCH v6 3/5] drm/i915/gen11: Convert combo PHY logic to use new 'enum phy' namespace

[Jani Nikula]

On Wed, 04 Sep 2019, Matt Roper <matthew.d.roper at intel.com> wrote:
> On Wed, Sep 04, 2019 at 04:42:49PM +0300, Jani Nikula wrote:
>> On Tue, 09 Jul 2019, Matt Roper <matthew.d.roper at intel.com> wrote:
>> > Convert the code that operates directly on gen11 combo PHY's to use the
>> > new namespace.  Combo PHY registers are those named "ICL_PORT_*" plus
>> > ICL_DPHY_CHKN.
>> >
>> > Note that a lot of the PHY programming happens in the MIPI DSI code.
>> > For clarity I've added a for_each_dsi_phy() to loop over the phys used
>> > by DSI.  Since DSI always uses A & B on gen11, port=phy in all cases so
>> > it doesn't actually matter which form we use in the DSI code.  I've used
>> > the phy iterator in code that's explicitly working with the combo PHY,
>> > but left the rest of the DSI code using the port iterator and namespace
>> > to minimize patch deltas.  We can switch the rest of the DSI code over
>> > to use phy terminology later if this winds up being too confusing.
>> 
>> One itsy-bitsy detail, where does this initialize intel_dsi->phys?
>> Looking at the code, I'm thinking nowhere.
>
> At the moment we're relying on the fact that port==phy always for DSI on
> the platforms this code applies to.  intel_dsi->ports is initialized
>
>         if (dev_priv->vbt.dsi.config->dual_link)
>                 intel_dsi->ports = BIT(PORT_A) | BIT(PORT_B);
>         else
>                 intel_dsi->ports = BIT(port);
>
> and that's unioned with intel_dsi->phys
>
>         /* bit mask of ports (vlv dsi) or phys (icl dsi) being driven */
>         union {
>                 u16 ports;      /* VLV DSI */
>                 u16 phys;       /* ICL DSI */
>         };
>
> so I think the initialization should be the same as it was before this patch.

Right, the union is what I missed completely. I was looking for ->phys
initialization but couldn't find it. Sorry for the noise!

> As I mentioned above, this patch only changes the instances of 'enum port' to
> 'enum phy' where they were specifically touching the combo PHY registers, and
> left a bunch of the other code using 'enum port' to minimize the initial code
> diffs.  It's probably time now to follow up with another patch that converts
> the rest of the file to phy usage to avoid confusion; I'll write a patch to do
> that later today or tomorrow.

Do you propose to index the rest of the relevant registers with phy too?
(I know there's no effective change, just the name.) Should we change
dsi_port_to_transcoder() to dsi_phy_to_transcoder()?

In any case I agree some de-confusing would be in order.

>> We have an ICL DSI machine in CI, which reports all green on this
>> patch... but most likely the display is all black instead. DSI being
>> DSI, it is entirely possible we have no way of knowing without having a
>> camera capture the display.
>
> Hmm.  All green all the time or only on specific tests/operations?  I assume
> this is https://intel-gfx-ci.01.org/hardware/fi-icl-dsi/ ?  Strangely I don't
> see an i915_display_info for that machine, but the current CI results seem to
> be mostly passing.  Is there a specific failing test I should look at?

This was all based on me failing to find the ->phys initialization; I
think it's conceivable we wouldn't have received any driver detectable
errors even if ->phys were zero. Just a black screen. But again, sorry
for the noise!


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center