Re: [PATCH 02/10] drm/i915: Extract intel_ddi_get_buf_trans_hdmi()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 18, 2017 at 08:09:24PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 16, 2017 at 04:41:49PM -0700, Ausmus, James wrote:
> > On Mon, Oct 16, 2017 at 7:56 AM, Ville Syrjala
> > <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Introduce intel_ddi_get_buf_trans_hdmi() and start using it where we
> > > can.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 50 ++++++++++++++++++++++------------------
> > >  1 file changed, 28 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 55937abda61f..e6c884a6d408 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -641,6 +641,24 @@ intel_ddi_get_buf_trans_fdi(struct drm_i915_private *dev_priv,
> > >         return NULL;
> > >  }
> > >
> > > +static const struct ddi_buf_trans *
> > > +intel_ddi_get_buf_trans_hdmi(struct drm_i915_private *dev_priv,
> > > +                            int *n_entries)
> > > +{
> > > +       if (IS_GEN9_BC(dev_priv)) {
> > > +               return skl_get_buf_trans_hdmi(dev_priv, n_entries);
> > > +       } else if (IS_BROADWELL(dev_priv)) {
> > > +               *n_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> > > +               return bdw_ddi_translations_hdmi;
> > > +       } else if (IS_HASWELL(dev_priv)) {
> > > +               *n_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> > > +               return hsw_ddi_translations_hdmi;
> > > +       }
> > > +
> > > +       *n_entries = 0;
> > > +       return NULL;
> > > +}
> > > +
> > >  static const struct cnl_ddi_buf_trans *
> > >  cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv, int *n_entries)
> > >  {
> > > @@ -723,18 +741,17 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por
> > >                 cnl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > >                 hdmi_default_entry = n_hdmi_entries - 1;
> > 
> > Why are you skipping the CNL case? If you extract it in to
> > intel_ddi_get_buf_trans_hdmi as well, you could just
> > unconditionally call intel_ddi_get_buf_trans_hdmi, and just set
> > hdmi_default_entry in the platform-specific section.
> 
> The problem is that CNL (and BXT) use a different structure for their
> buf trans values. So far I've not bothered to check if we could change
> them to use the HSW+ structure. I guess the other options would be to
> slap a union into the structure, or make these functions return
> void *, but neither option seems very appealing since we'd lose some
> type safety.
> 
> In any case that'd be material for followup work. Maybe someone else
> could take a look at it? ;)
>

I like the idea of the union in the buf_trans structures.
But yes for now, this looks good.

Reviewed-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>

Manasi
 
> > 
> > >         } else if (IS_GEN9_BC(dev_priv)) {
> > > -               skl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > +               intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > >                 hdmi_default_entry = 8;
> > >         } else if (IS_BROADWELL(dev_priv)) {
> > > -               n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> > > +               intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > >                 hdmi_default_entry = 7;
> > >         } else if (IS_HASWELL(dev_priv)) {
> > > -               n_hdmi_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> > > +               intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > >                 hdmi_default_entry = 6;
> > >         } else {
> > >                 WARN(1, "ddi translation table missing\n");
> > > -               n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> > > -               hdmi_default_entry = 7;
> > > +               return 0;
> > >         }
> > >
> > >         /* Choose a good default if VBT is badly populated */
> > > @@ -810,23 +827,12 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder)
> > >
> > >         hdmi_level = intel_ddi_hdmi_level(dev_priv, port);
> > >
> > > -       if (IS_GEN9_BC(dev_priv)) {
> > > -               ddi_translations_hdmi = skl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > +       ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > >
> > > -               /* If we're boosting the current, set bit 31 of trans1 */
> > > -               if (dev_priv->vbt.ddi_port_info[port].hdmi_boost_level)
> > > -                       iboost_bit = DDI_BUF_BALANCE_LEG_ENABLE;
> > > -       } else if (IS_BROADWELL(dev_priv)) {
> > > -               ddi_translations_hdmi = bdw_ddi_translations_hdmi;
> > > -               n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> > > -       } else if (IS_HASWELL(dev_priv)) {
> > > -               ddi_translations_hdmi = hsw_ddi_translations_hdmi;
> > > -               n_hdmi_entries = ARRAY_SIZE(hsw_ddi_translations_hdmi);
> > > -       } else {
> > > -               WARN(1, "ddi translation table missing\n");
> > > -               ddi_translations_hdmi = bdw_ddi_translations_hdmi;
> > > -               n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi);
> > > -       }
> > > +       /* If we're boosting the current, set bit 31 of trans1 */
> > > +       if (IS_GEN9_BC(dev_priv) &&
> > > +           dev_priv->vbt.ddi_port_info[port].hdmi_boost_level)
> > > +               iboost_bit = DDI_BUF_BALANCE_LEG_ENABLE;
> > >
> > >         /* Entry 9 is for HDMI: */
> > >         I915_WRITE(DDI_BUF_TRANS_LO(port, 9),
> > > @@ -1820,7 +1826,7 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, u32 level)
> > >                 if (hdmi_iboost) {
> > >                         iboost = hdmi_iboost;
> > >                 } else {
> > > -                       ddi_translations = skl_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > +                       ddi_translations = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > >                         iboost = ddi_translations[level].i_boost;
> > >                 }
> > >         } else {
> > > --
> > > 2.13.6
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > 
> > 
> > James Ausmus
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux