[Intel-gfx] [PATCH 3/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Thulasimani, Sivakumar]

On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote:
> Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
>> Unfortunately we don't know all panels and platforms out there and we
>> found internal prototypes without VBT proper set but where only
>> link in standby worked well.
:) if it is internal i assume someone has to set the vbt ,we encountered 
an issue
sometime back that blamed vbt as incorrect only to later learn that
the person who created the setup didn't care to configure the VBT.
>>
>> So, before enable PSR by default let's instrument the PSR parameter
>> in a way that we can identify different panels out there that might
>> require or work better with link standby mode.
>>
>> It is also useful to say that for backward compatibility I'm not
>> changing the meaning of this flag. So "0" still means disabled
>> and "1" means enabled with full support and maximum power savings.
>>
>> v2: Use positive value instead of negative for different operation
>> mode
>>      as suggested by Daniel.
>>
>> v3: As Paulo suggested use 2 to force link standby and 3 to force
>> link
>>      fully on. Also split the link_standby introduction in a separated
>> patch.
>>
>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_params.c |  7 ++++++-
>>   drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 835d609..f78ddf3 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists,
>>   	"(-1=auto [default], 0=disabled, 1=enabled)");
>>   
>>   module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
>> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
>> +MODULE_PARM_DESC(enable_psr, "Enable PSR "
>> +		 "(0=disabled [default], 1=enabled - link mode
>> chosen per-platform, 2=force link-standby mode, 3=force link-off
>> mode)"
>> +		 "In case you needed to force any different option,
>> please "
>> +		 "report PCI device ID, subsystem vendor and
>> subsystem device ID "
>> +		 "to intel-gfx at lists.freedesktop.org, if your
>> machine needs it. "
>> +		 "It will then be included in an upcoming module
>> version.");
> Are we making a promise here? Isn't that dangerous? :P
> I'd just tell the users to open bug reports.
> (I'm not requiring you to change anything here, but something something
> lawyers something)
>
>>   
>>   module_param_named_unsafe(preliminary_hw_support,
>> i915.preliminary_hw_support, int, 0600);
>>   MODULE_PARM_DESC(preliminary_hw_support,
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>> b/drivers/gpu/drm/i915/intel_psr.c
>> index b84ec80..c3c2bb8 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -335,6 +335,12 @@ static bool intel_psr_match_conditions(struct
>> intel_dp *intel_dp)
>>   		return false;
>>   	}
>>   
>> +	if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) &&
>> +	    dev_priv->psr.link_standby) {
IS_VALLEYVIEW() will return true for both valleyview and cherryview so
the above check for cherryview can be removed.
> s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/
>
>
> Also, I'm not sure if this chunk belongs here or at intel_psr_init(),
> since it effectively disables PSR. This means that i915.enable_psr=3
> disables PSR on VLV/CHV. But maybe we shouldn't care since users
> shouldn't be using the option anyway. On the other hand, users may
> start claiming that i915.enable_psr=X "fixed PSR" for them while
> effectively it just disabled PSR, so perhaps DRM_ERROR would be better.
> Anyway, I'm not requesting any change, just pointing things in case you
> or someone else has any idea, but maybe I'd go with DRM_ERROR since
> users usually don't know which platform supports what, so the loud
> message may help them.
i agree, psr_match_conditions should check for parameters that can change
dynamically post boot to decide if we can enable psr or not,
if link_standby cannot be changed post boot we should check for it in init
  so we can avoid psr being enabled in the first place.
> Another check which we seem to be missing is "if (HAS_DDI(dev_priv) &&
> transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but this
> depends on the result of the discussion of patch 1.
>
> Everything else looks good, but it would be nice to see the opinions of
> maintainers here since they always have something to say about new
> i915.ko options.
>
>
>> +		DRM_DEBUG_KMS("PSR condition failed: Link off
>> requested/needed but not supported on this platform\n");
>> +		return false;
>> +	}
>> +
sorry i came late to this thread, but can you point me to some issues for
link off in CHT/VLV ? we have enabled link off in CHT Android and it seems
to be working fine. we can check again if we have missed something.
>>   	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>>   	    dig_port->port != PORT_A) {
>>   		DRM_DEBUG_KMS("PSR condition failed: Link Off
>> requested/needed but not supported on this port\n");
>> @@ -771,6 +777,7 @@ void intel_psr_init(struct drm_device *dev)
>>   	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>>   		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>>   
>> +	/* Set link_standby x link_off defaults */
>>   	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>>   		/*
>>   		 * On HSW and BDW Source implementation as an issue
>> with PSR
>> @@ -786,6 +793,16 @@ void intel_psr_init(struct drm_device *dev)
>>   		/* For new platforms let's respect VBT back again */
>>   		dev_priv->psr.link_standby = dev_priv-
>>> vbt.psr.full_link;
>>   
>> +	/* Override link_standby x link_off defaults */
>> +	if (i915.enable_psr == 2 && !dev_priv->psr.link_standby) {
>> +		DRM_DEBUG_KMS("PSR: Forcing link standby\n");
>> +		dev_priv->psr.link_standby = true;
>> +	}
>> +	if (i915.enable_psr == 3 && dev_priv->psr.link_standby) {
>> +		DRM_DEBUG_KMS("PSR: Forcing main link off\n");
>> +		dev_priv->psr.link_standby = false;
>> +	}
>> +
>>   	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>>   	mutex_init(&dev_priv->psr.lock);
>>   }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx