| Message ID | 9f921701-910c-d749-378c-038e8405f656@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 11/08/17 13:57, Tomi Valkeinen wrote: >> >>> I'm doing some testing with this series on my panda. One issue I see is >>> that when I unload the display modules, I get: >>> >>> [ 75.180206] platform 58006000.encoder: enabled after unload, idling >>> [ 75.187896] platform 58001000.dispc: enabled after unload, idling >>> [ 75.198242] platform 58000000.dss: enabled after unload, idling >> >> This one is caused by hdmi_cec_adap_enable() never getting called with >> enable=false when unloading the modules. Should that be called >> explicitly in hdmi4_cec_uninit, or is the CEC framework supposed to call it? > > Nicely found! > > The cec_delete_adapter() function calls __cec_s_phys_addr(CEC_PHYS_ADDR_INVALID) > which would normally call adap_enable(false), except when the device node was > already unregistered, in which case it just returns immediately. > > The patch below should fix this. Let me know if it works, and I'll post a proper > patch and get that in for 4.14 (and possible backported as well, I'll have to > look at that). Thanks, this fixes the issue. I again saw "HDMICORE: omapdss HDMICORE error: operation stopped when reading edid" when I loaded the modules. My panda also froze just now when unloading the display modules, and it doesn't react to sysrq. After testing a bit without the CEC patches, I saw the above error, so I don't think it's related to your patches. I can't test with a TV, so no CEC for me... But otherwise I think the series works ok now, and looks ok. So I'll apply, but it's a bit late for the next merge window, so I'll aim for 4.15 with this. Tomi
Hi Tomi, On 08/22/2017 11:44 AM, Tomi Valkeinen wrote: > Hi Hans, > >>> > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > On 11/08/17 13:57, Tomi Valkeinen wrote: >>> >>>> I'm doing some testing with this series on my panda. One issue I see is >>>> that when I unload the display modules, I get: >>>> >>>> [ 75.180206] platform 58006000.encoder: enabled after unload, idling >>>> [ 75.187896] platform 58001000.dispc: enabled after unload, idling >>>> [ 75.198242] platform 58000000.dss: enabled after unload, idling >>> >>> This one is caused by hdmi_cec_adap_enable() never getting called with >>> enable=false when unloading the modules. Should that be called >>> explicitly in hdmi4_cec_uninit, or is the CEC framework supposed to call it? >> >> Nicely found! >> >> The cec_delete_adapter() function calls __cec_s_phys_addr(CEC_PHYS_ADDR_INVALID) >> which would normally call adap_enable(false), except when the device node was >> already unregistered, in which case it just returns immediately. >> >> The patch below should fix this. Let me know if it works, and I'll post a proper >> patch and get that in for 4.14 (and possible backported as well, I'll have to >> look at that). > > Thanks, this fixes the issue. > > I again saw "HDMICORE: omapdss HDMICORE error: operation stopped when > reading edid" when I loaded the modules. My panda also froze just now > when unloading the display modules, and it doesn't react to sysrq. > > After testing a bit without the CEC patches, I saw the above error, so I > don't think it's related to your patches. > > I can't test with a TV, so no CEC for me... But otherwise I think the > series works ok now, and looks ok. So I'll apply, but it's a bit late > for the next merge window, so I'll aim for 4.15 with this. What is the status? Do you need anything from me? I'd like to get this in for 4.15. Regards, Hans
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 12/10/17 09:50, Hans Verkuil wrote: >> I can't test with a TV, so no CEC for me... But otherwise I think the >> series works ok now, and looks ok. So I'll apply, but it's a bit late >> for the next merge window, so I'll aim for 4.15 with this. > > What is the status? Do you need anything from me? I'd like to get this in for 4.15. Thanks for reminding. I think I would've forgotten... I sent the pull request, so all should be fine. If possible, please test the pull request, preferably with drm-next merged (git://people.freedesktop.org/~airlied/linux drm-next), as I don't have a CEC capable display. Tomi
On 10/12/17 10:03, Tomi Valkeinen wrote: > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > On 12/10/17 09:50, Hans Verkuil wrote: > >>> I can't test with a TV, so no CEC for me... But otherwise I think the >>> series works ok now, and looks ok. So I'll apply, but it's a bit late >>> for the next merge window, so I'll aim for 4.15 with this. >> >> What is the status? Do you need anything from me? I'd like to get this in for 4.15. > > Thanks for reminding. I think I would've forgotten... > > I sent the pull request, so all should be fine. > > If possible, please test the pull request, preferably with drm-next > merged (git://people.freedesktop.org/~airlied/linux drm-next), as I > don't have a CEC capable display. I'll try to do that tomorrow or Monday. I have one other question for you: does keeping ls_oe_gpio high all the time affect this old bug fix: https://github.com/myfluxi/xxICSKernel/commit/21189f03d3ec3a74d9949907c828410d7a9a86d5 I don't think so, but I'm not 100% sure. As far as I can see the PHY power sequence (OFF, TXON, LDOON) does not change and that seems to be the crucial fix according to the commit above. I would hate being responsible for lots of burnt-out pandaboards :-) There is an alternative solution: when there is no HPD then it is also possible to pull the ls_oe_gpio high only when transmitting a CEC message. That would obviously require some code changes. Sorry for raising this issue so late, but this just came up today in internal discussions. Regards, Hans
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 12/10/17 12:42, Hans Verkuil wrote: > On 10/12/17 10:03, Tomi Valkeinen wrote: >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >> >> On 12/10/17 09:50, Hans Verkuil wrote: >> >>>> I can't test with a TV, so no CEC for me... But otherwise I think the >>>> series works ok now, and looks ok. So I'll apply, but it's a bit late >>>> for the next merge window, so I'll aim for 4.15 with this. >>> >>> What is the status? Do you need anything from me? I'd like to get this in for 4.15. >> >> Thanks for reminding. I think I would've forgotten... >> >> I sent the pull request, so all should be fine. >> >> If possible, please test the pull request, preferably with drm-next >> merged (git://people.freedesktop.org/~airlied/linux drm-next), as I >> don't have a CEC capable display. > > I'll try to do that tomorrow or Monday. > > I have one other question for you: does keeping ls_oe_gpio high all the > time affect this old bug fix: > > https://github.com/myfluxi/xxICSKernel/commit/21189f03d3ec3a74d9949907c828410d7a9a86d5 No, the issue is about the HDMI PHY. The i2c lines are not related. LS_OE only affects the i2c. > I don't think so, but I'm not 100% sure. As far as I can see the PHY power > sequence (OFF, TXON, LDOON) does not change and that seems to be the > crucial fix according to the commit above. > > I would hate being responsible for lots of burnt-out pandaboards :-) > > There is an alternative solution: when there is no HPD then it is also > possible to pull the ls_oe_gpio high only when transmitting a CEC message. > > That would obviously require some code changes. > > Sorry for raising this issue so late, but this just came up today in > internal discussions. Well, it would be nice to not have LS_OE enabled all the time. But I'm not sure how much that really matters. I'm sure it uses some power, but is that even measurable, I have no idea. Tomi
On 10/12/2017 10:03 AM, Tomi Valkeinen wrote: > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > On 12/10/17 09:50, Hans Verkuil wrote: > >>> I can't test with a TV, so no CEC for me... But otherwise I think the >>> series works ok now, and looks ok. So I'll apply, but it's a bit late >>> for the next merge window, so I'll aim for 4.15 with this. >> >> What is the status? Do you need anything from me? I'd like to get this in for 4.15. > > Thanks for reminding. I think I would've forgotten... > > I sent the pull request, so all should be fine. > > If possible, please test the pull request, preferably with drm-next > merged (git://people.freedesktop.org/~airlied/linux drm-next), as I > don't have a CEC capable display. Tested with drm-next. Works fine! Great to finally see this merged! Regards, Hans
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c index bf45977b2823..61dffe165565 100644 --- a/drivers/media/cec/cec-adap.c +++ b/drivers/media/cec/cec-adap.c @@ -1390,7 +1390,9 @@ static void cec_claim_log_addrs(struct cec_adapter *adap, bool block) */ void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block) { - if (phys_addr == adap->phys_addr || adap->devnode.unregistered) + if (phys_addr == adap->phys_addr) + return; + if (phys_addr != CEC_PHYS_ADDR_INVALID && adap->devnode.unregistered) return; dprintk(1, "new physical address %x.%x.%x.%x\n",