It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
cherryview: prevent concurrent access to GPIO controllers") for
the details. Taking all this into consideration replace the hack
with proper GPIO APIs being used.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
1 file changed, 10 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8fc82aceae14..a393ddaff0dd 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
#define CHV_GPIO_IDX_START_SW 100
#define CHV_GPIO_IDX_START_SE 198
-#define CHV_VBT_MAX_PINS_PER_FMLY 15
-
-#define CHV_GPIO_PAD_CFG0(f, i) (0x4400 + (f) * 0x400 + (i) * 8)
-#define CHV_GPIO_GPIOEN (1 << 15)
-#define CHV_GPIO_GPIOCFG_GPIO (0 << 8)
-#define CHV_GPIO_GPIOCFG_GPO (1 << 8)
-#define CHV_GPIO_GPIOCFG_GPI (2 << 8)
-#define CHV_GPIO_GPIOCFG_HIZ (3 << 8)
-#define CHV_GPIO_GPIOTXSTATE(state) ((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i) (0x4400 + (f) * 0x400 + (i) * 8 + 4)
-#define CHV_GPIO_CFGLOCK (1 << 31)
-
/* ICL DSI Display GPIO Pins */
#define ICL_GPIO_DDSP_HPD_A 0
#define ICL_GPIO_L_VDDEN_1 1
@@ -279,23 +266,21 @@ static void chv_exec_gpio(struct intel_connector *connector,
u8 gpio_source, u8 gpio_index, bool value)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- u16 cfg0, cfg1;
- u16 family_num;
- u8 port;
if (connector->panel.vbt.dsi.seq_version >= 3) {
if (gpio_index >= CHV_GPIO_IDX_START_SE) {
/* XXX: it's unclear whether 255->57 is part of SE. */
- gpio_index -= CHV_GPIO_IDX_START_SE;
- port = CHV_IOSF_PORT_GPIO_SE;
+ soc_exec_opaque_gpio(connector, "INT33FF:03", "Panel SE",
+ gpio_index - CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
- gpio_index -= CHV_GPIO_IDX_START_SW;
- port = CHV_IOSF_PORT_GPIO_SW;
+ soc_exec_opaque_gpio(connector, "INT33FF:00", "Panel SW",
+ gpio_index - CHV_GPIO_IDX_START_SW, value);
} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
- gpio_index -= CHV_GPIO_IDX_START_E;
- port = CHV_IOSF_PORT_GPIO_E;
+ soc_exec_opaque_gpio(connector, "INT33FF:02", "Panel E",
+ gpio_index - CHV_GPIO_IDX_START_E, value);
} else {
- port = CHV_IOSF_PORT_GPIO_N;
+ soc_exec_opaque_gpio(connector, "INT33FF:01", "Panel N",
+ gpio_index - CHV_GPIO_IDX_START_N, value);
}
} else {
/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -312,21 +297,9 @@ static void chv_exec_gpio(struct intel_connector *connector,
return;
}
- port = CHV_IOSF_PORT_GPIO_N;
+ soc_exec_opaque_gpio(connector, "INT33FF:01", "Panel N",
+ gpio_index - CHV_GPIO_IDX_START_N, value);
}
-
- family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
- gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
- cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
- cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
- vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
- vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
- vlv_iosf_sb_write(dev_priv, port, cfg0,
- CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
- CHV_GPIO_GPIOTXSTATE(value));
- vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
}
static void bxt_exec_gpio(struct intel_connector *connector,
--
2.40.0.1.gaa8946217a0b
On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
> It's a dirty hack in the driver that pokes GPIO registers behind
> the driver's back. Moreoever it might be problematic as simultaneous
> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> cherryview: prevent concurrent access to GPIO controllers") for
> the details. Taking all this into consideration replace the hack
> with proper GPIO APIs being used.
Ah, just realised that this won't work if it happens to request to GPIOs with
the same index but different communities. I will fix that in v3, but will wait
for Hans to test VLV and it might even work in most of the cases on CHV as it
seems quite unlikely that the above mentioned assertion is going to happen in
real life.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 10/24/23 18:11, Andy Shevchenko wrote:
> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>> It's a dirty hack in the driver that pokes GPIO registers behind
>> the driver's back. Moreoever it might be problematic as simultaneous
>> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
>> cherryview: prevent concurrent access to GPIO controllers") for
>> the details. Taking all this into consideration replace the hack
>> with proper GPIO APIs being used.
>
> Ah, just realised that this won't work if it happens to request to GPIOs with
> the same index but different communities. I will fix that in v3, but will wait
> for Hans to test VLV and it might even work in most of the cases on CHV as it
> seems quite unlikely that the above mentioned assertion is going to happen in
> real life.
I have added patches 1-5 to my personal tree + a small debug patch on top
which logs when soc_exec_opaque_gpio() actually gets called.
So these patches will now get run every time I run some tests on
one my tablets.
I'll get back to you with testing results when I've found a device where
the new soc_exec_opaque_gpio() actually gets called.
As for the CHT support, I have not added that to my tree yet, I would
prefer to directly test the correct/fixed patch.
Regards,
Hans
On Tue, Oct 31, 2023 at 05:07:39PM +0100, Hans de Goede wrote:
> On 10/24/23 18:11, Andy Shevchenko wrote:
> > On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
> >> It's a dirty hack in the driver that pokes GPIO registers behind
> >> the driver's back. Moreoever it might be problematic as simultaneous
> >> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> >> cherryview: prevent concurrent access to GPIO controllers") for
> >> the details. Taking all this into consideration replace the hack
> >> with proper GPIO APIs being used.
> >
> > Ah, just realised that this won't work if it happens to request to GPIOs with
> > the same index but different communities. I will fix that in v3, but will wait
> > for Hans to test VLV and it might even work in most of the cases on CHV as it
> > seems quite unlikely that the above mentioned assertion is going to happen in
> > real life.
>
> I have added patches 1-5 to my personal tree + a small debug patch on top
> which logs when soc_exec_opaque_gpio() actually gets called.
>
> So these patches will now get run every time I run some tests on
> one my tablets.
>
> I'll get back to you with testing results when I've found a device where
> the new soc_exec_opaque_gpio() actually gets called.
Thank you!
> As for the CHT support, I have not added that to my tree yet, I would
> prefer to directly test the correct/fixed patch.
Noted, I'll prepare a new version then.
--
With Best Regards,
Andy Shevchenko
Hi,
On 10/31/23 17:07, Hans de Goede wrote:
> Hi Andy,
>
> On 10/24/23 18:11, Andy Shevchenko wrote:
>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>> It's a dirty hack in the driver that pokes GPIO registers behind
>>> the driver's back. Moreoever it might be problematic as simultaneous
>>> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
>>> cherryview: prevent concurrent access to GPIO controllers") for
>>> the details. Taking all this into consideration replace the hack
>>> with proper GPIO APIs being used.
>>
>> Ah, just realised that this won't work if it happens to request to GPIOs with
>> the same index but different communities. I will fix that in v3, but will wait
>> for Hans to test VLV and it might even work in most of the cases on CHV as it
>> seems quite unlikely that the above mentioned assertion is going to happen in
>> real life.
>
> I have added patches 1-5 to my personal tree + a small debug patch on top
> which logs when soc_exec_opaque_gpio() actually gets called.
>
> So these patches will now get run every time I run some tests on
> one my tablets.
>
> I'll get back to you with testing results when I've found a device where
> the new soc_exec_opaque_gpio() actually gets called.
>
> As for the CHT support, I have not added that to my tree yet, I would
> prefer to directly test the correct/fixed patch.
And I hit the "jackpot" on the first device I tried and the code needed
some fixing to actually work, so here is something to fold into v3 to
fix things:
From 144fae4de91a6b5ed993b1722a07cca679f74cbe Mon Sep 17 00:00:00 2001
From: Hans de Goede <[email protected]>
Date: Tue, 31 Oct 2023 17:04:35 +0100
Subject: [PATCH] drm/i915/dsi: Fix GPIO lookup table used by
soc_exec_opaque_gpio()
There already is a GPIO lookup table for device "0000:00:02.0" and
there can be only one GPIO lookup per device.
Instead add an extra empty entry to the GPIO lookup table
registered by intel_dsi_vbt_gpio_init() and use that extra entry
in soc_exec_opaque_gpio().
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 60 ++++++++++----------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 8fc82aceae14..70f1d2c411e8 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
} else {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
con_id, gpio_index,
- value ? GPIOD_OUT_LOW :
- GPIOD_OUT_HIGH);
+ value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(&dev_priv->drm,
"GPIO index %u request failed (%pe)\n",
@@ -232,26 +231,20 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
}
}
+static struct gpiod_lookup *soc_exec_opaque_gpiod_lookup;
+
static void soc_exec_opaque_gpio(struct intel_connector *connector,
const char *chip, const char *con_id,
u8 gpio_index, bool value)
{
- struct gpiod_lookup_table *lookup;
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
- lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
- if (!lookup)
- return;
-
- lookup->dev_id = "0000:00:02.0";
- lookup->table[0] =
+ *soc_exec_opaque_gpiod_lookup =
GPIO_LOOKUP_IDX(chip, gpio_index, con_id, gpio_index, GPIO_ACTIVE_HIGH);
- gpiod_add_lookup_table(lookup);
-
soc_exec_gpio(connector, con_id, gpio_index, value);
- gpiod_remove_lookup_table(lookup);
- kfree(lookup);
+ soc_exec_opaque_gpiod_lookup->key = NULL;
}
static void vlv_exec_gpio(struct intel_connector *connector,
@@ -898,6 +891,7 @@ static struct gpiod_lookup_table pmic_panel_gpio_table = {
.table = {
/* Panel EN/DISABLE */
GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
{ }
},
};
@@ -907,6 +901,15 @@ static struct gpiod_lookup_table soc_panel_gpio_table = {
.table = {
GPIO_LOOKUP("INT33FC:01", 10, "backlight", GPIO_ACTIVE_HIGH),
GPIO_LOOKUP("INT33FC:01", 11, "panel", GPIO_ACTIVE_HIGH),
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
+ { }
+ },
+};
+
+static struct gpiod_lookup_table empty_gpio_table = {
+ .dev_id = "0000:00:02.0",
+ .table = {
+ { }, /* Extra lookup entry for soc_exec_opaque_gpiod_lookup */
{ }
},
};
@@ -916,6 +919,8 @@ static const struct pinctrl_map soc_pwm_pinctrl_map[] = {
"pwm0_grp", "pwm"),
};
+static struct gpiod_lookup_table *gpiod_lookup_table;
+
void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
{
struct drm_device *dev = intel_dsi->base.base.dev;
@@ -926,16 +931,16 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
bool want_backlight_gpio = false;
bool want_panel_gpio = false;
struct pinctrl *pinctrl;
- int ret;
+ int i, ret;
if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
mipi_config->pwm_blc == PPS_BLC_PMIC) {
- gpiod_add_lookup_table(&pmic_panel_gpio_table);
+ gpiod_lookup_table = &pmic_panel_gpio_table;
want_panel_gpio = true;
}
if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
- gpiod_add_lookup_table(&soc_panel_gpio_table);
+ gpiod_lookup_table = &soc_panel_gpio_table;
want_panel_gpio = true;
want_backlight_gpio = true;
@@ -952,6 +957,15 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
"Failed to set pinmux to PWM\n");
}
+ if (!gpiod_lookup_table)
+ gpiod_lookup_table = &empty_gpio_table;
+
+ /* Find first empty entry for soc_exec_opaque_gpiod_lookup */
+ for (i = 0; gpiod_lookup_table->table[i].key; i++) { }
+ soc_exec_opaque_gpiod_lookup = &gpiod_lookup_table->table[i];
+
+ gpiod_add_lookup_table(gpiod_lookup_table);
+
if (want_panel_gpio) {
intel_dsi->gpio_panel = gpiod_get(dev->dev, "panel", flags);
if (IS_ERR(intel_dsi->gpio_panel)) {
@@ -974,11 +988,6 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
{
- struct drm_device *dev = intel_dsi->base.base.dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_connector *connector = intel_dsi->attached_connector;
- struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
-
if (intel_dsi->gpio_panel) {
gpiod_put(intel_dsi->gpio_panel);
intel_dsi->gpio_panel = NULL;
@@ -989,12 +998,5 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
intel_dsi->gpio_backlight = NULL;
}
- if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
- mipi_config->pwm_blc == PPS_BLC_PMIC)
- gpiod_remove_lookup_table(&pmic_panel_gpio_table);
-
- if (IS_VALLEYVIEW(dev_priv) && mipi_config->pwm_blc == PPS_BLC_SOC) {
- pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
- gpiod_remove_lookup_table(&soc_panel_gpio_table);
- }
+ gpiod_remove_lookup_table(gpiod_lookup_table);
}
--
2.41.0
Regards,
Hans
On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
> On 10/31/23 17:07, Hans de Goede wrote:
> > On 10/24/23 18:11, Andy Shevchenko wrote:
> >> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
...
> > As for the CHT support, I have not added that to my tree yet, I would
> > prefer to directly test the correct/fixed patch.
>
> And I hit the "jackpot" on the first device I tried and the code needed
> some fixing to actually work, so here is something to fold into v3 to
> fix things:
Thanks!
But let me first send current v3 as it quite differs to v2 in the sense
of how I do instantiate GPIO lookup tables.
Meanwhile I will look into the change you sent (and hopefully we can
incorporate something in v3 for v4).
--
With Best Regards,
Andy Shevchenko
Hi,
On 11/1/23 10:32, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>> On 10/31/23 17:07, Hans de Goede wrote:
>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>
> ...
>
>>> As for the CHT support, I have not added that to my tree yet, I would
>>> prefer to directly test the correct/fixed patch.
>>
>> And I hit the "jackpot" on the first device I tried and the code needed
>> some fixing to actually work, so here is something to fold into v3 to
>> fix things:
>
> Thanks!
>
> But let me first send current v3 as it quite differs to v2 in the sense
> of how I do instantiate GPIO lookup tables.
The problem is there already is a GPIO lookup table registered for
the "0000:00:02.0" device by intel_dsi_vbt_gpio_init() and there can
be only be one GPIO lookup table per device. So no matter how you
instantiate GPIO lookup tables it will not work.
The solution that I chose is to not instantiate a GPIO lookup table
at all and instead to extend the existing table with an extra entry.
Although thinking more about it I must admit that this is racy.
So a better idea would be to unregister the GPIO lookup
table registered by intel_dsi_vbt_gpio_init() after getting
the GPIOs there, that would allow instantiating a new one
from soc_exec_opaque_gpio() as it currently does and that
would be race free.
> Meanwhile I will look into the change you sent (and hopefully we can
> incorporate something in v3 for v4).
Ok, lets go with your v3.
I'll prepare a patch to move the unregistering of the existing
conflicting GPIO lookup from intel_dsi_vbt_gpio_cleanup()
to the end of intel_dsi_vbt_gpio_init() to avoid the conflict
we have there.
Note you still need the first part of my patch which is
an unrelated bugfix:
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
} else {
gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
con_id, gpio_index,
- value ? GPIOD_OUT_LOW :
- GPIOD_OUT_HIGH);
+ value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
if (IS_ERR(gpio_desc)) {
drm_err(&dev_priv->drm,
"GPIO index %u request failed (%pe)\n",
Regards,
Hans
On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
> Hi,
>
> On 11/1/23 10:32, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
> >> On 10/31/23 17:07, Hans de Goede wrote:
> >>> On 10/24/23 18:11, Andy Shevchenko wrote:
> >>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
> >
> > ...
> >
> >>> As for the CHT support, I have not added that to my tree yet, I would
> >>> prefer to directly test the correct/fixed patch.
> >>
> >> And I hit the "jackpot" on the first device I tried and the code needed
> >> some fixing to actually work, so here is something to fold into v3 to
> >> fix things:
> >
> > Thanks!
> >
> > But let me first send current v3 as it quite differs to v2 in the sense
> > of how I do instantiate GPIO lookup tables.
>
> The problem is there already is a GPIO lookup table registered for
> the "0000:00:02.0" device by intel_dsi_vbt_gpio_init() and there can
> be only be one GPIO lookup table per device. So no matter how you
> instantiate GPIO lookup tables it will not work.
>
> The solution that I chose is to not instantiate a GPIO lookup table
> at all and instead to extend the existing table with an extra entry.
>
> Although thinking more about it I must admit that this is racy.
>
> So a better idea would be to unregister the GPIO lookup
> table registered by intel_dsi_vbt_gpio_init() after getting
> the GPIOs there, that would allow instantiating a new one
> from soc_exec_opaque_gpio() as it currently does and that
> would be race free.
The proper solution would likely be be to pre-parse the sequences
to determine which GPIOs are actually needed. That would also get
rid of the bxt_gpio_table[] eyesore.
--
Ville Syrj?l?
Intel
Hi,
On 11/1/23 11:34, Ville Syrjälä wrote:
> On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/1/23 10:32, Andy Shevchenko wrote:
>>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>>>> On 10/31/23 17:07, Hans de Goede wrote:
>>>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>>
>>> ...
>>>
>>>>> As for the CHT support, I have not added that to my tree yet, I would
>>>>> prefer to directly test the correct/fixed patch.
>>>>
>>>> And I hit the "jackpot" on the first device I tried and the code needed
>>>> some fixing to actually work, so here is something to fold into v3 to
>>>> fix things:
>>>
>>> Thanks!
>>>
>>> But let me first send current v3 as it quite differs to v2 in the sense
>>> of how I do instantiate GPIO lookup tables.
>>
>> The problem is there already is a GPIO lookup table registered for
>> the "0000:00:02.0" device by intel_dsi_vbt_gpio_init() and there can
>> be only be one GPIO lookup table per device. So no matter how you
>> instantiate GPIO lookup tables it will not work.
>>
>> The solution that I chose is to not instantiate a GPIO lookup table
>> at all and instead to extend the existing table with an extra entry.
>>
>> Although thinking more about it I must admit that this is racy.
>>
>> So a better idea would be to unregister the GPIO lookup
>> table registered by intel_dsi_vbt_gpio_init() after getting
>> the GPIOs there, that would allow instantiating a new one
>> from soc_exec_opaque_gpio() as it currently does and that
>> would be race free.
>
> The proper solution would likely be be to pre-parse the sequences
> to determine which GPIOs are actually needed. That would also get
> rid of the bxt_gpio_table[] eyesore.
Interesting suggestion. Note that intel_dsi_vbt_gpio_init() arm
only runs on byt and cht though, so that is something to keep
in mind.
Regards,
Hans
Hi,
On 11/1/23 11:20, Hans de Goede wrote:
> Hi,
>
> On 11/1/23 10:32, Andy Shevchenko wrote:
>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>>> On 10/31/23 17:07, Hans de Goede wrote:
>>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>>
>> ...
>>
>>>> As for the CHT support, I have not added that to my tree yet, I would
>>>> prefer to directly test the correct/fixed patch.
>>>
>>> And I hit the "jackpot" on the first device I tried and the code needed
>>> some fixing to actually work, so here is something to fold into v3 to
>>> fix things:
>>
>> Thanks!
>>
>> But let me first send current v3 as it quite differs to v2 in the sense
>> of how I do instantiate GPIO lookup tables.
>
> The problem is there already is a GPIO lookup table registered for
> the "0000:00:02.0" device by intel_dsi_vbt_gpio_init() and there can
> be only be one GPIO lookup table per device. So no matter how you
> instantiate GPIO lookup tables it will not work.
>
> The solution that I chose is to not instantiate a GPIO lookup table
> at all and instead to extend the existing table with an extra entry.
>
> Although thinking more about it I must admit that this is racy.
>
> So a better idea would be to unregister the GPIO lookup
> table registered by intel_dsi_vbt_gpio_init() after getting
> the GPIOs there, that would allow instantiating a new one
> from soc_exec_opaque_gpio() as it currently does and that
> would be race free.
>
>> Meanwhile I will look into the change you sent (and hopefully we can
>> incorporate something in v3 for v4).
>
> Ok, lets go with your v3.
>
> I'll prepare a patch to move the unregistering of the existing
> conflicting GPIO lookup from intel_dsi_vbt_gpio_cleanup()
> to the end of intel_dsi_vbt_gpio_init() to avoid the conflict
> we have there.
Attached is this patch, this should probably be one of
the first patches in the v3 submission.
Note that if you go with Ville's suggestion to preparse
the MIPI sequences, things will change significantly
and then the attached patch will likely be unnecessary.
Regards,
Hans
> Note you still need the first part of my patch which is
> an unrelated bugfix:
>
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
> } else {
> gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
> con_id, gpio_index,
> - value ? GPIOD_OUT_LOW :
> - GPIOD_OUT_HIGH);
> + value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> if (IS_ERR(gpio_desc)) {
> drm_err(&dev_priv->drm,
> "GPIO index %u request failed (%pe)\n",
>
> Regards,
>
> Hans
>
>
On Wed, Nov 01, 2023 at 12:01:31PM +0100, Hans de Goede wrote:
> On 11/1/23 11:20, Hans de Goede wrote:
...
> Attached is this patch, this should probably be one of
> the first patches in the v3 submission.
Thanks, noted!
> Note that if you go with Ville's suggestion to preparse
> the MIPI sequences, things will change significantly
> and then the attached patch will likely be unnecessary.
I don't think so I'm for that. My task is to get rid of the poking registers
of the GPIO IPs in the kernel when driver has no clue about them.
That's why I want to do minimum in that sense with less possible invasion
into existing flow.
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
> On 11/1/23 10:32, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
> >> On 10/31/23 17:07, Hans de Goede wrote:
> >>> On 10/24/23 18:11, Andy Shevchenko wrote:
> >>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
...
> Note you still need the first part of my patch which is
> an unrelated bugfix:
>
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
> } else {
> gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
> con_id, gpio_index,
> - value ? GPIOD_OUT_LOW :
> - GPIOD_OUT_HIGH);
> + value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> if (IS_ERR(gpio_desc)) {
> drm_err(&dev_priv->drm,
> "GPIO index %u request failed (%pe)\n",
Can you attach or send a formal submission, so I can incorporate it into one
(v3) series among other changes?
--
With Best Regards,
Andy Shevchenko
Hi,
On 11/2/23 15:19, Andy Shevchenko wrote:
> On Wed, Nov 01, 2023 at 11:20:23AM +0100, Hans de Goede wrote:
>> On 11/1/23 10:32, Andy Shevchenko wrote:
>>> On Tue, Oct 31, 2023 at 10:15:52PM +0100, Hans de Goede wrote:
>>>> On 10/31/23 17:07, Hans de Goede wrote:
>>>>> On 10/24/23 18:11, Andy Shevchenko wrote:
>>>>>> On Tue, Oct 24, 2023 at 06:57:38PM +0300, Andy Shevchenko wrote:
>
> ...
>
>> Note you still need the first part of my patch which is
>> an unrelated bugfix:
>>
>> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>> @@ -219,8 +219,7 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
>> } else {
>> gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
>> con_id, gpio_index,
>> - value ? GPIOD_OUT_LOW :
>> - GPIOD_OUT_HIGH);
>> + value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
>> if (IS_ERR(gpio_desc)) {
>> drm_err(&dev_priv->drm,
>> "GPIO index %u request failed (%pe)\n",
>
> Can you attach or send a formal submission, so I can incorporate it into one
> (v3) series among other changes?
I thought this fixed new code in your series and it is a trivial fix,
so my idea was that you would just fold the fix into the patch
introducing the code.
But I see now that this is existing code from bxt_exec_gpio().
A formal fix to use as a prep patch for your series is now attached,
this is based on top of drm-misc-next (I guess drm-intel-next
would be better but I had an up2date drm-misc-next handy).
Regards,
Hans