On 1/19/2016 2:14 PM, Daniel Vetter wrote:
On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
On 1/19/2016 2:35 AM, Lukas Wunner wrote:
Hi,
On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.
The link retraining part when EQ is not correct is
retained to intel_dp_check_link_status whereas other
operations are handled as part of intel_dp_short_pulse.
This change is required to avoid performing all DPCD
related operations on performing link retraining.
v2: Added WARN_ON to intel_dp_check_link_status()
Removed a call to intel_dp_get_link_status() (Ander)
Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@xxxxxxxxx>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@xxxxxxxxx>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 82ee18d..f8d9611 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4279,6 +4279,36 @@ go_again:
return -EINVAL;
}
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+ struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+ struct drm_device *dev = intel_dp_to_dev(intel_dp);
+ u8 link_status[DP_LINK_STATUS_SIZE];
+
+ WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+ if (!intel_dp_get_link_status(intel_dp, link_status)) {
+ DRM_ERROR("Failed to get link status\n");
+ return;
+ }
+
+ if (!intel_encoder->base.crtc)
+ return;
+
+ if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+ return;
Why do you change the order of the three if-clauses above?
The original order seems to make more sense. (Checking for
->base.crtc and ->active is cheap, whereas accessing AUX to
get the link status is time consuming. You don't want to
spend that time only to bail out, should one of the other two
if-clauses fail.)
Best regards,
Lukas
Actually it is expected to read link status whenever we receive short pulse
interrupt
irrespective of the panel being enabled or not. So this change is with
respect to
that rather than any performance based.
As a general rule please don't make functional changes like these in a
patch that just splits stuff up. Your patch summary sounds like simple
refactoring, which this doesn't seem to be.
-Daniel
Understood, will make the appropriate changes and move that to separate
patch.
regards,
Sivakumar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
