diff mbox series

[04/12] drm/i915/bios: limit default outputs to ports A through F

Message ID bf2f1c9527e17310d69a818e09a7212df4ada347.1613580193.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/bios: vbt child device rework | expand

Commit Message

Jani Nikula Feb. 17, 2021, 5:03 p.m. UTC 
With the defaults limited to non-TypeC ports in commit 828ccb31cf41
("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
a no-op, but clarifies the code and prepares for subsequent changes.

Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Lucas De Marchi Feb. 17, 2021, 5:23 p.m. UTC | #1 
On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>a no-op, but clarifies the code and prepares for subsequent changes.
>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>index e9cb15aa2f5a..b9d99324d66d 100644
>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>@@ -2057,11 +2057,12 @@ static void
> init_vbt_missing_defaults(struct drm_i915_private *i915)
> {
> 	enum port port;
>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;


I'd not spread the knowledge of what port uses tc phy like this.
It's painful to maintain.

Lucas De Marchi

>
> 	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
> 		return;
>
>-	for_each_port(port) {
>+	for_each_port_masked(port, ports) {
> 		struct ddi_vbt_port_info *info =
> 			&i915->vbt.ddi_port_info[port];
> 		enum phy phy = intel_port_to_phy(i915, port);
>-- 
>2.20.1
>
Lucas De Marchi Feb. 17, 2021, 5:28 p.m. UTC | #2 
On Wed, Feb 17, 2021 at 09:23:00AM -0800, Lucas De Marchi wrote:
>On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>>a no-op, but clarifies the code and prepares for subsequent changes.
>>
>>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>---
>>drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
>>1 file changed, 2 insertions(+), 1 deletion(-)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>index e9cb15aa2f5a..b9d99324d66d 100644
>>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>@@ -2057,11 +2057,12 @@ static void
>>init_vbt_missing_defaults(struct drm_i915_private *i915)
>>{
>>	enum port port;
>>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
>
>
>I'd not spread the knowledge of what port uses tc phy like this.
>It's painful to maintain.

also, not sure how this clarifies things if PORT_TC1 aliases PORT_D,
so I'd just drop this patch

Lucas De Marchi

>Lucas De Marchi
>
>>
>>	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
>>		return;
>>
>>-	for_each_port(port) {
>>+	for_each_port_masked(port, ports) {
>>		struct ddi_vbt_port_info *info =
>>			&i915->vbt.ddi_port_info[port];
>>		enum phy phy = intel_port_to_phy(i915, port);
>>-- 
>>2.20.1
>>
Jani Nikula Feb. 17, 2021, 7:49 p.m. UTC | #3 
On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Wed, Feb 17, 2021 at 09:23:00AM -0800, Lucas De Marchi wrote:
>>On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>>>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>>>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>>>a no-op, but clarifies the code and prepares for subsequent changes.
>>>
>>>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>---
>>>drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>>index e9cb15aa2f5a..b9d99324d66d 100644
>>>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>>>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>>@@ -2057,11 +2057,12 @@ static void
>>>init_vbt_missing_defaults(struct drm_i915_private *i915)
>>>{
>>>	enum port port;
>>>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
>>
>>
>>I'd not spread the knowledge of what port uses tc phy like this.
>>It's painful to maintain.

Umm, this wasn't meant to have anything to do with tc, really. Granted,
the commit message is misleading.

>
> also, not sure how this clarifies things if PORT_TC1 aliases PORT_D,
> so I'd just drop this patch

The point is that I'd like to reduce the number of ports set up by
default, perhaps even further than this. It's a bit silly to generate 9
dummy child devices on certain platforms when there's no VBT.


BR,
Jani.

>
> Lucas De Marchi
>
>>Lucas De Marchi
>>
>>>
>>>	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
>>>		return;
>>>
>>>-	for_each_port(port) {
>>>+	for_each_port_masked(port, ports) {
>>>		struct ddi_vbt_port_info *info =
>>>			&i915->vbt.ddi_port_info[port];
>>>		enum phy phy = intel_port_to_phy(i915, port);
>>>-- 
>>>2.20.1
>>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lucas De Marchi Feb. 17, 2021, 7:58 p.m. UTC | #4 
On Wed, Feb 17, 2021 at 09:49:57PM +0200, Jani Nikula wrote:
>On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> On Wed, Feb 17, 2021 at 09:23:00AM -0800, Lucas De Marchi wrote:
>>>On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>>>>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>>>>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>>>>a no-op, but clarifies the code and prepares for subsequent changes.
>>>>
>>>>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>---
>>>>drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
>>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>index e9cb15aa2f5a..b9d99324d66d 100644
>>>>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>>>>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>@@ -2057,11 +2057,12 @@ static void
>>>>init_vbt_missing_defaults(struct drm_i915_private *i915)
>>>>{
>>>>	enum port port;
>>>>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
>>>
>>>
>>>I'd not spread the knowledge of what port uses tc phy like this.
>>>It's painful to maintain.
>
>Umm, this wasn't meant to have anything to do with tc, really. Granted,
>the commit message is misleading.

ok, makes more sense now. I don't want us to keep updating this function
when the ports change on new platforms.

>
>>
>> also, not sure how this clarifies things if PORT_TC1 aliases PORT_D,
>> so I'd just drop this patch
>
>The point is that I'd like to reduce the number of ports set up by
>default, perhaps even further than this. It's a bit silly to generate 9
>dummy child devices on certain platforms when there's no VBT.

ok. So what would be the devices without vbt? I remember relying on this
for e.g. dg1 before we could read it.

What other platforms should we care about? And for those, should we
really care about ports E and F or could we reduce it to, say the first
4?

thanks
Lucas De Marchi

>
>
>BR,
>Jani.
>
>>
>> Lucas De Marchi
>>
>>>Lucas De Marchi
>>>
>>>>
>>>>	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
>>>>		return;
>>>>
>>>>-	for_each_port(port) {
>>>>+	for_each_port_masked(port, ports) {
>>>>		struct ddi_vbt_port_info *info =
>>>>			&i915->vbt.ddi_port_info[port];
>>>>		enum phy phy = intel_port_to_phy(i915, port);
>>>>--
>>>>2.20.1
>>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Feb. 23, 2021, 1:34 p.m. UTC | #5 
On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Wed, Feb 17, 2021 at 09:49:57PM +0200, Jani Nikula wrote:
>>On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> On Wed, Feb 17, 2021 at 09:23:00AM -0800, Lucas De Marchi wrote:
>>>>On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>>>>>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>>>>>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>>>>>a no-op, but clarifies the code and prepares for subsequent changes.
>>>>>
>>>>>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>>---
>>>>>drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
>>>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>index e9cb15aa2f5a..b9d99324d66d 100644
>>>>>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>@@ -2057,11 +2057,12 @@ static void
>>>>>init_vbt_missing_defaults(struct drm_i915_private *i915)
>>>>>{
>>>>>	enum port port;
>>>>>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
>>>>
>>>>
>>>>I'd not spread the knowledge of what port uses tc phy like this.
>>>>It's painful to maintain.
>>
>>Umm, this wasn't meant to have anything to do with tc, really. Granted,
>>the commit message is misleading.
>
> ok, makes more sense now. I don't want us to keep updating this function
> when the ports change on new platforms.

Agreed.

>
>>
>>>
>>> also, not sure how this clarifies things if PORT_TC1 aliases PORT_D,
>>> so I'd just drop this patch
>>
>>The point is that I'd like to reduce the number of ports set up by
>>default, perhaps even further than this. It's a bit silly to generate 9
>>dummy child devices on certain platforms when there's no VBT.
>
> ok. So what would be the devices without vbt? I remember relying on this
> for e.g. dg1 before we could read it.

If it were just for enabling, I'd start some igt tool to generate a
basic VBT to use with i915.vbt_firmware and the firmware loader. It's
the existing products that ended up depending on the defaults that are
the real issue I think.

> What other platforms should we care about? And for those, should we
> really care about ports E and F or could we reduce it to, say the first
> 4?

IIRC they were some Haswell or Broadwell era Chromebooks. They probably
don't use very many ports, but in theory it could be A-D,F on DDI
platforms (E requires VBT child device). On gen 11 it could be A-E, with
F requiring VBT child device.

A-F covers all cases, but does not try to create G-I or any further
future ports.


BR,
Jani.


>
> thanks
> Lucas De Marchi
>
>>
>>
>>BR,
>>Jani.
>>
>>>
>>> Lucas De Marchi
>>>
>>>>Lucas De Marchi
>>>>
>>>>>
>>>>>	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
>>>>>		return;
>>>>>
>>>>>-	for_each_port(port) {
>>>>>+	for_each_port_masked(port, ports) {
>>>>>		struct ddi_vbt_port_info *info =
>>>>>			&i915->vbt.ddi_port_info[port];
>>>>>		enum phy phy = intel_port_to_phy(i915, port);
>>>>>--
>>>>>2.20.1
>>>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>-- 
>>Jani Nikula, Intel Open Source Graphics Center
Lucas De Marchi Feb. 24, 2021, 12:34 a.m. UTC | #6 
On Tue, Feb 23, 2021 at 03:34:51PM +0200, Jani Nikula wrote:
>On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> On Wed, Feb 17, 2021 at 09:49:57PM +0200, Jani Nikula wrote:
>>>On Wed, 17 Feb 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>> On Wed, Feb 17, 2021 at 09:23:00AM -0800, Lucas De Marchi wrote:
>>>>>On Wed, Feb 17, 2021 at 07:03:34PM +0200, Jani Nikula wrote:
>>>>>>With the defaults limited to non-TypeC ports in commit 828ccb31cf41
>>>>>>("drm/i915/icl: Add TypeC ports only if VBT is present"), this should be
>>>>>>a no-op, but clarifies the code and prepares for subsequent changes.
>>>>>>
>>>>>>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>>>---
>>>>>>drivers/gpu/drm/i915/display/intel_bios.c | 3 ++-
>>>>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>>diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>>index e9cb15aa2f5a..b9d99324d66d 100644
>>>>>>--- a/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>>+++ b/drivers/gpu/drm/i915/display/intel_bios.c
>>>>>>@@ -2057,11 +2057,12 @@ static void
>>>>>>init_vbt_missing_defaults(struct drm_i915_private *i915)
>>>>>>{
>>>>>>	enum port port;
>>>>>>+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
>>>>>
>>>>>
>>>>>I'd not spread the knowledge of what port uses tc phy like this.
>>>>>It's painful to maintain.
>>>
>>>Umm, this wasn't meant to have anything to do with tc, really. Granted,
>>>the commit message is misleading.
>>
>> ok, makes more sense now. I don't want us to keep updating this function
>> when the ports change on new platforms.
>
>Agreed.
>
>>
>>>
>>>>
>>>> also, not sure how this clarifies things if PORT_TC1 aliases PORT_D,
>>>> so I'd just drop this patch
>>>
>>>The point is that I'd like to reduce the number of ports set up by
>>>default, perhaps even further than this. It's a bit silly to generate 9
>>>dummy child devices on certain platforms when there's no VBT.
>>
>> ok. So what would be the devices without vbt? I remember relying on this
>> for e.g. dg1 before we could read it.
>
>If it were just for enabling, I'd start some igt tool to generate a
>basic VBT to use with i915.vbt_firmware and the firmware loader. It's
>the existing products that ended up depending on the defaults that are
>the real issue I think.
>
>> What other platforms should we care about? And for those, should we
>> really care about ports E and F or could we reduce it to, say the first
>> 4?
>
>IIRC they were some Haswell or Broadwell era Chromebooks. They probably
>don't use very many ports, but in theory it could be A-D,F on DDI
>platforms (E requires VBT child device). On gen 11 it could be A-E, with
>F requiring VBT child device.
>
>A-F covers all cases, but does not try to create G-I or any further
>future ports.


ok, thanks for confirming.


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi


>
>
>BR,
>Jani.
>
>
>>
>> thanks
>> Lucas De Marchi
>>
>>>
>>>
>>>BR,
>>>Jani.
>>>
>>>>
>>>> Lucas De Marchi
>>>>
>>>>>Lucas De Marchi
>>>>>
>>>>>>
>>>>>>	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
>>>>>>		return;
>>>>>>
>>>>>>-	for_each_port(port) {
>>>>>>+	for_each_port_masked(port, ports) {
>>>>>>		struct ddi_vbt_port_info *info =
>>>>>>			&i915->vbt.ddi_port_info[port];
>>>>>>		enum phy phy = intel_port_to_phy(i915, port);
>>>>>>--
>>>>>>2.20.1
>>>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>--
>>>Jani Nikula, Intel Open Source Graphics Center
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index e9cb15aa2f5a..b9d99324d66d 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -2057,11 +2057,12 @@  static void
 init_vbt_missing_defaults(struct drm_i915_private *i915)
 {
 	enum port port;
+	int ports = PORT_A | PORT_B | PORT_C | PORT_D | PORT_E | PORT_F;
 
 	if (!HAS_DDI(i915) && !IS_CHERRYVIEW(i915))
 		return;
 
-	for_each_port(port) {
+	for_each_port_masked(port, ports) {
 		struct ddi_vbt_port_info *info =
 			&i915->vbt.ddi_port_info[port];
 		enum phy phy = intel_port_to_phy(i915, port);