Re: [Intel-gfx] [PATCH V3 6/13] input: serio: use time_is_before_jiffies() instead of open coding it
On Tue, Feb 15, 2022 at 2:57 AM Qing Wang wrote:
>
> From: Wang Qing
>
> Use the helper function time_is_{before,after}_jiffies() to improve
> code readability.
>
> Signed-off-by: Wang Qing
> ---
Reviewed-by: Benjamin Tissoires
Cheers,
Benjamin
> drivers/input/serio/ps2-gpio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
> index 8970b49..7834296
> --- a/drivers/input/serio/ps2-gpio.c
> +++ b/drivers/input/serio/ps2-gpio.c
> @@ -136,7 +136,7 @@ static irqreturn_t ps2_gpio_irq_rx(struct ps2_gpio_data
> *drvdata)
> if (old_jiffies == 0)
> old_jiffies = jiffies;
>
> - if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> + if (time_is_before_jiffies(old_jiffies + usecs_to_jiffies(100))) {
> dev_err(drvdata->dev,
> "RX: timeout, probably we missed an interrupt\n");
> goto err;
> @@ -237,7 +237,7 @@ static irqreturn_t ps2_gpio_irq_tx(struct ps2_gpio_data
> *drvdata)
> if (old_jiffies == 0)
> old_jiffies = jiffies;
>
> - if ((jiffies - old_jiffies) > usecs_to_jiffies(100)) {
> + if (time_is_before_jiffies(old_jiffies + usecs_to_jiffies(100))) {
> dev_err(drvdata->dev,
> "TX: timeout, probably we missed an interrupt\n");
> goto err;
> --
> 2.7.4
>
Re: [Intel-gfx] [PATCH V3 5/13] hid: use time_is_after_jiffies() instead of open coding it
On Tue, Feb 15, 2022 at 2:56 AM Qing Wang wrote:
>
> From: Wang Qing
>
> Use the helper function time_is_{before,after}_jiffies() to improve
> code readability.
>
> Signed-off-by: Wang Qing
> Acked-by: Srinivas Pandruvada
FWIW, this one is
Acked-by: Benjamin Tissoires
Wang, is there any plan to take this series through the trivial tree
or should each maintainer take the matching patches?
Cheers,
Benjamin
> ---
> drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c
> b/drivers/hid/intel-ish-hid/ipc/ipc.c
> index 8ccb246..15e1423
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -578,7 +578,7 @@ static void _ish_sync_fw_clock(struct ishtp_device *dev)
> static unsigned longprev_sync;
> uint64_tusec;
>
> - if (prev_sync && jiffies - prev_sync < 20 * HZ)
> + if (prev_sync && time_is_after_jiffies(prev_sync + 20 * HZ))
> return;
>
> prev_sync = jiffies;
> --
> 2.7.4
>
Re: [Intel-gfx] [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
Hi Lv,
On May 27 2017 or thereabouts, Lv Zheng wrote:
> Both nouveau and i915, the only 2 kernel space lid notification listeners,
> invoke acpi_lid_open() API to obtain _LID returning value instead of using
> the notified value.
>
> So this patch moves this logic from listeners to lid driver, always notify
> kernel space listeners using _LID returning value.
>
> This is a no-op cleanup, but facilitates administrators to configure to
> notify kernel drivers with faked lid init states via command line
> "button.lid_notify_init_state=Y".
>
> Cc:
> Cc:
> Cc: Benjamin Tissoires
> Cc: Peter Hutterer
> Signed-off-by: Lv Zheng
> ---
> drivers/acpi/button.c | 16 ++--
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 4abf8ae..e047d34 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> static unsigned long lid_report_interval __read_mostly = 500;
> module_param(lid_report_interval, ulong, 0644);
> MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key
> events");
> +static bool lid_notify_init_state __read_mostly = false;
> +module_param(lid_notify_init_state, bool, 0644);
> +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel
> drivers after boot/resume");
>
> /* --
>FS Interface (/proc)
> @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device
> *device,
> if (state)
> pm_wakeup_event(&device->dev, 0);
>
> + if (!lid_notify_init_state) {
> + /*
> + * There are cases "state" is not a _LID return value, so
> + * correct it before notification.
> + */
> + if (!bios_notify &&
> + lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> + state = acpi_lid_evaluate_state(device);
> + }
> acpi_lid_notifier_call(device, state);
> }
>
> @@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val,
> struct kernel_param *kp)
>
> if (!strncmp(val, "open", sizeof("open") - 1)) {
> lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> - pr_info("Notify initial lid state as open\n");
> + pr_info("Notify initial lid state to users space as open and
> kernel drivers with _LID return value\n");
> } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> - pr_info("Notify initial lid state with _LID return value\n");
> + pr_info("Notify initial lid state to user/kernel space with
> _LID return value\n");
> } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> pr_info("Do not notify initial lid state\n");
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
> unsigned long val,
> /* Don't force modeset on machines where it causes a GPU lockup */
> if (dmi_check_system(intel_no_modeset_on_lid))
> goto exit;
> - if (!acpi_lid_open()) {
> + if (!val) {
> /* do modeset on next lid open event */
> dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> goto exit;
This last hunk should really be in its own patch because the intel GPU
folks would need to apply the rest of the series for their CI suite, and
also because there is no reason for this change to be alongside any
other acpi/button.c change.
Cheers,
Benjamin
> --
> 2.7.4
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
On Mon, May 15, 2017 at 10:42 AM, Jani Nikula wrote: > On Mon, 15 May 2017, Benjamin Tissoires wrote: >> I'll answer everything in the other thread, where there are slightly >> more other points raised: https://lkml.org/lkml/2017/5/15/10 > > If you are discussing changes impacting i915, please keep intel-gfx list > in the loop. > I can add intel-gfx to the other thread if you want, but this will IMO just add more noise to your list. The question is whether or not the kernel should provide a fake state for the _LID acpi call, and until we reach an agreement on how to handle things, there is no point changing the currently working code in i915. It is true that there is an issue in i915 regarding the fact that intel_lid_notify() doesn't use the provided value but calls acpi_lid_open(), but this is something that can be solved in https://bugs.freedesktop.org/show_bug.cgi?id=100923, when the situation clarifies. Cheers, Benjamin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
Hi Lv, On Mon, May 15, 2017 at 5:55 AM, Zheng, Lv wrote: > Hi, Benjamin > >> From: linux-acpi-ow...@vger.kernel.org >> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Benjamin >> Tissoires >> Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() >> invocations >> >> Hi Lv, >> >> I am trying to reduce the number of parallel discussion we have on the >> same subject, but there is something here I can't let you have. > > OK. So let's stop talking in PATCH 4-5. > They are actually cleanups from my point of view. > > In fact, I don't see any big conflicts between us. > > My point of view is: > There is a gap between (BIOS ensured/Windows expected) acpi control method > lid device behavior and Linux user space expected acpi control method lid > device behavior. > Button driver default behavior should be: button.lid_init_state=ignore > If user space programs have special needs, they can fix problems on their > own, via the following mean: > echo -n "open" > /sys/modules/button/parameters/lid_init_state > echo -n "close" > /sys/modules/button/parameters/lid_init_state > Keeping open/close modes is because I don't think there is any bug in button > driver. > So I need to prepare quirk modes from button driver's point of view and use > them as a response to related bug reports reported in acpi community. > Your point of view is: > There is a gap between (BIOS ensured/Windows expected) acpi control method > lid device behavior and Linux user space expected acpi control method lid > device behavior. > Button driver default behavior should be (not 100% sure if this is your > opinion): button.lid_init_state=method > If user space programs have special needs, they can fix them on their own, > via the following mean: > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:* > LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open > From this point of view, we actually don't need open/close modes. > > It seems we just need to determine the following first: > 1. Who should be responsible for solving bugs triggered by the conflict > between bios and linux user space expectations: >button driver? libinput? Some other user space programs? Users? > 2. What should be the default button driver behavior? >button.lid_init_state=ignore? button.lid_init_state=method? > 3. If non button driver quirks are working, button driver quirk modes are > useless. >The question is: Should button.lid_init_state=open/close be kept? > 4. From button driver's point of view, button.lid_init_state=ignore seems to > be always correct, so we won't abandon it. >If we can use libinput to manage platform quirks, then > button.lid_init_state=method also looks useless. >The question is: Should button.lid_init_state=method be kept? I'll answer everything in the other thread, where there are slightly more other points raised: https://lkml.org/lkml/2017/5/15/10 > > I should also let you know my preference: > 1. using button.lid_init_state=ignore and button.lid_init_state=method as > default behavior is ok for me if answer to 1 is not button driver, otherwise > using button.lid_init_state=method is not ok for me > 2. deletion of button.lid_init_state=open/close is ok for me if answer to 1 > is not button driver, otherwise deletion of button.lid_init_state=open/close > is not ok for me > 3. deletion of button.lid_init_state=method is always ok for me > > See the base line from my side is very clear: > If acpi community need to handle such bug reports, > button.lid_init_state=method cannot be the default behavior. > We are just using a different default behavior than "method" to drive things > to reach the final root caused solution. > > Could you let me know your preference so that we can figure out an agreement > between us. > Though I don't know if end users will buy it (they may keep on filing > regression reports in ACPI community). > > Can this make the discussion simpler? I really hope so :) Cheers, Benjamin > > So before determining whether we should keep button.lid_init_state=open/close > or not. > We obviously should stop talking here. > You can copy above questions to PATCH 1-2 discussion and reply in order to > stop this. > We can revisit PATCH 4-5 when the basic questions are answered. :) > >> >> On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv wrote: >> > Hi, >> > >> > If my previous reply is not persuasive enough. >> > Let me do that in a different way. >> > >> >> From: linux-acpi-ow...@vger.kernel.org >> >> [mailto:linu
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
Hi Lv, I am trying to reduce the number of parallel discussion we have on the same subject, but there is something here I can't let you have. On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv wrote: > Hi, > > If my previous reply is not persuasive enough. > Let me do that in a different way. > >> From: linux-acpi-ow...@vger.kernel.org >> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, >> Lv >> Subject: RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() >> invocations >> >> Hi, >> >> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@gmail.com] >> > Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() >> > invocations >> > >> > On Tue, May 9, 2017 at 9:02 AM, Lv Zheng wrote: >> > > Since notification side has been changed to always notify kernel >> > > listeners >> > > using _LID returning value. Now listeners needn't invoke acpi_lid_open(), >> > > it should use a spec suggested control method lid device usage model: >> > > register lid notification and use the notified value instead, which is >> > > the >> > > only way to ensure the value is correct for >> > > "button.lid_init_state=ignore" >> > > mode or other modes with "button.lid_fake_events=N" specified. >> > > >> > > This patch fixes i915 driver to drop acpi_lid_open() user. It's not >> > > possible to change nouveau_connector.c user using a simple way now. So >> > > this >> > > patch only marks acpi_lid_open() as deprecated to accelerate this process >> > > by indicating this change to the nouveau developers. >> > >> > If the only 2 users of acpi_lid_open() are intel and nouveau, and if >> > they should rely on the value stored in the input node, not the one >> > exported by the _LID method (which can be wrong on some platforms), >> > how about simply changing the implementation of acpi_lid_open() to >> > fetch the value from the input node directly? > > If acpi_lid_open() returns stored value in input node, > then it actually returns the value we notified to the input layer. > While i915 and nouveau explicitly do not trust that value and invoke > acpi_lid_open(). > > For button.lid_init_state=method, PATCH 4/5 is useless as the values are same. > > However in my reply of PATCH 2. > I explained the difference of method/close, for the same reason, we can also > sense the difference of method/open. > The difference then can explain the usefulness of open/close modes. > > Given open/close modes are all useful: > button.lid_init_state=open > 1. button driver sends open to input layer after boot/resume > 2. i915/nouveau uses _LID return value after boot/resume > If _LID return value after boot/resume is "close", they are different. > > button.lid_init_state=close > 1. button driver sends close to input layer after boot/resume > 2. i915/nouveau uses _LID return value after boot/resume > If _LID return value after boot/resume is "open", they are different. > > The only way to be consistent to old i915/nouveau behavior is: > button.lid_init_state=open/close But these two modes are wrong in the absolute case: - a laptop has no reasons for not being powered up with the lid physically closed (wake on lan?) -> so open is an approximation already made on good assumption that there is not a high chance of the users powering/resuming the laptop with the lid closed - in the "close" case, this setting works for docked laptops, but if the laptop can be docked, it can also be undocked. And if you boot with button.lid_init_state=close, undock your laptop, go into suspend, resume -> the lid state is now "closed" while it should be opened. So no, these are just workarounds. i915/nouveau expect the acpi/button state to be reliable, or they should ignore it. But you can't fake events when you are not absolutely sure of what is happening. > 1. button driver sends open/close to input layer after boot/resume > 2. button driver sends _LID return value to i915 after boot/resume (PATCH 4) > So that i915 can just use the notified value in this patch (PATCH 5). > For nouveau, no change has been made for now, and as a concequence, > acpi_lid_open() is still kept but marked as deprecated. > >> >> See my reply of PATCH 4. >> It seems they trust _LID return value more than what we send to them. >> >> We can actually send faked "open/close" to them when >> button.lid_init_state=open/close is specified. >> >> So these 2 patches [PATCH 4-5/5] are used to do a smal
Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
On Tue, May 9, 2017 at 9:02 AM, Lv Zheng wrote:
> Since notification side has been changed to always notify kernel listeners
> using _LID returning value. Now listeners needn't invoke acpi_lid_open(),
> it should use a spec suggested control method lid device usage model:
> register lid notification and use the notified value instead, which is the
> only way to ensure the value is correct for "button.lid_init_state=ignore"
> mode or other modes with "button.lid_fake_events=N" specified.
>
> This patch fixes i915 driver to drop acpi_lid_open() user. It's not
> possible to change nouveau_connector.c user using a simple way now. So this
> patch only marks acpi_lid_open() as deprecated to accelerate this process
> by indicating this change to the nouveau developers.
If the only 2 users of acpi_lid_open() are intel and nouveau, and if
they should rely on the value stored in the input node, not the one
exported by the _LID method (which can be wrong on some platforms),
how about simply changing the implementation of acpi_lid_open() to
fetch the value from the input node directly?
Cheers,
Benjamin
>
> Cc:
> Cc:
> Signed-off-by: Lv Zheng
> ---
> drivers/acpi/button.c | 7 ++-
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 7ff3a75..50d7898 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct notifier_block
> *nb)
> }
> EXPORT_SYMBOL(acpi_lid_notifier_unregister);
>
> -int acpi_lid_open(void)
> +/*
> + * The intentional usage model is to register a lid notifier and use the
> + * notified value instead. Directly evaluating _LID without seeing a
> + * Notify(lid, 0x80) is not reliable.
> + */
> +int __deprecated acpi_lid_open(void)
> {
> if (!lid_device)
> return -ENODEV;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
> unsigned long val,
> /* Don't force modeset on machines where it causes a GPU lockup */
> if (dmi_check_system(intel_no_modeset_on_lid))
> goto exit;
> - if (!acpi_lid_open()) {
> + if (!val) {
> /* do modeset on next lid open event */
> dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> goto exit;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] ACPI: Switch to use generic UUID API
On May 04 2017 or thereabouts, Andy Shevchenko wrote: > acpi_evaluate_dsm() and friends take a pointer to a raw buffer of 16 > bytes. Instead we convert them to use uuid_le type. At the same time we > convert current users. > > acpi_str_to_uuid() becomes useless after the conversion and it's safe to > get rid of it. > > The conversion fixes a potential bug in int340x_thermal as well since > we have to use memcmp() on binary data. > > Cc: Rafael J. Wysocki > Cc: Mika Westerberg > Cc: Borislav Petkov > Cc: Dan Williams > Cc: Amir Goldstein > Cc: Jarkko Sakkinen > Cc: Jani Nikula > Cc: Ben Skeggs > Cc: Benjamin Tissoires > Cc: Joerg Roedel > Cc: Adrian Hunter > Cc: Yisen Zhuang > Cc: Bjorn Helgaas > Cc: Zhang Rui > Cc: Felipe Balbi > Cc: Mathias Nyman > Cc: Heikki Krogerus > Cc: Liam Girdwood > Cc: Mark Brown > Signed-off-by: Andy Shevchenko > --- For i2c-hid: Acked-by: Benjamin Tissoires ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
On Aug 26 2015 or thereabouts, Sivakumar Thulasimani wrote: > > > On 8/18/2015 1:36 AM, Benjamin Tissoires wrote: > >On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote: > >>On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires > >> wrote: > >>>On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote: > >>>> > >>>>On 7/29/2015 8:52 PM, Benjamin Tissoires wrote: > >>>>>On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote: > >>>>>>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way > >>>>>>you can > >>>>>>identify both lane count and reversal state without touching anything > >>>>>>in the > >>>>>>link training code. i am yet to upstream my changes for CHT that i can > >>>>>>share > >>>>>>if required that does the same in intel_dp_detect without touching any > >>>>>>line > >>>>>>in link training path. > >>>>>With my current limited knowledge of the dp hotplug (and i915 driver) I > >>>>>am not sure we could detect the reversed state without trying to train 1 > >>>>>lane only. I'd be glad to look at your changes and test them on my > >>>>>system if you think that could help having a cleaner solution. > >>>>> > >>>>>Cheers, > >>>>>Benjamin > >>>>No, what i recommended was to do link training but in intel_dp_detect. > >>>>Since > >>>>USB Type C cable > >>>>also has its own lane count restriction (it can have different lane count > >>>>than the one supported > >>>>by panel) you might have to figure that out as well. so both reversal and > >>>>lane count detection > >>>>can be done outside the modeset path and keep the code free of type C > >>>>changes outside > >>>>detection path. > >>>> > >>>>Please find below the code to do the same. Do not waste time trying to > >>>>apply > >>>>this directly on > >>>>nightly since this is based on a local tree and because this is pre- > >>>>atomic > >>>>changes code, so you > >>>>might have to modify chv_upfront_link_train to work on top of the latest > >>>>nightly code. we > >>>>are supposed to upstream this and is in my todo list. > >>>> > >>>[original patch snipped...] > >>> > >>>Hi Sivakumar, > >>> > >>>So I managed to manually re-apply your patch on top of > >>>drm-intel-nightly, and tried to port it to make Broadwell working too. > >>>It works OK if the system is already boot without any external DP used. > >>>In this case, the detection works and I can see my external monitor > >>>working properly. > >>> > >>>However, if the monitor is cold plugged, the cpu/GPU hangs and I can not > >>>understand why. I think I enabled all that is mentioned in the PRM to be > >>>able to train the DP link, but I am obviously missing something else. > >>>Can you have a look? > >>> > >>Hi Benjamin, > >> > >>I would recommend against this approach. Some adapters will claim that > >>they recovered a clock even when it isn't on the lanes you enabled, > >>which means that the reversal detection doesn't always work. The only > >>reliable way to do this is to go talk to the Chrome OS EC (you can > >>find these patches later in the Chrome OS tree). It's not as generic, > >>but we might be able to abstract that logic, maybe. > >> > >Hi Stéphane, > > > >This is a very good news. I was afraid we would not have access to the > >hardware controller because the Intel controller hub spec was not > >public. > > > >I will try to have a look at it, but the latest chromeos branch (3.18) > >seems to differ quite a lot from the upstream one. Anyway, fingers > >crossed. > > > >Cheers, > >Benjamin > Hi Benjamin/Stéphan, Hi Sivakumar, > All Intel platforms (at-least those i inquired about) handle lane > reversal in HW. That is the theory and what is written in the USB Type C spec IIRC. Problem is, the Chromebook Pixel 2 external display does not work when a USB Type-C adapter is in the reversed position (or believe me, I would definitively not have submitted a patch for the beauty of it). Everything else works (link training when 4 lanes are activated, or other communication channels). Only the order of the 4 data lanes matters in this situation and the USB controller does not reverse them for us on this laptop. > your statement that link training will pass even on reversed lane seems to > point > to the same fact. in such a scenario why should the encoder/connector care > if the lane is reversed or not ? Problem is that Stephane said some adapters are lying regarding the clock recovery. They claim everything is fine while in the end, the display doesn't show anything because the lanes are reversed. If this is just a chromebook Pixel 2 issue, that's better then. I won't have to try to put some generic interface to notify that the display port lanes have to be reversed. Cheers, Benjamin > > -- > regards, > Sivakumar > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
On Aug 14 2015 or thereabouts, Stéphane Marchesin wrote: > On Wed, Aug 5, 2015 at 12:34 PM, Benjamin Tissoires > wrote: > > On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote: > >> > >> > >> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote: > >> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote: > >> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you > >> >>can > >> >>identify both lane count and reversal state without touching anything in > >> >>the > >> >>link training code. i am yet to upstream my changes for CHT that i can > >> >>share > >> >>if required that does the same in intel_dp_detect without touching any > >> >>line > >> >>in link training path. > >> >With my current limited knowledge of the dp hotplug (and i915 driver) I > >> >am not sure we could detect the reversed state without trying to train 1 > >> >lane only. I'd be glad to look at your changes and test them on my > >> >system if you think that could help having a cleaner solution. > >> > > >> >Cheers, > >> >Benjamin > >> No, what i recommended was to do link training but in intel_dp_detect. > >> Since > >> USB Type C cable > >> also has its own lane count restriction (it can have different lane count > >> than the one supported > >> by panel) you might have to figure that out as well. so both reversal and > >> lane count detection > >> can be done outside the modeset path and keep the code free of type C > >> changes outside > >> detection path. > >> > >> Please find below the code to do the same. Do not waste time trying to > >> apply > >> this directly on > >> nightly since this is based on a local tree and because this is pre- atomic > >> changes code, so you > >> might have to modify chv_upfront_link_train to work on top of the latest > >> nightly code. we > >> are supposed to upstream this and is in my todo list. > >> > > > > [original patch snipped...] > > > > Hi Sivakumar, > > > > So I managed to manually re-apply your patch on top of > > drm-intel-nightly, and tried to port it to make Broadwell working too. > > It works OK if the system is already boot without any external DP used. > > In this case, the detection works and I can see my external monitor > > working properly. > > > > However, if the monitor is cold plugged, the cpu/GPU hangs and I can not > > understand why. I think I enabled all that is mentioned in the PRM to be > > able to train the DP link, but I am obviously missing something else. > > Can you have a look? > > > > Hi Benjamin, > > I would recommend against this approach. Some adapters will claim that > they recovered a clock even when it isn't on the lanes you enabled, > which means that the reversal detection doesn't always work. The only > reliable way to do this is to go talk to the Chrome OS EC (you can > find these patches later in the Chrome OS tree). It's not as generic, > but we might be able to abstract that logic, maybe. > Hi Stéphane, This is a very good news. I was afraid we would not have access to the hardware controller because the Intel controller hub spec was not public. I will try to have a look at it, but the latest chromeos branch (3.18) seems to differ quite a lot from the upstream one. Anyway, fingers crossed. Cheers, Benjamin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>
>
> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >>identify both lane count and reversal state without touching anything in the
> >>link training code. i am yet to upstream my changes for CHT that i can share
> >>if required that does the same in intel_dp_detect without touching any line
> >>in link training path.
> >With my current limited knowledge of the dp hotplug (and i915 driver) I
> >am not sure we could detect the reversed state without trying to train 1
> >lane only. I'd be glad to look at your changes and test them on my
> >system if you think that could help having a cleaner solution.
> >
> >Cheers,
> >Benjamin
> No, what i recommended was to do link training but in intel_dp_detect. Since
> USB Type C cable
> also has its own lane count restriction (it can have different lane count
> than the one supported
> by panel) you might have to figure that out as well. so both reversal and
> lane count detection
> can be done outside the modeset path and keep the code free of type C
> changes outside
> detection path.
>
> Please find below the code to do the same. Do not waste time trying to apply
> this directly on
> nightly since this is based on a local tree and because this is pre- atomic
> changes code, so you
> might have to modify chv_upfront_link_train to work on top of the latest
> nightly code. we
> are supposed to upstream this and is in my todo list.
>
[original patch snipped...]
Hi Sivakumar,
So I managed to manually re-apply your patch on top of
drm-intel-nightly, and tried to port it to make Broadwell working too.
It works OK if the system is already boot without any external DP used.
In this case, the detection works and I can see my external monitor
working properly.
However, if the monitor is cold plugged, the cpu/GPU hangs and I can not
understand why. I think I enabled all that is mentioned in the PRM to be
able to train the DP link, but I am obviously missing something else.
Can you have a look?
My patch is now:
From 11e9c42f55ae27bd6e7b0168bf24d15082c7d517 Mon Sep 17 00:00:00 2001
From: Durgadoss R
Date: Mon, 3 Aug 2015 11:51:16 -0400
Subject: [PATCH] drm/i915: Enable Upfront link training for type-C DP support
To support USB type C alternate DP mode, the display driver needs to know the
number of lanes required by DP panel as well as number of lanes that can be
supported by the type-C cable. Sometimes, the type-C cable may limit the
bandwidth even if Panel can support more lanes. To address these scenarios,
the display driver will start link training with max lanes, and if the link
training fails, the driver then falls back to x2 lanes.
* Since link training is done before modeset, planes are not enabled. Only
encoder and the its associated PLLs are enabled.
* Once link training is done, the encoder and its PLLs are disabled; so that
the subsequent modeset is not aware of these changes.
* As of now, this is tested only on CHV.
Signed-off-by: Durgadoss R
[BT: add broadwell support and USB-C reverted state detection]
Signed-off-by: Benjamin Tissoires
---
drivers/gpu/drm/i915/intel_display.c | 163 +++
drivers/gpu/drm/i915/intel_dp.c | 22 -
drivers/gpu/drm/i915/intel_drv.h | 7 ++
3 files changed, 190 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 818f3a3..e8ddba0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15799,3 +15799,166 @@ void intel_modeset_preclose(struct drm_device *dev,
struct drm_file *file)
spin_unlock_irq(&dev->event_lock);
}
}
+
+static bool
+intel_upfront_link_train_config(struct drm_device *dev,
+ struct intel_dp *intel_dp,
+ struct intel_crtc *crtc,
+ uint8_t link_bw,
+ uint8_t lane_count)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_connector *connector = intel_dp->attached_connector;
+ struct intel_encoder *encoder = connector->encoder;
+ bool success;
+
+ intel_dp->link_bw = link_bw;
+ intel_dp->lane_count = lane_count;
+
+ /* Find port clock from link_bw */
+ crtc->config->port_clock =
drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+ /* Enable PLL followed by port */
+ if (IS_BROADWELL(dev)) {
+ hsw_dp_set_ddi_pll_sel(crtc->config, intel_dp->link_bw);
+ if (intel_crtc_to_s
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
On Jul 30 2015 or thereabouts, Sivakumar Thulasimani wrote:
>
>
> On 7/29/2015 8:52 PM, Benjamin Tissoires wrote:
> >On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> >>why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> >>identify both lane count and reversal state without touching anything in the
> >>link training code. i am yet to upstream my changes for CHT that i can share
> >>if required that does the same in intel_dp_detect without touching any line
> >>in link training path.
> >With my current limited knowledge of the dp hotplug (and i915 driver) I
> >am not sure we could detect the reversed state without trying to train 1
> >lane only. I'd be glad to look at your changes and test them on my
> >system if you think that could help having a cleaner solution.
> >
> >Cheers,
> >Benjamin
> No, what i recommended was to do link training but in intel_dp_detect. Since
> USB Type C cable
> also has its own lane count restriction (it can have different lane count
> than the one supported
> by panel) you might have to figure that out as well. so both reversal and
> lane count detection
> can be done outside the modeset path and keep the code free of type C
> changes outside
> detection path.
Thanks for the detailed explanations. Now I see what you mean and
definitively understand why this is better.
>
> Please find below the code to do the same. Do not waste time trying to apply
> this directly on
> nightly since this is based on a local tree and because this is pre- atomic
> changes code, so you
> might have to modify chv_upfront_link_train to work on top of the latest
> nightly code. we
> are supposed to upstream this and is in my todo list.
Thanks for this patch. I'll try to adapt it to test on the Broadwell
laptop I have and get the reversed state detected here.
Cheers,
Benjamin
>
> ---
>
> Author: Durgadoss R
> Date: Fri May 22 14:30:07 2015 +0530
>
>drm/i915: Enable Upfront link training for type-C DP support
>
> To support USB type C alternate DP mode, the display driver needs to
> know the
> number of lanes required by DP panel as well as number of lanes that can
> be
> supported by the type-C cable. Sometimes, the type-C cable may limit the
> bandwidth even if Panel can support more lanes. To address these
> scenarios,
> the display driver will start link training with max lanes, and if the
> link
> training fails, the driver then falls back to x2 lanes.
>
> * Since link training is done before modeset, planes are not enabled.
> Only
> encoder and the its associated PLLs are enabled.
> * Once link training is done, the encoder and its PLLs are disabled; so
> that
> the subsequent modeset is not aware of these changes.
> * As of now, this is tested only on CHV.
>
> Signed-off-by: Durgadoss R
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0c8ae2a..c72dcaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14793,3 +14793,121 @@ intel_display_print_error_state(struct
> drm_i915_error_state_buf *m,
> err_printf(m, " VSYNC: %08x\n", error->transcoder[i].vsync);
> }
> }
> +
> +bool chv_upfront_link_train(struct drm_device *dev,
> +struct intel_dp *intel_dp, struct intel_crtc *crtc)
> +{
> +struct drm_i915_private *dev_priv = dev->dev_private;
> +struct intel_connector *connector = intel_dp->attached_connector;
> +struct intel_encoder *encoder = connector->encoder;
> +bool found = false;
> +bool valid_crtc = false;
> +
> +if (!connector || !encoder) {
> +DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
> +return false;
> +}
> +
> +/* If we already have a crtc, start link training directly */
> +if (crtc) {
> +valid_crtc = true;
> +goto start_link_train;
> +}
> +
> +/* Find an unused crtc and use it for link training */
> +for_each_intel_crtc(dev, crtc) {
> +if (intel_crtc_active(&crtc->base))
> +continue;
> +
> +connector->new_encoder = encoder;
> +encoder->new_crtc = crtc;
> +encoder->base.crtc = &crtc->base;
> +
> +/* Make sure the new crtc will work with the encoder */
> +if (drm_encoder_crtc_ok(&encoder->base,
> + &crtc->base)) {
> +
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
On Jul 29 2015 or thereabouts, Sivakumar Thulasimani wrote:
> why not detect reverse in intel_dp_detect/intel_hpd_pulse ? that way you can
> identify both lane count and reversal state without touching anything in the
> link training code. i am yet to upstream my changes for CHT that i can share
> if required that does the same in intel_dp_detect without touching any line
> in link training path.
With my current limited knowledge of the dp hotplug (and i915 driver) I
am not sure we could detect the reversed state without trying to train 1
lane only. I'd be glad to look at your changes and test them on my
system if you think that could help having a cleaner solution.
Cheers,
Benjamin
>
> On 7/28/2015 9:33 PM, Benjamin Tissoires wrote:
> >The DP outputs connected through a USB Type-C port can have inverted
> >lanes. To detect that case, we implement autodetection by training only
> >the first lane if it doesn't work, we assume that we need to invert
> >the lanes.
> >
> >Tested on a Chromebook Pixel 2015 (samus) with a USB Type-C to HDMI
> >adapter and a Dell 4K and some various regular monitors.
> >
> >Based on 2 patches from the ChromeOS tree by:
> >Stéphane Marchesin
> >Todd Broch
> >
> >Signed-off-by: Benjamin Tissoires
> >---
> > drivers/gpu/drm/i915/intel_ddi.c | 13 +
> > drivers/gpu/drm/i915/intel_dp.c | 36
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 50 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >b/drivers/gpu/drm/i915/intel_ddi.c
> >index 9a40bfb..0b0c1ec 100644
> >--- a/drivers/gpu/drm/i915/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >@@ -2249,6 +2249,7 @@ static void intel_ddi_pre_enable(struct intel_encoder
> >*intel_encoder)
> > enum port port = intel_ddi_get_encoder_port(intel_encoder);
> > int type = intel_encoder->type;
> > int hdmi_level;
> >+bool reversed = false;
> > if (type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >@@ -2295,8 +2296,20 @@ static void intel_ddi_pre_enable(struct intel_encoder
> >*intel_encoder)
> > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >+if (IS_BROADWELL(dev) && type == INTEL_OUTPUT_DISPLAYPORT) {
> >+intel_ddi_init_dp_buf_reg(intel_encoder);
> >+reversed = intel_dp_is_reversed(intel_dp);
> >+}
> >+
> > intel_ddi_init_dp_buf_reg(intel_encoder);
> >+if (IS_BROADWELL(dev)) {
> >+if (reversed)
> >+intel_dp->DP |= DDI_BUF_PORT_REVERSAL;
> >+else
> >+intel_dp->DP &= ~DDI_BUF_PORT_REVERSAL;
> >+}
> >+
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > intel_dp_start_link_train(intel_dp);
> > intel_dp_complete_link_train(intel_dp);
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >b/drivers/gpu/drm/i915/intel_dp.c
> >index b740987..18280cc 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -3820,6 +3820,42 @@ intel_dp_complete_link_train(struct intel_dp
> >*intel_dp)
> > intel_dp->DP = DP;
> > }
> >+bool intel_dp_is_reversed(struct intel_dp *intel_dp)
> >+{
> >+struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> >+struct drm_device *dev = encoder->dev;
> >+struct drm_i915_private *dev_priv = dev->dev_private;
> >+uint32_t DP = intel_dp->DP;
> >+
> >+/*
> >+ * Train with 1 lane. There is no guarantee that the monitor supports
> >+ * 2 or 4 lanes, and we wouldn't see any asymetricity with 4 lanes.
> >+ */
> >+const uint8_t lane_count = 1;
> >+bool reversed;
> >+
> >+if (!HAS_DDI(dev))
> >+return false;
> >+
> >+DP &= ~(DDI_BUF_PORT_REVERSAL | DDI_PORT_WIDTH(4));
> >+DP |= DDI_PORT_WIDTH(lane_count);
> >+
> >+I915_WRITE(intel_dp->output_reg, DP);
> >+POSTING_READ(intel_dp->output_reg);
> >+udelay(600);
> >+
> >+if (!_intel_dp_start_link_train(intel_dp, lane_count, &DP, true))
> >+return true;
> >+
> >+reversed = !_intel_dp_complete_link_train(intel_dp, lane_cou
Re: [Intel-gfx] [PATCH 2/3] drm/i915: hide errors when probing for a reverse display port
On Jul 28 2015 or thereabouts, Chris Wilson wrote: > On Tue, Jul 28, 2015 at 12:03:28PM -0400, Benjamin Tissoires wrote: > > We check the polarity of the attached dp, so it is normal to fail. > > Do not send errors to the users. > > if (probe) DRM_DEBUG else DRM_ERROR is fairly offensive. > > It strikes me that you could make each of these functions report the > failure to the caller (have the caller even do so error handling!) and > as part of that have the caller report an error and so demote all of > these to DRM_DEBUG_KMS(). Yes, sorry for that. I will change it to return an actual error code and the error string if I still need to use these functions given Sivakumar ideas. > > Who knows, actually doing some error handling may make monitor training > more reliable! Or we may even get carried away and report the failure > all the way back to userspace. That would be a very good improvement indeed. But I can already tell you that I will not do it by myself, I already have too much on my plate. I'll do my share for this feature, but don't count on me for the whole error handling rewrite :) Cheers, Benjamin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/3] drm/i915: fix USB Type-C reversed connector
Hi, plugging a USB Type-C to HDMI adapter on a Chromebook Pixel 2015 works only if the HDMI port is in its normal (not reverted) state. Theorically, the USB chip should provide the information whether or not the lane are resversed, but I could not find anything in the Intel PRM regarding this. So use the technical solution implemented in ChromeOS by Stéphane and Todd: try to train one lane in the normal setting, if it doesn't work, then chances are that the lane are reverted. The ChromeOS commits are: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e8c654a0a6d466b3a7b0e84fd27e3a7236a2243e https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4fed50fad79ba3fde782d913bac2968b2d0262bc If I could have a Signed-off-by by Stéphane and Todd, that would be even better :) I should also note that while testing this, I discovered 2 regressions in drm-intel-nightly: - b432e5cfd (drm/i915: BDW clock change support): this one prevents the Broadwell-U GPU to correctly set the clock when we connect a 4K monitor over HDMI (30.0 Hz) Using a lower resolution works (the internal display is 2560x1700) I ended up disabling in intel_display.c all of the codes that are broadwell specific, and the default are just working. - aaf5ec2e5 (drm/i915: Handle HPD when it has actually occurred): this one raises a storm of errors: [drm:gen8_irq_handler [i915]] *ERROR* The master control interrupt lied (SDE)! This doesn't seems to affect the DP output, but having that many errors is not always a good sign IMO :) Reverting this commit makes them go away. Cheers, Benjamin Benjamin Tissoires (3): drm/i915: add parameters to dp_start_link_train and dp_complete_link_train drm/i915: hide errors when probing for a reverse display port drm/i915: Support DDI lane reversal for DP drivers/gpu/drm/i915/intel_ddi.c | 13 +++ drivers/gpu/drm/i915/intel_dp.c | 173 ++- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 149 insertions(+), 38 deletions(-) -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: add parameters to dp_start_link_train and dp_complete_link_train
In order to detect if the Display Port is reversed or not (when connected
to a USC type-C connector), we need to probe the training with one lane
to check if the polarity is correct.
Factor out the code that we need later on.
This commit has no functional change
Signed-off-by: Benjamin Tissoires
---
drivers/gpu/drm/i915/intel_dp.c | 75 ++---
1 file changed, 47 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f93..f2352d8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3566,15 +3566,15 @@ static void intel_dp_set_idle_link_train(struct
intel_dp *intel_dp)
}
/* Enable corresponding port and start training pattern 1 */
-void
-intel_dp_start_link_train(struct intel_dp *intel_dp)
+static bool
+_intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
+ uint32_t *DP)
{
struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
struct drm_device *dev = encoder->dev;
int i;
uint8_t voltage;
int voltage_tries, loop_tries;
- uint32_t DP = intel_dp->DP;
uint8_t link_config[2];
if (HAS_DDI(dev))
@@ -3582,7 +3582,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
/* Write the link configuration data */
link_config[0] = intel_dp->link_bw;
- link_config[1] = intel_dp->lane_count;
+ link_config[1] = lane_count;
if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
@@ -3594,14 +3594,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
link_config[1] = DP_SET_ANSI_8B10B;
drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
- DP |= DP_PORT_EN;
+ *DP |= DP_PORT_EN;
/* clock recovery */
- if (!intel_dp_reset_link_train(intel_dp, &DP,
+ if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
- return;
+ return false;
}
voltage = 0xff;
@@ -3616,7 +3616,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
break;
}
- if (drm_dp_clock_recovery_ok(link_status,
intel_dp->lane_count)) {
+ if (drm_dp_clock_recovery_ok(link_status, lane_count)) {
DRM_DEBUG_KMS("clock recovery OK\n");
break;
}
@@ -3629,26 +3629,26 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
DRM_DEBUG_KMS("clock recovery not ok, reset");
/* clear the flag as we are not reusing train set */
intel_dp->train_set_valid = false;
- if (!intel_dp_reset_link_train(intel_dp, &DP,
+ if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
- return;
+ return false;
}
continue;
}
/* Check to see if we've tried the max voltage */
- for (i = 0; i < intel_dp->lane_count; i++)
+ for (i = 0; i < lane_count; i++)
if ((intel_dp->train_set[i] &
DP_TRAIN_MAX_SWING_REACHED) == 0)
break;
- if (i == intel_dp->lane_count) {
+ if (i == lane_count) {
++loop_tries;
if (loop_tries == 5) {
DRM_ERROR("too many full retries, give up\n");
break;
}
- intel_dp_reset_link_train(intel_dp, &DP,
+ intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE);
voltage_tries = 0;
@@ -3667,21 +3667,31 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
/* Update training set as requested by target */
- if (!intel_dp_update_link_train(intel_dp, &DP,
[Intel-gfx] [PATCH 3/3] drm/i915: Support DDI lane reversal for DP
The DP outputs connected through a USB Type-C port can have inverted
lanes. To detect that case, we implement autodetection by training only
the first lane if it doesn't work, we assume that we need to invert
the lanes.
Tested on a Chromebook Pixel 2015 (samus) with a USB Type-C to HDMI
adapter and a Dell 4K and some various regular monitors.
Based on 2 patches from the ChromeOS tree by:
Stéphane Marchesin
Todd Broch
Signed-off-by: Benjamin Tissoires
---
drivers/gpu/drm/i915/intel_ddi.c | 13 +
drivers/gpu/drm/i915/intel_dp.c | 36
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9a40bfb..0b0c1ec 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2249,6 +2249,7 @@ static void intel_ddi_pre_enable(struct intel_encoder
*intel_encoder)
enum port port = intel_ddi_get_encoder_port(intel_encoder);
int type = intel_encoder->type;
int hdmi_level;
+ bool reversed = false;
if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
@@ -2295,8 +2296,20 @@ static void intel_ddi_pre_enable(struct intel_encoder
*intel_encoder)
if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ if (IS_BROADWELL(dev) && type == INTEL_OUTPUT_DISPLAYPORT) {
+ intel_ddi_init_dp_buf_reg(intel_encoder);
+ reversed = intel_dp_is_reversed(intel_dp);
+ }
+
intel_ddi_init_dp_buf_reg(intel_encoder);
+ if (IS_BROADWELL(dev)) {
+ if (reversed)
+ intel_dp->DP |= DDI_BUF_PORT_REVERSAL;
+ else
+ intel_dp->DP &= ~DDI_BUF_PORT_REVERSAL;
+ }
+
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b740987..18280cc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3820,6 +3820,42 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
intel_dp->DP = DP;
}
+bool intel_dp_is_reversed(struct intel_dp *intel_dp)
+{
+ struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
+ struct drm_device *dev = encoder->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint32_t DP = intel_dp->DP;
+
+ /*
+* Train with 1 lane. There is no guarantee that the monitor supports
+* 2 or 4 lanes, and we wouldn't see any asymetricity with 4 lanes.
+*/
+ const uint8_t lane_count = 1;
+ bool reversed;
+
+ if (!HAS_DDI(dev))
+ return false;
+
+ DP &= ~(DDI_BUF_PORT_REVERSAL | DDI_PORT_WIDTH(4));
+ DP |= DDI_PORT_WIDTH(lane_count);
+
+ I915_WRITE(intel_dp->output_reg, DP);
+ POSTING_READ(intel_dp->output_reg);
+ udelay(600);
+
+ if (!_intel_dp_start_link_train(intel_dp, lane_count, &DP, true))
+ return true;
+
+ reversed = !_intel_dp_complete_link_train(intel_dp, lane_count, &DP,
true);
+
+ /* clear training, we had only one lane */
+ intel_dp->train_set_valid = false;
+
+ return reversed;
+
+}
+
void intel_dp_stop_link_train(struct intel_dp *intel_dp)
{
intel_dp_set_link_train(intel_dp, &intel_dp->DP,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6..cba00c6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1169,6 +1169,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
bool intel_dp_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *pipe_config);
bool intel_dp_is_edp(struct drm_device *dev, enum port port);
+bool intel_dp_is_reversed(struct intel_dp *intel_dp);
enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
bool long_hpd);
void intel_edp_backlight_on(struct intel_dp *intel_dp);
--
2.4.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: hide errors when probing for a reverse display port
We check the polarity of the attached dp, so it is normal to fail.
Do not send errors to the users.
Signed-off-by: Benjamin Tissoires
---
drivers/gpu/drm/i915/intel_dp.c | 74 -
1 file changed, 58 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2352d8..b740987 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3568,7 +3568,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp
*intel_dp)
/* Enable corresponding port and start training pattern 1 */
static bool
_intel_dp_start_link_train(struct intel_dp *intel_dp, uint8_t lane_count,
- uint32_t *DP)
+ uint32_t *DP, bool probing)
{
struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
struct drm_device *dev = encoder->dev;
@@ -3576,6 +3576,7 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
uint8_t voltage;
int voltage_tries, loop_tries;
uint8_t link_config[2];
+ char *error_str;
if (HAS_DDI(dev))
intel_ddi_prepare_link_retrain(encoder);
@@ -3600,7 +3601,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
- DRM_ERROR("failed to enable link training\n");
+ error_str = "failed to enable link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
return false;
}
@@ -3612,7 +3617,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
if (!intel_dp_get_link_status(intel_dp, link_status)) {
- DRM_ERROR("failed to get link status\n");
+ error_str = "failed to get link status\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
@@ -3632,7 +3641,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
if (!intel_dp_reset_link_train(intel_dp, DP,
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
- DRM_ERROR("failed to enable link training\n");
+ error_str = "failed to enable link training\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
return false;
}
continue;
@@ -3645,7 +3658,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
if (i == lane_count) {
++loop_tries;
if (loop_tries == 5) {
- DRM_ERROR("too many full retries, give up\n");
+ error_str = "too many full retries, give up\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
intel_dp_reset_link_train(intel_dp, DP,
@@ -3659,7 +3676,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) ==
voltage) {
++voltage_tries;
if (voltage_tries == 5) {
- DRM_ERROR("too many voltage retries, give
up\n");
+ error_str = "too many voltage retries, give
up\n";
+ if (!probing)
+ DRM_ERROR(error_str);
+ else
+ DRM_DEBUG(error_str);
break;
}
} else
@@ -3668,7 +3689,11 @@ _intel_dp_start_link_train(struct intel_dp *intel_dp,
uint8_t lane_count,
/* Update training set as requested by target */
if (!inte
