| Message ID | 87twjdx33g.fsf@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > >
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. >> >> >>
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
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. >>> >>> >>> >
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
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