Re: [Intel-gfx] [PATCH] drm/i915: Protect DDI port to DPLL map from theoretical race.
On Fri, Dec 15, 2017 at 2:35 PM, Rodrigo Vivi wrote:
> In case we have multiple modesets for different connectors
> happening in parallel we could have a race on the RMW on these
> shared registers.
>
> This possibility was initially raised by Paulo when reviewing
> commit '555e38d27317 ("drm/i915/cnl: DDI - PLL mapping")'
> but the original possibility comes from commit '5416d871136d
> ("drm/i915/skl: Set the eDP link rate on DPLL0")'. Or maybe
> later when atomic commits entered into picture.
>
> Apparently the discussion around this topic showed that the
> right solution would be on serializing the atomic commits in
> a way that we don't have the possibility of races here since
> if that parallel modeset happenings apparently many other
> things will be on fire.
>
> Code is there since SKL and there was no report of issue,
> but since we never looked back to that serialization possibility,
> and also we don't have an igt case for that it is better to at
> least protect this corner.
>
> Suggested-by: Paulo Zanoni
> Fixes: 555e38d27317 ("drm/i915/cnl: DDI - PLL mapping")
> Fixes: 5416d871136d ("drm/i915/skl: Set the eDP link rate on DPLL0")
> Cc: Paulo Zanoni
> Cc: Ville Syrjälä
> Cc: Maarten Lankhorst maarten.lankho...@linux.intel.com
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 8467a797a70b..eb816ff2fa77 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2095,6 +2095,8 @@ static void intel_ddi_clk_select(struct intel_encoder
> *encoder,
Shouldn't we also do the same in intel_ddi_clk_disable?
> if (WARN_ON(!pll))
> return;
>
> +mutex_lock(&dev_priv->dpll_lock);
> +
> if (IS_CANNONLAKE(dev_priv)) {
> /* Configure DPCLKA_CFGCR0 to map the DPLL to the DDI. */
> val = I915_READ(DPCLKA_CFGCR0);
> @@ -2124,6 +2126,8 @@ static void intel_ddi_clk_select(struct intel_encoder
> *encoder,
> } else if (INTEL_INFO(dev_priv)->gen < 9) {
> I915_WRITE_LOG(PORT_CLK_SEL(port),
> hsw_pll_to_ddi_pll_sel(pll));
> }
> +
> + mutex_unlock(&dev_priv->dpll_lock);
> }
>
> static void intel_ddi_clk_disable(struct intel_encoder *encoder)
> --
> 2.13.6
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> Print connector name in destroy_connect() and this doesn't add any extra
> lines to dmesg. The debug macro has been moved before the unregister
> call so that we don't lose the connector name and id.
>
> Signed-off-by: Dhinakaran Pandiyan
Reviewed-by: James Ausmus
While I was looking around here, though, I noticed we don't currently
handle error returns from the drm layer calls in
intel_dp_add_mst_connector. I'll try to send a patch along tomorrow
for that.
> ---
> drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8e3aad0ea60b..88d1d2b9ac56 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct
> drm_dp_mst_topology_mgr *mgr,
> struct intel_connector *intel_connector =
> to_intel_connector(connector);
> struct drm_i915_private *dev_priv = to_i915(connector->dev);
>
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> connector->name);
> drm_connector_unregister(connector);
>
> if (dev_priv->fbdev)
> @@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct
> drm_dp_mst_topology_mgr *mgr,
> drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
>
> drm_connector_unreference(connector);
> - DRM_DEBUG_KMS("\n");
> }
>
> static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/9] drm/i915/dp: Avoid double-printing esi regs
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> We debug log the esi dpcd's twice after reading them back, which is not
> necessary.
>
> Signed-off-by: Dhinakaran Pandiyan
While you're cleaning this up, why not move the do_again label beneath
the if - the only time the goto happens is immediately after you've
already checked if (bret == true), so no need to check again.
Either way -
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_dp.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2886a2ef1591..aab9ba31f79e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4224,10 +4224,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> }
>
> bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> - if (bret == true) {
> - DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> + if (bret == true)
> goto go_again;
> - }
> } else {
> ret = 0;
> }
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/i915/mst: Print active mst links after update
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> Both mst_disable_dp and mst_post_disable_dp print number of active links
> before the variable has been updated. Move the print statement in
> mst_disable_dp after the decrement so that the printed values indicate
> the disabing of a mst connector. Also, add some text to clarify what we
> are printing.
>
> Signed-off-by: Dhinakaran Pandiyan
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_dp_mst.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 88d1d2b9ac56..9a396f483f8b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -133,7 +133,7 @@ static void intel_mst_disable_dp(struct intel_encoder
> *encoder,
> to_intel_connector(old_conn_state->connector);
> int ret;
>
> - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> + DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);
>
> @@ -155,8 +155,6 @@ static void intel_mst_post_disable_dp(struct
> intel_encoder *encoder,
> struct intel_connector *connector =
> to_intel_connector(old_conn_state->connector);
>
> - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> -
> /* this can fail */
> drm_dp_check_act_status(&intel_dp->mst_mgr);
> /* and this can also fail */
> @@ -173,6 +171,7 @@ static void intel_mst_post_disable_dp(struct
> intel_encoder *encoder,
>
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> }
> + DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> }
>
> static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> @@ -195,7 +194,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder
> *encoder,
> connector->encoder = encoder;
> intel_mst->connector = connector;
>
> - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> + DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> if (intel_dp->active_mst_links == 0)
> intel_dig_port->base.pre_enable(&intel_dig_port->base,
> @@ -229,7 +228,7 @@ static void intel_mst_enable_dp(struct intel_encoder
> *encoder,
> enum port port = intel_dig_port->port;
> int ret;
>
> - DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> + DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> if (intel_wait_for_register(dev_priv,
> DP_TP_STATUS(port),
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/9] drm/i915/dp: Fix buffer size for sink_irq_esi read
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> The buffer size defined is 16 bytes whereas only 14 bytes are read. Add a
> macro to avoid this discrepancy.
>
> Signed-off-by: Dhinakaran Pandiyan
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_dp.c | 15 +--
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 887953c0f495..98e7b96ca826 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -42,6 +42,7 @@
> #include "i915_drv.h"
>
> #define DP_LINK_CHECK_TIMEOUT (10 * 1000)
> +#define DP_DPRX_ESI_LEN 14
>
> /* Compliance test status bits */
> #define INTEL_DP_RESOLUTION_SHIFT_MASK 0
> @@ -3991,15 +3992,9 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8
> *sink_irq_vector)
> static bool
> intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> {
> - int ret;
> -
> - ret = drm_dp_dpcd_read(&intel_dp->aux,
> -DP_SINK_COUNT_ESI,
> -sink_irq_vector, 14);
> - if (ret != 14)
> - return false;
> -
> - return true;
> + return drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT_ESI,
> + sink_irq_vector, DP_DPRX_ESI_LEN) ==
> + DP_DPRX_ESI_LEN;
> }
>
> static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> @@ -4199,7 +4194,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> bool bret;
>
> if (intel_dp->is_mst) {
> - u8 esi[16] = { 0 };
> + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> int ret = 0;
> int retry;
> bool handled;
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915/dp: Handle check_mst_status() failure in one place
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> The caller already has code to handle failure, no need to duplicate
> that.
>
> Signed-off-by: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3f2ca10ccbcd..2886a2ef1591 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> ret = 0;
> }
> } else {
> - struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -
> DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> - intel_dp->is_mst = false;
> - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> - intel_dp->is_mst);
> - drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
It looks like intel_dp_hpd_pulse doesn't call
drm_kms_helper_hotplug_event on a failure in
intel_dp_check_mst_status, so we would lose that path with this patch
- do we need that?
> ret = -EINVAL;
> }
> return ret;
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> The other instances of link training are protected with
> connection_mutex, so do the same in check_mst_status() too.
We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
- is it needed in all instances?
>
> Signed-off-by: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/i915/intel_dp.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aab9ba31f79e..644463ba313e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> static int
> intel_dp_check_mst_status(struct intel_dp *intel_dp)
> {
> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> bool bret;
> u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> int ret = 0;
> @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> if (intel_dp->active_mst_links &&
> !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> intel_dp_start_link_train(intel_dp);
> intel_dp_stop_link_train(intel_dp);
> +
> drm_modeset_unlock(&dev->mode_config.connection_mutex);
> }
>
> DRM_DEBUG_KMS("got esi %3ph\n", esi);
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()
On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
wrote:
> intel_dp_can_mst() performs these checks.
>
> Signed-off-by: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 644463ba313e..a4465b46bb27 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
> static void
> intel_dp_configure_mst(struct intel_dp *intel_dp)
> {
> - if (!i915.enable_dp_mst)
> - return;
> -
> - if (!intel_dp->can_mst)
> - return;
> -
By dropping these returns, we will now always get DRM_DEBUG_KMS output
on whether the sink is MST capable, even if the i915.enable_dp_mst
param is false - maybe drop these from intel_dp_can_mst instead?
> intel_dp->is_mst = intel_dp_can_mst(intel_dp);
>
> if (intel_dp->is_mst)
> --
> 2.11.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
On Fri, Sep 15, 2017 at 3:18 AM, Ville Syrjälä
wrote:
> On Thu, Sep 14, 2017 at 12:59:35PM -0700, James Ausmus wrote:
>> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
>> Add intel_connector_destroy to support cleanup on the error path.
>
> That name makes one think that it could be plugged into the connector
> .destroy() hook. So I'd rename it to intel_connector_free() or something
> like that.
Sounds good.
>
> Also the this MST thing just looks like the tip of the iceberg.
> I think pretty much every connector has the same problem. But I
> guess MST is slightly more interesting since it can happen at
> runtime.
Yeah, I'll look at fixing up the other connectors too, as I can.
>
>>
>> Signed-off-by: James Ausmus
>> ---
>>
>> Note that this patch is compile/boot tested only on non-MST, as I
>> currently don't have MST-enabled HW.
>>
>> drivers/gpu/drm/i915/intel_display.c | 10 ++
>> drivers/gpu/drm/i915/intel_dp_mst.c | 24
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> 3 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 8599e425abb1..ab261c063032 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>> return connector;
>> }
>>
>> +/*
>> + * Free the bits allocated by intel_connector_alloc.
>> + * Type-specific connector cleanup should be done prior to this.
>> + */
>> +void intel_connector_destroy(struct intel_connector *connector)
>> +{
>> + kfree(to_intel_digital_connector_state(connector->base.state));
>> + kfree(connector);
>> +}
>> +
>> /* Simple connector->get_hw_state implementation for encoders that support
>> only
>> * one connector and no cloning and hence the encoder state determines the
>> state
>> * of the connector. */
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
>> b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index 8e3aad0ea60b..80b3d665e988 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -451,14 +451,18 @@ static struct drm_connector
>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>> struct drm_device *dev = intel_dig_port->base.base.dev;
>> struct intel_connector *intel_connector;
>> struct drm_connector *connector;
>> - int i;
>> + int i, ret;
>>
>> intel_connector = intel_connector_alloc();
>> if (!intel_connector)
>> return NULL;
>>
>> connector = &intel_connector->base;
>> - drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> DRM_MODE_CONNECTOR_DisplayPort);
>> + ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> + DRM_MODE_CONNECTOR_DisplayPort);
>> + if (ret)
>> + goto out_init;
>> +
>> drm_connector_helper_add(connector,
>> &intel_dp_mst_connector_helper_funcs);
>>
>> intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
>> @@ -466,14 +470,26 @@ static struct drm_connector
>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>> intel_connector->port = port;
>>
>> for (i = PIPE_A; i <= PIPE_C; i++) {
>> - drm_mode_connector_attach_encoder(&intel_connector->base,
>> + ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>>
>> &intel_dp->mst_encoders[i]->base.base);
>> + if (ret)
>> + goto out_conn;
>> }
>>
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.path_property, 0);
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.tile_property, 0);
>>
>> - drm_mode_connector_set_path_property(connector, pathprop);
>> + ret = drm_mode_connector_set_path_property(connector, pathprop);
>> +
>
> 'return connector' here so you can skip the 'if (ret)' things below?
Will do.
Thanks for the review!
-James
>
>> +out_conn:
>> + if (ret)
>> + drm_connector_cleanup(connector);
>> +out_init:
>> + if (ret) {
>> + intel_connector_destroy(intel_connector);
>> + connector = NULL;
>> + }
>> +
>> return connector;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 307807672896..d13c5c6b46e9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private
>> *dev_priv);
>> void intel_encoder_destroy(struct drm_encoder *encoder);
>> int intel_connector_init(struct intel_connector *);
>> struct intel_connector *intel_connector_alloc(void);
>> +void intel_connector_destroy(struct intel_connector *connector);
>> bool intel_connector_get_hw_state(stru
Re: [Intel-gfx] [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex
On Fri, Sep 15, 2017 at 11:38 AM, Manasi Navare
wrote:
> On Thu, Sep 14, 2017 at 03:26:37PM -0700, Ausmus, James wrote:
>> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
>> wrote:
>> > The other instances of link training are protected with
>> > connection_mutex, so do the same in check_mst_status() too.
>>
>> We don't seem to be taking connection_mutex around
>> intel_dp_start/stop_link_train in the intel_enable_dp or
>> intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
>> - is it needed in all instances?
>>
>
> intel_ddi_pre_enable_dp() gets called from the crtc_enable hook higher
> up in the stack which gets called essentially from atomic_commit() hook
> that traces back eventually to the drm_mode_setcrtc call that grabs the
> mod_config mutex there: mutex_lock(&crtc->dev->mode_config.mutex);
Got it - thanks for the pointer!
-James
>
> Manasi
>
>> >
>> > Signed-off-by: Dhinakaran Pandiyan
>> > ---
>> > drivers/gpu/drm/i915/intel_dp.c | 4
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > index aab9ba31f79e..644463ba313e 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct
>> > intel_dp *intel_dp)
>> > static int
>> > intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> > {
>> > + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> > bool bret;
>> > u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> > int ret = 0;
>> > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> > if (intel_dp->active_mst_links &&
>> > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count))
>> > {
>> > DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
>> > +
>> > +
>> > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> > intel_dp_start_link_train(intel_dp);
>> > intel_dp_stop_link_train(intel_dp);
>> > +
>> > drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> > }
>> >
>> > DRM_DEBUG_KMS("got esi %3ph\n", esi);
>> > --
>> > 2.11.0
>> >
>> > ___
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>>
>>
>> James Ausmus
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
On Fri, Sep 15, 2017 at 11:09 AM, Ville Syrjälä
wrote:
> On Fri, Sep 15, 2017 at 10:49:09AM -0700, James Ausmus wrote:
>> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
>> Add intel_connector_free to support cleanup on the error path.
>>
>> v2: Rename new function to avoid confusion, and simplify error
>> paths (Ville)
>>
>> Signed-off-by: James Ausmus
>> ---
>>
>> Note that this patch is compile/boot tested only on non-MST, as I
>> currently don't have MST-enabled HW.
>>
>> drivers/gpu/drm/i915/intel_display.c | 10 ++
>> drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>> 3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 8599e425abb1..e3b1a6ead4fb 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6146,6 +6146,16 @@ struct intel_connector *intel_connector_alloc(void)
>> return connector;
>> }
>>
>> +/*
>> + * Free the bits allocated by intel_connector_alloc.
>> + * Type-specific connector cleanup should be done prior to this.
>> + */
>> +void intel_connector_free(struct intel_connector *connector)
>> +{
>> + kfree(to_intel_digital_connector_state(connector->base.state));
>> + kfree(connector);
>> +}
>> +
>> /* Simple connector->get_hw_state implementation for encoders that support
>> only
>> * one connector and no cloning and hence the encoder state determines the
>> state
>> * of the connector. */
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
>> b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index 8e3aad0ea60b..6e4447cbbe0e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -451,14 +451,18 @@ static struct drm_connector
>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>> struct drm_device *dev = intel_dig_port->base.base.dev;
>> struct intel_connector *intel_connector;
>> struct drm_connector *connector;
>> - int i;
>> + int i, ret;
>>
>> intel_connector = intel_connector_alloc();
>> if (!intel_connector)
>> return NULL;
>>
>> connector = &intel_connector->base;
>> - drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> DRM_MODE_CONNECTOR_DisplayPort);
>> + ret = drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
>> + DRM_MODE_CONNECTOR_DisplayPort);
>> + if (ret)
>> + goto out_init;
>> +
>> drm_connector_helper_add(connector,
>> &intel_dp_mst_connector_helper_funcs);
>>
>> intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
>> @@ -466,14 +470,25 @@ static struct drm_connector
>> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>> intel_connector->port = port;
>>
>> for (i = PIPE_A; i <= PIPE_C; i++) {
>
> BTW a followup patch to switch these mst_encoders[] loops over
> to for_each_pipe() might be nice.
I'll queue that one up as well :)
>
>> - drm_mode_connector_attach_encoder(&intel_connector->base,
>> + ret = drm_mode_connector_attach_encoder(&intel_connector->base,
>>
>> &intel_dp->mst_encoders[i]->base.base);
>
> Please reindent appropriately.
Done
>
>> + if (ret)
>> + goto out_conn;
>> }
>>
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.path_property, 0);
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.tile_property, 0);
>>
>> - drm_mode_connector_set_path_property(connector, pathprop);
>> + ret = drm_mode_connector_set_path_property(connector, pathprop);
>> + if (ret == 0)
>> + return connector;
>
> if (ret)
> goto ...;
>
> return connector;
>
> is more idiomatic.
Done
>
>> +
>> +out_conn:
>> + drm_connector_cleanup(connector);
>> +out_init:
>> + intel_connector_free(intel_connector);
>
> Hmm. I might call these out_cleanup and out_free. But I guess we have no
> consistent style when it comes to goto labes. So feel free to ignore me.
Sounds good to me - done :)
>
>> + connector = NULL;
>> +
>> return connector;
>
> Just return NULL.
Done
Thanks for the review!
>
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 307807672896..2a5cee4302f7 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1358,6 +1358,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private
>> *dev_priv);
>> void intel_encoder_destroy(struct drm_encoder *encoder);
>> int intel_connector_init(struct intel_connector *);
>> struct intel_connector *intel_connector_alloc(void);
>> +void intel_connector_free(struct intel_connector *connector);
>> bool intel_connector_get_hw_state(struct i
Re: [Intel-gfx] [PATCH 4/5] drm/i915/dp: Clean up intel_dp_check_mst_status
On Mon, Sep 18, 2017 at 3:21 PM, Dhinakaran Pandiyan
wrote:
> Rewriting this code without the goto, I believe, makes it more readable.
> One functional change that has been included is the handling of failed ESI
> register reads. Instead of disabling MST only for the first failed read, we
> now disable MST on subsequent failed reads too. A failed ESI read is
> problematic irrespective of whether it is the first or not.
>
> Cc: James Ausmus
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Signed-off-by: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/i915/intel_dp.c | 75
> +
> 1 file changed, 31 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 98e7b96ca826..cc129aa444ac 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4191,57 +4191,44 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> static int
> intel_dp_check_mst_status(struct intel_dp *intel_dp)
> {
> - bool bret;
> + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>
> - if (intel_dp->is_mst) {
> - u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> - int ret = 0;
> - int retry;
> + if (!intel_dp->is_mst)
> + return -EINVAL;
> +
> + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) {
It looks like if the underlying drm_dp_dpcd_read fails and returns
-EIO, for instance, you'll get true back from
intel_dp_get_sink_irq_esi, and you'll still go in to the while, but
with a potentially invalid esi. Granted, this is a problem in the
original code as well, but it seems like something that should be
fixed during the refactoring.
> + int ret, retry;
> bool handled;
> - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -go_again:
> - if (bret == true) {
> -
> - /* check link status - esi[10] = 0x200c */
> - if (intel_dp->active_mst_links &&
> - !drm_dp_channel_eq_ok(&esi[10],
> intel_dp->lane_count)) {
> - DRM_DEBUG_KMS("channel EQ not ok,
> retraining\n");
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> - }
>
> - DRM_DEBUG_KMS("got esi %3ph\n", esi);
> - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi,
> &handled);
> -
> - if (handled) {
> - for (retry = 0; retry < 3; retry++) {
> - int wret;
> - wret =
> drm_dp_dpcd_write(&intel_dp->aux,
> -
> DP_SINK_COUNT_ESI+1,
> -&esi[1], 3);
> - if (wret == 3) {
> - break;
> - }
> - }
> + DRM_DEBUG_KMS("ESI %3ph\n", esi);
>
> - bret = intel_dp_get_sink_irq_esi(intel_dp,
> esi);
> - if (bret == true) {
> - DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> - goto go_again;
> - }
> - } else
> - ret = 0;
> + /* check link status - esi[10] = 0x200c */
> + if (intel_dp->active_mst_links &&
> + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> + intel_dp_start_link_train(intel_dp);
> + intel_dp_stop_link_train(intel_dp);
> + }
>
> - return ret;
> - } else {
> - struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> - DRM_DEBUG_KMS("failed to get ESI - device may have
> failed\n");
> - intel_dp->is_mst = false;
> - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> intel_dp->is_mst);
> - /* send a hotplug event */
> -
> drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
You're no longer using the value returned by drm_dp_mst_hpd_irq
> + if (!handled)
> + return 0;
> +
> + for (retry = 0; retry < 3; retry++) {
> + int wret;
> +
> + wret = drm_dp_dpcd_write(&intel_dp->aux,
> +
Re: [Intel-gfx] [PATCH 4/5] drm/i915/dp: Clean up intel_dp_check_mst_status
On Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran
wrote:
> On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote:
>> On Mon, Sep 18, 2017 at 3:21 PM, Dhinakaran Pandiyan
>> wrote:
>> > Rewriting this code without the goto, I believe, makes it more readable.
>> > One functional change that has been included is the handling of failed ESI
>> > register reads. Instead of disabling MST only for the first failed read, we
>> > now disable MST on subsequent failed reads too. A failed ESI read is
>> > problematic irrespective of whether it is the first or not.
>> >
>> > Cc: James Ausmus
>> > Cc: Jani Nikula
>> > Cc: Ville Syrjälä
>> > Signed-off-by: Dhinakaran Pandiyan
>> > ---
>> > drivers/gpu/drm/i915/intel_dp.c | 75
>> > +
>> > 1 file changed, 31 insertions(+), 44 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > index 98e7b96ca826..cc129aa444ac 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -4191,57 +4191,44 @@ static void intel_dp_handle_test_request(struct
>> > intel_dp *intel_dp)
>> > static int
>> > intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> > {
>> > - bool bret;
>> > + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> > + struct intel_digital_port *intel_dig_port =
>> > dp_to_dig_port(intel_dp);
>> >
>> > - if (intel_dp->is_mst) {
>> > - u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> > - int ret = 0;
>> > - int retry;
>> > + if (!intel_dp->is_mst)
>> > + return -EINVAL;
>> > +
>> > + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) {
>>
>> It looks like if the underlying drm_dp_dpcd_read fails and returns
>> -EIO, for instance, you'll get true back from
>> intel_dp_get_sink_irq_esi,
>
> Wait, anything other than 14 from that dpcd read is a false, isn't it?
D'oh! You're right - I completely glossed over the whole " ==
DP_DPRX_ESI_LEN" bit - sorry for the noise...
>
>> and you'll still go in to the while, but
>> with a potentially invalid esi. Granted, this is a problem in the
>> original code as well, but it seems like something that should be
>> fixed during the refactoring.
>>
>>
>> > + int ret, retry;
>> > bool handled;
>> > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>> > -go_again:
>> > - if (bret == true) {
>> > -
>> > - /* check link status - esi[10] = 0x200c */
>> > - if (intel_dp->active_mst_links &&
>> > - !drm_dp_channel_eq_ok(&esi[10],
>> > intel_dp->lane_count)) {
>> > - DRM_DEBUG_KMS("channel EQ not ok,
>> > retraining\n");
>> > - intel_dp_start_link_train(intel_dp);
>> > - intel_dp_stop_link_train(intel_dp);
>> > - }
>> >
>> > - DRM_DEBUG_KMS("got esi %3ph\n", esi);
>> > - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi,
>> > &handled);
>> > -
>> > - if (handled) {
>> > - for (retry = 0; retry < 3; retry++) {
>> > - int wret;
>> > - wret =
>> > drm_dp_dpcd_write(&intel_dp->aux,
>> > -
>> > DP_SINK_COUNT_ESI+1,
>> > -&esi[1],
>> > 3);
>> > - if (wret == 3) {
>> > - break;
>> > - }
>> > - }
>> > + DRM_DEBUG_KMS("ESI %3ph\n", esi);
>> >
>> > - bret = intel_dp_get_sink_irq_esi(intel_dp,
>> > esi);
>> > - if (bret == true) {
>> > - DRM_DEBUG_KMS("got esi2
Re: [Intel-gfx] [PATCH 4/5] drm/i915/dp: Clean up intel_dp_check_mst_status
On Wed, Sep 20, 2017 at 3:47 PM, Pandiyan, Dhinakaran
wrote:
> On Wed, 2017-09-20 at 13:02 -0700, Ausmus, James wrote:
>> On Wed, Sep 20, 2017 at 12:55 PM, Pandiyan, Dhinakaran
>> wrote:
>> > On Wed, 2017-09-20 at 12:11 -0700, Ausmus, James wrote:
>> >> On Mon, Sep 18, 2017 at 3:21 PM, Dhinakaran Pandiyan
>> >> wrote:
>> >> > Rewriting this code without the goto, I believe, makes it more readable.
>> >> > One functional change that has been included is the handling of failed
>> >> > ESI
>> >> > register reads. Instead of disabling MST only for the first failed
>> >> > read, we
>> >> > now disable MST on subsequent failed reads too. A failed ESI read is
>> >> > problematic irrespective of whether it is the first or not.
>> >> >
>> >> > Cc: James Ausmus
>> >> > Cc: Jani Nikula
>> >> > Cc: Ville Syrjälä
>> >> > Signed-off-by: Dhinakaran Pandiyan
>> >> > ---
>> >> > drivers/gpu/drm/i915/intel_dp.c | 75
>> >> > +
>> >> > 1 file changed, 31 insertions(+), 44 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> >> > b/drivers/gpu/drm/i915/intel_dp.c
>> >> > index 98e7b96ca826..cc129aa444ac 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> > @@ -4191,57 +4191,44 @@ static void intel_dp_handle_test_request(struct
>> >> > intel_dp *intel_dp)
>> >> > static int
>> >> > intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> >> > {
>> >> > - bool bret;
>> >> > + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> >> > + struct intel_digital_port *intel_dig_port =
>> >> > dp_to_dig_port(intel_dp);
>> >> >
>> >> > - if (intel_dp->is_mst) {
>> >> > - u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> >> > - int ret = 0;
>> >> > - int retry;
>> >> > + if (!intel_dp->is_mst)
>> >> > + return -EINVAL;
>> >> > +
>> >> > + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) {
>> >>
>> >> It looks like if the underlying drm_dp_dpcd_read fails and returns
>> >> -EIO, for instance, you'll get true back from
>> >> intel_dp_get_sink_irq_esi,
>> >
>> > Wait, anything other than 14 from that dpcd read is a false, isn't it?
>>
>> D'oh! You're right - I completely glossed over the whole " ==
>> DP_DPRX_ESI_LEN" bit - sorry for the noise...
>>
>> >
>> >> and you'll still go in to the while, but
>> >> with a potentially invalid esi. Granted, this is a problem in the
>> >> original code as well, but it seems like something that should be
>> >> fixed during the refactoring.
>> >>
>> >>
>> >> > + int ret, retry;
>> >> > bool handled;
>> >> > - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>> >> > -go_again:
>> >> > - if (bret == true) {
>> >> > -
>> >> > - /* check link status - esi[10] = 0x200c */
>> >> > - if (intel_dp->active_mst_links &&
>> >> > - !drm_dp_channel_eq_ok(&esi[10],
>> >> > intel_dp->lane_count)) {
>> >> > - DRM_DEBUG_KMS("channel EQ not ok,
>> >> > retraining\n");
>> >> > - intel_dp_start_link_train(intel_dp);
>> >> > - intel_dp_stop_link_train(intel_dp);
>> >> > - }
>> >> >
>> >> > - DRM_DEBUG_KMS("got esi %3ph\n", esi);
>> >> > - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr,
>> >> > esi, &handled);
>> >> > -
>> >> > - if (handled) {
>> >> > - for (retry = 0; retry < 3; retry++) {
>> >> > -
Re: [Intel-gfx] [PATCH v2] drm/i915/dp: Clean up intel_dp_check_mst_status
On Thu, Sep 21, 2017 at 11:54 AM, Dhinakaran Pandiyan
wrote:
> Rewriting this code without the goto, I believe, makes it more readable.
> One functional change that has been included is the handling of failed ESI
> register reads. Instead of disabling MST only for the first failed read, we
> now disable MST on subsequent failed reads too. A failed ESI read is
> problematic irrespective of whether it is the first or not.
>
> v2: Don't ignore return from _mst_hpd_irq() (James)
>
> Cc: James Ausmus
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Signed-off-by: Dhinakaran Pandiyan
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_dp.c | 78
> ++---
> 1 file changed, 34 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 98e7b96ca826..aa97bd825369 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4191,57 +4191,47 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> static int
> intel_dp_check_mst_status(struct intel_dp *intel_dp)
> {
> - bool bret;
> + u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>
> - if (intel_dp->is_mst) {
> - u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> - int ret = 0;
> - int retry;
> + if (!intel_dp->is_mst)
> + return -EINVAL;
> +
> + while (intel_dp_get_sink_irq_esi(intel_dp, esi)) {
> + int ret, retry;
> bool handled;
> - bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -go_again:
> - if (bret == true) {
> -
> - /* check link status - esi[10] = 0x200c */
> - if (intel_dp->active_mst_links &&
> - !drm_dp_channel_eq_ok(&esi[10],
> intel_dp->lane_count)) {
> - DRM_DEBUG_KMS("channel EQ not ok,
> retraining\n");
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> - }
>
> - DRM_DEBUG_KMS("got esi %3ph\n", esi);
> - ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi,
> &handled);
> -
> - if (handled) {
> - for (retry = 0; retry < 3; retry++) {
> - int wret;
> - wret =
> drm_dp_dpcd_write(&intel_dp->aux,
> -
> DP_SINK_COUNT_ESI+1,
> -&esi[1], 3);
> - if (wret == 3) {
> - break;
> - }
> - }
> + DRM_DEBUG_KMS("ESI %3ph\n", esi);
>
> - bret = intel_dp_get_sink_irq_esi(intel_dp,
> esi);
> - if (bret == true) {
> - DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> - goto go_again;
> - }
> - } else
> - ret = 0;
> + /* check link status - esi[10] = 0x200c */
> + if (intel_dp->active_mst_links &&
> + !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> + intel_dp_start_link_train(intel_dp);
> + intel_dp_stop_link_train(intel_dp);
> + }
>
> - return ret;
> - } else {
> - struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> - DRM_DEBUG_KMS("failed to get ESI - device may have
> failed\n");
> - intel_dp->is_mst = false;
> - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> intel_dp->is_mst);
> - /* send a hotplug event */
> -
> drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> + ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> + if (!handled)
> + return 0;
> +
> + if (ret)
> + DRM_DEBUG_KMS("error handling MST IRQ_HPD %d\n", ret);
> +
> + for (retry = 0; retry < 3; retry++) {
> + int wret;
> +
> + wret = drm_dp_dpcd_write(&intel_dp->aux,
> +DP_SINK_COUNT_ESI + 1,
> &esi[1],
> +3);
> + if (wret == 3)
> + break;
> }
>
Re: [Intel-gfx] [PATCH i-g-t] Fix compilation on some distros
On Thu, Sep 28, 2017 at 1:40 AM, Petri Latvala wrote: > On Wed, Sep 27, 2017 at 04:08:27PM -0700, James Ausmus wrote: >> Some distros (such as Gentoo) are removing the include of >> sys/sysmacros.h from sys/types.h. Explicitly include sysmacros.h in >> files where we use the minor() and major() functions. >> >> Signed-off-by: James Ausmus > > Reviewed-by: Petri Latvala > Thanks for the review! Can you push? I don't have access rights. Thanks! -James > > >> --- >> lib/igt_debugfs.c | 1 + >> lib/igt_sysfs.c | 1 + >> tools/aubdump.c | 1 + >> 3 files changed, 3 insertions(+) >> >> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c >> index 1e8c8cc3cd44..60b29e3a025a 100644 >> --- a/lib/igt_debugfs.c >> +++ b/lib/igt_debugfs.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c >> index 817678bc28ed..f4e306003b01 100644 >> --- a/lib/igt_sysfs.c >> +++ b/lib/igt_sysfs.c >> @@ -24,6 +24,7 @@ >> >> #include >> #include >> +#include >> #include >> #include >> #include >> diff --git a/tools/aubdump.c b/tools/aubdump.c >> index 78d183f49adc..ee4d99b06ed1 100644 >> --- a/tools/aubdump.c >> +++ b/tools/aubdump.c >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> -- >> 2.14.1 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- James Ausmus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/cnl: Fix PLL mapping.
On Tue, Oct 3, 2017 at 3:08 PM, Rodrigo Vivi wrote:
> On PLL Enable sequence we need to "Configure DPCLKA_CFGCR0 to turn on
> the clock for the DDI and map the DPLL to the DDI"
>
> So we first do the map and then we unset DDI_CLK_OFF to turn the clock
> on. We do this in 2 separated steps.
>
> However, on this second step where we should only unset the off bit we are
> also unmapping the ddi from the pll. So we end up using the pll 0
> for almost everything. Consequently breaking cases with more than one
> display.
>
> Fixes: 555e38d27317 ("drm/i915/cnl: DDI - PLL mapping")
> Cc: Paulo Zanoni
> Cc: Manasi Navare
> Cc: Kahola, Mika
> Signed-off-by: Rodrigo Vivi
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 55c43b333d3c..bf8ec0bd349f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2144,8 +2144,7 @@ static void intel_ddi_clk_select(struct intel_encoder
> *encoder,
> * register writes.
> */
> val = I915_READ(DPCLKA_CFGCR0);
> - val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> -DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
> + val &= ~DPCLKA_CFGCR0_DDI_CLK_OFF(port);
> I915_WRITE(DPCLKA_CFGCR0, val);
> } else if (IS_GEN9_BC(dev_priv)) {
> /* DDI -> PLL mapping */
> --
> 2.13.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/cnl: Fix PLL initialization for HDMI.
On Tue, Oct 3, 2017 at 3:08 PM, Rodrigo Vivi wrote:
> HDMI Mode selection on CNL is on CFGCR0 for that PLL, not
> on in a global CTRL1 as it was on SKL.
>
> The original patch addressed this difference, but leaving behind
> this single entry here. So we were checking the wrong bits during
> the PLL initialization and consequently avoiding the CFGCR1 setup
> during HDMI initialization. Luckly when only HDMI was in use BIOS
> had already setup this for us. But the dual display with hot plug
> were messed up.
>
> Fixes: a927c927de34 ("drm/i915/cnl: Initialize PLLs")
> Cc: Paulo Zanoni
> Cc: Manasi Navare
> Cc: Kahola, Mika
> Signed-off-by: Rodrigo Vivi
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 55997389a29f..032fd915e929 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2000,7 +2000,7 @@ static void cnl_ddi_pll_enable(struct drm_i915_private
> *dev_priv,
>
> /* 3. Configure DPLL_CFGCR0 */
> /* Avoid touch CFGCR1 if HDMI mode is not enabled */
> - if (pll->state.hw_state.cfgcr0 & DPLL_CTRL1_HDMI_MODE(pll->id)) {
> + if (pll->state.hw_state.cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> val = pll->state.hw_state.cfgcr1;
> I915_WRITE(CNL_DPLL_CFGCR1(pll->id), val);
> /* 4. Reab back to ensure writes completed */
> --
> 2.13.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/13] drm/i915/cnl: extract cnl_dvfs_{pre, post}_change
On Tue, Oct 3, 2017 at 12:06 AM, Rodrigo Vivi wrote:
> From: Paulo Zanoni
>
> These functions even have their own page in our spec,
> so extract them from cnl_set_cdclk().
>
> v2: (By Rodrigo) Fixed inverted logic on error return of
> cnl_dvfs_pre_change.
>
> Cc: Ville Syrjälä
> Signed-off-by: Paulo Zanoni
> Signed-off-by: Rodrigo Vivi
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 33 +++--
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index 87fc42b19336..b35eb145d66e 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1510,12 +1510,8 @@ static void cnl_cdclk_pll_enable(struct
> drm_i915_private *dev_priv, int vco)
> dev_priv->cdclk.hw.vco = vco;
> }
>
> -static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_state *cdclk_state)
> +static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
> {
> - int cdclk = cdclk_state->cdclk;
> - int vco = cdclk_state->vco;
> - u32 val, divider, pcu_ack;
> int ret;
>
> mutex_lock(&dev_priv->rps.hw_lock);
> @@ -1524,11 +1520,30 @@ static void cnl_set_cdclk(struct drm_i915_private
> *dev_priv,
> SKL_CDCLK_READY_FOR_CHANGE,
> SKL_CDCLK_READY_FOR_CHANGE, 3);
> mutex_unlock(&dev_priv->rps.hw_lock);
> - if (ret) {
> +
> + if (ret)
> DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> ret);
> +
> + return ret;
> +}
> +
> +static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> level)
> +{
> + mutex_lock(&dev_priv->rps.hw_lock);
> + sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
> +static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state)
> +{
> + int cdclk = cdclk_state->cdclk;
> + int vco = cdclk_state->vco;
> + u32 val, divider, pcu_ack;
> +
> + if (cnl_dvfs_pre_change(dev_priv))
> return;
> - }
>
> /* cdclk = vco / 2 / div{1,2} */
> switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
> @@ -1575,9 +1590,7 @@ static void cnl_set_cdclk(struct drm_i915_private
> *dev_priv,
> I915_WRITE(CDCLK_CTL, val);
>
> /* inform PCU of the change */
> - mutex_lock(&dev_priv->rps.hw_lock);
> - sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, pcu_ack);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> + cnl_dvfs_post_change(dev_priv, pcu_ack);
>
> intel_update_cdclk(dev_priv);
> }
> --
> 2.13.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/bdw: Fix DP_AUX_CH_CTL_TIME_OUT setting
On Wed, Oct 4, 2017 at 3:54 PM, Runyan, Arthur J
wrote:
> I think the failure was with one particularly slow eDP panel, but it is safer
> to apply this to all ports.
>
Thanks Art. Anyone else have thoughts/comments on this, or should this
be good for merge now?
Thanks!
-James
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Wednesday, 4 October, 2017 1:25 PM
> To: Ausmus, James
> Cc: intel-gfx@lists.freedesktop.org; ville.syrj...@linux.intel.com;
> jani.nik...@linux.intel.com; Runyan, Arthur J ;
> b...@bwidawsk.net
> Subject: Re: [PATCH v2 2/2] drm/i915/bdw: Fix DP_AUX_CH_CTL_TIME_OUT setting
>
> On Wed, Oct 04, 2017 at 08:09:22PM +, James Ausmus wrote:
>> Per BSpec, 400us is "BDW+ Do not use this setting." - not just PORT_A.
>> Set BDW to 600us unconditionally.
>
> Besides that statement I also found on BSpec:
>
> "
> Workaround
> Project
> BDW, EXCLUDE(CHV)
> Set the Timeout timer value to at least 600us before initiating a transaction.
> "
>
> Also I tracked this on the log and arrived to commit 'a81a507d487c
> ("drm/i915/bdw: Change dp aux timeout to 600us on DDIA")'
>
> It seems during BDW enabling HW team found that need but only for port A
> and later they might have extended it and we never noticed.
>
> Ccin't Art and Ben here to see if they can comment on that.
>
> But I believe we should add this so
>
> Reviewed-by: Rodrigo Vivi
>
>>
>> v2:
>> -Split in to two patches (Rodrigo)
>>
>> Cc: Jani Nikula
>> Cc: Ville Syrjälä
>> Signed-off-by: James Ausmus
>> ---
>> drivers/gpu/drm/i915/intel_dp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index 5b4c9484575b..df301e00d9d9 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1019,7 +1019,7 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp
>> *intel_dp,
>> else
>> precharge = 5;
>>
>> - if (IS_BROADWELL(dev_priv) && intel_dig_port->port == PORT_A)
>> + if (IS_BROADWELL(dev_priv))
>> timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
>> else
>> timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>> --
>> 2.14.1
>>
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector
On Wed, Sep 27, 2017 at 11:29 AM, James Ausmus wrote:
> Make intel_dp_add_mst_connector handle error returns from the drm_ calls.
> Add intel_connector_free to support cleanup on the error path.
>
> v2: Rename new function to avoid confusion, and simplify error
> paths (Ville)
>
> v3: Indentation fixup, style fixes (Ville)
>
> v4: Clarify usage of intel_connector_free, and fix usage of
> intel_connector_free
Ville - any additional issues you see with the latest spin?
Thanks!
-James
>
> Cc: Ville Syrjälä
> Signed-off-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_display.c | 13 +
> drivers/gpu/drm/i915/intel_dp_mst.c | 27 ++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c4b224a3a0ee..725014bf6f0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6146,6 +6146,19 @@ struct intel_connector *intel_connector_alloc(void)
> return connector;
> }
>
> +/*
> + * Free the bits allocated by intel_connector_alloc.
> + * This should only be used after intel_connector_alloc has returned
> + * successfully, and before drm_connector_init returns successfully.
> + * Otherwise the destroy callbacks for the connector and the state should
> + * take care of proper cleanup/free
> + */
> +void intel_connector_free(struct intel_connector *connector)
> +{
> + kfree(to_intel_digital_connector_state(connector->base.state));
> + kfree(connector);
> +}
> +
> /* Simple connector->get_hw_state implementation for encoders that support
> only
> * one connector and no cloning and hence the encoder state determines the
> state
> * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 9a396f483f8b..8ceffad3e665 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -450,14 +450,20 @@ static struct drm_connector
> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> struct drm_device *dev = intel_dig_port->base.base.dev;
> struct intel_connector *intel_connector;
> struct drm_connector *connector;
> - int i;
> + int i, ret;
>
> intel_connector = intel_connector_alloc();
> if (!intel_connector)
> return NULL;
>
> connector = &intel_connector->base;
> - drm_connector_init(dev, connector, &intel_dp_mst_connector_funcs,
> DRM_MODE_CONNECTOR_DisplayPort);
> + ret = drm_connector_init(dev, connector,
> &intel_dp_mst_connector_funcs,
> +DRM_MODE_CONNECTOR_DisplayPort);
> + if (ret) {
> + intel_connector_free(intel_connector);
> + return NULL;
> + }
> +
> drm_connector_helper_add(connector,
> &intel_dp_mst_connector_helper_funcs);
>
> intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> @@ -465,15 +471,26 @@ static struct drm_connector
> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> intel_connector->port = port;
>
> for (i = PIPE_A; i <= PIPE_C; i++) {
> - drm_mode_connector_attach_encoder(&intel_connector->base,
> -
> &intel_dp->mst_encoders[i]->base.base);
> + struct drm_encoder *enc =
> &intel_dp->mst_encoders[i]->base.base;
> +
> + ret =
> drm_mode_connector_attach_encoder(&intel_connector->base,
> + enc);
> + if (ret)
> + goto err;
> }
>
> drm_object_attach_property(&connector->base,
> dev->mode_config.path_property, 0);
> drm_object_attach_property(&connector->base,
> dev->mode_config.tile_property, 0);
>
> - drm_mode_connector_set_path_property(connector, pathprop);
> + ret = drm_mode_connector_set_path_property(connector, pathprop);
> + if (ret)
> + goto err;
> +
> return connector;
> +
> +err:
> + drm_connector_cleanup(connector);
> + return NULL;
> }
>
> static void intel_dp_register_mst_connector(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0cab667fff57..b549a0b45e57 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1360,6 +1360,7 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private
> *dev_priv);
> void intel_encoder_destroy(struct drm_encoder *encoder);
> int intel_connector_init(struct intel_connector *);
> struct intel_connector *intel_connector_alloc(void);
> +void intel_connector_free(struct intel_connector *connector);
> bool intel_connector_get_hw_state(struct intel_connector *connector);
> void intel_connector_attach_encoder(struct
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/bdw: Fix DP_AUX_CH_CTL_TIME_OUT setting
On Thu, Oct 12, 2017 at 1:59 PM, Rodrigo Vivi wrote:
> On Thu, Oct 12, 2017 at 06:58:20PM +0000, Ausmus, James wrote:
>> On Wed, Oct 4, 2017 at 3:54 PM, Runyan, Arthur J
>> wrote:
>> > I think the failure was with one particularly slow eDP panel, but it is
>> > safer to apply this to all ports.
>> >
>>
>> Thanks Art. Anyone else have thoughts/comments on this, or should this
>> be good for merge now?
>
> There was a warning on CI. And v2 didn't get fully tested because CI got
> confused about name changes on v2.
>
> So, could you please resend as a new series. Without the "in-reply-to"
> so we get the full CI run on it?
Done - thanks!
-James
>
> Thanks,
> Rodrigo
>
>>
>> Thanks!
>>
>> -James
>>
>> > -Original Message-
>> > From: Vivi, Rodrigo
>> > Sent: Wednesday, 4 October, 2017 1:25 PM
>> > To: Ausmus, James
>> > Cc: intel-gfx@lists.freedesktop.org; ville.syrj...@linux.intel.com;
>> > jani.nik...@linux.intel.com; Runyan, Arthur J ;
>> > b...@bwidawsk.net
>> > Subject: Re: [PATCH v2 2/2] drm/i915/bdw: Fix DP_AUX_CH_CTL_TIME_OUT
>> > setting
>> >
>> > On Wed, Oct 04, 2017 at 08:09:22PM +, James Ausmus wrote:
>> >> Per BSpec, 400us is "BDW+ Do not use this setting." - not just PORT_A.
>> >> Set BDW to 600us unconditionally.
>> >
>> > Besides that statement I also found on BSpec:
>> >
>> > "
>> > Workaround
>> > Project
>> > BDW, EXCLUDE(CHV)
>> > Set the Timeout timer value to at least 600us before initiating a
>> > transaction.
>> > "
>> >
>> > Also I tracked this on the log and arrived to commit 'a81a507d487c
>> > ("drm/i915/bdw: Change dp aux timeout to 600us on DDIA")'
>> >
>> > It seems during BDW enabling HW team found that need but only for port A
>> > and later they might have extended it and we never noticed.
>> >
>> > Ccin't Art and Ben here to see if they can comment on that.
>> >
>> > But I believe we should add this so
>> >
>> > Reviewed-by: Rodrigo Vivi
>> >
>> >>
>> >> v2:
>> >> -Split in to two patches (Rodrigo)
>> >>
>> >> Cc: Jani Nikula
>> >> Cc: Ville Syrjälä
>> >> Signed-off-by: James Ausmus
>> >> ---
>> >> drivers/gpu/drm/i915/intel_dp.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> >> b/drivers/gpu/drm/i915/intel_dp.c
>> >> index 5b4c9484575b..df301e00d9d9 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> @@ -1019,7 +1019,7 @@ static uint32_t g4x_get_aux_send_ctl(struct
>> >> intel_dp *intel_dp,
>> >> else
>> >> precharge = 5;
>> >>
>> >> - if (IS_BROADWELL(dev_priv) && intel_dig_port->port == PORT_A)
>> >> + if (IS_BROADWELL(dev_priv))
>> >> timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
>> >> else
>> >> timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>> >> --
>> >> 2.14.1
>> >>
>>
>>
>>
>> --
>>
>>
>> James Ausmus
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6)
On Fri, Oct 13, 2017 at 8:35 PM, Patchwork wrote: > == Series Details == > > Series: drm/i915: Handle drm-layer errors in intel_dp_add_mst_connector (rev6) > URL : https://patchwork.freedesktop.org/series/30384/ > State : failure > > == Summary == > > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > pass -> FAIL (shard-hsw) > > shard-hswtotal:2553 pass:1440 dwarn:1 dfail:0 fail:9 skip:1103 > time:9649s > > == Logs == > > For more details see: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6031/shards.html I'm not seeing how this failure could relate to the patch involved - am I missing something, or was this a fluke? -James -- James Ausmus ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 01/10] drm/i915: Relocate intel_ddi_get_buf_trans_*() functions
On Mon, Oct 16, 2017 at 7:56 AM, Ville Syrjala
wrote:
> From: Ville Syrjälä
>
> We'll want to use the intel_ddi_get_buf_trans_*() functions a bit
> earlier in the file, so move them up. While at it start using them
> in the iboost setup to get rid of the platform checks there.
>
> v2: Rebase due to BDW FDI buf trans fix
>
> Signed-off-by: Ville Syrjälä
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 115
> +++
> 1 file changed, 55 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 8ef65941b8fd..55937abda61f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -588,6 +588,59 @@ skl_get_buf_trans_hdmi(struct drm_i915_private
> *dev_priv, int *n_entries)
> }
> }
>
> +static const struct ddi_buf_trans *
> +intel_ddi_get_buf_trans_dp(struct drm_i915_private *dev_priv,
> + int *n_entries)
> +{
> + if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> + return kbl_get_buf_trans_dp(dev_priv, n_entries);
> + } else if (IS_SKYLAKE(dev_priv)) {
> + return skl_get_buf_trans_dp(dev_priv, n_entries);
> + } else if (IS_BROADWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(bdw_ddi_translations_dp);
> + return bdw_ddi_translations_dp;
> + } else if (IS_HASWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(hsw_ddi_translations_dp);
> + return hsw_ddi_translations_dp;
> + }
> +
> + *n_entries = 0;
> + return NULL;
> +}
> +
> +static const struct ddi_buf_trans *
> +intel_ddi_get_buf_trans_edp(struct drm_i915_private *dev_priv,
> + int *n_entries)
> +{
> + if (IS_GEN9_BC(dev_priv)) {
> + return skl_get_buf_trans_edp(dev_priv, n_entries);
> + } else if (IS_BROADWELL(dev_priv)) {
> + return bdw_get_buf_trans_edp(dev_priv, n_entries);
> + } else if (IS_HASWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(hsw_ddi_translations_dp);
> + return hsw_ddi_translations_dp;
> + }
> +
> + *n_entries = 0;
> + return NULL;
> +}
> +
> +static const struct ddi_buf_trans *
> +intel_ddi_get_buf_trans_fdi(struct drm_i915_private *dev_priv,
> + int *n_entries)
> +{
> + if (IS_BROADWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(bdw_ddi_translations_fdi);
> + return bdw_ddi_translations_fdi;
> + } else if (IS_HASWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(hsw_ddi_translations_fdi);
> + return hsw_ddi_translations_fdi;
> + }
> +
> + *n_entries = 0;
> + return NULL;
> +}
> +
> static const struct cnl_ddi_buf_trans *
> cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv, int *n_entries)
> {
> @@ -692,59 +745,6 @@ static int intel_ddi_hdmi_level(struct drm_i915_private
> *dev_priv, enum port por
> return hdmi_level;
> }
>
> -static const struct ddi_buf_trans *
> -intel_ddi_get_buf_trans_dp(struct drm_i915_private *dev_priv,
> - int *n_entries)
> -{
> - if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> - return kbl_get_buf_trans_dp(dev_priv, n_entries);
> - } else if (IS_SKYLAKE(dev_priv)) {
> - return skl_get_buf_trans_dp(dev_priv, n_entries);
> - } else if (IS_BROADWELL(dev_priv)) {
> - *n_entries = ARRAY_SIZE(bdw_ddi_translations_dp);
> - return bdw_ddi_translations_dp;
> - } else if (IS_HASWELL(dev_priv)) {
> - *n_entries = ARRAY_SIZE(hsw_ddi_translations_dp);
> - return hsw_ddi_translations_dp;
> - }
> -
> - *n_entries = 0;
> - return NULL;
> -}
> -
> -static const struct ddi_buf_trans *
> -intel_ddi_get_buf_trans_edp(struct drm_i915_private *dev_priv,
> - int *n_entries)
> -{
> - if (IS_GEN9_BC(dev_priv)) {
> - return skl_get_buf_trans_edp(dev_priv, n_entries);
> - } else if (IS_BROADWELL(dev_priv)) {
> - return bdw_get_buf_trans_edp(dev_priv, n_entries);
> - } else if (IS_HASWELL(dev_priv)) {
> - *n_entries = ARRAY_SIZE(hsw_ddi_translations_dp);
> - return hsw_ddi_translations_dp;
> - }
> -
> - *n_entries = 0;
> - return NULL;
> -}
> -
> -static const struct ddi_buf_trans *
> -intel_ddi_get_buf_trans_fdi(struct drm_i915_private *dev_priv,
> - int *n_entries)
> -{
> - if (IS_BROADWELL(dev_priv)) {
> - *n_entries = ARRAY_SIZE(bdw_ddi_translations_fdi);
> - return bdw_ddi_translations_fdi;
> - } else if (IS_HASWELL(dev_priv)) {
> - *n_entries = ARRAY_SIZE(hsw_ddi_translations_fdi);
> - return hsw_ddi_translation
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Extract intel_ddi_get_buf_trans_hdmi()
On Mon, Oct 16, 2017 at 7:56 AM, Ville Syrjala
wrote:
> From: Ville Syrjälä
>
> Introduce intel_ddi_get_buf_trans_hdmi() and start using it where we
> can.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 50
> ++--
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 55937abda61f..e6c884a6d408 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -641,6 +641,24 @@ intel_ddi_get_buf_trans_fdi(struct drm_i915_private
> *dev_priv,
> return NULL;
> }
>
> +static const struct ddi_buf_trans *
> +intel_ddi_get_buf_trans_hdmi(struct drm_i915_private *dev_priv,
> +int *n_entries)
> +{
> + if (IS_GEN9_BC(dev_priv)) {
> + return skl_get_buf_trans_hdmi(dev_priv, n_entries);
> + } else if (IS_BROADWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> + return bdw_ddi_translations_hdmi;
> + } else if (IS_HASWELL(dev_priv)) {
> + *n_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> + return hsw_ddi_translations_hdmi;
> + }
> +
> + *n_entries = 0;
> + return NULL;
> +}
> +
> static const struct cnl_ddi_buf_trans *
> cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv, int *n_entries)
> {
> @@ -723,18 +741,17 @@ static int intel_ddi_hdmi_level(struct drm_i915_private
> *dev_priv, enum port por
> cnl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> hdmi_default_entry = n_hdmi_entries - 1;
Why are you skipping the CNL case? If you extract it in to
intel_ddi_get_buf_trans_hdmi as well, you could just
unconditionally call intel_ddi_get_buf_trans_hdmi, and just set
hdmi_default_entry in the platform-specific section.
> } else if (IS_GEN9_BC(dev_priv)) {
> - skl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> hdmi_default_entry = 8;
> } else if (IS_BROADWELL(dev_priv)) {
> - n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> hdmi_default_entry = 7;
> } else if (IS_HASWELL(dev_priv)) {
> - n_hdmi_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> hdmi_default_entry = 6;
> } else {
> WARN(1, "ddi translation table missing\n");
> - n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> - hdmi_default_entry = 7;
> + return 0;
> }
>
> /* Choose a good default if VBT is badly populated */
> @@ -810,23 +827,12 @@ static void intel_prepare_hdmi_ddi_buffers(struct
> intel_encoder *encoder)
>
> hdmi_level = intel_ddi_hdmi_level(dev_priv, port);
>
> - if (IS_GEN9_BC(dev_priv)) {
> - ddi_translations_hdmi = skl_get_buf_trans_hdmi(dev_priv,
> &n_hdmi_entries);
> + ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv,
> &n_hdmi_entries);
>
> - /* If we're boosting the current, set bit 31 of trans1 */
> - if (dev_priv->vbt.ddi_port_info[port].hdmi_boost_level)
> - iboost_bit = DDI_BUF_BALANCE_LEG_ENABLE;
> - } else if (IS_BROADWELL(dev_priv)) {
> - ddi_translations_hdmi = bdw_ddi_translations_hdmi;
> - n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> - } else if (IS_HASWELL(dev_priv)) {
> - ddi_translations_hdmi = hsw_ddi_translations_hdmi;
> - n_hdmi_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> - } else {
> - WARN(1, "ddi translation table missing\n");
> - ddi_translations_hdmi = bdw_ddi_translations_hdmi;
> - n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> - }
> + /* If we're boosting the current, set bit 31 of trans1 */
> + if (IS_GEN9_BC(dev_priv) &&
> + dev_priv->vbt.ddi_port_info[port].hdmi_boost_level)
> + iboost_bit = DDI_BUF_BALANCE_LEG_ENABLE;
>
> /* Entry 9 is for HDMI: */
> I915_WRITE(DDI_BUF_TRANS_LO(port, 9),
> @@ -1820,7 +1826,7 @@ static void skl_ddi_set_iboost(struct intel_encoder
> *encoder, u32 level)
> if (hdmi_iboost) {
> iboost = hdmi_iboost;
> } else {
> - ddi_translations = skl_get_buf_trans_hdmi(dev_priv,
> &n_entries);
> + ddi_translations =
> intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> iboost = ddi_translations[level].i_boost;
> }
> } else
Re: [Intel-gfx] [PATCH v2 03/10] drm/i915: Pass the encoder type explicitly to skl_set_iboost()
On Mon, Oct 16, 2017 at 7:56 AM, Ville Syrjala
wrote:
> From: Ville Syrjälä
>
> encoder->type isn't reliable for DP/HDMI encoders, so pass the type
> explicity to skl_set_iboost(). Also take the opportunity to streamline
> the code.
>
> v2: Clean up the argument types to skl_ddi_set_iboost() while at it
>
> Signed-off-by: Ville Syrjälä
That's a much cleaner read now!
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 59
>
> 1 file changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index e6c884a6d408..cf0b2d3de15f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1788,49 +1788,36 @@ static void _skl_ddi_set_iboost(struct
> drm_i915_private *dev_priv,
> I915_WRITE(DISPIO_CR_TX_BMU_CR0, tmp);
> }
>
> -static void skl_ddi_set_iboost(struct intel_encoder *encoder, u32 level)
> +static void skl_ddi_set_iboost(struct intel_encoder *encoder,
> + int level, enum intel_output_type type)
> {
> struct intel_digital_port *intel_dig_port =
> enc_to_dig_port(&encoder->base);
> struct drm_i915_private *dev_priv =
> to_i915(intel_dig_port->base.base.dev);
> enum port port = intel_dig_port->port;
> - int type = encoder->type;
> - const struct ddi_buf_trans *ddi_translations;
> uint8_t iboost;
> - uint8_t dp_iboost, hdmi_iboost;
> - int n_entries;
>
> - /* VBT may override standard boost values */
> - dp_iboost = dev_priv->vbt.ddi_port_info[port].dp_boost_level;
> - hdmi_iboost = dev_priv->vbt.ddi_port_info[port].hdmi_boost_level;
> + if (type == INTEL_OUTPUT_HDMI)
> + iboost = dev_priv->vbt.ddi_port_info[port].hdmi_boost_level;
> + else
> + iboost = dev_priv->vbt.ddi_port_info[port].dp_boost_level;
>
> - if (type == INTEL_OUTPUT_DP) {
> - if (dp_iboost) {
> - iboost = dp_iboost;
> - } else {
> - ddi_translations =
> intel_ddi_get_buf_trans_dp(dev_priv, &n_entries);
> - iboost = ddi_translations[level].i_boost;
> - }
> - } else if (type == INTEL_OUTPUT_EDP) {
> - if (dp_iboost) {
> - iboost = dp_iboost;
> - } else {
> - ddi_translations =
> intel_ddi_get_buf_trans_edp(dev_priv, &n_entries);
> + if (iboost == 0) {
> + const struct ddi_buf_trans *ddi_translations;
> + int n_entries;
>
> - if (WARN_ON(port != PORT_A &&
> - port != PORT_E && n_entries > 9))
> - n_entries = 9;
> -
> - iboost = ddi_translations[level].i_boost;
> - }
> - } else if (type == INTEL_OUTPUT_HDMI) {
> - if (hdmi_iboost) {
> - iboost = hdmi_iboost;
> - } else {
> + if (type == INTEL_OUTPUT_HDMI)
> ddi_translations =
> intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> - iboost = ddi_translations[level].i_boost;
> - }
> - } else {
> - return;
> + else if (type == INTEL_OUTPUT_EDP)
> + ddi_translations =
> intel_ddi_get_buf_trans_edp(dev_priv, &n_entries);
> + else
> + ddi_translations =
> intel_ddi_get_buf_trans_dp(dev_priv, &n_entries);
> +
> + if (WARN_ON(type != INTEL_OUTPUT_HDMI &&
> + port != PORT_A &&
> + port != PORT_E && n_entries > 9))
> + n_entries = 9;
> +
> + iboost = ddi_translations[level].i_boost;
> }
>
> /* Make sure that the requested I_boost is valid */
> @@ -2096,7 +2083,7 @@ uint32_t ddi_signal_levels(struct intel_dp *intel_dp)
> uint32_t level = intel_ddi_dp_level(intel_dp);
>
> if (IS_GEN9_BC(dev_priv))
> - skl_ddi_set_iboost(encoder, level);
> + skl_ddi_set_iboost(encoder, level, encoder->type);
>
> return DDI_BUF_TRANS_SELECT(level);
> }
> @@ -2219,7 +2206,7 @@ static void intel_ddi_pre_enable_hdmi(struct
> intel_encoder *encoder,
> intel_prepare_hdmi_ddi_buffers(encoder);
>
> if (IS_GEN9_BC(dev_priv))
> - skl_ddi_set_iboost(encoder, level);
> + skl_ddi_set_iboost(encoder, level, INTEL_OUTPUT_HDMI);
>
> intel_dig_port->set_infoframes(&encoder->base,
>crtc_state->has_infoframe,
> --
> 2.13.6
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/10] drm/i915: Pass the level to intel_prepare_hdmi_ddi_buffers()
On Mon, Oct 16, 2017 at 7:56 AM, Ville Syrjala
wrote:
> From: Ville Syrjälä
>
> The caller of intel_prepare_hdmi_ddi_buffers() alreday figured out the
> level, so let's just pass it in instead if figuring it out again.
>
> Signed-off-by: Ville Syrjälä
Reviewed-by: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index cf0b2d3de15f..f61b6c20005e 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -817,16 +817,15 @@ static void intel_prepare_dp_ddi_buffers(struct
> intel_encoder *encoder)
> * values in advance. This function programs the correct values for
> * HDMI/DVI use cases.
> */
> -static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
> +static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> + int hdmi_level)
> {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> u32 iboost_bit = 0;
> - int n_hdmi_entries, hdmi_level;
> + int n_hdmi_entries;
> enum port port = intel_ddi_get_encoder_port(encoder);
> const struct ddi_buf_trans *ddi_translations_hdmi;
>
> - hdmi_level = intel_ddi_hdmi_level(dev_priv, port);
> -
> ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv,
> &n_hdmi_entries);
>
> /* If we're boosting the current, set bit 31 of trans1 */
> @@ -2203,7 +2202,7 @@ static void intel_ddi_pre_enable_hdmi(struct
> intel_encoder *encoder,
> bxt_ddi_vswing_sequence(dev_priv, level, port,
> INTEL_OUTPUT_HDMI);
> else
> - intel_prepare_hdmi_ddi_buffers(encoder);
> + intel_prepare_hdmi_ddi_buffers(encoder, level);
>
> if (IS_GEN9_BC(dev_priv))
> skl_ddi_set_iboost(encoder, level, INTEL_OUTPUT_HDMI);
> --
> 2.13.6
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915/icp: Add backlight Support for ICP
On Fri, Jan 19, 2018 at 10:48 AM, Paulo Zanoni
wrote:
>
> From: Anusha Srivatsa
>
> ICP has two backlight controllers - similar to previous platforms like
> BXT -, but we only use one controller for now, so we can just reuse
> the CNP code.
>
> v2: Remove the usage of ICP_SECOND_PPS_BACKLIGHT register.(Jani)
> Reuse CNP code since it is very similar.(Ville)
> v3 (from Paulo): Rebase.
> v4 (from Paulo): adjust commit message (James) and comment (Rodrigo).
>
> Cc: Jani Nikula
> Cc: Ville Syrjala
> Cc: James Ausmus
> Cc: Rodrigo Vivi
> Reviewed-by: Paulo Zanoni
> Signed-off-by: Anusha Srivatsa
> Signed-off-by: Paulo Zanoni
Looks good, thanks!
Acked-by: James Ausmus
>
> ---
> drivers/gpu/drm/i915/intel_panel.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
> index fa6831f8c004..e702a6487aa9 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1719,9 +1719,9 @@ cnp_setup_backlight(struct intel_connector
*connector, enum pipe unused)
> u32 pwm_ctl, val;
>
> /*
> -* CNP has the BXT implementation of backlight, but with only
> -* one controller. Future platforms could have multiple
controllers
> -* so let's make this extensible and prepared for the future.
> +* CNP has the BXT implementation of backlight, but with only one
> +* controller. TODO: ICP has multiple controllers but we only use
> +* controller 0 for now.
> */
> panel->backlight.controller = 0;
>
> @@ -1865,7 +1865,7 @@ intel_panel_init_backlight_funcs(struct intel_panel
*panel)
> panel->backlight.set = bxt_set_backlight;
> panel->backlight.get = bxt_get_backlight;
> panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> - } else if (HAS_PCH_CNP(dev_priv)) {
> + } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) {
> panel->backlight.setup = cnp_setup_backlight;
> panel->backlight.enable = cnp_enable_backlight;
> panel->backlight.disable = cnp_disable_backlight;
> --
> 2.14.3
>
--
James Ausmus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915/icl: add the main CDCLK functions
On Mon, Feb 05, 2018 at 01:40:42PM -0200, Paulo Zanoni wrote:
> This commit adds the basic CDCLK functions, but it's still missing
> pieces of the display initialization sequence.
>
> v2:
> - Implement the voltage levels.
> - Rebase.
> v3:
> - Adjust to the new "bypass" clock (Imre).
> - Call intel_dump_cdclk_state() too.
> - Rename a variable to avoid confusion.
> - Simplify the DVFS part.
^^^
Shouldn't this be something more like
"Drop DVFS part and replace with a TODO"?
"Simplify" makes it sound like it's still there, but it's not, unless
I'm missing something?
> v4:
> - Remove wrong bit definition (James).
> - Also drive-by fix the coding style for the register definition we
>touched.
>
> Cc: James Ausmus
> Cc: Imre Deak
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/i915_reg.h| 35 +++---
> drivers/gpu/drm/i915/intel_cdclk.c | 235
> -
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 3 files changed, 255 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6e1677e8211..2b6a908056d6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7182,8 +7182,12 @@ enum {
> #define SKL_DFSM_PIPE_B_DISABLE (1 << 21)
> #define SKL_DFSM_PIPE_C_DISABLE (1 << 28)
>
> -#define SKL_DSSM _MMIO(0x51004)
> -#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31)
> +#define SKL_DSSM _MMIO(0x51004)
> +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31)
> +#define ICL_DSSM_CDCLK_PLL_REFCLK_MASK (7 << 29)
> +#define ICL_DSSM_CDCLK_PLL_REFCLK_24MHz (0 << 29)
> +#define ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz (1 << 29)
> +#define ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz (2 << 29)
>
> #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0)
> #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14)
> @@ -8816,20 +8820,21 @@ enum skl_power_gate {
>
> /* CDCLK_CTL */
> #define CDCLK_CTL _MMIO(0x46000)
> -#define CDCLK_FREQ_SEL_MASK (3<<26)
> -#define CDCLK_FREQ_450_432 (0<<26)
> -#define CDCLK_FREQ_540 (1<<26)
> -#define CDCLK_FREQ_337_308 (2<<26)
> -#define CDCLK_FREQ_675_617 (3<<26)
> -#define BXT_CDCLK_CD2X_DIV_SEL_MASK (3<<22)
> -#define BXT_CDCLK_CD2X_DIV_SEL_1 (0<<22)
> -#define BXT_CDCLK_CD2X_DIV_SEL_1_5 (1<<22)
> -#define BXT_CDCLK_CD2X_DIV_SEL_2 (2<<22)
> -#define BXT_CDCLK_CD2X_DIV_SEL_4 (3<<22)
> -#define BXT_CDCLK_CD2X_PIPE(pipe) ((pipe)<<20)
> -#define CDCLK_DIVMUX_CD_OVERRIDE (1<<19)
> +#define CDCLK_FREQ_SEL_MASK (3 << 26)
> +#define CDCLK_FREQ_450_432 (0 << 26)
> +#define CDCLK_FREQ_540 (1 << 26)
> +#define CDCLK_FREQ_337_308 (2 << 26)
> +#define CDCLK_FREQ_675_617 (3 << 26)
> +#define BXT_CDCLK_CD2X_DIV_SEL_MASK (3 << 22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_1 (0 << 22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_1_5 (1 << 22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_2 (2 << 22)
> +#define BXT_CDCLK_CD2X_DIV_SEL_4 (3 << 22)
> +#define BXT_CDCLK_CD2X_PIPE(pipe) ((pipe) << 20)
> +#define CDCLK_DIVMUX_CD_OVERRIDE (1 << 19)
> #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3)
> -#define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1<<16)
> +#define ICL_CDCLK_CD2X_PIPE_NONE (7 << 19)
> +#define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1 << 16)
> #define CDCLK_FREQ_DECIMAL_MASK (0x7ff)
>
> /* LCPLL_CTL */
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index ee788d5be5e3..52a15d0eaae9 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1778,6 +1778,197 @@ static void cnl_sanitize_cdclk(struct
> drm_i915_private *dev_priv)
> dev_priv->cdclk.hw.vco = -1;
> }
>
> +static int icl_calc_cdclk(int min_cdclk, unsigned int ref)
> +{
> + int ranges_24[] = { 312000, 552000, 648000 };
> + int ranges_19_38[] = { 307200, 556800, 652800 };
> + int *ranges;
> +
> + switch (ref) {
> + default:
> + MISSING_CASE(ref);
> + case 24000:
> + ranges = ranges_24;
> + break;
> + case 19200:
> + case 38400:
> + ranges = ranges_19_38;
> + break;
> + }
> +
> + if (min_cdclk > ranges[1])
> + return ranges[2];
> + else if (min_cdclk > ranges[0])
> + return ranges[1];
> + else
> + return ranges[0];
> +}
> +
> +static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int
> cdclk)
> +{
> + int ratio;
> +
> + if (cdclk == dev_priv->cdclk.hw.bypass)
> + return 0;
> +
> + switch (cdclk) {
> + default:
> + MISSING_CASE(cdclk);
> + case 307200:
> + case 556800:
> + case 652800:
> + WARN_ON(dev_priv->cdclk.hw.ref != 19200 &&
> + dev_priv->cdclk.hw.ref != 38400);
> + break;
> + case 312000:
> + case 552000:
> + case 648000:
> + WARN_ON(dev_priv->cdclk.hw.ref != 24000);
> + }
> +
> + ratio = cdclk / (dev_priv->cdclk.hw.ref / 2);
> +
> + return dev_priv->cdclk.hw.ref * ratio;
> +}
> +
> +static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state)
> +{
> + unsigned int cdclk = cdclk_state->cdclk;
> + unsigned int vco = cdclk_state->vco;
> + int ret
Re: [Intel-gfx] [RFC] drm/i915: Non-upstream ChromeOS patches from 3.8
On Fri, Aug 16, 2013 at 1:15 AM, Jani Nikula wrote: > > On Fri, 16 Aug 2013, Daniel Vetter wrote: > > On Thu, Aug 15, 2013 at 05:30:25PM -0700, james.aus...@intel.com wrote: > >> Hello All- > >> > >> I'm trying to determine if the ChromeOS-only patches being carried by > >> Google still make sense and are the right way to do things in the 3.11+ > >> world, and Jesse asked me to forward the patches to the list for evaluation > >> and potential upstreaming. > > > > I've quickly read through the pile here and there's a few things we > > need to look at. > > Ditto, and agreed. > > > But one thing which makes assessing the patches here a bit a pain is > > that often there's a fixup later on again. > > Another pain is that sometimes the fixup is first, i.e. the series does > not seem to be in the right order. One of the issues is that it's not truly a series, but rather individual patches dating back through 2011 - I thought I had gotten git-send-email to push them out in chronological order, but maybe I was less successful than I thought. :) I'll see what I can do in regards to squishing some of these down - I have fairly limited knowledge when it comes to the i915 driver, so it may not end up perfect, but I'll give it a go. :) Thanks! -James > > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: Restore PPAT on thaw
On Tue, Mar 18, 2014 at 4:09 PM, Ben Widawsky
wrote:
> Apparently it is wiped out from under us, and we get some really fun
> caching artifacts upon resume (it seems to be WB for all types by
> default).
>
> Reported-by: James Ausmus
> Signed-off-by: Ben Widawsky
Tested-by: James Ausmus
Works for me backported on to both a 3.14-rc3 w/ ChromeOS sauce and a
vanilla 3.14-rc6. Thanks!
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index bd016e2..1b45a04 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -30,6 +30,8 @@
> #include "i915_trace.h"
> #include "intel_drv.h"
>
> +static void gen8_setup_private_ppat(struct drm_i915_private *dev_priv);
> +
> bool intel_enable_ppgtt(struct drm_device *dev, bool full)
> {
> if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev))
> @@ -1371,8 +1373,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device
> *dev)
> }
>
>
> - if (INTEL_INFO(dev)->gen >= 8)
> + if (INTEL_INFO(dev)->gen >= 8) {
> + gen8_setup_private_ppat(dev_priv);
> return;
> + }
>
> list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> /* TODO: Perhaps it shouldn't be gen6 specific */
> --
> 1.9.0
>
--
James Ausmus
Sr. Software Engineer
SSG-OTC ChromeOS Integration
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Sat, Mar 22, 2014 at 4:34 AM, Daniel Vetter wrote: > On Fri, Mar 21, 2014 at 05:51:01PM -0700, Ben Widawsky wrote: >> Let's try this again. I've pushed a branch here: >> http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=bdw-backports >> >> I need to re-review some of the merge conflicts for 4g GGTT, which I >> will try to do before Monday. >> >> Daniel: please make sure this is what you had in mind. I don't know >> where you want Cc: stable tags. > > We don't need cc: stable, we just need to submit it (since it has the > upstream sha1s already, which is the requirement for stable patches). Cc: > stable is only for when you want it to get backport anyway. Otherwise > looks good. I dunno whether git cherry-pick can be told to use the sha1 > reference layout Greg prefers or not, he uses "This is in > upstream." between the commit header and the actual commit message. But > ime his scripts reformat your commit messages anyway. > >> James: please test this as soon as possible. > > Once this is tested and we conclude it's sufficient to get bdw going on > 3.14 without hilarity I think we should do a quick review on intel-gfx to > check that the impact outside of bdw is indeed minimal. Then once drm-next > has landed with all the referenced commits we can submit it to Greg with a > small cover letter why we want this and that plan B would be to kill bdw > in 3.14. This seems to be working well for me, with the one caveat that on boot and once per resume I'm hitting the WARN(!i915_preliminary_hw_support, "GEN8_CENTROID_PIXEL_OPT_DIS not be needed for production") code in gen8_init_clock_gating - can that WARN be dropped via "drm/i915: Don't use i915_preliminary_hw_support to mean pre-production" ? Both with and without that patch added, the series is: Tested-by: James Ausmus > > Thanks for doing this, > Daniel > >> >> Thanks. >> >> >> On Fri, Mar 21, 2014 at 11:48:09AM -0700, Ben Widawsky wrote: >> > The following patches are the backported "simple" fixes for 3.14. Some >> > of these already had Cc: stable on them, but required conflict >> > resolution which I've provided (presumably they canbe dropped if it's >> > easier for upstream). There will be another series of backports which >> > has fixes that require more than a single patch. >> > >> > I will not have a machine to test these on until Monday, but I am >> > mailing them out now in case our QA can get it tested sooner. >> > >> > Ben Widawsky (2): >> > drm/i915/bdw: Use scratch page table for GEN8 PPGTT >> > drm/i915/bdw: Restore PPAT on thaw >> > >> > Damien Lespiau (1): >> > drm/i915/bdw: The TLB invalidation mechanism has been removed from >> > INSTPM >> > >> > Jani Nikula (1): >> > drm/i915: don't flood the logs about bdw semaphores >> > >> > Kenneth Graunke (2): >> > drm/i915: Add a partial instruction shootdown workaround on Broadwell. >> > drm/i915: Add thread stall DOP clock gating workaround on Broadwell. >> > >> > Mika Kuoppala (2): >> > drm/i915: Fix forcewake counts for gen8 >> > drm/i915: Do forcewake reset on gen8 >> > >> > Ville Syrjälä (4): >> > drm/i915: Disable semaphore wait event idle message on BDW >> > drm/i915: Implement WaDisableSDEUnitClockGating:bdw >> > drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw >> > drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW >> > >> > drivers/gpu/drm/i915/i915_drv.c | 5 - >> > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++ >> > drivers/gpu/drm/i915/i915_reg.h | 10 ++ >> > drivers/gpu/drm/i915/intel_pm.c | 18 -- >> > drivers/gpu/drm/i915/intel_ringbuffer.c | 12 +--- >> > drivers/gpu/drm/i915/intel_uncore.c | 29 +++-- >> > 6 files changed, 61 insertions(+), 20 deletions(-) >> > >> > -- >> > 1.9.1 >> > >> > ___ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Ben Widawsky, Intel Open Source Technology Center > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- James Ausmus Sr. Software Engineer SSG-OTC ChromeOS Integration ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/9] drm-intel-collector - update
On Mon, Nov 18, 2013 at 6:32 PM, Rodrigo Vivi wrote: > > This is another drm-intel-collector updated notice: > http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=drm-intel-collector > > Here goes the update list in order for better reviewers assignment: > > Patch drm/i915: Asynchronously perform the set-base for a simple modeset > - Reviewed by me. > Patch drm/i915: Fix gen3/4 vblank counter wraparound - Reviewer: > Patch drm/i915: Use frame counter for intel_wait_for_vblank() on CTG - > Reviewer: > Patch drm/i915: Do hw quiescing first during unload - Reviewer: > Patch drm/i915: print object bindings in debugfs - Reviewer: > Patch drm/i915/vlv: enable HDMI audio for Valleyview2 - Reviewer: > Patch drm/i915: Hold pc8 lock around toggling pc8.gpu_idle - Reviewed by > Paulo > Patch drm/i915: Do not enable package C8 on unsupported hardware - > Reviewed by Paulo > Patch drm/i915: Enable pipe gamma for sprites - Reviewer: > > Overall Process: > > drm-intel-collector - review request > 1. Daniel pushs drm-intel-testing (every 2 weeks) > 2. I rebase drm-intel-collector onto drm-intel-testing > 3. Add Reviewer: tag with voluntered reviewers. If you don't believe you > should be assigned on a particular patch please don't get mad just tell you > wont review or volunteer someone else. > 4. I resubmit remaining patches for review without picking any new > (drm-intel-collector - review request) > > drm-intel-collector - updated > 5. One week later I collect all simple (1-2) patches that wasn't yet > reviewed and not queued by Daniel from one testing update until another. > 6. Request automated QA's PRTS automated i-g-t tests comparing > drm-intel-testing x drm-intel-collector > 7. If tests are ok I send the update notification or the patches as a series > to intel-gfx mailing list for better tracking and to be reviewed. > (drm-intel-collector - updated) > 8. Let me know volunteers for review new patches and also let me know if > I've picked any patch that I shouldn't. > > There are some reasons that some patches can be left behind: > 1. Your patch didn't applied cleanly and I couldn't easily solve the > conflicts. > 2. Kernel didn't compiled with your patch. > 3. I simply missed it. If you believe this is the case please warn me. > > Please help me to get these patches reviewed and queued by Daniel. > > Also, please let me know if you have further ideas how to improve this > process. > > Thanks in advance, > Rodrigo. > > > Chris Wilson (4): > drm/i915: Asynchronously perform the set-base for a simple modeset > drm/i915: Do hw quiescing first during unload > drm/i915: Hold pc8 lock around toggling pc8.gpu_idle > drm/i915: Do not enable package C8 on unsupported hardware > > Daniel Vetter (1): > drm/i915: print object bindings in debugfs > > Mengdong Lin (1): > drm/i915/vlv: enable HDMI audio for Valleyview2 A v4 of this patch was already merged in with a slightly renamed subject: 9ca2fe731b3f12afbc97cf0050dfa4184bd2234c drm/i915/vlv: enable HDA display audio for Valleyview2 -James > > Ville Syrjälä (3): > drm/i915: Fix gen3/4 vblank counter wraparound > drm/i915: Use frame counter for intel_wait_for_vblank() on CTG > drm/i915: Enable pipe gamma for sprites > > drivers/gpu/drm/i915/i915_debugfs.c | 6 > drivers/gpu/drm/i915/i915_dma.c | 10 +++--- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 2 +- > drivers/gpu/drm/i915/i915_reg.h | 20 ++- > drivers/gpu/drm/i915/intel_display.c | 65 > > drivers/gpu/drm/i915/intel_sprite.c | 18 ++ > 7 files changed, 103 insertions(+), 19 deletions(-) > > -- > 1.8.3.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
Re: [Intel-gfx] intel_drv.so segfault
On Wed, Nov 6, 2013 at 6:22 AM, Grant wrote: >>> >> > I'm getting the following segfault intermittently when using sna. >>> >> > I've tried xf86-video-intel-2.99.905 and the latest from git a week or >>> >> > so ago. >>> >> >>> >> Symbols, please. >>> >> >>> >> addr2line -e /usr/lib64/xorg/modules/drivers/intel_drv.so -i >>> >> 0x2fe790x3037f >>> > >>> > addr2line -e /usr/lib64/xorg/modules/drivers/intel_drv.so -i 0x2fe79 >>> > 0x3037f >>> >>> Maybe this needs to be run immediately after the crash? Or before >>> rebooting? >> >> Ah, it does require xf86-video-intel to have been built with debug >> symbols (-g in CFLAGS) and not stripped upon installation. > > I'm running Gentoo and I added -g to the following in make.conf: > > CFLAGS="-march=native -O2 -pipe -fomit-frame-pointer -g" > > I rebooted but I still get nothing from the addr2line command. What > am I missing? FEATURES="nostrip" emerge xf86-video-intel > > - Grant > ___ > 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
Re: [Intel-gfx] intel_drv.so segfault
On Wed, Nov 6, 2013 at 10:17 AM, Grant wrote: >>> Thank you, here's what I get: >>> >>> # addr2line -e /usr/lib64/xorg/modules/drivers/intel_drv.so -i 0x2fe79 >>> 0x3037f >>> /var/tmp/portage/x11-drivers/xf86-video-intel-2.99.905-r1/work/xf86-video-intel-2.99.905/src/sna/sna_accel.c:6079 >>> /var/tmp/portage/x11-drivers/xf86-video-intel-2.99.905-r1/work/xf86-video-intel-2.99.905/src/sna/sna_accel.c:6240 >>> (discriminator 1) >> >> I need to know what commit id that corresponds to as well. Thanks, >> -Chris > > I'm sorry Chris, I'm just a lowly user. How can I get that info for you? I happen to have a few Gentoo systems sitting under my desk - if you can send me the output of emerge -pv xf86-video-intel I can assist with backtracking that to a specific commit. -James > > - Grant > ___ > 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
Re: [Intel-gfx] intel_drv.so segfault
On Wed, Nov 6, 2013 at 10:48 AM, Grant wrote: > Thank you, here's what I get: > > # addr2line -e /usr/lib64/xorg/modules/drivers/intel_drv.so -i 0x2fe79 > 0x3037f Grant - I'm assuming that this was done on the emerged xf86-video-intel, not the git-compiled one? > /var/tmp/portage/x11-drivers/xf86-video-intel-2.99.905-r1/work/xf86-video-intel-2.99.905/src/sna/sna_accel.c:6079 git blame shows sna_accel.c:6079 (as of the 2.99.905 tag) last touched by 85fdc314 > /var/tmp/portage/x11-drivers/xf86-video-intel-2.99.905-r1/work/xf86-video-intel-2.99.905/src/sna/sna_accel.c:6240 git blame shows sna_accel.c:6240 (as of the 2.99.905 tag) last touched by 07a46333 Also note that xf86-video-intel-2.99.905-r1 has a backport patch of 8e44b1f5543f6d36c33c743f1ba2143514f8afbf (sna: Fix canonical mode name to correctly use asprintf) applied that touches sna/sna_display.c -James > (discriminator 1) I need to know what commit id that corresponds to as well. Thanks, -Chris >>> >>> I'm sorry Chris, I'm just a lowly user. How can I get that info for you? >> >> I happen to have a few Gentoo systems sitting under my desk - if you >> can send me the output of >> >> emerge -pv xf86-video-intel >> >> I can assist with backtracking that to a specific commit. >> >> -James > > Many thanks James! > > # emerge -pv xf86-video-intel > These are the packages that would be merged, in order: > Calculating dependencies... done! > [ebuild R ~] x11-drivers/xf86-video-intel-2.99.905-r1 USE="dri > sna udev -glamor -uxa -xvmc" 0 kB > Total: 1 package (1 reinstall), Size of downloads: 0 kB > > - Grant ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Disable caches for Global GTT.
On Fri, Oct 31, 2014 at 5:27 AM, Rodrigo Vivi wrote: > > Global GTT doesn't have pat_sel[2:0] so it always point to pat_sel = 000; > So the only way to avoid screen corruptions is setting PAT 0 to Uncached. > > MOCS can still be used though. But if userspace is trusting PTE for > cache selection the safest thing to do is to let caches disabled. > > BSpec: "For GGTT, there is NO pat_sel[2:0] from the entry, > so RTL will always use the value corresponding to pat_sel = 000" > > v2: Cleaner patch as suggested by Chris. > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85576 > Cc: Chris Wilson > Cc: James Ausmus > Signed-off-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index cb7adab..ae568a2 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1920,6 +1920,15 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv) > GEN8_PPAT(6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2)) | > GEN8_PPAT(7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3)); > > + if (!USES_PPGTT(dev_priv->dev)) > + /* Spec: "For GGTT, there is NO pat_sel[2:0] from the entry, > +* so RTL will always use the value corresponding to > +* pat_sel = 000". > +* So let's disable cache for GGTT to avoid screen corruptions. > +* MOCS still can be used though. > +*/ > + pat = GEN8_PPAT(0, GEN8_PPAT_UC); > + > /* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b > * write would work. */ > I915_WRITE(GEN8_PRIVATE_PAT, pat); > -- > 1.9.3 > Tested-by: James Ausmus -- James Ausmus Sr. Software Engineer SSG-OTC ChromeOS Integration ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Disable caches for Global GTT.
On Fri, Oct 31, 2014 at 5:27 AM, Rodrigo Vivi wrote: > > Global GTT doesn't have pat_sel[2:0] so it always point to pat_sel = 000; > So the only way to avoid screen corruptions is setting PAT 0 to Uncached. > > MOCS can still be used though. But if userspace is trusting PTE for > cache selection the safest thing to do is to let caches disabled. > > BSpec: "For GGTT, there is NO pat_sel[2:0] from the entry, > so RTL will always use the value corresponding to pat_sel = 000" > > v2: Cleaner patch as suggested by Chris. > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85576 > Cc: Chris Wilson > Cc: James Ausmus > Signed-off-by: Rodrigo Vivi Matches my read of BSpec. Reviewed-by: James Ausmus > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index cb7adab..ae568a2 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1920,6 +1920,15 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv) > GEN8_PPAT(6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2)) | > GEN8_PPAT(7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3)); > > + if (!USES_PPGTT(dev_priv->dev)) > + /* Spec: "For GGTT, there is NO pat_sel[2:0] from the entry, > +* so RTL will always use the value corresponding to > +* pat_sel = 000". > +* So let's disable cache for GGTT to avoid screen corruptions. > +* MOCS still can be used though. > +*/ > + pat = GEN8_PPAT(0, GEN8_PPAT_UC); > + > /* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b > * write would work. */ > I915_WRITE(GEN8_PRIVATE_PAT, pat); > -- > 1.9.3 > -- James Ausmus Sr. Software Engineer SSG-OTC ChromeOS Integration ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Redo WMs when cursor size changes
On Thu, Feb 26, 2015 at 2:48 PM, Joe Konno
wrote:
>
> From: Joe Konno
>
> In instances where cursor sizes change, as in Chromium Ozone/Freon,
> watermarks should be recomputed. There should be no hard-coded
> assumptions about cursor widths. This was corrected originally here:
>
> commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf
> Author: Chris Wilson
> Date: Wed Mar 26 12:38:15 2014 +
>
> drm/i915: Recompute WM when the cursor size changes
>
> However, it seems the recompute logic got lost in refactoring.
> Re-introduce the relevant WM re-compute code from the original patch.
>
> Cc: Chris Wilson
> Cc: Matt Roper
> Fixes: 32b7eeec4d1e ("drm/i915: Refactor work that can sleep out of
commit (v7)")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89346
> Signed-off-by: Joe Konno
> ---
> drivers/gpu/drm/i915/intel_display.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
> index b0f113d9daab..c5cbfea0551e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12234,6 +12234,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> uint32_t addr;
> + unsigned old_width;
>
> crtc = crtc ? crtc : plane->crtc;
> intel_crtc = to_intel_crtc(crtc);
> @@ -12243,6 +12244,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> crtc->cursor_y = state->base.crtc_y;
>
> intel_plane->obj = obj;
> + old_width = intel_crtc->cursor_width;
>
> if (intel_crtc->cursor_bo == obj)
> goto update;
> @@ -12260,8 +12262,11 @@ update:
> intel_crtc->cursor_width = state->base.crtc_w;
> intel_crtc->cursor_height = state->base.crtc_h;
>
> - if (intel_crtc->active)
> + if (intel_crtc->active) {
> + if (old_width != intel_crtc->cursor_width)
> + intel_update_watermarks(crtc);
> intel_crtc_update_cursor(crtc, state->visible);
> + }
> }
>
> static struct drm_plane *intel_cursor_plane_create(struct drm_device
*dev,
> --
> 2.3.0
>
Tested-by: James Ausmus
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
James Ausmus
Sr. Software Engineer
SSG-OTC ChromeOS Integration
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware
On Wed, Jul 15, 2015 at 2:34 AM, Daniel Vetter wrote:
>
> On Tue, Jul 14, 2015 at 01:37:32PM -0700, Greg KH wrote:
> > On Tue, Jul 14, 2015 at 11:22:35AM +0200, Daniel Vetter wrote:
> > > On Mon, Jul 13, 2015 at 09:36:45AM -0700, jay.p.pa...@intel.com wrote:
> > > > From: Jay Patel
> > > >
> > > > NOTE: This is an interim solution which is targeted towards
> > > > Chrome OS/Android to be used until a long term solution is
available.
> > > >
> > > > In this patch, request_firmware() is called in a worker thread
> > > > which initially waits for file system to be initialized and then
> > > > attempts to load the firmware.
> > >
> > > Aside: I posted a bunch of proof-of-principle patches to clean up dmc
> > > loading and convert over to using an explicit workqueue. They're being
> > > tested/made-to-actually-work right now I think.
> > >
> > > > "request_firmware_nowait()" is also using an asynchronous thread
> > > > running concurrently with the rest of the system initialization.
> > > > However, it tries to load firmware only once without checking the
> > > > sytem status and fails most of the time.
> > > >
> > > > Change-Id: I2cb16a768e54a85f48a6682d9690b4c8af844668
> >
> > What's this line for? :)
> >
> > > > Signed-off-by: Jay Patel
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c | 2 ++
> > > > drivers/gpu/drm/i915/intel_csr.c | 58
> > > > 2 files changed, 49 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 8c8407d..eb6f7e3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -559,6 +559,7 @@ void intel_hpd_cancel_work(struct
drm_i915_private *dev_priv)
> > > > void i915_firmware_load_error_print(const char *fw_path, int err)
> > > > {
> > > > DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
> > > > + DRM_ERROR("The firmware file may be missing\n");
> > > >
> > > > /*
> > > >* If the reason is not known assume -ENOENT since that's the most
> > > > @@ -574,6 +575,7 @@ void i915_firmware_load_error_print(const char
*fw_path, int err)
> > > > "The driver is built-in, so to load the firmware you need to\n"
> > > > "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE)
or\n"
> > > > "in your initrd/initramfs image.\n");
> > > > +
> > > > }
> > > >
> > > > static void intel_suspend_encoders(struct drm_i915_private
*dev_priv)
> > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c
b/drivers/gpu/drm/i915/intel_csr.c
> > > > index 9311cdd..8d1f08c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > > @@ -349,7 +349,7 @@ static void finish_csr_load(const struct
firmware *fw, void *context)
> > > > /* load csr program during system boot, as needed for DC states */
> > > > intel_csr_load_program(dev);
> > > > fw_loaded = true;
> > > > -
> > > > + DRM_INFO("CSR Firmware Loaded\n");
> > > > out:
> > > > if (fw_loaded)
> > > > intel_runtime_pm_put(dev_priv);
> > > > @@ -359,11 +359,46 @@ out:
> > > > release_firmware(fw);
> > > > }
> > > >
> > > > +struct csr_firmware_work {
> > > > + struct work_struct work;
> > > > + struct module *module;
> > > > + struct drm_device *dev;
> > > > +};
> > > > +
> > > > +/* csr_firmware_work_func() - thread function for loading the
firmware*/
> > > > +static void csr_firmware_work_func(struct work_struct *work)
> > > > +{
> > > > + const struct firmware *fw;
> > > > + const struct csr_firmware_work *fw_work = container_of(work,
struct csr_firmware_work, work);
> > > > + int ret;
> > > > + struct drm_device *dev = fw_work->dev;
> > > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > > + struct intel_csr *csr = &dev_priv->csr;
> > > > +
> > > > + /* Wait until root filesystem is loaded in case the firmware
> > > > + * is not built-in but in /lib/firmware */
> > > > + while(system_state != SYSTEM_RUNNING){
> > > > + msleep(500);
> > > > + }
> > >
> > > Yeah, not going to merge that right now until we've had a decent
> > > discussion with Greg KH (since imo his stance of every driver creating
> > > it's own retry loop just doesn't work, especially not with gfx where
init
> > > is hairy and you just don't want to retry without end).
> >
> > Exactly, this type of thing isn't good at all (especially given that
> > the code isn't even checkpatch clean...)
> >
> > Don't do this. If you really want to somehow handle built-in drivers
> > that need firmware before the root filesystem is present, then use the
> > async firmware loading interface, don't sit and spin, that's crazy.
>
> This code is called from a work queue already to facilitate async loading.
> I want an explicit work queue so that we properly sync with it everywhere
> like driver unload or resume (otherwise we need a completion or
> something). And with an explicit worker I can put the entire
