Re: [PATCH] drm/i915: fix for_each_digital_port

[Imre Deak <imre.deak@intel.com>]

On pe, 2015-04-17 at 09:36 -0300, Paulo Zanoni wrote:
> 2015-04-17 8:58 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > We should check if a given encoder is of a digital type before casting
> > it to a digital port object. This broke on HSW when iterating the VGA
> > encoder.
> >
> > Introduced in
> > commit b403745c84592b26a0713e6944c2b109f6df5c82
> > Author: Damien Lespiau <damien.lespiau@intel.com>
> > Date:   Mon Aug 4 22:01:33 2014 +0100
> >
> >     drm/i915: Iterate through the initialized DDIs to prepare their buffers
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90067
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 10 ++++++----
> >  drivers/gpu/drm/i915/intel_ddi.c |  3 ++-
> >  drivers/gpu/drm/i915/intel_drv.h | 10 ++++++++++
> >  3 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 40ef672..7160fc8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -251,10 +251,12 @@ enum hpd_pin {
> >                             &dev->mode_config.connector_list,   \
> >                             base.head)
> >
> > -#define for_each_digital_port(dev, digital_port)               \
> > -       list_for_each_entry(digital_port,                       \
> > -                           &dev->mode_config.encoder_list,     \
> > -                           base.base.head)
> > +#define for_each_digital_port(dev, __intel_encoder, digital_port)              \
> > +       list_for_each_entry(__intel_encoder,                            \
> > +                           &dev->mode_config.encoder_list,             \
> > +                           base.head)                                  \
> > +               if (intel_enc_is_digital(__intel_encoder) &&            \
> > +                   ((digital_port) = enc_to_dig_port(&__intel_encoder->base)))
> >
> >  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
> >         list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 455d44b..b5d7e74 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -370,13 +370,14 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev,
> >   */
> >  void intel_prepare_ddi(struct drm_device *dev)
> >  {
> > +       struct intel_encoder *__intel_encoder;
> >         struct intel_digital_port *intel_dig_port;
> >         bool visited[I915_MAX_PORTS] = { 0, };
> >
> >         if (!HAS_DDI(dev))
> >                 return;
> >
> > -       for_each_digital_port(dev, intel_dig_port) {
> > +       for_each_digital_port(dev, __intel_encoder, intel_dig_port) {
> 
> The problem is that now we're not gonna call
> intel_prepare_ddi_buffers() for PORT_E, which is the CRT port.
> 
> Perhaps we could make that function accept intel_encoder() and then
> somehow call intel_ddi_get_encoder_port() or something... Or maybe
> just special-case the CRT encoder. Just suggestions...

Thanks for catching this, completely missed that. I also noticed now
that MST encoders don't have an embedding digital_port object either, so
for those instead of the digital_port port field we were looking at the
mst_encoder pipe field :/

I'll try again with the above things fixed.

> 
> >                 if (visited[intel_dig_port->port])
> >                         continue;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 23a42a4..a354f26 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -870,6 +870,16 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> >         return container_of(intel_hdmi, struct intel_digital_port, hdmi);
> >  }
> >
> > +static inline bool intel_enc_is_digital(struct intel_encoder *intel_encoder)
> > +{
> > +       return  intel_encoder->type == INTEL_OUTPUT_HDMI ||
> > +               intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> > +               intel_encoder->type == INTEL_OUTPUT_EDP ||
> > +               intel_encoder->type == INTEL_OUTPUT_UNKNOWN ||
> > +               intel_encoder->type == INTEL_OUTPUT_DP_MST;
> > +
> > +}
> > +
> >  /*
> >   * Returns the number of planes for this pipe, ie the number of sprites + 1
> >   * (primary plane). This doesn't count the cursor plane then.
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx