Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933789AbcCRP4i (ORCPT ); Fri, 18 Mar 2016 11:56:38 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35901 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933569AbcCRP4d (ORCPT ); Fri, 18 Mar 2016 11:56:33 -0400 Date: Fri, 18 Mar 2016 16:57:21 +0100 From: Daniel Vetter To: Jani Nikula Cc: Lyude , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, David Airlie , Daniel Vetter , arthur.j.runyan@intel.com, open list Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake() Message-ID: <20160318155720.GG14170@phenom.ffwll.local> Mail-Followup-To: Jani Nikula , Lyude , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, David Airlie , Daniel Vetter , arthur.j.runyan@intel.com, open list References: <1458229245-8634-1-git-send-email-cpaul@redhat.com> <1458229245-8634-2-git-send-email-cpaul@redhat.com> <877fh1htta.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <877fh1htta.fsf@intel.com> X-Operating-System: Linux phenom 4.3.0-1-amd64 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: 8460 Lines: 212 On Thu, Mar 17, 2016 at 07:56:33PM +0200, Jani Nikula wrote: > On Thu, 17 Mar 2016, 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); > > Last time around I dug out the commit adding the line above, which fixed > a bug, and said it should be accounted for somehow. But I've had a > change of heart. I want this function gone already. We've improved the > dp helpers considerably, and if this regresses, we need to look at > fixing this either at the drm level or in our ->transfer hook. > > Acked-by: Jani Nikula Hm, my idea was actually to 1:1 move intel_dp_dpcd_read_wake logic into drm_dp_dpcd_read, without any functional change for i915.ko. And everyone else shouldn't complain about a few more retries either. That would also address Ville's grumpy nack. -Daniel > > > > - > > - 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; > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch