Re: [PATCH v3 070/105] drm/vc4: hdmi: rework connectors and encoders

[Stefan Wahren]

Am 02.06.20 um 17:54 schrieb Maxime Ripard:
> On Wed, May 27, 2020 at 11:41:24AM -0700, Eric Anholt wrote:
>> On Wed, May 27, 2020 at 8:51 AM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>>> the vc4_hdmi driver has some custom structures to hold the data it needs to
>>> associate with the drm_encoder and drm_connector structures.
>>>
>>> However, it allocates them separately from the vc4_hdmi structure which
>>> makes it more complicated than it needs to be.
>>>
>>> Move those structures to be contained by vc4_hdmi and update the code
>>> accordingly.
>>
>>> @@ -1220,7 +1219,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>>         struct drm_device *drm = dev_get_drvdata(master);
>>>         struct vc4_dev *vc4 = drm->dev_private;
>>>         struct vc4_hdmi *hdmi;
>>> -       struct vc4_hdmi_encoder *vc4_hdmi_encoder;
>>> +       struct drm_encoder *encoder;
>>>         struct device_node *ddc_node;
>>>         u32 value;
>>>         int ret;
>>> @@ -1229,14 +1228,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>>         if (!hdmi)
>>>                 return -ENOMEM;
>>>
>>> -       vc4_hdmi_encoder = devm_kzalloc(dev, sizeof(*vc4_hdmi_encoder),
>>> -                                       GFP_KERNEL);
>>> -       if (!vc4_hdmi_encoder)
>>> -               return -ENOMEM;
>>> -       vc4_hdmi_encoder->base.type = VC4_ENCODER_TYPE_HDMI0;
>>> -       hdmi->encoder = &vc4_hdmi_encoder->base.base;
>>> -
>>>         hdmi->pdev = pdev;
>>> +       encoder = &hdmi->encoder.base.base;
>>> +       encoder->base.type = VC4_ENCODER_TYPE_HDMI0;
>> Wait, does this patch build?
> All those patches were build tested, so yep
>
>> setting struct drm_encoder->base.type = VC4_* seems very wrong, when
>> previously we were setting struct vc4_hdmi_encoder->base.type (struct
>> vc4_encoder->type).
> So the structure layout now is that vc4_hdmi embeds vc4_hdmi_encoder as
> encoder. So &hdmi->encoder is a pointer to vc4_hdmi_encoder.
> vc4_hdmi_encoder's base is since that patch a struct vc4_encoder. and
> vc4_encoder's base is a drm_encoder.
>
> so encoder being a drm_encoder is correct there.
>
> However, drm_encoder's base is drm_mode_object that does have a type
> field, which is an uint32_t, which will accept a VC4_ENCODER_TYPE_* just
> fine...
>
> Now, drm_encoder_init will then kick in and call drm_mode_object_add
> which will override it to a proper value and since the clock select bit
> in the PV is the same for both HDMI0 and HDMI1, everything works just
> fine...
>
> Good catch, I'll fix it. And I guess it's a good indication we don't
> need a separate HDMI0 and HDMI1 encoder type.
>
FWIW this is the first patch which breaks X on my Raspberry Pi 3 B.

Here are the bisect results:

587d6e4a529a8d807a5c0bae583dd432d77064d6 bad (black screen, no heartbeat)

b0523c7b1c9d0edcd6c0fe6d2cb558a9ad5c60a8 good

2c6a651cac6359cb0244a40d3b7a14e72918f169 good

1705c3cb40906863ec0d24ee5ea5092f5ee2e994 bad (black screen, but heartbeat)

601527fea6bb226abd088a864e74b25368218e87 good

2165607ede34d229d0cbce916c70c7fb6c0337be good

f094f388fc2df848227e2ae648df2c97872df42b good

020de18840a1075b2671736c6cc2e451030fad74 bad (black screen, but heartbeat)

4c4da3823e4d1a8189e96a59a79451fff372f70b good

020de18840a1075b2671736c6cc2e451030fad74 is the first bad commit
commit 020de18840a1075b2671736c6cc2e451030fad74
Author: Maxime Ripard <maxime@xxxxxxxxxx>
Date:   Mon Jan 6 17:17:29 2020 +0100

    drm/vc4: hdmi: rework connectors and encoders
   
    the vc4_hdmi driver has some custom structures to hold the data it
needs to
    associate with the drm_encoder and drm_connector structures.
   
    However, it allocates them separately from the vc4_hdmi structure which
    makes it more complicated than it needs to be.
   
    Move those structures to be contained by vc4_hdmi and update the code
    accordingly.
   
    Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>