? Fi.CI.BAT: success for series starting with [1/5] drm/i915: Splitting intel_dp_detect
diff mbox

Message ID 87twjdx33g.fsf@intel.com
State New
Headers show

Commit Message

Jani Nikula April 7, 2016, 7:58 a.m. UTC
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?


BR,
Jani.

Comments

Ander Conselvan de Oliveira April 7, 2016, 8:24 a.m. UTC | #1
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
>  	/*
>  	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
>  	 * a dongle is present but no display. Unless we require to know
> 
> BR,
> Jani.
> 
> 
>
Sivakumar Thulasimani April 7, 2016, 9:56 a.m. UTC | #2
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.
>>
>>
>>
Tvrtko Ursulin April 7, 2016, 10 a.m. UTC | #3
On 07/04/16 08:58, 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);
> +
>   	/*
>   	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
>   	 * a dongle is present but no display. Unless we require to know

FWIW this patch fixes it on my BDW RVP.

I just had to change it to max_t since max has an issue with taking an 
address of const 1 by the look of it.

Regards,

Tvrtko
Shubhangi Shrivastava April 7, 2016, 10:12 a.m. UTC | #4
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@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

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.
>>>
>>>
>>>
>
Joonas Lahtinen April 7, 2016, 10:56 a.m. UTC | #5
On to, 2016-04-07 at 11:00 +0100, Tvrtko Ursulin wrote:
> On 07/04/16 08:58, Jani Nikula wrote:
> > 
> > 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);

It should be max(intel_dp->sink_count, (u8)1)

Which is essentially the same as max_t(u8, ...)

> > +
> >   	/*
> >   	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> >   	 * a dongle is present but no display. Unless we require to know
> FWIW this patch fixes it on my BDW RVP.
> 
> I just had to change it to max_t since max has an issue with taking an 
> address of const 1 by the look of it.

The problem is differing types, taking address of a constant is not a
problem, differing types when comparing pointers is.

The whole address taking line in max macro is there to make the pointer
type comparison at compile time.

Regards, joonas

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

Patch
diff mbox

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