[Intel-gfx] [PATCH 2/4] drm/i915: encoder/connector internal structure rework (v5)

[Zhenyu Wang]

On 2010.03.25 12:39:47 -0700, Eric Anholt wrote:
> 
> I sat down and did the application of this patch and the sed job, and
> looked at the diff between the two.  It makes the change reviewable, but
> still
> 
> 11 files changed, 524 insertions(+), 417 deletions(-)
> 
> As far as the changes in that diff go:
> 
> hotplug method becomes a connector function?  We get hotplug events from
> encoders, not connectors.

yeah, your're right, hotplug should come from encoder.

> 
> It's encoders that ask for load detect pipes -- why is load detect
> taking a connector, then trying to work back to what encoder needs the
> pipe?

yeah, should just use encoder.

> 
> DDC bus pointer moved to the connector?  That seems wrong.  We use a
> different DDC bus for a DVI connector when detecting through the VGA
> encoder than through the HDMI/SDVO encoder, right?  So each encoder
> would want to keep track of its DDC buses for detecting its connectors.

As we usually use DDC bus for EDID check in connector detect function, I
thought it's easy to let connector hold the ddc bus. Yeah for case in DVI-I
and the fact that our DDC bus is bound with some GPIO port actually, it looks
the right thing for DDC bus is moved to encoder too.

> 
> @@ -351,7 +357,8 @@ intel_dvo_get_current_mode (struct drm_connector *connector)
>  {
>         struct drm_device *dev = connector->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct intel_encoder *intel_encoder = to_intel_encoder(connector);
> +       struct drm_encoder *encoder = intel_best_encoder(connector);
> +       struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
>         struct intel_dvo_device *dvo = intel_encoder->dev_priv;
> 
> This is bad.  We're going to look up an encoder off of a connector
> pointer and just *hope* we get a DVO one back?  I mean, right now the
> split doesn't really exist and it's a 1:1 mapping, but it pretty clearly
> indicates that the arg should be an encoder, not a connector.  It's all
> over this code, including in encoders where we do want to kill the 1:1
> mapping of encoders to connectors.

My first patch doesn't kill any 1:1 mapping, intel_best_encoder() just
checks the already bound ids between connector and encoder when setup, so
it should point to the specific encoder. This is also kept as it for 
multifunction SDVO, which is multiple connectors bound to one encoder.
But we need to check encoder type for connector like DVI-I, that could
be driven by VGA or DVI-D encoder, but my patch didn't cover that.

> 
> Overall, there are a few good bits in this patch (like walking over
> encoder lists in intel_display.c, instead of walking over the connector
> list and then looking at the encoders attached to them to see if they're
> the ones we're looking for), but they should be split out into
> individual fixes instead of being a giant patchbomb.
> 
> I'm sending out two patches for the mechanical parts of the diff.  I'm
> hoping we can work off of those going forward.

Thanks a lot! This approach seems better, my idea stuck in binding 
structure change with conversion all together. I'll rebase and fix stuff
on your patches.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100326/501126ac/attachment.sig>