Re: [PATCH 2/4] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>]

On 9/1/2015 6:45 PM, Jani Nikula wrote:
> On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> On 9/1/2015 3:59 PM, Jani Nikula wrote:
>>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>>>
>>>> This patch reads sink_count dpcd always and removes its
>>>> read operation based on values in downstream port dpcd.
>>>>
>>>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>>>> SINK_COUNT denotes if a display is attached, while
>>>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>>>> in the dongle where display can be attached. so it is possible
>>>> for sink count to change irrespective of value in downstream
>>>> port dpcd.
>>> This makes sense.
>>>
>>> I'm scared this may break something, if there are displays/adapters out
>>> there that don't set sink count properly. I guess only one way to find
>>> out... And this might actually fix something else.
>>>
>>>> Here is a table of possible values and scenarios
>>>>
>>>> sink_count      downstream_port
>>>>                   present
>>>> 0               0               no display is attached
>>>> 0               1               dongle is connected without display
>>>> 1               0               display connected directly
>>>> 1               1               display connected through dongle
>>> Do you think you handle this last case properly now? Previously we'd
>>> return with "connected" at the sink count check, but now we'll go for
>>> the "pke ddc gently" phase.
>>>
>>> See below.
>>>
>>>> v2: moved out crtc enabled checks to prior patch(Jani)
>>>>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>>>    1 file changed, 8 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 76561e0..9e4e27d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>>    	struct drm_device *dev = dig_port->base.base.dev;
>>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>    	uint8_t rev;
>>>> +	uint8_t reg;
>>>>    
>>>>    	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>>>    				    sizeof(intel_dp->dpcd)) < 0)
>>>> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>>    	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>>>    		return false; /* DPCD not present */
>>>>    
>>>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>>> +					    &reg, 1) < 0)
>>>> +			return false;
>>>> +
>>>> +	if (!DP_GET_SINK_COUNT(reg))
>>>> +		return false;
>>>> +
>>>>    	/* Check if the panel supports PSR */
>>>>    	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>>    	if (is_edp(intel_dp)) {
>>>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>>    	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>>>    		return connector_status_connected;
>>>>    
>>>> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>>>> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>>> -		uint8_t reg;
>>>> -
>>>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>>> -					    &reg, 1) < 0)
>>>> -			return connector_status_unknown;
>>>> -
>>>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>>>> -					      : connector_status_disconnected;
>>>> -	}
>>>> -
>>> Why do we proceed here if we know that we have sink count > 0 and we
>>> have a downstream port present? Admittedly I'm not sure I understand (or
>>> remember) why we had this logic in the first place...
>>>
>>> BR,
>>> Jani.
>> based on what little i can dig up, we proceed in case of dongles to
>> check if the
>> display is CRT or not.
>> if edid read succeeded, it is DP or any display with proper edid to work
>> with.
>> If edid read failed, then it could be CRT, without edid
> My point is, with the current code, if we have downstream port present
> and sink count > 0 and the first downsteam port supports hpd, we'll
> directly say status is connected. The hpd check may be bogus, but we
> still won't go probing ddc on them.
>
> With your patch, we'd always do the ddc probe if there is a downstream
> port present and the sink count > 0.
>
> BR,
> Jani.
understood, will check once by adding the old check if hpd supported 
downstream
is present and share new patch.
>
>
>> we can make it work with a fake edid, but it seems current code just
>> sets it as connector_status_unknown and ignores the display
>>
>>>>    	/* If no HPD, poke DDC gently */
>>>>    	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>>>    		return connector_status_connected;
>>>> -- 
>>>> 1.7.9.5
>>>>
>> -- 
>> regards,
>> Sivakumar
>>

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx