Re: [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation

[Todd Previte <tprevite@gmail.com>]

On 4/14/15 12:00 PM, Paulo Zanoni wrote:
> 2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>
>> On 4/14/15 4:29 AM, Paulo Zanoni wrote:
>>> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>> Update the hot plug function to handle the SST case. Instead of placing
>>>> the SST case within the long/short pulse block, it is now handled after
>>>> determining that MST mode is not in use. This way, the topology
>>>> management
>>>> layer can handle any MST-related operations while SST operations are
>>>> still
>>>> correctly handled afterwards.
>>>>
>>>> This patch also corrects the problem of SST mode only being handled in
>>>> the
>>>> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing
>>>> purposes
>>>> both short and long pulses are used by the different tests, thus both
>>>> cases
>>>> need to be addressed for SST.
>>>>
>>>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in
>>>> the
>>>> previous compliance testing patch sequence. Review feedback on that patch
>>>> indicated that updating intel_dp_hot_plug() was not the correct place for
>>>> the test handler.
>>>>
>>>> For the SST case, the main stream is disabled for long HPD pulses as this
>>>> generally indicates either a connect/disconnect event or link failure.
>>>> For
>>>> a number of case in compliance testing, the source is required to disable
>>>> the main link upon detection of a long HPD.
>>>>
>>>> V2:
>>>> - N/A
>>>> V3:
>>>> - Place the SST mode link status check into the mst_fail case
>>>> - Remove obsolete comment regarding SST mode operation
>>>> - Removed an erroneous line of code that snuck in during rebasing
>>>> V4:
>>>> - Added a disable of the main stream (DP transport) for the long pulse
>>>> case
>>>>     for SST to support compliance testing
>>>> V5:
>>>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>>> V6:
>>>> - Reformatted a comment
>>>>
>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>>>>    1 file changed, 8 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 77b6b15..ba2da44 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port
>>>> *intel_dig_port, bool long_hpd)
>>>>                           if (intel_dp_check_mst_status(intel_dp) ==
>>>> -EINVAL)
>>>>                                   goto mst_fail;
>>>>                   }
>>>> -
>>>> -               if (!intel_dp->is_mst) {
>>>> -                       /*
>>>> -                        * we'll check the link status via the normal hot
>>>> plug path later -
>>>> -                        * but for short hpds we should check it now
>>>> -                        */
>>>> -
>>>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>>> -                       intel_dp_check_link_status(intel_dp);
>>>> -
>>>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>>> -               }
>>>>           }
>>> Shouldn't the code be moved to exactly this spot instead of after the
>>> put_power label? Why would we want to call check_link_status in case
>>> we goto mst_fail? In case there is a valid reason, maybe it would be
>>> better to do a big reorganization of this function because it's going
>>> to start looking very weird - or at least rename the labels.
>> No because then you don't get long pulses, only short ones.
> No, what I mean is:
>
> if (long_hpd) {
>      ... code ...
> } else {
>      ... code ....
> }
>
> if (!intel_dp->is_mst) {
>      drm_modeset_lock(...)
>      ... code ...
> }
>
> mst_fail:
>      ... code ...
>
> The other problem I point is: imagine we're SST and we get a long_hpd.
> Then we run ibx_digital_port_connected(), and since the monitor is
> disconnected we "goto mst_fail". We'll end up running
> intel_dp_check_link_status() before returning, but we really shouldn't
> run it since we know the monitor is disconnected.
>
I see what you're saying, however under normal operation for SST (when 
connected and everything is working) the code will hit this line:

if (!intel_dp_probe_mst(intel_dp))

And proceed to the mst_fail block, thus skipping that block of code 
entirely and missing the SST handler. The result is a missed long pulse 
for the SST case.

Your second point has validity though. This can be addressed with a 
"connected" flag just before the if (long_pulse) statement:

     connected = intel_dp_digital_port_connected(intel_dp);
     if (!connected)
         goto mst_fail;

The pulse handler for the most part can then be skipped, since the 
device is gone. In mst_fail, the MST topology manager is updated though, 
so that still has to happen. With the SST pulse handler in put_power, it 
can simply fall through now and hit the updated if-statement there which is:

if (!intel_dp->is_mst && connected) {
         ... code ...
}

And all should be well for a disconnected device as well as normal 
operational modes of SST and MST.

Oh the intel_dp_digital_port_connected() function is just the 
encapsulation of this code from the long_pulse segment:

         if (HAS_PCH_SPLIT(dev)) {
             if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
                 goto mst_fail;
         } else {
             if (g4x_digital_port_connected(dev, intel_dig_port) != 1)
                 goto mst_fail;
         }

And it's been added to the updated patch.

>> The put_power
>> case is where this belongs, unless you want to duplicate code in both the
>> long_pulse and the else clause. There is a separate mst_check_link_status
>> call so this one is specific to SST mode. There is also a check to make sure
>> it doesn't get called when MST is active and MST has hit a failure mode, so
>> that is a non-issue.
>>> Also, for the long_hpd case, I see that check_link_status() will redo
>>> some of the stuff we already did on this function, such as get_dpcd().
>>> And if you follow my advice on patch 2, you will end up having even
>>> more repeated code. I think you could try to do a careful analysis
>>> here to make sure we're not calling stuff twice here, especially since
>>> some of those operations are potentially slow.
>> I see a couple places where the code is duplicated, specifically the
>> connection check (which I encapsulated in a function and I'll likely roll
>> forward into this one since it makes things more clear) and the DPCD read in
>> the long pulse case. I removed the code in check_link_status for both of
>> these things and it still passes compliance. Good catch Paulo. This has been
>> fixed and tested and will be in the updated patch posted shortly.
>>>>           ret = IRQ_HANDLED;
>>>>
>>>>           goto put_power;
>>>>    mst_fail:
>>>> -       /* if we were in MST mode, and device is not there get out of MST
>>>> mode */
>>>>           if (intel_dp->is_mst) {
>>>> +               /* if we were in MST mode, and device is not there get
>>>> out of MST mode */
>>> I don't see the need for changes such as the one above - I saw similar
>>> cases in other patches you submitted. I often use git blame on
>>> comments in order to be able to see the whole context of the change,
>>> and a simple change like this makes it harder to blame. Also, you're
>>> not even fixing the 80 column problem here. And I do prefer the
>>> comment on top of the if statement.
>> This is just an artifact of moving things around, as it likely was in the
>> other patches. The only reason I will move comments is to clarify what they
>> pertain to if code is moving around it. It's back where it belongs now so it
>> doesn't even show up in the patch. Fixed for the next version.
>>
>>>>                   DRM_DEBUG_KMS("MST device may have disappeared %d vs
>>>> %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>>>                   intel_dp->is_mst = false;
>>>>                   drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
>>>> intel_dp->is_mst);
>>>>           }
>>>>    put_power:
>>>> +       /* SST mode - handle short/long pulses here */
>>>> +       if (!intel_dp->is_mst) {
>>>> +               drm_modeset_lock(&dev->mode_config.connection_mutex,
>>>> NULL);
>>>> +               intel_dp_check_link_status(intel_dp);
>>>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>>> +               ret = IRQ_HANDLED;
>>>> +       }
>>>>           intel_display_power_put(dev_priv, power_domain);
>>>>
>>>>           return ret;
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx