On Tue, Dec 31, 2019 at 2:25 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 23 Dec 2019, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote:
> > Support remaining platforms under HAS_DDI() by providing a slightly more
> > complex is_port_present() hook. The downside is that now we call
> > I915_READ(SFUSE_STRAP) for each port being initialized, but that's only
> > on initialization: a few more mmio reads won't hurt.
> >
> > Alternatives would be to provide one hook per port, or to have a
> > "pre_init()" hook that takes care of the mmio read. However I think this
> > is simpler - we may need to adapt if future platforms don't follow the
> > same initialization "template".
>
> All of this really makes me wonder if we end up being *more* complicated
> overall by trying very hard to make this generic, when, in reality, it
> doesn't seem to be all that generic.
maybe, but it seems the ddi init part has settled and we are now mainly doing
if / else for additional platforms with just calls to intel_ddi_init()
and intel_dsi_init(),
so I think it was worth merging them together. I also think this
prevents additional
hacks from sprinkling here.
Lucas De Marchi
>
> As I said, two relatively low hanging improvements would be a) moving
> VBT specific hacks to intel_bios.c and b) adding port mask to
> intel_device_info. Those two alone would go a long way in simplifying
> intel_setup_outputs().
>
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 72 +++++++++++++-------
> > 1 file changed, 46 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6b4d320ff92c..ad85cf75c815 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -16246,6 +16246,34 @@ static bool icl_is_port_present(struct drm_i915_private *i915,
> > intel_bios_is_port_present(i915, PORT_F);
> > }
> >
> > +static bool ddi_is_port_present(struct drm_i915_private *i915,
> > + const struct intel_ddi_port_info *port_info)
> > +{
> > + /* keep I915_READ() happy */
>
> Display could get rid of I915_READ and I915_WRITE after
> https://patchwork.freedesktop.org/series/70298/ ...
>
> BR,
> Jani.
>
> > + struct drm_i915_private *dev_priv = i915;
> > +
> > + if (port_info->port == PORT_A)
> > + return I915_READ(DDI_BUF_CTL(PORT_A))
> > + & DDI_INIT_DISPLAY_DETECTED;
> > +
> > + if (port_info->port == PORT_E)
> > + return IS_GEN9_BC(dev_priv) &&
> > + intel_bios_is_port_present(i915, PORT_E);
> > +
> > + switch (port_info->port) {
> > + case PORT_B:
> > + return I915_READ(SFUSE_STRAP) & SFUSE_STRAP_DDIB_DETECTED;
> > + case PORT_C:
> > + return I915_READ(SFUSE_STRAP) & SFUSE_STRAP_DDIC_DETECTED;
> > + case PORT_D:
> > + return I915_READ(SFUSE_STRAP) & SFUSE_STRAP_DDID_DETECTED;
> > + case PORT_F:
> > + return I915_READ(SFUSE_STRAP) & SFUSE_STRAP_DDIF_DETECTED;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static const struct intel_output tgl_output = {
> > .dsi_init = icl_dsi_init,
> > .ddi_ports = {
> > @@ -16296,11 +16324,24 @@ static const struct intel_output gen9lp_output = {
> > },
> > };
> >
> > +static const struct intel_output ddi_output = {
> > + .is_port_present = ddi_is_port_present,
> > + .ddi_ports = {
> > + { .port = PORT_A },
> > + { .port = PORT_B },
> > + { .port = PORT_C },
> > + { .port = PORT_D },
> > + { .port = PORT_E },
> > + { .port = PORT_F },
> > + { .port = PORT_NONE }
> > + }
> > +};
> > +
> > /*
> > * Use a description-based approach for platforms that can be supported with a
> > * static table
> > */
> > -static void setup_ddi_outputs_desc(struct drm_i915_private *i915)
> > +static void setup_ddi_outputs(struct drm_i915_private *i915)
> > {
> > const struct intel_output *output;
> > const struct intel_ddi_port_info *port_info;
> > @@ -16313,6 +16354,8 @@ static void setup_ddi_outputs_desc(struct drm_i915_private *i915)
> > output = &icl_output;
> > else if (IS_GEN9_LP(i915))
> > output = &gen9lp_output;
> > + else
> > + output = &ddi_output;
> >
> > for (port_info = output->ddi_ports;
> > port_info->port != PORT_NONE; port_info++) {
> > @@ -16338,35 +16381,12 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
> > return;
> >
> > if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
> > - setup_ddi_outputs_desc(dev_priv);
> > + setup_ddi_outputs(dev_priv);
> > } else if (HAS_DDI(dev_priv)) {
> > - int found;
> > -
> > if (intel_ddi_crt_present(dev_priv))
> > intel_crt_init(dev_priv);
> >
> > - found = I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_INIT_DISPLAY_DETECTED;
> > - if (found)
> > - intel_ddi_init(dev_priv, PORT_A);
> > -
> > - /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP
> > - * register */
> > - found = I915_READ(SFUSE_STRAP);
> > -
> > - if (found & SFUSE_STRAP_DDIB_DETECTED)
> > - intel_ddi_init(dev_priv, PORT_B);
> > - if (found & SFUSE_STRAP_DDIC_DETECTED)
> > - intel_ddi_init(dev_priv, PORT_C);
> > - if (found & SFUSE_STRAP_DDID_DETECTED)
> > - intel_ddi_init(dev_priv, PORT_D);
> > - if (found & SFUSE_STRAP_DDIF_DETECTED)
> > - intel_ddi_init(dev_priv, PORT_F);
> > - /*
> > - * On SKL we don't have a way to detect DDI-E so we rely on VBT.
> > - */
> > - if (IS_GEN9_BC(dev_priv) &&
> > - intel_bios_is_port_present(dev_priv, PORT_E))
> > - intel_ddi_init(dev_priv, PORT_E);
> > + setup_ddi_outputs(dev_priv);
> > } else if (HAS_PCH_SPLIT(dev_priv)) {
> > int found;
>
> --
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
