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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/14/15 4:29 AM, Paulo Zanoni wrote:
2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
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@xxxxxxxxx>
---
  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. 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux