diff mbox series

[8/9] drm/i915/bios: check port presence based on child device

Message ID 4338a29e4ed49e69f859dff1490fd85f6ae6177e.1579270868.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/bios: stop using vbt.ddi_port_info directly | expand

Commit Message

Jani Nikula Jan. 17, 2020, 2:29 p.m. UTC 
Affects only two calls in output setup, and ddi init will check the
features in more fine grained way.

This will make future changes easier.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Ville Syrjälä Jan. 17, 2020, 3:12 p.m. UTC | #1 
On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
> Affects only two calls in output setup, and ddi init will check the
> features in more fine grained way.
> 
> This will make future changes easier.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 4c69253739ec..70fb87e7afb6 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>  		const struct ddi_vbt_port_info *port_info =
>  			&dev_priv->vbt.ddi_port_info[port];
>  
> -		return port_info->supports_dp ||
> -		       port_info->supports_dvi ||
> -		       port_info->supports_hdmi;
> +		return port_info->child;

Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
VBT... I guess those should not have their dvo port set to anything we
accept?

>  	}
>  
>  	/* FIXME maybe deal with port A as well? */
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Jan. 17, 2020, 3:13 p.m. UTC | #2 
On Fri, Jan 17, 2020 at 05:12:38PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
> > Affects only two calls in output setup, and ddi init will check the
> > features in more fine grained way.
> > 
> > This will make future changes easier.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 4c69253739ec..70fb87e7afb6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
> >  		const struct ddi_vbt_port_info *port_info =
> >  			&dev_priv->vbt.ddi_port_info[port];
> >  
> > -		return port_info->supports_dp ||
> > -		       port_info->supports_dvi ||
> > -		       port_info->supports_hdmi;
> > +		return port_info->child;
> 
> Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
> VBT... I guess those should not have their dvo port set to anything we
> accept?

Umm, no. We accept DVO_PORT_CRT as PORT_E.

> 
> >  	}
> >  
> >  	/* FIXME maybe deal with port A as well? */
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Jan. 17, 2020, 3:23 p.m. UTC | #3 
On Fri, Jan 17, 2020 at 05:13:25PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 17, 2020 at 05:12:38PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
> > > Affects only two calls in output setup, and ddi init will check the
> > > features in more fine grained way.
> > > 
> > > This will make future changes easier.
> > > 
> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > > index 4c69253739ec..70fb87e7afb6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > > @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
> > >  		const struct ddi_vbt_port_info *port_info =
> > >  			&dev_priv->vbt.ddi_port_info[port];
> > >  
> > > -		return port_info->supports_dp ||
> > > -		       port_info->supports_dvi ||
> > > -		       port_info->supports_hdmi;
> > > +		return port_info->child;
> > 
> > Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
> > VBT... I guess those should not have their dvo port set to anything we
> > accept?
> 
> Umm, no. We accept DVO_PORT_CRT as PORT_E.

Maybe it doesn't matter though. Hopefully no VBT has that on skl+,
and on hsw/bdw the current CRT init code doesn't seem to care
what this says.

> 
> > 
> > >  	}
> > >  
> > >  	/* FIXME maybe deal with port A as well? */
> > > -- 
> > > 2.20.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel
Jani Nikula Jan. 17, 2020, 3:28 p.m. UTC | #4 
On Fri, 17 Jan 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jan 17, 2020 at 05:12:38PM +0200, Ville Syrjälä wrote:
>> On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
>> > Affects only two calls in output setup, and ddi init will check the
>> > features in more fine grained way.
>> > 
>> > This will make future changes easier.
>> > 
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> > index 4c69253739ec..70fb87e7afb6 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> > @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>> >  		const struct ddi_vbt_port_info *port_info =
>> >  			&dev_priv->vbt.ddi_port_info[port];
>> >  
>> > -		return port_info->supports_dp ||
>> > -		       port_info->supports_dvi ||
>> > -		       port_info->supports_hdmi;
>> > +		return port_info->child;
>> 
>> Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
>> VBT... I guess those should not have their dvo port set to anything we
>> accept?
>
> Umm, no. We accept DVO_PORT_CRT as PORT_E.

My argument is this:

- Shouldn't intel_bios_is_port_present(PORT_E) return true in that case?

- Where does the change make a functional difference in the
  intel_bios_is_port_present() users anyway? AFAICT all the cases will
  also check the supports_X flags anyway (or have checked before).


BR,
Jani.


>
>> 
>> >  	}
>> >  
>> >  	/* FIXME maybe deal with port A as well? */
>> > -- 
>> > 2.20.1
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Ville Syrjälä
>> Intel
Ville Syrjälä Jan. 17, 2020, 3:37 p.m. UTC | #5 
On Fri, Jan 17, 2020 at 05:28:23PM +0200, Jani Nikula wrote:
> On Fri, 17 Jan 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jan 17, 2020 at 05:12:38PM +0200, Ville Syrjälä wrote:
> >> On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
> >> > Affects only two calls in output setup, and ddi init will check the
> >> > features in more fine grained way.
> >> > 
> >> > This will make future changes easier.
> >> > 
> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
> >> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > index 4c69253739ec..70fb87e7afb6 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
> >> >  		const struct ddi_vbt_port_info *port_info =
> >> >  			&dev_priv->vbt.ddi_port_info[port];
> >> >  
> >> > -		return port_info->supports_dp ||
> >> > -		       port_info->supports_dvi ||
> >> > -		       port_info->supports_hdmi;
> >> > +		return port_info->child;
> >> 
> >> Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
> >> VBT... I guess those should not have their dvo port set to anything we
> >> accept?
> >
> > Umm, no. We accept DVO_PORT_CRT as PORT_E.
> 
> My argument is this:
> 
> - Shouldn't intel_bios_is_port_present(PORT_E) return true in that case?

Only on hsw/bdw really.

> 
> - Where does the change make a functional difference in the
>   intel_bios_is_port_present() users anyway? AFAICT all the cases will
>   also check the supports_X flags anyway (or have checked before).

After some trawling I can't see anything that should break from this,
so probably fine. But the commit message should probably elaborate
on this a bit more, just in case there is a regression.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> >  	}
> >> >  
> >> >  	/* FIXME maybe deal with port A as well? */
> >> > -- 
> >> > 2.20.1
> >> > 
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> 
> >> -- 
> >> Ville Syrjälä
> >> Intel
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Jan. 21, 2020, 9:44 a.m. UTC | #6 
On Fri, 17 Jan 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jan 17, 2020 at 05:28:23PM +0200, Jani Nikula wrote:
>> On Fri, 17 Jan 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Jan 17, 2020 at 05:12:38PM +0200, Ville Syrjälä wrote:
>> >> On Fri, Jan 17, 2020 at 04:29:28PM +0200, Jani Nikula wrote:
>> >> > Affects only two calls in output setup, and ddi init will check the
>> >> > features in more fine grained way.
>> >> > 
>> >> > This will make future changes easier.
>> >> > 
>> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 4 +---
>> >> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >> > 
>> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > index 4c69253739ec..70fb87e7afb6 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> >> > @@ -2236,9 +2236,7 @@ bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
>> >> >  		const struct ddi_vbt_port_info *port_info =
>> >> >  			&dev_priv->vbt.ddi_port_info[port];
>> >> >  
>> >> > -		return port_info->supports_dp ||
>> >> > -		       port_info->supports_dvi ||
>> >> > -		       port_info->supports_hdmi;
>> >> > +		return port_info->child;
>> >> 
>> >> Pondering what happens if there's a non-DP/DVI/HDMI port declared in the
>> >> VBT... I guess those should not have their dvo port set to anything we
>> >> accept?
>> >
>> > Umm, no. We accept DVO_PORT_CRT as PORT_E.
>> 
>> My argument is this:
>> 
>> - Shouldn't intel_bios_is_port_present(PORT_E) return true in that case?
>
> Only on hsw/bdw really.
>
>> 
>> - Where does the change make a functional difference in the
>>   intel_bios_is_port_present() users anyway? AFAICT all the cases will
>>   also check the supports_X flags anyway (or have checked before).
>
> After some trawling I can't see anything that should break from this,
> so probably fine. But the commit message should probably elaborate
> on this a bit more, just in case there is a regression.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for all the reviews! Expanded on the commit message on this one,
and pushed the lot.

BR,
Jani.


>
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >> 
>> >> >  	}
>> >> >  
>> >> >  	/* FIXME maybe deal with port A as well? */
>> >> > -- 
>> >> > 2.20.1
>> >> > 
>> >> > _______________________________________________
>> >> > Intel-gfx mailing list
>> >> > Intel-gfx@lists.freedesktop.org
>> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >> 
>> >> -- 
>> >> Ville Syrjälä
>> >> Intel
>> 
>> -- 
>> 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 4c69253739ec..70fb87e7afb6 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -2236,9 +2236,7 @@  bool intel_bios_is_port_present(struct drm_i915_private *dev_priv, enum port por
 		const struct ddi_vbt_port_info *port_info =
 			&dev_priv->vbt.ddi_port_info[port];
 
-		return port_info->supports_dp ||
-		       port_info->supports_dvi ||
-		       port_info->supports_hdmi;
+		return port_info->child;
 	}
 
 	/* FIXME maybe deal with port A as well? */