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

[Eric Anholt]

On Thu, 25 Mar 2010 10:30:18 +0800, Zhenyu Wang <zhenyuw at linux.intel.com> wrote:
> On 2010.03.24 14:03:16 -0700, Eric Anholt wrote:
> > 
> > This is not acceptable.  The problem in this series is still that step 1
> > is:
> > a) s/intel_output/intel_encoder/
> > b) also change a bunch of code.
> 
> 'intel_output' is replaced by 'intel_encoder' and 'intel_connector',
> which I hope should be the clean way to represent our display hw in drm
> KMS framework. As I tried to remove it completely, and it's used in all
> places e.g all outputs driver code, I have to make them in one place for
> bisectable. Each output driver's original 'intel_output' becomes 'intel_encoder' 
> part and 'intel_connector' part. 
> 
> > 
> > When someone bisects down to this, they will probably just give up.
> > Doing just the sed is:
> > 
> > 11 files changed, 663 insertions(+), 663 deletions(-)
> > 
> > Why should someone have to look through 872 lines of code to find the
> > bug in the 209 lines of non-automated patch?  And why should I have to
> > review all that sed job even before a bug's been found?
> 
> It's not a sed job to me, I have to check the functional meaning for the
> change to apply to encoder or connector, to be sure in the new representation
> they still do the right reference.
> 
> Yeah, maybe the first restructure patch for the different output presentation
> is mixed with structure name change and some semantic change too. But the whole
> point is to have no regression compared to origin driver, and just convert the
> structures. Or do you have suggestion on a better way?

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.

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?

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.

@@ -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.

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100325/6a89a62a/attachment.sig>