[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Thursday 07 April 2016 03:26 PM, Thulasimani, Sivakumar wrote:
>
>
> On 4/7/2016 1:54 PM, Ander Conselvan De Oliveira wrote:
>> On Thu, 2016-04-07 at 10:58 +0300, Jani Nikula wrote:
>>> On Wed, 06 Apr 2016, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> 
>>> wrote:
>>>> On 04/04/16 12:41, Tvrtko Ursulin wrote:
>>>>> On 04/04/16 12:08, Jani Nikula wrote:
>>>>>> On Mon, 04 Apr 2016, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>>>>> wrote:
>>>>>>> On 01/04/16 08:41, Ander Conselvan De Oliveira wrote:
>>>>>>>> On Thu, 2016-03-31 at 12:38 +0000, Patchwork wrote:
>>>>>>>>> == Series Details ==
>>>>>>>>>
>>>>>>>>> Series: series starting with [1/5] drm/i915: Splitting
>>>>>>>>> intel_dp_detect
>>>>>>>>> URL   : https://patchwork.freedesktop.org/series/5044/
>>>>>>>>> State : success
>>>>>>>> I pushed those to dinq.
>>>>>>> This series seems to break eDP detection on BDW RVP.
>>>>>> I presume this is due to the sink count check. Can you add debug 
>>>>>> logging
>>>>>> to print intel_dp->sink_count after it's been read in
>>>>>> intel_dp_get_dpcd() please?
>>>>> intel_dp->sink_count is zero here. (raw value, before the
>>>>> DP_GET_SINK_COUNT.)
>>>>>
>>>>> Also, intel_dp_dpcd_read_wake suggests a possibility for reading 
>>>>> garbage
>>>>> with not overly confident wording for the workaround there.
>>>>>
>>>>>> Then the question is, is this just because you have an RVP with who
>>>>>> knows what panel, or do we have to take into account potentially 
>>>>>> broken
>>>>>> panels too? Then I assume the fix would be to to ignore sink 
>>>>>> count for
>>>>>> eDP.
>>>>> No idea. :)
>>>> I could really use a solution for this. My only development 
>>>> platform is
>>>> incapacitated unless I revert this series which, apart from the extra
>>>> work when preparing and sending out patches this is taking, including
>>>> lost time waiting on CI which I suspect dislikes patches from top of
>>>> unknown bases, I think it won't be so easy to continue doing so 
>>>> when the
>>>> conflicts start arriving. :(
>>> Ander, Shubhangi?
>>>
>>> Would something like this be sensible? Tvrtko, can you give it a go?
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index da0c3d29fda8..0890e71db188 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3799,6 +3799,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>        */
>>>       intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
>>>   +    if (is_edp(intel_dp))
>>> +        intel_dp->sink_count = max(intel_dp->sink_count, 1);
>> I couldn't find anything in the spec that would make SINK_COUNT behave
>> differently for eDP, but eDP with 0 sinks simply doesn't make sense, 
>> so this
>> seems sensible to me.
>>
>> Ander
> its possible that manufacturers might not have filled sink count dpcd 
> registers.
> i would prefer ignoring sink_count for edp rather than hardcoding 1 in 
> case of edp.
>
> Also just to be safe, we should add a similar check in short pulse 
> handling too
> where we check sink count.
>
> Shubhangi, can you share a patch to handle this asap ?
>
> regards,
> Sivakumar

Shared the patch.. https://patchwork.freedesktop.org/patch/79924/

Shubhangi.

>>> /*
>>>        * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
>>>        * a dongle is present but no display. Unless we require to know
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>
>