Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752301AbcCRQlr (ORCPT ); Fri, 18 Mar 2016 12:41:47 -0400 Received: from mga04.intel.com ([192.55.52.120]:63800 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750976AbcCRQlp (ORCPT ); Fri, 18 Mar 2016 12:41:45 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,355,1455004800"; d="scan'208";a="927134117" Date: Fri, 18 Mar 2016 18:41:40 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Cc: intel-gfx@lists.freedesktop.org, arthur.j.runyan@intel.com, open list , dri-devel@lists.freedesktop.org, Daniel Vetter Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake() Message-ID: <20160318164140.GO4329@intel.com> References: <1458229245-8634-1-git-send-email-cpaul@redhat.com> <1458229245-8634-2-git-send-email-cpaul@redhat.com> <20160318141345.GG4329@intel.com> <20160318161235.GN4329@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160318161235.GN4329@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9606 Lines: 238 On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrj?l? wrote: > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote: > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote: > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things > > > timeout, there's no use for having this function anymore. Good riddens. > > > > > > Signed-off-by: Lyude > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++----------------------------- > > > 1 file changed, 22 insertions(+), 57 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index cdc2c15..fb4cbbe5 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder) > > > } > > > > > > /* > > > - * Native read with retry for link status and receiver capability reads for > > > - * cases where the sink may still be asleep. > > > - * > > > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also > > > - * supposed to retry 3 times per the spec. > > > - */ > > > -static ssize_t > > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset, > > > - void *buffer, size_t size) > > > -{ > > > - ssize_t ret; > > > - int i; > > > - > > > - /* > > > - * Sometime we just get the same incorrect byte repeated > > > - * over the entire buffer. Doing just one throw away read > > > - * initially seems to "solve" it. > > > - */ > > > - drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1); > > > > NAK > > > > If people keep intentionally breaking my shit I'm going to become > > really grumpy soon. > > Oh, and just in case someone wants to come up with a better kludge, > I just spent a few minutes analyzing the behavior of this crappy > monitor a. > > What happens is that when the monitor is fully powered up (LED is blue) > things are fine. After the monitor goes to sleep (LED turns orange) > the first DPCD read will produce garbage. Further DPCD reads are fine, > even if I wait a significant amount of time between the reads, as long > as the monitor didn't do a power on->off cycle in between. So it looks > like it's always just the first read after power down that gets > corrupted. > > Now I think I'll go and test how writes behave, assuming I can find a > decently sized chunk of DPCD address space I can write. And maybe I > should also try i2c-over-aux... The first DPCD write after powerdown also got corrupted. But i2c-over-aux seems unaffected for whatever reason. > > > > > > - > > > - for (i = 0; i < 3; i++) { > > > - ret = drm_dp_dpcd_read(aux, offset, buffer, size); > > > - if (ret == size) > > > - return ret; > > > - msleep(1); > > > - } > > > - > > > - return ret; > > > -} > > > - > > > -/* > > > * Fetch AUX CH registers 0x202 - 0x207 which contain > > > * link status information > > > */ > > > bool > > > intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]) > > > { > > > - return intel_dp_dpcd_read_wake(&intel_dp->aux, > > > - DP_LANE0_1_STATUS, > > > - link_status, > > > - DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE; > > > + return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status, > > > + DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE; > > > } > > > > > > /* These are source-specific values. */ > > > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > uint8_t rev; > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd, > > > - sizeof(intel_dp->dpcd)) < 0) > > > + if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd, > > > + sizeof(intel_dp->dpcd)) < 0) > > > return false; /* aux transfer failed */ > > > > > > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); > > > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > /* Check if the panel supports PSR */ > > > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); > > > if (is_edp(intel_dp)) { > > > - intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT, > > > - intel_dp->psr_dpcd, > > > - sizeof(intel_dp->psr_dpcd)); > > > + drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT, > > > + intel_dp->psr_dpcd, > > > + sizeof(intel_dp->psr_dpcd)); > > > if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { > > > dev_priv->psr.sink_support = true; > > > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > > > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > uint8_t frame_sync_cap; > > > > > > dev_priv->psr.sink_support = true; > > > - intel_dp_dpcd_read_wake(&intel_dp->aux, > > > - DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP, > > > - &frame_sync_cap, 1); > > > + drm_dp_dpcd_read(&intel_dp->aux, > > > + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP, > > > + &frame_sync_cap, 1); > > > dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false; > > > /* PSR2 needs frame sync as well */ > > > dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync; > > > @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > /* Intermediate frequency support */ > > > if (is_edp(intel_dp) && > > > (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && > > > - (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) && > > > + (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) && > > > (rev >= 0x03)) { /* eDp v1.4 or higher */ > > > __le16 sink_rates[DP_MAX_SUPPORTED_RATES]; > > > int i; > > > > > > - intel_dp_dpcd_read_wake(&intel_dp->aux, > > > - DP_SUPPORTED_LINK_RATES, > > > - sink_rates, > > > - sizeof(sink_rates)); > > > + drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES, > > > + sink_rates, sizeof(sink_rates)); > > > > > > for (i = 0; i < ARRAY_SIZE(sink_rates); i++) { > > > int val = le16_to_cpu(sink_rates[i]); > > > @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > if (intel_dp->dpcd[DP_DPCD_REV] == 0x10) > > > return true; /* no per-port downstream info */ > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0, > > > - intel_dp->downstream_ports, > > > - DP_MAX_DOWNSTREAM_PORTS) < 0) > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0, > > > + intel_dp->downstream_ports, > > > + DP_MAX_DOWNSTREAM_PORTS) < 0) > > > return false; /* downstream port status fetch failed */ > > > > > > return true; > > > @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp) > > > if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT)) > > > return; > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3) > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3) > > > DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n", > > > buf[0], buf[1], buf[2]); > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3) > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3) > > > DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n", > > > buf[0], buf[1], buf[2]); > > > } > > > @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp) > > > if (intel_dp->dpcd[DP_DPCD_REV] < 0x12) > > > return false; > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) { > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) { > > > if (buf[0] & DP_MST_CAP) { > > > DRM_DEBUG_KMS("Sink is MST capable\n"); > > > intel_dp->is_mst = true; > > > @@ -4112,7 +4077,7 @@ stop: > > > static bool > > > intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) > > > { > > > - return intel_dp_dpcd_read_wake(&intel_dp->aux, > > > + return drm_dp_dpcd_read(&intel_dp->aux, > > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > > sink_irq_vector, 1) == 1; > > > } > > > @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) > > > { > > > int ret; > > > > > > - ret = intel_dp_dpcd_read_wake(&intel_dp->aux, > > > + ret = drm_dp_dpcd_read(&intel_dp->aux, > > > DP_SINK_COUNT_ESI, > > > sink_irq_vector, 14); > > > if (ret != 14) > > > @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > > > intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { > > > uint8_t reg; > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT, > > > ®, 1) < 0) > > > return connector_status_unknown; > > > > > > -- > > > 2.5.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrj?l? > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrj?l? > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrj?l? Intel OTC