| Message ID | 20240715-x1e80100-crd-backlight-v2-2-31b7f2f658a3@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: atna33xc20: Fix the Samsung ATNA45AF01 panel | expand |
On Mon, Jul 15, 2024 at 02:15:38PM +0200, Stephan Gerhold wrote: > This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > The panel should be handled through the samsung-atna33xc20 driver for > correct power up timings. Otherwise the backlight does not work correctly. > > We have existing users of this panel through the generic "edp-panel" > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > partially in that configuration: It works after boot but once the screen > gets disabled it does not turn on again until after reboot. It behaves the > same way with the default "conservative" timings, so we might as well drop > the configuration from the panel-edp driver. That way, users with old DTBs > will get a warning and can move to the new driver. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Tested-by: Johan Hovold <johan+linaro@kernel.org>
Hi, On 15/07/2024 14:15, Stephan Gerhold wrote: > This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > The panel should be handled through the samsung-atna33xc20 driver for > correct power up timings. Otherwise the backlight does not work correctly. > > We have existing users of this panel through the generic "edp-panel" > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > partially in that configuration: It works after boot but once the screen > gets disabled it does not turn on again until after reboot. It behaves the > same way with the default "conservative" timings, so we might as well drop > the configuration from the panel-edp driver. That way, users with old DTBs > will get a warning and can move to the new driver. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > --- > drivers/gpu/drm/panel/panel-edp.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > index 3a574a9b46e7..d2d682385e89 100644 > --- a/drivers/gpu/drm/panel/panel-edp.c > +++ b/drivers/gpu/drm/panel/panel-edp.c > @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > > - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > - > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > How will we handle current/old crd DT with new kernels ? Same question for patch 3, thie serie introduces a bindings that won't be valid if we backport patch 3. I don't think patch should be backported, and this patch should be dropped. Neil
On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > On 15/07/2024 14:15, Stephan Gerhold wrote: > > This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > > > The panel should be handled through the samsung-atna33xc20 driver for > > correct power up timings. Otherwise the backlight does not work correctly. > > > > We have existing users of this panel through the generic "edp-panel" > > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > partially in that configuration: It works after boot but once the screen > > gets disabled it does not turn on again until after reboot. It behaves the > > same way with the default "conservative" timings, so we might as well drop > > the configuration from the panel-edp driver. That way, users with old DTBs > > will get a warning and can move to the new driver. > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > --- > > drivers/gpu/drm/panel/panel-edp.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > > index 3a574a9b46e7..d2d682385e89 100644 > > --- a/drivers/gpu/drm/panel/panel-edp.c > > +++ b/drivers/gpu/drm/panel/panel-edp.c > > @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > > - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > > - > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > > > > How will we handle current/old crd DT with new kernels ? > I think this is answered in the commit message: > > We have existing users of this panel through the generic "edp-panel" > > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > partially in that configuration: It works after boot but once the screen > > gets disabled it does not turn on again until after reboot. It behaves the > > same way with the default "conservative" timings, so we might as well drop > > the configuration from the panel-edp driver. That way, users with old DTBs > > will get a warning and can move to the new driver. Basically with the entry removed, the panel-edp driver will fallback to default "conservative" timings when using old DTBs. There will be a warning in dmesg, but otherwise the panel will somewhat work just as before. I think this is a good way to remind users to upgrade. > Same question for patch 3, thie serie introduces a bindings that won't be valid > if we backport patch 3. I don't think patch should be backported, and this patch > should be dropped. There would be a dtbs_check warning, yeah. Functionally, it would work just fine. Is that reason enough to keep display partially broken for 6.11? We could also apply the minor binding change for 6.11 if needed. I'm also fine if this just goes into 6.12 though. Thanks, Stephan
On 15/07/2024 14:54, Stephan Gerhold wrote: > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: >> On 15/07/2024 14:15, Stephan Gerhold wrote: >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. >>> >>> The panel should be handled through the samsung-atna33xc20 driver for >>> correct power up timings. Otherwise the backlight does not work correctly. >>> >>> We have existing users of this panel through the generic "edp-panel" >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>> partially in that configuration: It works after boot but once the screen >>> gets disabled it does not turn on again until after reboot. It behaves the >>> same way with the default "conservative" timings, so we might as well drop >>> the configuration from the panel-edp driver. That way, users with old DTBs >>> will get a warning and can move to the new driver. >>> >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>> --- >>> drivers/gpu/drm/panel/panel-edp.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c >>> index 3a574a9b46e7..d2d682385e89 100644 >>> --- a/drivers/gpu/drm/panel/panel-edp.c >>> +++ b/drivers/gpu/drm/panel/panel-edp.c >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), >>> - >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), >>> >> >> How will we handle current/old crd DT with new kernels ? >> > > I think this is answered in the commit message: > >>> We have existing users of this panel through the generic "edp-panel" >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>> partially in that configuration: It works after boot but once the screen >>> gets disabled it does not turn on again until after reboot. It behaves the >>> same way with the default "conservative" timings, so we might as well drop >>> the configuration from the panel-edp driver. That way, users with old DTBs >>> will get a warning and can move to the new driver. > > Basically with the entry removed, the panel-edp driver will fallback to > default "conservative" timings when using old DTBs. There will be a > warning in dmesg, but otherwise the panel will somewhat work just as > before. I think this is a good way to remind users to upgrade. I consider this as a regression > >> Same question for patch 3, thie serie introduces a bindings that won't be valid >> if we backport patch 3. I don't think patch should be backported, and this patch >> should be dropped. > > There would be a dtbs_check warning, yeah. Functionally, it would work > just fine. Is that reason enough to keep display partially broken for > 6.11? We could also apply the minor binding change for 6.11 if needed. I don't know how to answer this, I'll let the DT maintainer comment this. The problem is I do not think we can pass the whole patchset as fixes for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. Neil > > I'm also fine if this just goes into 6.12 though. > > Thanks, > Stephan
On Mon, Jul 15, 2024 at 02:54:59PM +0200, Stephan Gerhold wrote: > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > > How will we handle current/old crd DT with new kernels ? > > I think this is answered in the commit message: > > > > We have existing users of this panel through the generic "edp-panel" > > > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > > partially in that configuration: It works after boot but once the screen > > > gets disabled it does not turn on again until after reboot. It behaves the > > > same way with the default "conservative" timings, so we might as well drop > > > the configuration from the panel-edp driver. That way, users with old DTBs > > > will get a warning and can move to the new driver. > > Basically with the entry removed, the panel-edp driver will fallback to > default "conservative" timings when using old DTBs. There will be a > warning in dmesg, but otherwise the panel will somewhat work just as > before. I think this is a good way to remind users to upgrade. > > > Same question for patch 3, thie serie introduces a bindings that won't be valid > > if we backport patch 3. I don't think patch should be backported, and this patch > > should be dropped. > > There would be a dtbs_check warning, yeah. Functionally, it would work > just fine. Is that reason enough to keep display partially broken for > 6.11? We could also apply the minor binding change for 6.11 if needed. > > I'm also fine if this just goes into 6.12 though. No, we should definitely fix this for 6.11. This machine is not very useable without it. Whether to backport is a separate question, but note that patch 3 is not even marked for backport currently. Fixing the backlight at the cost of a dtb checker warning should not be an issue, but backporting would break existing setups unless people have the new panel driver enabled and this may be a valid concern. On the other hand, support for this platform is in a bit of flux already and it looks like most fixes aren't even tagged for stable (presumably for that reason). Johan
On Mon, Jul 15, 2024 at 03:01:57PM +0200, Neil Armstrong wrote: > On 15/07/2024 14:54, Stephan Gerhold wrote: > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > > > On 15/07/2024 14:15, Stephan Gerhold wrote: > > > > This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > > > > > > > The panel should be handled through the samsung-atna33xc20 driver for > > > > correct power up timings. Otherwise the backlight does not work correctly. > > > > > > > > We have existing users of this panel through the generic "edp-panel" > > > > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > > > partially in that configuration: It works after boot but once the screen > > > > gets disabled it does not turn on again until after reboot. It behaves the > > > > same way with the default "conservative" timings, so we might as well drop > > > > the configuration from the panel-edp driver. That way, users with old DTBs > > > > will get a warning and can move to the new driver. > > > > > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > > > --- > > > > drivers/gpu/drm/panel/panel-edp.c | 2 -- > > > > 1 file changed, 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > > > > index 3a574a9b46e7..d2d682385e89 100644 > > > > --- a/drivers/gpu/drm/panel/panel-edp.c > > > > +++ b/drivers/gpu/drm/panel/panel-edp.c > > > > @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > > > > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > > > > EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > > > > - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > > > > - > > > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > > > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > > > > EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > > > > > > > > > > How will we handle current/old crd DT with new kernels ? > > > > > > > I think this is answered in the commit message: > > > > > > We have existing users of this panel through the generic "edp-panel" > > > > compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > > > partially in that configuration: It works after boot but once the screen > > > > gets disabled it does not turn on again until after reboot. It behaves the > > > > same way with the default "conservative" timings, so we might as well drop > > > > the configuration from the panel-edp driver. That way, users with old DTBs > > > > will get a warning and can move to the new driver. > > > > Basically with the entry removed, the panel-edp driver will fallback to > > default "conservative" timings when using old DTBs. There will be a > > warning in dmesg, but otherwise the panel will somewhat work just as > > before. I think this is a good way to remind users to upgrade. > > I consider this as a regression > Personally, I don't think we can regress something that was already broken. There is no point in continuing to use the broken state - it is rather frustrating if your display goes off for power saving or suspend and you cannot get it back on until you reboot. > > > > > Same question for patch 3, thie serie introduces a bindings that won't be valid > > > if we backport patch 3. I don't think patch should be backported, and this patch > > > should be dropped. > > > > There would be a dtbs_check warning, yeah. Functionally, it would work > > just fine. Is that reason enough to keep display partially broken for > > 6.11? We could also apply the minor binding change for 6.11 if needed. > > I don't know how to answer this, I'll let the DT maintainer comment this. > > The problem is I do not think we can pass the whole patchset as fixes > for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > Fair enough, I'm also fine if these patches go just into 6.12. I think there are no changes in the patches needed for that, the Fixes tag is still appropriate and I intentionally omitted the Cc stable tag. Thanks, Stephan
Hi, On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 15/07/2024 14:54, Stephan Gerhold wrote: > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > >> On 15/07/2024 14:15, Stephan Gerhold wrote: > >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > >>> > >>> The panel should be handled through the samsung-atna33xc20 driver for > >>> correct power up timings. Otherwise the backlight does not work correctly. > >>> > >>> We have existing users of this panel through the generic "edp-panel" > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>> partially in that configuration: It works after boot but once the screen > >>> gets disabled it does not turn on again until after reboot. It behaves the > >>> same way with the default "conservative" timings, so we might as well drop > >>> the configuration from the panel-edp driver. That way, users with old DTBs > >>> will get a warning and can move to the new driver. > >>> > >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > >>> --- > >>> drivers/gpu/drm/panel/panel-edp.c | 2 -- > >>> 1 file changed, 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > >>> index 3a574a9b46e7..d2d682385e89 100644 > >>> --- a/drivers/gpu/drm/panel/panel-edp.c > >>> +++ b/drivers/gpu/drm/panel/panel-edp.c > >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > >>> - > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > >>> > >> > >> How will we handle current/old crd DT with new kernels ? > >> > > > > I think this is answered in the commit message: > > > >>> We have existing users of this panel through the generic "edp-panel" > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>> partially in that configuration: It works after boot but once the screen > >>> gets disabled it does not turn on again until after reboot. It behaves the > >>> same way with the default "conservative" timings, so we might as well drop > >>> the configuration from the panel-edp driver. That way, users with old DTBs > >>> will get a warning and can move to the new driver. > > > > Basically with the entry removed, the panel-edp driver will fallback to > > default "conservative" timings when using old DTBs. There will be a > > warning in dmesg, but otherwise the panel will somewhat work just as > > before. I think this is a good way to remind users to upgrade. > > I consider this as a regression > > > > >> Same question for patch 3, thie serie introduces a bindings that won't be valid > >> if we backport patch 3. I don't think patch should be backported, and this patch > >> should be dropped. > > > > There would be a dtbs_check warning, yeah. Functionally, it would work > > just fine. Is that reason enough to keep display partially broken for > > 6.11? We could also apply the minor binding change for 6.11 if needed. > > I don't know how to answer this, I'll let the DT maintainer comment this. > > The problem is I do not think we can pass the whole patchset as fixes > for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > > Neil IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree whenever those folks agree to it. If we're worried about the dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go through the Qualcomm tree as long as it made it into 6.11-rc1. I have a hunch that there are going to be more Samsung OLED panels in the future that will need to touch the same file, but if the change is in -rc1 it should make it back into drm-misc quickly, right? Personally I think patch #2 could go in anytime since, as people have said, things are pretty broken today and the worst that happens is that someone gets an extra warning. That would be my preference. That being said, we could also snooze that patch for a month or two and land it later. There's no real hurry. -Doug
On 15/07/2024 15:51, Doug Anderson wrote: > Hi, > > On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong > <neil.armstrong@linaro.org> wrote: >> >> On 15/07/2024 14:54, Stephan Gerhold wrote: >>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: >>>> On 15/07/2024 14:15, Stephan Gerhold wrote: >>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. >>>>> >>>>> The panel should be handled through the samsung-atna33xc20 driver for >>>>> correct power up timings. Otherwise the backlight does not work correctly. >>>>> >>>>> We have existing users of this panel through the generic "edp-panel" >>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>> partially in that configuration: It works after boot but once the screen >>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>> same way with the default "conservative" timings, so we might as well drop >>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>> will get a warning and can move to the new driver. >>>>> >>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/panel/panel-edp.c | 2 -- >>>>> 1 file changed, 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c >>>>> index 3a574a9b46e7..d2d682385e89 100644 >>>>> --- a/drivers/gpu/drm/panel/panel-edp.c >>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c >>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { >>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), >>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), >>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), >>>>> - >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), >>>>> >>>> >>>> How will we handle current/old crd DT with new kernels ? >>>> >>> >>> I think this is answered in the commit message: >>> >>>>> We have existing users of this panel through the generic "edp-panel" >>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>> partially in that configuration: It works after boot but once the screen >>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>> same way with the default "conservative" timings, so we might as well drop >>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>> will get a warning and can move to the new driver. >>> >>> Basically with the entry removed, the panel-edp driver will fallback to >>> default "conservative" timings when using old DTBs. There will be a >>> warning in dmesg, but otherwise the panel will somewhat work just as >>> before. I think this is a good way to remind users to upgrade. >> >> I consider this as a regression >> >>> >>>> Same question for patch 3, thie serie introduces a bindings that won't be valid >>>> if we backport patch 3. I don't think patch should be backported, and this patch >>>> should be dropped. >>> >>> There would be a dtbs_check warning, yeah. Functionally, it would work >>> just fine. Is that reason enough to keep display partially broken for >>> 6.11? We could also apply the minor binding change for 6.11 if needed. >> >> I don't know how to answer this, I'll let the DT maintainer comment this. >> >> The problem is I do not think we can pass the whole patchset as fixes >> for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. >> >> Neil > > IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree > whenever those folks agree to it. If we're worried about the > dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go > through the Qualcomm tree as long as it made it into 6.11-rc1. I have > a hunch that there are going to be more Samsung OLED panels in the > future that will need to touch the same file, but if the change is in > -rc1 it should make it back into drm-misc quickly, right? Not sure the Soc maintainer would accept any patches for -rc1 at this point, but Bjorn can try to push both #3 and #4 as fixes for -rc2. Not sure #1 would be accepted as fix via any tree, but having a bindings error for a kernel release is not a big deal if in-fine the bindings change has been reviewed and queued for next version. Basically when the -rc is tagged it gets backmerged into drm-misc-next, so we can backmerge any -rc we want. > > Personally I think patch #2 could go in anytime since, as people have > said, things are pretty broken today and the worst that happens is > that someone gets an extra warning. That would be my preference. That > being said, we could also snooze that patch for a month or two and > land it later. There's no real hurry. We can push it now to drm-misc-next so it gets naturally delayed until v6.12 Neil > > -Doug >
Hi, On Mon, Jul 15, 2024 at 6:57 AM <neil.armstrong@linaro.org> wrote: > > On 15/07/2024 15:51, Doug Anderson wrote: > > Hi, > > > > On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong > > <neil.armstrong@linaro.org> wrote: > >> > >> On 15/07/2024 14:54, Stephan Gerhold wrote: > >>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > >>>> On 15/07/2024 14:15, Stephan Gerhold wrote: > >>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > >>>>> > >>>>> The panel should be handled through the samsung-atna33xc20 driver for > >>>>> correct power up timings. Otherwise the backlight does not work correctly. > >>>>> > >>>>> We have existing users of this panel through the generic "edp-panel" > >>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>>>> partially in that configuration: It works after boot but once the screen > >>>>> gets disabled it does not turn on again until after reboot. It behaves the > >>>>> same way with the default "conservative" timings, so we might as well drop > >>>>> the configuration from the panel-edp driver. That way, users with old DTBs > >>>>> will get a warning and can move to the new driver. > >>>>> > >>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > >>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > >>>>> --- > >>>>> drivers/gpu/drm/panel/panel-edp.c | 2 -- > >>>>> 1 file changed, 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > >>>>> index 3a574a9b46e7..d2d682385e89 100644 > >>>>> --- a/drivers/gpu/drm/panel/panel-edp.c > >>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c > >>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > >>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > >>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > >>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > >>>>> - > >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > >>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > >>>>> > >>>> > >>>> How will we handle current/old crd DT with new kernels ? > >>>> > >>> > >>> I think this is answered in the commit message: > >>> > >>>>> We have existing users of this panel through the generic "edp-panel" > >>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>>>> partially in that configuration: It works after boot but once the screen > >>>>> gets disabled it does not turn on again until after reboot. It behaves the > >>>>> same way with the default "conservative" timings, so we might as well drop > >>>>> the configuration from the panel-edp driver. That way, users with old DTBs > >>>>> will get a warning and can move to the new driver. > >>> > >>> Basically with the entry removed, the panel-edp driver will fallback to > >>> default "conservative" timings when using old DTBs. There will be a > >>> warning in dmesg, but otherwise the panel will somewhat work just as > >>> before. I think this is a good way to remind users to upgrade. > >> > >> I consider this as a regression > >> > >>> > >>>> Same question for patch 3, thie serie introduces a bindings that won't be valid > >>>> if we backport patch 3. I don't think patch should be backported, and this patch > >>>> should be dropped. > >>> > >>> There would be a dtbs_check warning, yeah. Functionally, it would work > >>> just fine. Is that reason enough to keep display partially broken for > >>> 6.11? We could also apply the minor binding change for 6.11 if needed. > >> > >> I don't know how to answer this, I'll let the DT maintainer comment this. > >> > >> The problem is I do not think we can pass the whole patchset as fixes > >> for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > >> > >> Neil > > > > IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree > > whenever those folks agree to it. If we're worried about the > > dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go > > through the Qualcomm tree as long as it made it into 6.11-rc1. I have > > a hunch that there are going to be more Samsung OLED panels in the > > future that will need to touch the same file, but if the change is in > > -rc1 it should make it back into drm-misc quickly, right? > > Not sure the Soc maintainer would accept any patches for -rc1 at this > point, but Bjorn can try to push both #3 and #4 as fixes for -rc2. Yeah, I guess it's pretty late for -rc1. > Not sure #1 would be accepted as fix via any tree, but having a bindings > error for a kernel release is not a big deal if in-fine the bindings change > has been reviewed and queued for next version. In general my understanding is that we don't worry too much about temporary bindings errors as long as things meet up in linuxnext and get fixed. ...but in this case we're talking about the dts going into 6.11 and the bindings in 6.12 which means that v6.11 will be released and still have the bindings error. That's non-ideal... Can we really not consider the bindings as "Fix" since it's required to pass dts validation for the dts patch which is a "Fix"? If we can consider this bindings change a Fix then we could land it in drm-misc-fixes and then things could meet up nicely I think, right? -Doug
On 15/07/2024 16:40, Doug Anderson wrote: > Hi, > > On Mon, Jul 15, 2024 at 6:57 AM <neil.armstrong@linaro.org> wrote: >> >> On 15/07/2024 15:51, Doug Anderson wrote: >>> Hi, >>> >>> On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong >>> <neil.armstrong@linaro.org> wrote: >>>> >>>> On 15/07/2024 14:54, Stephan Gerhold wrote: >>>>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: >>>>>> On 15/07/2024 14:15, Stephan Gerhold wrote: >>>>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. >>>>>>> >>>>>>> The panel should be handled through the samsung-atna33xc20 driver for >>>>>>> correct power up timings. Otherwise the backlight does not work correctly. >>>>>>> >>>>>>> We have existing users of this panel through the generic "edp-panel" >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>>>> partially in that configuration: It works after boot but once the screen >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>>>> same way with the default "conservative" timings, so we might as well drop >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>>>> will get a warning and can move to the new driver. >>>>>>> >>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>>>>>> --- >>>>>>> drivers/gpu/drm/panel/panel-edp.c | 2 -- >>>>>>> 1 file changed, 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c >>>>>>> index 3a574a9b46e7..d2d682385e89 100644 >>>>>>> --- a/drivers/gpu/drm/panel/panel-edp.c >>>>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c >>>>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), >>>>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), >>>>>>> - >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), >>>>>>> >>>>>> >>>>>> How will we handle current/old crd DT with new kernels ? >>>>>> >>>>> >>>>> I think this is answered in the commit message: >>>>> >>>>>>> We have existing users of this panel through the generic "edp-panel" >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>>>> partially in that configuration: It works after boot but once the screen >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>>>> same way with the default "conservative" timings, so we might as well drop >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>>>> will get a warning and can move to the new driver. >>>>> >>>>> Basically with the entry removed, the panel-edp driver will fallback to >>>>> default "conservative" timings when using old DTBs. There will be a >>>>> warning in dmesg, but otherwise the panel will somewhat work just as >>>>> before. I think this is a good way to remind users to upgrade. >>>> >>>> I consider this as a regression >>>> >>>>> >>>>>> Same question for patch 3, thie serie introduces a bindings that won't be valid >>>>>> if we backport patch 3. I don't think patch should be backported, and this patch >>>>>> should be dropped. >>>>> >>>>> There would be a dtbs_check warning, yeah. Functionally, it would work >>>>> just fine. Is that reason enough to keep display partially broken for >>>>> 6.11? We could also apply the minor binding change for 6.11 if needed. >>>> >>>> I don't know how to answer this, I'll let the DT maintainer comment this. >>>> >>>> The problem is I do not think we can pass the whole patchset as fixes >>>> for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. >>>> >>>> Neil >>> >>> IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree >>> whenever those folks agree to it. If we're worried about the >>> dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go >>> through the Qualcomm tree as long as it made it into 6.11-rc1. I have >>> a hunch that there are going to be more Samsung OLED panels in the >>> future that will need to touch the same file, but if the change is in >>> -rc1 it should make it back into drm-misc quickly, right? >> >> Not sure the Soc maintainer would accept any patches for -rc1 at this >> point, but Bjorn can try to push both #3 and #4 as fixes for -rc2. > > Yeah, I guess it's pretty late for -rc1. > > >> Not sure #1 would be accepted as fix via any tree, but having a bindings >> error for a kernel release is not a big deal if in-fine the bindings change >> has been reviewed and queued for next version. > > In general my understanding is that we don't worry too much about > temporary bindings errors as long as things meet up in linuxnext and > get fixed. ...but in this case we're talking about the dts going into > 6.11 and the bindings in 6.12 which means that v6.11 will be released > and still have the bindings error. That's non-ideal... > > Can we really not consider the bindings as "Fix" since it's required > to pass dts validation for the dts patch which is a "Fix"? If we can > consider this bindings change a Fix then we could land it in > drm-misc-fixes and then things could meet up nicely I think, right? Probably, we'll need insight from Krzysztof/Conor/Rob on this point, and perhaps Maxime aswell. Neil > > -Doug >
Hi, On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong > <neil.armstrong@linaro.org> wrote: > > > > On 15/07/2024 14:54, Stephan Gerhold wrote: > > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > > >> On 15/07/2024 14:15, Stephan Gerhold wrote: > > >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > >>> > > >>> The panel should be handled through the samsung-atna33xc20 driver for > > >>> correct power up timings. Otherwise the backlight does not work correctly. > > >>> > > >>> We have existing users of this panel through the generic "edp-panel" > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > >>> partially in that configuration: It works after boot but once the screen > > >>> gets disabled it does not turn on again until after reboot. It behaves the > > >>> same way with the default "conservative" timings, so we might as well drop > > >>> the configuration from the panel-edp driver. That way, users with old DTBs > > >>> will get a warning and can move to the new driver. > > >>> > > >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > >>> --- > > >>> drivers/gpu/drm/panel/panel-edp.c | 2 -- > > >>> 1 file changed, 2 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > > >>> index 3a574a9b46e7..d2d682385e89 100644 > > >>> --- a/drivers/gpu/drm/panel/panel-edp.c > > >>> +++ b/drivers/gpu/drm/panel/panel-edp.c > > >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > > >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > > >>> - > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > > >>> > > >> > > >> How will we handle current/old crd DT with new kernels ? > > >> > > > > > > I think this is answered in the commit message: > > > > > >>> We have existing users of this panel through the generic "edp-panel" > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > >>> partially in that configuration: It works after boot but once the screen > > >>> gets disabled it does not turn on again until after reboot. It behaves the > > >>> same way with the default "conservative" timings, so we might as well drop > > >>> the configuration from the panel-edp driver. That way, users with old DTBs > > >>> will get a warning and can move to the new driver. > > > > > > Basically with the entry removed, the panel-edp driver will fallback to > > > default "conservative" timings when using old DTBs. There will be a > > > warning in dmesg, but otherwise the panel will somewhat work just as > > > before. I think this is a good way to remind users to upgrade. > > > > I consider this as a regression > > > > > > > >> Same question for patch 3, thie serie introduces a bindings that won't be valid > > >> if we backport patch 3. I don't think patch should be backported, and this patch > > >> should be dropped. > > > > > > There would be a dtbs_check warning, yeah. Functionally, it would work > > > just fine. Is that reason enough to keep display partially broken for > > > 6.11? We could also apply the minor binding change for 6.11 if needed. > > > > I don't know how to answer this, I'll let the DT maintainer comment this. > > > > The problem is I do not think we can pass the whole patchset as fixes > > for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > > > > Neil > > IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree > whenever those folks agree to it. If we're worried about the > dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go > through the Qualcomm tree as long as it made it into 6.11-rc1. I have > a hunch that there are going to be more Samsung OLED panels in the > future that will need to touch the same file, but if the change is in > -rc1 it should make it back into drm-misc quickly, right? > > Personally I think patch #2 could go in anytime since, as people have > said, things are pretty broken today and the worst that happens is > that someone gets an extra warning. That would be my preference. That > being said, we could also snooze that patch for a month or two and > land it later. There's no real hurry. For now I'm going to snooze this patch for a month just to avoid any controversy. I'll plan to apply it (to drm-misc-next) when I see the device tree patch land. Since the device tree patch should land as a fix that should keep things landing in the correct order. ...and, as per above, the worst case is that if someone has an old DTS and a new kernel then a panel that was already not working well will print a fat warning and startup a bit slower. If somehow I mess up and forget about this patch, feel free to send me a poke when the device tree patch is landed. -Doug
Hi, On Mon, Jul 22, 2024 at 8:49 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong > > <neil.armstrong@linaro.org> wrote: > > > > > > On 15/07/2024 14:54, Stephan Gerhold wrote: > > > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > > > >> On 15/07/2024 14:15, Stephan Gerhold wrote: > > > >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > > > >>> > > > >>> The panel should be handled through the samsung-atna33xc20 driver for > > > >>> correct power up timings. Otherwise the backlight does not work correctly. > > > >>> > > > >>> We have existing users of this panel through the generic "edp-panel" > > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > > >>> partially in that configuration: It works after boot but once the screen > > > >>> gets disabled it does not turn on again until after reboot. It behaves the > > > >>> same way with the default "conservative" timings, so we might as well drop > > > >>> the configuration from the panel-edp driver. That way, users with old DTBs > > > >>> will get a warning and can move to the new driver. > > > >>> > > > >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > >>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > > > >>> --- > > > >>> drivers/gpu/drm/panel/panel-edp.c | 2 -- > > > >>> 1 file changed, 2 deletions(-) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > > > >>> index 3a574a9b46e7..d2d682385e89 100644 > > > >>> --- a/drivers/gpu/drm/panel/panel-edp.c > > > >>> +++ b/drivers/gpu/drm/panel/panel-edp.c > > > >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > > > >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > > > >>> - > > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > > > >>> > > > >> > > > >> How will we handle current/old crd DT with new kernels ? > > > >> > > > > > > > > I think this is answered in the commit message: > > > > > > > >>> We have existing users of this panel through the generic "edp-panel" > > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > > > >>> partially in that configuration: It works after boot but once the screen > > > >>> gets disabled it does not turn on again until after reboot. It behaves the > > > >>> same way with the default "conservative" timings, so we might as well drop > > > >>> the configuration from the panel-edp driver. That way, users with old DTBs > > > >>> will get a warning and can move to the new driver. > > > > > > > > Basically with the entry removed, the panel-edp driver will fallback to > > > > default "conservative" timings when using old DTBs. There will be a > > > > warning in dmesg, but otherwise the panel will somewhat work just as > > > > before. I think this is a good way to remind users to upgrade. > > > > > > I consider this as a regression > > > > > > > > > > >> Same question for patch 3, thie serie introduces a bindings that won't be valid > > > >> if we backport patch 3. I don't think patch should be backported, and this patch > > > >> should be dropped. > > > > > > > > There would be a dtbs_check warning, yeah. Functionally, it would work > > > > just fine. Is that reason enough to keep display partially broken for > > > > 6.11? We could also apply the minor binding change for 6.11 if needed. > > > > > > I don't know how to answer this, I'll let the DT maintainer comment this. > > > > > > The problem is I do not think we can pass the whole patchset as fixes > > > for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > > > > > > Neil > > > > IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree > > whenever those folks agree to it. If we're worried about the > > dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go > > through the Qualcomm tree as long as it made it into 6.11-rc1. I have > > a hunch that there are going to be more Samsung OLED panels in the > > future that will need to touch the same file, but if the change is in > > -rc1 it should make it back into drm-misc quickly, right? > > > > Personally I think patch #2 could go in anytime since, as people have > > said, things are pretty broken today and the worst that happens is > > that someone gets an extra warning. That would be my preference. That > > being said, we could also snooze that patch for a month or two and > > land it later. There's no real hurry. > > For now I'm going to snooze this patch for a month just to avoid any > controversy. I'll plan to apply it (to drm-misc-next) when I see the > device tree patch land. Since the device tree patch should land as a > fix that should keep things landing in the correct order. ...and, as > per above, the worst case is that if someone has an old DTS and a new > kernel then a panel that was already not working well will print a fat > warning and startup a bit slower. > > If somehow I mess up and forget about this patch, feel free to send me > a poke when the device tree patch is landed. More than a month has passed now. One last warning before I apply this revert in a few more days. -Doug
On 27/08/2024 17:36, Doug Anderson wrote: > Hi, > > On Mon, Jul 22, 2024 at 8:49 AM Doug Anderson <dianders@chromium.org> wrote: >> >> Hi, >> >> On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson <dianders@chromium.org> wrote: >>> >>> Hi, >>> >>> On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong >>> <neil.armstrong@linaro.org> wrote: >>>> >>>> On 15/07/2024 14:54, Stephan Gerhold wrote: >>>>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: >>>>>> On 15/07/2024 14:15, Stephan Gerhold wrote: >>>>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. >>>>>>> >>>>>>> The panel should be handled through the samsung-atna33xc20 driver for >>>>>>> correct power up timings. Otherwise the backlight does not work correctly. >>>>>>> >>>>>>> We have existing users of this panel through the generic "edp-panel" >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>>>> partially in that configuration: It works after boot but once the screen >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>>>> same way with the default "conservative" timings, so we might as well drop >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>>>> will get a warning and can move to the new driver. >>>>>>> >>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> >>>>>>> --- >>>>>>> drivers/gpu/drm/panel/panel-edp.c | 2 -- >>>>>>> 1 file changed, 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c >>>>>>> index 3a574a9b46e7..d2d682385e89 100644 >>>>>>> --- a/drivers/gpu/drm/panel/panel-edp.c >>>>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c >>>>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), >>>>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), >>>>>>> - >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), >>>>>>> >>>>>> >>>>>> How will we handle current/old crd DT with new kernels ? >>>>>> >>>>> >>>>> I think this is answered in the commit message: >>>>> >>>>>>> We have existing users of this panel through the generic "edp-panel" >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only >>>>>>> partially in that configuration: It works after boot but once the screen >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the >>>>>>> same way with the default "conservative" timings, so we might as well drop >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs >>>>>>> will get a warning and can move to the new driver. >>>>> >>>>> Basically with the entry removed, the panel-edp driver will fallback to >>>>> default "conservative" timings when using old DTBs. There will be a >>>>> warning in dmesg, but otherwise the panel will somewhat work just as >>>>> before. I think this is a good way to remind users to upgrade. >>>> >>>> I consider this as a regression >>>> >>>>> >>>>>> Same question for patch 3, thie serie introduces a bindings that won't be valid >>>>>> if we backport patch 3. I don't think patch should be backported, and this patch >>>>>> should be dropped. >>>>> >>>>> There would be a dtbs_check warning, yeah. Functionally, it would work >>>>> just fine. Is that reason enough to keep display partially broken for >>>>> 6.11? We could also apply the minor binding change for 6.11 if needed. >>>> >>>> I don't know how to answer this, I'll let the DT maintainer comment this. >>>> >>>> The problem is I do not think we can pass the whole patchset as fixes >>>> for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. >>>> >>>> Neil >>> >>> IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree >>> whenever those folks agree to it. If we're worried about the >>> dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go >>> through the Qualcomm tree as long as it made it into 6.11-rc1. I have >>> a hunch that there are going to be more Samsung OLED panels in the >>> future that will need to touch the same file, but if the change is in >>> -rc1 it should make it back into drm-misc quickly, right? >>> >>> Personally I think patch #2 could go in anytime since, as people have >>> said, things are pretty broken today and the worst that happens is >>> that someone gets an extra warning. That would be my preference. That >>> being said, we could also snooze that patch for a month or two and >>> land it later. There's no real hurry. >> >> For now I'm going to snooze this patch for a month just to avoid any >> controversy. I'll plan to apply it (to drm-misc-next) when I see the >> device tree patch land. Since the device tree patch should land as a >> fix that should keep things landing in the correct order. ...and, as >> per above, the worst case is that if someone has an old DTS and a new >> kernel then a panel that was already not working well will print a fat >> warning and startup a bit slower. >> >> If somehow I mess up and forget about this patch, feel free to send me >> a poke when the device tree patch is landed. > > More than a month has passed now. One last warning before I apply this > revert in a few more days. It's fine if you apply it now Neil > > -Doug >
Hi, On Tue, Aug 27, 2024 at 9:26 AM <neil.armstrong@linaro.org> wrote: > > On 27/08/2024 17:36, Doug Anderson wrote: > > Hi, > > > > On Mon, Jul 22, 2024 at 8:49 AM Doug Anderson <dianders@chromium.org> wrote: > >> > >> Hi, > >> > >> On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson <dianders@chromium.org> wrote: > >>> > >>> Hi, > >>> > >>> On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong > >>> <neil.armstrong@linaro.org> wrote: > >>>> > >>>> On 15/07/2024 14:54, Stephan Gerhold wrote: > >>>>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote: > >>>>>> On 15/07/2024 14:15, Stephan Gerhold wrote: > >>>>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01. > >>>>>>> > >>>>>>> The panel should be handled through the samsung-atna33xc20 driver for > >>>>>>> correct power up timings. Otherwise the backlight does not work correctly. > >>>>>>> > >>>>>>> We have existing users of this panel through the generic "edp-panel" > >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>>>>>> partially in that configuration: It works after boot but once the screen > >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the > >>>>>>> same way with the default "conservative" timings, so we might as well drop > >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs > >>>>>>> will get a warning and can move to the new driver. > >>>>>>> > >>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > >>>>>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> > >>>>>>> --- > >>>>>>> drivers/gpu/drm/panel/panel-edp.c | 2 -- > >>>>>>> 1 file changed, 2 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > >>>>>>> index 3a574a9b46e7..d2d682385e89 100644 > >>>>>>> --- a/drivers/gpu/drm/panel/panel-edp.c > >>>>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c > >>>>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { > >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), > >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), > >>>>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), > >>>>>>> - > >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), > >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), > >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"), > >>>>>>> > >>>>>> > >>>>>> How will we handle current/old crd DT with new kernels ? > >>>>>> > >>>>> > >>>>> I think this is answered in the commit message: > >>>>> > >>>>>>> We have existing users of this panel through the generic "edp-panel" > >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only > >>>>>>> partially in that configuration: It works after boot but once the screen > >>>>>>> gets disabled it does not turn on again until after reboot. It behaves the > >>>>>>> same way with the default "conservative" timings, so we might as well drop > >>>>>>> the configuration from the panel-edp driver. That way, users with old DTBs > >>>>>>> will get a warning and can move to the new driver. > >>>>> > >>>>> Basically with the entry removed, the panel-edp driver will fallback to > >>>>> default "conservative" timings when using old DTBs. There will be a > >>>>> warning in dmesg, but otherwise the panel will somewhat work just as > >>>>> before. I think this is a good way to remind users to upgrade. > >>>> > >>>> I consider this as a regression > >>>> > >>>>> > >>>>>> Same question for patch 3, thie serie introduces a bindings that won't be valid > >>>>>> if we backport patch 3. I don't think patch should be backported, and this patch > >>>>>> should be dropped. > >>>>> > >>>>> There would be a dtbs_check warning, yeah. Functionally, it would work > >>>>> just fine. Is that reason enough to keep display partially broken for > >>>>> 6.11? We could also apply the minor binding change for 6.11 if needed. > >>>> > >>>> I don't know how to answer this, I'll let the DT maintainer comment this. > >>>> > >>>> The problem is I do not think we can pass the whole patchset as fixes > >>>> for v6.11, patches 2 & 3 could, patches 1 & 4 definitely can't. > >>>> > >>>> Neil > >>> > >>> IMO: patch #3 (dts) and #4 (CONFIG) go through the Qualcomm tree > >>> whenever those folks agree to it. If we're worried about the > >>> dtbs_check breakage I personally wouldn't mind "Ack"ing patch #1 to go > >>> through the Qualcomm tree as long as it made it into 6.11-rc1. I have > >>> a hunch that there are going to be more Samsung OLED panels in the > >>> future that will need to touch the same file, but if the change is in > >>> -rc1 it should make it back into drm-misc quickly, right? > >>> > >>> Personally I think patch #2 could go in anytime since, as people have > >>> said, things are pretty broken today and the worst that happens is > >>> that someone gets an extra warning. That would be my preference. That > >>> being said, we could also snooze that patch for a month or two and > >>> land it later. There's no real hurry. > >> > >> For now I'm going to snooze this patch for a month just to avoid any > >> controversy. I'll plan to apply it (to drm-misc-next) when I see the > >> device tree patch land. Since the device tree patch should land as a > >> fix that should keep things landing in the correct order. ...and, as > >> per above, the worst case is that if someone has an old DTS and a new > >> kernel then a panel that was already not working well will print a fat > >> warning and startup a bit slower. > >> > >> If somehow I mess up and forget about this patch, feel free to send me > >> a poke when the device tree patch is landed. > > > > More than a month has passed now. One last warning before I apply this > > revert in a few more days. > > It's fine if you apply it now Thanks! Pushed to drm-misc-next: [2/4] Revert "drm/panel-edp: Add SDC ATNA45AF01" commit: 01cc7b2e8a59fcae0c4493720561e5b33a195fe7 -Doug
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 3a574a9b46e7..d2d682385e89 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[] = { EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200, "Unknown"), EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200, "Unknown"), - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200, "ATNA45AF01"), - EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50, "LQ140M1JW48"), EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50, "LQ140M1JW46"), EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50, "LQ140T1JH01"),