| Message ID | 020de18840a1075b2671736c6cc2e451030fad74.1590594512.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Support BCM2711 Display Pipeline | expand |
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@cerno.tech> 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. > Other than this, patch 68-78 r-b. Thanks for your review! Maxime
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@cerno.tech> 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@cerno.tech> 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@cerno.tech>
Hi Stefan, On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: > 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@cerno.tech> 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@cerno.tech> > 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@cerno.tech> So it looks like there was two issues on the Pi3. The first one was causing the timeouts (and therefore likely the black screen but heartbeat case you had) and I've fixed it. However, I can indeed reproduce the case with the black screen / no heartbeat you mentionned. My bisection however returns that it's the patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. I've pushed my updated branch, if you have some spare time, it would be great if you could confirm it on your Pi. Thanks! Maxime
Hi Maxime, Am 05.06.20 um 16:35 schrieb Maxime Ripard: > Hi Stefan, > > On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: >> 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@cerno.tech> 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@cerno.tech> >> 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@cerno.tech> > So it looks like there was two issues on the Pi3. The first one was > causing the timeouts (and therefore likely the black screen but > heartbeat case you had) and I've fixed it. > > However, I can indeed reproduce the case with the black screen / no > heartbeat you mentionned. My bisection however returns that it's the > patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. > I've pushed my updated branch, if you have some spare time, it would be > great if you could confirm it on your Pi. yesterday i checked out your latest rpi4-kms branch, but i was still facing similiar issues with my Raspberry Pi 3 and multi_v7_defconfig (heartbeat stops, splashscreen freeze, heartbeat is abnormal fast). So i tried to bisect but the offending commit didn't cause an issue the second time. By accident i noticed that a simple reboot seems to hang for at least 8 minutes (using b0523c7b1c9d0edcd the base of your branch). This usually take a few seconds. So i consider this base on linux-next as too unstable for reliable testing. Is it possible to rebase this on something more stable like linux-5.7 or at least drm-misc-next? This should avoid chasing unrelated issues. Regards Stefan > > Thanks! > Maxime
Hi Stefan, On Sat, Jun 06, 2020 at 10:06:12AM +0200, Stefan Wahren wrote: > Hi Maxime, > > Am 05.06.20 um 16:35 schrieb Maxime Ripard: > > Hi Stefan, > > > > On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: > >> 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@cerno.tech> 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@cerno.tech> > >> 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@cerno.tech> > > So it looks like there was two issues on the Pi3. The first one was > > causing the timeouts (and therefore likely the black screen but > > heartbeat case you had) and I've fixed it. > > > > However, I can indeed reproduce the case with the black screen / no > > heartbeat you mentionned. My bisection however returns that it's the > > patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. > > I've pushed my updated branch, if you have some spare time, it would be > > great if you could confirm it on your Pi. > > yesterday i checked out your latest rpi4-kms branch, but i was still > facing similiar issues with my Raspberry Pi 3 and multi_v7_defconfig > (heartbeat stops, splashscreen freeze, heartbeat is abnormal fast). So i > tried to bisect but the offending commit didn't cause an issue the > second time. > > By accident i noticed that a simple reboot seems to hang for at least 8 > minutes (using b0523c7b1c9d0edcd the base of your branch). This usually > take a few seconds. So i consider this base on linux-next as too > unstable for reliable testing. > > Is it possible to rebase this on something more stable like linux-5.7 or > at least drm-misc-next? This should avoid chasing unrelated issues. I've rebased it on 5.7 here: https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms-5.7 And it looks to be indeed an issue coming from next. That branch can start the desktop just fine on an RPi3 here. It would be great if you could confirm on your end. Thanks! Maxime
Hi Maxime, Am 11.06.20 um 15:34 schrieb Maxime Ripard: > Hi Stefan, > > On Sat, Jun 06, 2020 at 10:06:12AM +0200, Stefan Wahren wrote: >> Hi Maxime, >> >> Am 05.06.20 um 16:35 schrieb Maxime Ripard: >>> Hi Stefan, >>> >>> On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: >>>> Am 02.06.20 um 17:54 schrieb Maxime Ripard: >>>> 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@cerno.tech> >>>> 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@cerno.tech> >>> So it looks like there was two issues on the Pi3. The first one was >>> causing the timeouts (and therefore likely the black screen but >>> heartbeat case you had) and I've fixed it. >>> >>> However, I can indeed reproduce the case with the black screen / no >>> heartbeat you mentionned. My bisection however returns that it's the >>> patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. >>> I've pushed my updated branch, if you have some spare time, it would be >>> great if you could confirm it on your Pi. >> yesterday i checked out your latest rpi4-kms branch, but i was still >> facing similiar issues with my Raspberry Pi 3 and multi_v7_defconfig >> (heartbeat stops, splashscreen freeze, heartbeat is abnormal fast). So i >> tried to bisect but the offending commit didn't cause an issue the >> second time. >> >> By accident i noticed that a simple reboot seems to hang for at least 8 >> minutes (using b0523c7b1c9d0edcd the base of your branch). This usually >> take a few seconds. So i consider this base on linux-next as too >> unstable for reliable testing. >> >> Is it possible to rebase this on something more stable like linux-5.7 or >> at least drm-misc-next? This should avoid chasing unrelated issues. > I've rebased it on 5.7 here: > https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms-5.7 > > And it looks to be indeed an issue coming from next. That branch can > start the desktop just fine on an RPi3 here. It would be great if you > could confirm on your end. > > Thanks! > Maxime thank you very much. The good news are that the "black screen, but heartbeat" issue and reboot hang are gone. Unfortunately the "no heartbeat" issue is still there. Here are more details about the issue. It doesn't occur everytime. I would guess the probability is about 40 percent, which made bisecting much harder. It is reproducible on my 2 Raspberry Pi 3 B Rev 1.2. It is also seems independent from the display because the problem occured on my Computer display and my TV. Yes, i can confirm that i was able to bisect the issue to this commit: commit 7b7e335f98c6f3199dcc7db83469dccac66dca1e Author: Maxime Ripard <maxime@cerno.tech> Date: Tue May 12 11:36:13 2020 +0200 drm/vc4: hdmi: Implement finer-grained hooks In order to prevent some pixels getting stuck in an unflushable FIFO on bcm2711, we need to enable the HVS, the pixelvalve (the CRTC) and the HDMI controller (the encoder) in an intertwined way, and with tight delays. However, the atomic callbacks don't really provide a way to work with either constraints, so we need to roll our own callbacks so that we can provide those guarantees. Since those callbacks have been implemented and called in the CRTC code, we can just implement them in the HDMI driver now. Signed-off-by: Maxime Ripard <maxime@cerno.tech>
On Sun, Jun 14, 2020 at 06:16:56PM +0200, Stefan Wahren wrote: > Am 11.06.20 um 15:34 schrieb Maxime Ripard: > > Hi Stefan, > > > > On Sat, Jun 06, 2020 at 10:06:12AM +0200, Stefan Wahren wrote: > >> Hi Maxime, > >> > >> Am 05.06.20 um 16:35 schrieb Maxime Ripard: > >>> Hi Stefan, > >>> > >>> On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: > >>>> Am 02.06.20 um 17:54 schrieb Maxime Ripard: > >>>> 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@cerno.tech> > >>>> 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@cerno.tech> > >>> So it looks like there was two issues on the Pi3. The first one was > >>> causing the timeouts (and therefore likely the black screen but > >>> heartbeat case you had) and I've fixed it. > >>> > >>> However, I can indeed reproduce the case with the black screen / no > >>> heartbeat you mentionned. My bisection however returns that it's the > >>> patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. > >>> I've pushed my updated branch, if you have some spare time, it would be > >>> great if you could confirm it on your Pi. > >> yesterday i checked out your latest rpi4-kms branch, but i was still > >> facing similiar issues with my Raspberry Pi 3 and multi_v7_defconfig > >> (heartbeat stops, splashscreen freeze, heartbeat is abnormal fast). So i > >> tried to bisect but the offending commit didn't cause an issue the > >> second time. > >> > >> By accident i noticed that a simple reboot seems to hang for at least 8 > >> minutes (using b0523c7b1c9d0edcd the base of your branch). This usually > >> take a few seconds. So i consider this base on linux-next as too > >> unstable for reliable testing. > >> > >> Is it possible to rebase this on something more stable like linux-5.7 or > >> at least drm-misc-next? This should avoid chasing unrelated issues. > > I've rebased it on 5.7 here: > > https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms-5.7 > > > > And it looks to be indeed an issue coming from next. That branch can > > start the desktop just fine on an RPi3 here. It would be great if you > > could confirm on your end. > > > > Thanks! > > Maxime > > thank you very much. The good news are that the "black screen, but > heartbeat" issue and reboot hang are gone. Unfortunately the "no > heartbeat" issue is still there. > > Here are more details about the issue. It doesn't occur everytime. I > would guess the probability is about 40 percent, which made bisecting > much harder. Are you sure about that 40% reliability? I found out that the culprit was that the commit we mentionned was actually running atomic_disable before our own custom callbacks, meaning that we would run the custom callbacks with the clocks and the power domain shut down, resulting in a stall. I was seeing it all the time when X was shutting down the display, but maybe you were changing the resolution between the framebuffer console or something, and since the power domain is shut down asynchronously, it wasn't running fast enough for the next enable to come up and re-enable it again? > It is reproducible on my 2 Raspberry Pi 3 B Rev 1.2. It is > also seems independent from the display because the problem occured on > my Computer display and my TV. But only on HDMI, right? I've pushed a new branch with that fix. Maxime
Hi Maxime, Am 16.06.20 um 14:30 schrieb Maxime Ripard: > On Sun, Jun 14, 2020 at 06:16:56PM +0200, Stefan Wahren wrote: >> Am 11.06.20 um 15:34 schrieb Maxime Ripard: >>> Hi Stefan, >>> >>> On Sat, Jun 06, 2020 at 10:06:12AM +0200, Stefan Wahren wrote: >>>> Hi Maxime, >>>> >>>> Am 05.06.20 um 16:35 schrieb Maxime Ripard: >>>>> Hi Stefan, >>>>> >>>>> On Wed, Jun 03, 2020 at 07:32:30PM +0200, Stefan Wahren wrote: >>>>>> Am 02.06.20 um 17:54 schrieb Maxime Ripard: >>>>>> 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@cerno.tech> >>>>>> 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@cerno.tech> >>>>> So it looks like there was two issues on the Pi3. The first one was >>>>> causing the timeouts (and therefore likely the black screen but >>>>> heartbeat case you had) and I've fixed it. >>>>> >>>>> However, I can indeed reproduce the case with the black screen / no >>>>> heartbeat you mentionned. My bisection however returns that it's the >>>>> patch "drm/vc4: hdmi: Implement finer-grained hooks" that is at fault. >>>>> I've pushed my updated branch, if you have some spare time, it would be >>>>> great if you could confirm it on your Pi. >>>> yesterday i checked out your latest rpi4-kms branch, but i was still >>>> facing similiar issues with my Raspberry Pi 3 and multi_v7_defconfig >>>> (heartbeat stops, splashscreen freeze, heartbeat is abnormal fast). So i >>>> tried to bisect but the offending commit didn't cause an issue the >>>> second time. >>>> >>>> By accident i noticed that a simple reboot seems to hang for at least 8 >>>> minutes (using b0523c7b1c9d0edcd the base of your branch). This usually >>>> take a few seconds. So i consider this base on linux-next as too >>>> unstable for reliable testing. >>>> >>>> Is it possible to rebase this on something more stable like linux-5.7 or >>>> at least drm-misc-next? This should avoid chasing unrelated issues. >>> I've rebased it on 5.7 here: >>> https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms-5.7 >>> >>> And it looks to be indeed an issue coming from next. That branch can >>> start the desktop just fine on an RPi3 here. It would be great if you >>> could confirm on your end. >>> >>> Thanks! >>> Maxime >> thank you very much. The good news are that the "black screen, but >> heartbeat" issue and reboot hang are gone. Unfortunately the "no >> heartbeat" issue is still there. >> >> Here are more details about the issue. It doesn't occur everytime. I >> would guess the probability is about 40 percent, which made bisecting >> much harder. > Are you sure about that 40% reliability? it's more a gut feeling than a statistical analyze. It's definitely not 100% in my setup. > I found out that the culprit > was that the commit we mentionned was actually running atomic_disable > before our own custom callbacks, meaning that we would run the custom > callbacks with the clocks and the power domain shut down, resulting in a > stall. > > I was seeing it all the time when X was shutting down the display, but > maybe you were changing the resolution between the framebuffer console > or something, and since the power domain is shut down asynchronously, it > wasn't running fast enough for the next enable to come up and re-enable > it again? > >> It is reproducible on my 2 Raspberry Pi 3 B Rev 1.2. It is >> also seems independent from the display because the problem occured on >> my Computer display and my TV. > But only on HDMI, right? I only tested it with HDMI displays. All tests without any display were always successful. > > I've pushed a new branch with that fix. I tested 8 times in row without any issue. You got it. Thanks Stefan > > Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 41573fca5a40..c1cb8790b552 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -191,20 +191,15 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = .get_modes = vc4_hdmi_connector_get_modes, }; -static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev, - struct drm_encoder *encoder, - struct i2c_adapter *ddc) +static int vc4_hdmi_connector_init(struct drm_device *dev, + struct vc4_hdmi *vc4_hdmi, + struct i2c_adapter *ddc) { - struct drm_connector *connector; - struct vc4_hdmi_connector *hdmi_connector; + struct vc4_hdmi_connector *hdmi_connector = &vc4_hdmi->connector; + struct drm_connector *connector = &hdmi_connector->base; + struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; int ret; - hdmi_connector = devm_kzalloc(dev->dev, sizeof(*hdmi_connector), - GFP_KERNEL); - if (!hdmi_connector) - return ERR_PTR(-ENOMEM); - connector = &hdmi_connector->base; - hdmi_connector->encoder = encoder; drm_connector_init_with_ddc(dev, connector, @@ -216,7 +211,7 @@ static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev, /* Create and attach TV margin props to this connector. */ ret = drm_mode_create_tv_margin_properties(dev); if (ret) - return ERR_PTR(ret); + return ret; drm_connector_attach_tv_margin_properties(connector); @@ -228,7 +223,7 @@ static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev, drm_connector_attach_encoder(connector, encoder); - return connector; + return 0; } static int vc4_hdmi_stop_packet(struct drm_encoder *encoder, @@ -298,21 +293,22 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct vc4_dev *vc4 = encoder->dev->dev_private; struct vc4_hdmi *hdmi = vc4->hdmi; - struct drm_connector_state *cstate = hdmi->connector->state; + struct drm_connector *connector = &hdmi->connector.base; + struct drm_connector_state *cstate = connector->state; struct drm_crtc *crtc = encoder->crtc; const struct drm_display_mode *mode = &crtc->state->adjusted_mode; union hdmi_infoframe frame; int ret; ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, - hdmi->connector, mode); + connector, mode); if (ret < 0) { DRM_ERROR("couldn't fill AVI infoframe\n"); return; } drm_hdmi_avi_infoframe_quant_range(&frame.avi, - hdmi->connector, mode, + connector, mode, vc4_encoder->limited_rgb_range ? HDMI_QUANTIZATION_RANGE_LIMITED : HDMI_QUANTIZATION_RANGE_FULL); @@ -628,7 +624,8 @@ static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = { /* HDMI audio codec callbacks */ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *hdmi) { - struct drm_device *drm = hdmi->encoder->dev; + struct drm_encoder *encoder = &hdmi->encoder.base.base; + struct drm_device *drm = encoder->dev; struct vc4_dev *vc4 = to_vc4_dev(drm); u32 hsm_clock = clk_get_rate(hdmi->hsm_clock); unsigned long n, m; @@ -647,7 +644,7 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *hdmi) static void vc4_hdmi_set_n_cts(struct vc4_hdmi *hdmi) { - struct drm_encoder *encoder = hdmi->encoder; + struct drm_encoder *encoder = &hdmi->encoder.base.base; struct drm_crtc *crtc = encoder->crtc; struct drm_device *drm = encoder->dev; struct vc4_dev *vc4 = to_vc4_dev(drm); @@ -685,7 +682,8 @@ static int vc4_hdmi_audio_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct vc4_hdmi *hdmi = dai_to_hdmi(dai); - struct drm_encoder *encoder = hdmi->encoder; + struct drm_encoder *encoder = &hdmi->encoder.base.base; + struct drm_connector *connector = &hdmi->connector.base; struct vc4_dev *vc4 = to_vc4_dev(encoder->dev); int ret; @@ -702,8 +700,7 @@ static int vc4_hdmi_audio_startup(struct snd_pcm_substream *substream, VC4_HDMI_RAM_PACKET_ENABLE)) return -ENODEV; - ret = snd_pcm_hw_constraint_eld(substream->runtime, - hdmi->connector->eld); + ret = snd_pcm_hw_constraint_eld(substream->runtime, connector->eld); if (ret) return ret; @@ -717,7 +714,7 @@ static int vc4_hdmi_audio_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) static void vc4_hdmi_audio_reset(struct vc4_hdmi *hdmi) { - struct drm_encoder *encoder = hdmi->encoder; + struct drm_encoder *encoder = &hdmi->encoder.base.base; struct drm_device *drm = encoder->dev; struct device *dev = &hdmi->pdev->dev; struct vc4_dev *vc4 = to_vc4_dev(drm); @@ -751,7 +748,7 @@ static int vc4_hdmi_audio_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct vc4_hdmi *hdmi = dai_to_hdmi(dai); - struct drm_encoder *encoder = hdmi->encoder; + struct drm_encoder *encoder = &hdmi->encoder.base.base; struct drm_device *drm = encoder->dev; struct device *dev = &hdmi->pdev->dev; struct vc4_dev *vc4 = to_vc4_dev(drm); @@ -824,7 +821,7 @@ static int vc4_hdmi_audio_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct vc4_hdmi *hdmi = dai_to_hdmi(dai); - struct drm_encoder *encoder = hdmi->encoder; + struct drm_encoder *encoder = &hdmi->encoder.base.base; struct drm_device *drm = encoder->dev; struct vc4_dev *vc4 = to_vc4_dev(drm); @@ -868,9 +865,10 @@ static int vc4_hdmi_audio_eld_ctl_info(struct snd_kcontrol *kcontrol, { struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); struct vc4_hdmi *hdmi = snd_component_to_hdmi(component); + struct drm_connector *connector = &hdmi->connector.base; uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES; - uinfo->count = sizeof(hdmi->connector->eld); + uinfo->count = sizeof(connector->eld); return 0; } @@ -880,9 +878,10 @@ static int vc4_hdmi_audio_eld_ctl_get(struct snd_kcontrol *kcontrol, { struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); struct vc4_hdmi *hdmi = snd_component_to_hdmi(component); + struct drm_connector *connector = &hdmi->connector.base; - memcpy(ucontrol->value.bytes.data, hdmi->connector->eld, - sizeof(hdmi->connector->eld)); + memcpy(ucontrol->value.bytes.data, connector->eld, + sizeof(connector->eld)); return 0; } @@ -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; + hdmi->hdmicore_regs = vc4_ioremap_regs(pdev, 0); if (IS_ERR(hdmi->hdmicore_regs)) return PTR_ERR(hdmi->hdmicore_regs); @@ -1322,15 +1317,13 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) } pm_runtime_enable(dev); - drm_simple_encoder_init(drm, hdmi->encoder, DRM_MODE_ENCODER_TMDS); - drm_encoder_helper_add(hdmi->encoder, &vc4_hdmi_encoder_helper_funcs); + drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); + drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs); - hdmi->connector = - vc4_hdmi_connector_init(drm, hdmi->encoder, hdmi->ddc); - if (IS_ERR(hdmi->connector)) { - ret = PTR_ERR(hdmi->connector); + ret = vc4_hdmi_connector_init(drm, hdmi, hdmi->ddc); + if (ret) goto err_destroy_encoder; - } + #ifdef CONFIG_DRM_VC4_HDMI_CEC hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, vc4, "vc4", @@ -1340,7 +1333,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) if (ret < 0) goto err_destroy_conn; - cec_fill_conn_info_from_drm(&conn_info, hdmi->connector); + cec_fill_conn_info_from_drm(&conn_info, &hdmi->connector.base); cec_s_conn_info(hdmi->cec_adap, &conn_info); HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, 0xffffffff); @@ -1377,10 +1370,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) err_delete_cec_adap: cec_delete_adapter(hdmi->cec_adap); err_destroy_conn: - vc4_hdmi_connector_destroy(hdmi->connector); + vc4_hdmi_connector_destroy(&hdmi->connector.base); #endif err_destroy_encoder: - drm_encoder_cleanup(hdmi->encoder); + drm_encoder_cleanup(encoder); err_unprepare_hsm: clk_disable_unprepare(hdmi->hsm_clock); pm_runtime_disable(dev); @@ -1398,8 +1391,8 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, struct vc4_hdmi *hdmi = vc4->hdmi; cec_unregister_adapter(hdmi->cec_adap); - vc4_hdmi_connector_destroy(hdmi->connector); - drm_encoder_cleanup(hdmi->encoder); + vc4_hdmi_connector_destroy(&hdmi->connector.base); + drm_encoder_cleanup(&hdmi->encoder.base.base); clk_disable_unprepare(hdmi->hsm_clock); pm_runtime_disable(dev); diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 5ec5d1f6b1e6..17079a39f1b1 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -8,6 +8,36 @@ #include "vc4_drv.h" +/* VC4 HDMI encoder KMS struct */ +struct vc4_hdmi_encoder { + struct vc4_encoder base; + bool hdmi_monitor; + bool limited_rgb_range; +}; + +static inline struct vc4_hdmi_encoder * +to_vc4_hdmi_encoder(struct drm_encoder *encoder) +{ + return container_of(encoder, struct vc4_hdmi_encoder, base.base); +} + +/* VC4 HDMI connector KMS struct */ +struct vc4_hdmi_connector { + struct drm_connector base; + + /* Since the connector is attached to just the one encoder, + * this is the reference to it so we can do the best_encoder() + * hook. + */ + struct drm_encoder *encoder; +}; + +static inline struct vc4_hdmi_connector * +to_vc4_hdmi_connector(struct drm_connector *connector) +{ + return container_of(connector, struct vc4_hdmi_connector, base); +} + /* HDMI audio information */ struct vc4_hdmi_audio { struct snd_soc_card card; @@ -25,8 +55,8 @@ struct vc4_hdmi_audio { struct vc4_hdmi { struct platform_device *pdev; - struct drm_encoder *encoder; - struct drm_connector *connector; + struct vc4_hdmi_encoder encoder; + struct vc4_hdmi_connector connector; struct vc4_hdmi_audio audio; @@ -53,34 +83,4 @@ struct vc4_hdmi { #define HD_READ(offset) readl(vc4->hdmi->hd_regs + offset) #define HD_WRITE(offset, val) writel(val, vc4->hdmi->hd_regs + offset) -/* VC4 HDMI encoder KMS struct */ -struct vc4_hdmi_encoder { - struct vc4_encoder base; - bool hdmi_monitor; - bool limited_rgb_range; -}; - -static inline struct vc4_hdmi_encoder * -to_vc4_hdmi_encoder(struct drm_encoder *encoder) -{ - return container_of(encoder, struct vc4_hdmi_encoder, base.base); -} - -/* VC4 HDMI connector KMS struct */ -struct vc4_hdmi_connector { - struct drm_connector base; - - /* Since the connector is attached to just the one encoder, - * this is the reference to it so we can do the best_encoder() - * hook. - */ - struct drm_encoder *encoder; -}; - -static inline struct vc4_hdmi_connector * -to_vc4_hdmi_connector(struct drm_connector *connector) -{ - return container_of(connector, struct vc4_hdmi_connector, base); -} - #endif /* _VC4_HDMI_H_ */
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@cerno.tech> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 87 ++++++++++++++++------------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 64 +++++++++++++------------- 2 files changed, 72 insertions(+), 79 deletions(-)