Re: [Intel-gfx] [PATCH 4/7] drm/i915/bios: use alternate aux channel directly from child data
On Thu, 26 Aug 2021, "Nautiyal, Ankit K" wrote:
> On 8/24/2021 7:04 PM, Jani Nikula wrote:
>> Avoid extra caching of the data.
>>
>> Cc: José Roberto de Souza
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/i915/display/intel_bios.c | 26 +++
>> drivers/gpu/drm/i915/i915_drv.h | 1 -
>> 2 files changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
>> b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 10b2beddc121..674f1424fcc2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1565,28 +1565,29 @@ static enum port get_port_by_aux_ch(struct
>> drm_i915_private *i915, u8 aux_ch)
>> for_each_port(port) {
>> info = >vbt.ddi_port_info[port];
>>
>> -if (info->devdata && aux_ch == info->alternate_aux_channel)
>> +if (info->devdata && aux_ch == info->devdata->child.aux_channel)
>> return port;
>> }
>>
>> return PORT_NONE;
>> }
>>
>> -static void sanitize_aux_ch(struct drm_i915_private *i915,
>> +static void sanitize_aux_ch(struct intel_bios_encoder_data *devdata,
>> enum port port)
>> {
>> -struct ddi_vbt_port_info *info = >vbt.ddi_port_info[port];
>> +struct drm_i915_private *i915 = devdata->i915;
>> +struct ddi_vbt_port_info *info;
>> struct child_device_config *child;
>> enum port p;
>>
>> -p = get_port_by_aux_ch(i915, info->alternate_aux_channel);
>> +p = get_port_by_aux_ch(i915, devdata->child.aux_channel);
>> if (p == PORT_NONE)
>> return;
>>
>> drm_dbg_kms(>drm,
>> "port %c trying to use the same AUX CH (0x%x) as port %c, "
>> "disabling port %c DP support\n",
>> -port_name(port), info->alternate_aux_channel,
>> +port_name(port), devdata->child.aux_channel,
>> port_name(p), port_name(p));
>>
>> /*
>> @@ -1602,7 +1603,7 @@ static void sanitize_aux_ch(struct drm_i915_private
>> *i915,
>> child = >devdata->child;
>>
>> child->device_type &= ~DEVICE_TYPE_DISPLAYPORT_OUTPUT;
>> -info->alternate_aux_channel = 0;
>> +child->aux_channel = 0;
>> }
>>
>> static const u8 cnp_ddc_pin_map[] = {
>> @@ -1980,11 +1981,8 @@ static void parse_ddi_port(struct drm_i915_private
>> *i915,
>> }
>> }
>>
>> -if (is_dp) {
>> -info->alternate_aux_channel = child->aux_channel;
>> -
>> -sanitize_aux_ch(i915, port);
>> -}
>> +if (is_dp)
>> +sanitize_aux_ch(devdata, port);
>>
>> hdmi_level_shift = _intel_bios_hdmi_level_shift(devdata);
>> if (hdmi_level_shift >= 0) {
>> @@ -2863,7 +2861,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
>> drm_i915_private *i915,
>> >vbt.ddi_port_info[port];
>> enum aux_ch aux_ch;
>>
>> -if (!info->alternate_aux_channel) {
>> +if (!info->devdata->child.aux_channel) {
>
> Hi Jani,
>
> The series and the change make sense to me.
>
> From the CI results it seems that cases with LVDS panel connected are
> getting issues here.
>
> Apparently info->devdata is not set in this case. I guess that,
> parse_ddi_port() returns early before info->devdata gets set.
>
> I think without the patch, this situation is not encountered due to the
> fact that 'info->alternate_aux_channel, is initialized to 0.
>
> With this change, perhaps we should check for 'info->devdata' before
> checking for info->devdata->child.aux_channel.
>
> (This will translate to checking for 'devdata' in the final patch as it
> removes ddi_port_info).
>
> Hope it helps.
Yes, indeed, thanks for figuring this out!
And thanks for the reviews.
BR,
Jani.
>
> Regards,
>
> Ankit
>
>
>> aux_ch = (enum aux_ch)port;
>>
>> drm_dbg_kms(>drm,
>> @@ -2879,7 +2877,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
>> drm_i915_private *i915,
>> * ADL-S VBT uses PHY based mapping. Combo PHYs A,B,C,D,E
>> * map to DDI A,TC1,TC2,TC3,TC4 respectively.
>> */
>> -switch (info->alternate_aux_channel) {
>> +switch (info->devdata->child.aux_channel) {
>> case DP_AUX_A:
>> aux_ch = AUX_CH_A;
>> break;
>> @@ -2940,7 +2938,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
>> drm_i915_private *i915,
>> aux_ch = AUX_CH_I;
>> break;
>> default:
>> -MISSING_CASE(info->alternate_aux_channel);
>> +MISSING_CASE(info->devdata->child.aux_channel);
>> aux_ch = AUX_CH_A;
>> break;
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index a0dead9f9222..91097526cd96 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -640,7 +640,6 @@ struct ddi_vbt_port_info {
>>
Re: [Intel-gfx] [PATCH 4/7] drm/i915/bios: use alternate aux channel directly from child data
On 8/24/2021 7:04 PM, Jani Nikula wrote:
Avoid extra caching of the data.
Cc: José Roberto de Souza
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/display/intel_bios.c | 26 +++
drivers/gpu/drm/i915/i915_drv.h | 1 -
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
b/drivers/gpu/drm/i915/display/intel_bios.c
index 10b2beddc121..674f1424fcc2 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1565,28 +1565,29 @@ static enum port get_port_by_aux_ch(struct
drm_i915_private *i915, u8 aux_ch)
for_each_port(port) {
info = >vbt.ddi_port_info[port];
- if (info->devdata && aux_ch == info->alternate_aux_channel)
+ if (info->devdata && aux_ch == info->devdata->child.aux_channel)
return port;
}
return PORT_NONE;
}
-static void sanitize_aux_ch(struct drm_i915_private *i915,
+static void sanitize_aux_ch(struct intel_bios_encoder_data *devdata,
enum port port)
{
- struct ddi_vbt_port_info *info = >vbt.ddi_port_info[port];
+ struct drm_i915_private *i915 = devdata->i915;
+ struct ddi_vbt_port_info *info;
struct child_device_config *child;
enum port p;
- p = get_port_by_aux_ch(i915, info->alternate_aux_channel);
+ p = get_port_by_aux_ch(i915, devdata->child.aux_channel);
if (p == PORT_NONE)
return;
drm_dbg_kms(>drm,
"port %c trying to use the same AUX CH (0x%x) as port %c, "
"disabling port %c DP support\n",
- port_name(port), info->alternate_aux_channel,
+ port_name(port), devdata->child.aux_channel,
port_name(p), port_name(p));
/*
@@ -1602,7 +1603,7 @@ static void sanitize_aux_ch(struct drm_i915_private *i915,
child = >devdata->child;
child->device_type &= ~DEVICE_TYPE_DISPLAYPORT_OUTPUT;
- info->alternate_aux_channel = 0;
+ child->aux_channel = 0;
}
static const u8 cnp_ddc_pin_map[] = {
@@ -1980,11 +1981,8 @@ static void parse_ddi_port(struct drm_i915_private *i915,
}
}
- if (is_dp) {
- info->alternate_aux_channel = child->aux_channel;
-
- sanitize_aux_ch(i915, port);
- }
+ if (is_dp)
+ sanitize_aux_ch(devdata, port);
hdmi_level_shift = _intel_bios_hdmi_level_shift(devdata);
if (hdmi_level_shift >= 0) {
@@ -2863,7 +2861,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
>vbt.ddi_port_info[port];
enum aux_ch aux_ch;
- if (!info->alternate_aux_channel) {
+ if (!info->devdata->child.aux_channel) {
Hi Jani,
The series and the change make sense to me.
From the CI results it seems that cases with LVDS panel connected are
getting issues here.
Apparently info->devdata is not set in this case. I guess that,
parse_ddi_port() returns early before info->devdata gets set.
I think without the patch, this situation is not encountered due to the
fact that 'info->alternate_aux_channel, is initialized to 0.
With this change, perhaps we should check for 'info->devdata' before
checking for info->devdata->child.aux_channel.
(This will translate to checking for 'devdata' in the final patch as it
removes ddi_port_info).
Hope it helps.
Regards,
Ankit
aux_ch = (enum aux_ch)port;
drm_dbg_kms(>drm,
@@ -2879,7 +2877,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
* ADL-S VBT uses PHY based mapping. Combo PHYs A,B,C,D,E
* map to DDI A,TC1,TC2,TC3,TC4 respectively.
*/
- switch (info->alternate_aux_channel) {
+ switch (info->devdata->child.aux_channel) {
case DP_AUX_A:
aux_ch = AUX_CH_A;
break;
@@ -2940,7 +2938,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
aux_ch = AUX_CH_I;
break;
default:
- MISSING_CASE(info->alternate_aux_channel);
+ MISSING_CASE(info->devdata->child.aux_channel);
aux_ch = AUX_CH_A;
break;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0dead9f9222..91097526cd96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -640,7 +640,6 @@ struct ddi_vbt_port_info {
/* Non-NULL if port present. */
struct intel_bios_encoder_data *devdata;
- u8 alternate_aux_channel;
u8 alternate_ddc_pin;
};
[Intel-gfx] [PATCH 4/7] drm/i915/bios: use alternate aux channel directly from child data
Avoid extra caching of the data.
Cc: José Roberto de Souza
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/display/intel_bios.c | 26 +++
drivers/gpu/drm/i915/i915_drv.h | 1 -
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
b/drivers/gpu/drm/i915/display/intel_bios.c
index 10b2beddc121..674f1424fcc2 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1565,28 +1565,29 @@ static enum port get_port_by_aux_ch(struct
drm_i915_private *i915, u8 aux_ch)
for_each_port(port) {
info = >vbt.ddi_port_info[port];
- if (info->devdata && aux_ch == info->alternate_aux_channel)
+ if (info->devdata && aux_ch == info->devdata->child.aux_channel)
return port;
}
return PORT_NONE;
}
-static void sanitize_aux_ch(struct drm_i915_private *i915,
+static void sanitize_aux_ch(struct intel_bios_encoder_data *devdata,
enum port port)
{
- struct ddi_vbt_port_info *info = >vbt.ddi_port_info[port];
+ struct drm_i915_private *i915 = devdata->i915;
+ struct ddi_vbt_port_info *info;
struct child_device_config *child;
enum port p;
- p = get_port_by_aux_ch(i915, info->alternate_aux_channel);
+ p = get_port_by_aux_ch(i915, devdata->child.aux_channel);
if (p == PORT_NONE)
return;
drm_dbg_kms(>drm,
"port %c trying to use the same AUX CH (0x%x) as port %c, "
"disabling port %c DP support\n",
- port_name(port), info->alternate_aux_channel,
+ port_name(port), devdata->child.aux_channel,
port_name(p), port_name(p));
/*
@@ -1602,7 +1603,7 @@ static void sanitize_aux_ch(struct drm_i915_private *i915,
child = >devdata->child;
child->device_type &= ~DEVICE_TYPE_DISPLAYPORT_OUTPUT;
- info->alternate_aux_channel = 0;
+ child->aux_channel = 0;
}
static const u8 cnp_ddc_pin_map[] = {
@@ -1980,11 +1981,8 @@ static void parse_ddi_port(struct drm_i915_private *i915,
}
}
- if (is_dp) {
- info->alternate_aux_channel = child->aux_channel;
-
- sanitize_aux_ch(i915, port);
- }
+ if (is_dp)
+ sanitize_aux_ch(devdata, port);
hdmi_level_shift = _intel_bios_hdmi_level_shift(devdata);
if (hdmi_level_shift >= 0) {
@@ -2863,7 +2861,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
>vbt.ddi_port_info[port];
enum aux_ch aux_ch;
- if (!info->alternate_aux_channel) {
+ if (!info->devdata->child.aux_channel) {
aux_ch = (enum aux_ch)port;
drm_dbg_kms(>drm,
@@ -2879,7 +2877,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
* ADL-S VBT uses PHY based mapping. Combo PHYs A,B,C,D,E
* map to DDI A,TC1,TC2,TC3,TC4 respectively.
*/
- switch (info->alternate_aux_channel) {
+ switch (info->devdata->child.aux_channel) {
case DP_AUX_A:
aux_ch = AUX_CH_A;
break;
@@ -2940,7 +2938,7 @@ enum aux_ch intel_bios_port_aux_ch(struct
drm_i915_private *i915,
aux_ch = AUX_CH_I;
break;
default:
- MISSING_CASE(info->alternate_aux_channel);
+ MISSING_CASE(info->devdata->child.aux_channel);
aux_ch = AUX_CH_A;
break;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0dead9f9222..91097526cd96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -640,7 +640,6 @@ struct ddi_vbt_port_info {
/* Non-NULL if port present. */
struct intel_bios_encoder_data *devdata;
- u8 alternate_aux_channel;
u8 alternate_ddc_pin;
};
--
2.20.1
