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

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

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@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@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
>>   	/*
>>   	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
>>   	 * a dongle is present but no display. Unless we require to know
>>
>> BR,
>> Jani.
>>
>>
>>

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