linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
       [not found] <20230526100801.16310-1-uwu@icenowy.me>
@ 2023-05-26 14:24 ` Doug Anderson
  2023-05-26 15:29   ` Icenowy Zheng
  2023-05-29  8:02   ` AngeloGioacchino Del Regno
  0 siblings, 2 replies; 8+ messages in thread
From: Doug Anderson @ 2023-05-26 14:24 UTC (permalink / raw)
  To: Icenowy Zheng, Pin-yen Lin, AngeloGioacchino Del Regno
  Cc: Rob Herring, Krzysztof Kozlowski, Matthias Brugger, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel

Hi,

On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> Currently a specific panel number is used in the Elm DTSI, which is
> corresponded to a 12" panel. However, according to the official Chrome
> OS devices document, Elm refers to Acer Chromebook R13, which, as the
> name specifies, uses a 13.3" panel, which comes with EDID information.
>
> As the kernel currently prioritizes the hardcoded timing parameters
> matched with the panel number compatible, a wrong timing will be applied
> to the 13.3" panel on Acer Chromebook R13, which leads to blank display.
>
> Because the Elm DTSI is shared with Hana board, and Hana corresponds to
> multiple devices from 11" to 14", a certain panel model number shouldn't
> be present, and driving the panel according to its EDID information is
> necessary.
>
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

We went through a bunch of back-and-forth here but in the end in the
ChromeOS tree we have "edp-panel" as the "compatible" here in the
ChromeOS 5.15 tree and this makes sense.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

...in theory one would wish for a "Fixes" tag, but I think in previous
discussions it was decided that it was too complicated. Hardcoding the
other compatible string has always been technically wrong, but I guess
it worked at some point in time. The more correct way (as you're doing
here) needs the DP AUX bus support and the generic eDP panels, both of
which are significantly newer than the elm dts. So I guess leaving no
"Fixes" tag is OK, or perhaps you could do the somewhat weak:

Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move display
to ps8640 auxiliary bus")

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-26 14:24 ` [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT Doug Anderson
@ 2023-05-26 15:29   ` Icenowy Zheng
  2023-05-26 19:51     ` Doug Anderson
  2023-05-29  4:19     ` Hsin-Yi Wang
  2023-05-29  8:02   ` AngeloGioacchino Del Regno
  1 sibling, 2 replies; 8+ messages in thread
From: Icenowy Zheng @ 2023-05-26 15:29 UTC (permalink / raw)
  To: Doug Anderson, Pin-yen Lin, AngeloGioacchino Del Regno
  Cc: Rob Herring, Krzysztof Kozlowski, Matthias Brugger, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel

在 2023-05-26星期五的 07:24 -0700,Doug Anderson写道:
> Hi,
> 
> On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> > 
> > Currently a specific panel number is used in the Elm DTSI, which is
> > corresponded to a 12" panel. However, according to the official
> > Chrome
> > OS devices document, Elm refers to Acer Chromebook R13, which, as
> > the
> > name specifies, uses a 13.3" panel, which comes with EDID
> > information.
> > 
> > As the kernel currently prioritizes the hardcoded timing parameters
> > matched with the panel number compatible, a wrong timing will be
> > applied
> > to the 13.3" panel on Acer Chromebook R13, which leads to blank
> > display.
> > 
> > Because the Elm DTSI is shared with Hana board, and Hana
> > corresponds to
> > multiple devices from 11" to 14", a certain panel model number
> > shouldn't
> > be present, and driving the panel according to its EDID information
> > is
> > necessary.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> We went through a bunch of back-and-forth here but in the end in the
> ChromeOS tree we have "edp-panel" as the "compatible" here in the
> ChromeOS 5.15 tree and this makes sense.

I only have Elm, so I am curious that do all Hana's only rely on panel
EDID to use different displays?

BTW The Chrome OS document say that Elm and Hana are both board based
on Oak baseboard, should the DTSI be renamed mt8173-oak.dtsi, and still
let mt8173-elm.dts include it and then set model information?

> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> ...in theory one would wish for a "Fixes" tag, but I think in
> previous
> discussions it was decided that it was too complicated. Hardcoding
> the
> other compatible string has always been technically wrong, but I
> guess
> it worked at some point in time. The more correct way (as you're
> doing
> here) needs the DP AUX bus support and the generic eDP panels, both
> of
> which are significantly newer than the elm dts. So I guess leaving no
> "Fixes" tag is OK, or perhaps you could do the somewhat weak:

Well I remembered when I was developing the support for Pine64
Pinebook, which is also an ARM64 laptop with an eDP panel (via a DPI-
eDP bridge, ANX6345). At first I didn't use any panel node in the DT,
and the kernel maintainers argued to the bridge that seems to be
connected to nothing (because DP is a discoverable port), and
fortunately 2 Pinebook SKUs (11.6" and 14") is finally reduced to one,
and it's then possible to hardcode a panel model in the Pinebook DT.
According to my memory, the need to specify the panel is to properly
handle eDP panel power up timing, because it's not a very standard
thing. (Well, in my memory, when I was testing that code, on a
(engineering sample) 14" Pinebook, the EDID timing overrided the
hardcoded 11.6" timing and it properly works, the 14" panel is 1366x768
but the 11.6" panel is 1920x1080.)

(BTW when I checked the DT of Olimex TERES-I, which uses the same DPI-
eDP bridge, it is still in the status of a dangling bridge, and of
course it works ;-) )

> 
> Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move display
> to ps8640 auxiliary bus")

Well this sound quite reasonable, as the kernel should have proper AUX
support at this commit.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-26 15:29   ` Icenowy Zheng
@ 2023-05-26 19:51     ` Doug Anderson
  2023-05-29  4:19     ` Hsin-Yi Wang
  1 sibling, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2023-05-26 19:51 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Pin-yen Lin, AngeloGioacchino Del Regno, Rob Herring,
	Krzysztof Kozlowski, Matthias Brugger, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, dri-devel

Hi,

On Fri, May 26, 2023 at 8:33 AM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2023-05-26星期五的 07:24 -0700,Doug Anderson写道:
> > Hi,
> >
> > On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> > >
> > > Currently a specific panel number is used in the Elm DTSI, which is
> > > corresponded to a 12" panel. However, according to the official
> > > Chrome
> > > OS devices document, Elm refers to Acer Chromebook R13, which, as
> > > the
> > > name specifies, uses a 13.3" panel, which comes with EDID
> > > information.
> > >
> > > As the kernel currently prioritizes the hardcoded timing parameters
> > > matched with the panel number compatible, a wrong timing will be
> > > applied
> > > to the 13.3" panel on Acer Chromebook R13, which leads to blank
> > > display.
> > >
> > > Because the Elm DTSI is shared with Hana board, and Hana
> > > corresponds to
> > > multiple devices from 11" to 14", a certain panel model number
> > > shouldn't
> > > be present, and driving the panel according to its EDID information
> > > is
> > > necessary.
> > >
> > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > ---
> > >  arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > We went through a bunch of back-and-forth here but in the end in the
> > ChromeOS tree we have "edp-panel" as the "compatible" here in the
> > ChromeOS 5.15 tree and this makes sense.
>
> I only have Elm, so I am curious that do all Hana's only rely on panel
> EDID to use different displays?
>
> BTW The Chrome OS document say that Elm and Hana are both board based
> on Oak baseboard, should the DTSI be renamed mt8173-oak.dtsi, and still
> let mt8173-elm.dts include it and then set model information?

I wasn't deeply involved in mt8173, so I'll let treapking@ comment. I
think he's done some research here recently.


> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > ...in theory one would wish for a "Fixes" tag, but I think in
> > previous
> > discussions it was decided that it was too complicated. Hardcoding
> > the
> > other compatible string has always been technically wrong, but I
> > guess
> > it worked at some point in time. The more correct way (as you're
> > doing
> > here) needs the DP AUX bus support and the generic eDP panels, both
> > of
> > which are significantly newer than the elm dts. So I guess leaving no
> > "Fixes" tag is OK, or perhaps you could do the somewhat weak:
>
> Well I remembered when I was developing the support for Pine64
> Pinebook, which is also an ARM64 laptop with an eDP panel (via a DPI-
> eDP bridge, ANX6345). At first I didn't use any panel node in the DT,
> and the kernel maintainers argued to the bridge that seems to be
> connected to nothing (because DP is a discoverable port), and
> fortunately 2 Pinebook SKUs (11.6" and 14") is finally reduced to one,
> and it's then possible to hardcode a panel model in the Pinebook DT.
> According to my memory, the need to specify the panel is to properly
> handle eDP panel power up timing, because it's not a very standard
> thing. (Well, in my memory, when I was testing that code, on a
> (engineering sample) 14" Pinebook, the EDID timing overrided the
> hardcoded 11.6" timing and it properly works, the 14" panel is 1366x768
> but the 11.6" panel is 1920x1080.)
>
> (BTW when I checked the DT of Olimex TERES-I, which uses the same DPI-
> eDP bridge, it is still in the status of a dangling bridge, and of
> course it works ;-) )

Before the generic eDP panel support, several devices worked according
to the "little white lie" theory. They would pick some arbitrary panel
to put in the DT because they had to, but then that panel would just
be used for the power up / power down timing and everything else would
be overridden. This was obviously not a great situation to be in, and
so we had many discussions on the mailing list about how to do better.
The end result was the generic edp-panel support.

With eDP panel support, you still need to add the timings for your
specific panel, but it was realized that in _most_ cases we could
power up the panel enough to read the "panel ID" and then we could use
that to lookup the timings. In the few cases where we needed a little
extra help (if HPD is broken or not connected), the DP folks agreed to
allow a few properties to specify it. :-) Hopefully today all new code
uses the general panel-edp.

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-26 15:29   ` Icenowy Zheng
  2023-05-26 19:51     ` Doug Anderson
@ 2023-05-29  4:19     ` Hsin-Yi Wang
  1 sibling, 0 replies; 8+ messages in thread
From: Hsin-Yi Wang @ 2023-05-29  4:19 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Doug Anderson, Pin-yen Lin, AngeloGioacchino Del Regno,
	Rob Herring, Krzysztof Kozlowski, Matthias Brugger, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel

On Mon, May 29, 2023 at 12:14 PM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2023-05-26星期五的 07:24 -0700,Doug Anderson写道:
> > Hi,
> >
> > On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> > >
> > > Currently a specific panel number is used in the Elm DTSI, which is
> > > corresponded to a 12" panel. However, according to the official
> > > Chrome
> > > OS devices document, Elm refers to Acer Chromebook R13, which, as
> > > the
> > > name specifies, uses a 13.3" panel, which comes with EDID
> > > information.
> > >
> > > As the kernel currently prioritizes the hardcoded timing parameters
> > > matched with the panel number compatible, a wrong timing will be
> > > applied
> > > to the 13.3" panel on Acer Chromebook R13, which leads to blank
> > > display.
> > >
> > > Because the Elm DTSI is shared with Hana board, and Hana
> > > corresponds to
> > > multiple devices from 11" to 14", a certain panel model number
> > > shouldn't
> > > be present, and driving the panel according to its EDID information
> > > is
> > > necessary.
> > >
> > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > ---
> > >  arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > We went through a bunch of back-and-forth here but in the end in the
> > ChromeOS tree we have "edp-panel" as the "compatible" here in the
> > ChromeOS 5.15 tree and this makes sense.
>
> I only have Elm, so I am curious that do all Hana's only rely on panel
> EDID to use different displays?
>
> BTW The Chrome OS document say that Elm and Hana are both board based
> on Oak baseboard, should the DTSI be renamed mt8173-oak.dtsi, and still
> let mt8173-elm.dts include it and then set model information?
>
Oak is a reference design board which is not in the public. Since only
elm and hana board are in the public and the difference between elm
and hana are not much, instead of creating oak.dtsi, elm.dtsi (inherit
from oak.dtsi), hana.dtsi (inherit from oak.dtsi), we decided to make
elm.dtsi as the main dtsi and let hana inherit it and make its own
changes.

> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > ...in theory one would wish for a "Fixes" tag, but I think in
> > previous
> > discussions it was decided that it was too complicated. Hardcoding
> > the
> > other compatible string has always been technically wrong, but I
> > guess
> > it worked at some point in time. The more correct way (as you're
> > doing
> > here) needs the DP AUX bus support and the generic eDP panels, both
> > of
> > which are significantly newer than the elm dts. So I guess leaving no
> > "Fixes" tag is OK, or perhaps you could do the somewhat weak:
>
> Well I remembered when I was developing the support for Pine64
> Pinebook, which is also an ARM64 laptop with an eDP panel (via a DPI-
> eDP bridge, ANX6345). At first I didn't use any panel node in the DT,
> and the kernel maintainers argued to the bridge that seems to be
> connected to nothing (because DP is a discoverable port), and
> fortunately 2 Pinebook SKUs (11.6" and 14") is finally reduced to one,
> and it's then possible to hardcode a panel model in the Pinebook DT.
> According to my memory, the need to specify the panel is to properly
> handle eDP panel power up timing, because it's not a very standard
> thing. (Well, in my memory, when I was testing that code, on a
> (engineering sample) 14" Pinebook, the EDID timing overrided the
> hardcoded 11.6" timing and it properly works, the 14" panel is 1366x768
> but the 11.6" panel is 1920x1080.)
>
> (BTW when I checked the DT of Olimex TERES-I, which uses the same DPI-
> eDP bridge, it is still in the status of a dangling bridge, and of
> course it works ;-) )
>
> >
> > Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move display
> > to ps8640 auxiliary bus")
>
> Well this sound quite reasonable, as the kernel should have proper AUX
> support at this commit.
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-26 14:24 ` [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT Doug Anderson
  2023-05-26 15:29   ` Icenowy Zheng
@ 2023-05-29  8:02   ` AngeloGioacchino Del Regno
  2023-05-29  8:45     ` Icenowy Zheng
  1 sibling, 1 reply; 8+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-29  8:02 UTC (permalink / raw)
  To: Doug Anderson, Icenowy Zheng, Pin-yen Lin
  Cc: Rob Herring, Krzysztof Kozlowski, Matthias Brugger, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel

Il 26/05/23 16:24, Doug Anderson ha scritto:
> Hi,
> 
> On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me> wrote:
>>
>> Currently a specific panel number is used in the Elm DTSI, which is
>> corresponded to a 12" panel. However, according to the official Chrome
>> OS devices document, Elm refers to Acer Chromebook R13, which, as the
>> name specifies, uses a 13.3" panel, which comes with EDID information.
>>
>> As the kernel currently prioritizes the hardcoded timing parameters
>> matched with the panel number compatible, a wrong timing will be applied
>> to the 13.3" panel on Acer Chromebook R13, which leads to blank display.
>>
>> Because the Elm DTSI is shared with Hana board, and Hana corresponds to
>> multiple devices from 11" to 14", a certain panel model number shouldn't
>> be present, and driving the panel according to its EDID information is
>> necessary.
>>
>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> We went through a bunch of back-and-forth here but in the end in the
> ChromeOS tree we have "edp-panel" as the "compatible" here in the
> ChromeOS 5.15 tree and this makes sense.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> ...in theory one would wish for a "Fixes" tag, but I think in previous
> discussions it was decided that it was too complicated. Hardcoding the
> other compatible string has always been technically wrong, but I guess
> it worked at some point in time. The more correct way (as you're doing
> here) needs the DP AUX bus support and the generic eDP panels, both of
> which are significantly newer than the elm dts. So I guess leaving no
> "Fixes" tag is OK, or perhaps you could do the somewhat weak:
> 
> Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move display
> to ps8640 auxiliary bus")

I remember I didn't change the compatible to panel-edp because it didn't
work at that time, but it does now... I'm not sure what actually fixed that
and if the commit(s) was/were backported to that suggested point, so I
would leave the Fixes tag out, as that may break older kernels.

Anyway, for this commit:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-29  8:02   ` AngeloGioacchino Del Regno
@ 2023-05-29  8:45     ` Icenowy Zheng
  2023-05-29 16:28       ` Matthias Brugger
  0 siblings, 1 reply; 8+ messages in thread
From: Icenowy Zheng @ 2023-05-29  8:45 UTC (permalink / raw)
  To: AngeloGioacchino Del Regno, Doug Anderson, Pin-yen Lin
  Cc: Rob Herring, Krzysztof Kozlowski, Matthias Brugger, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, dri-devel

在 2023-05-29星期一的 10:02 +0200,AngeloGioacchino Del Regno写道:
> Il 26/05/23 16:24, Doug Anderson ha scritto:
> > Hi,
> > 
> > On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me>
> > wrote:
> > > 
> > > Currently a specific panel number is used in the Elm DTSI, which
> > > is
> > > corresponded to a 12" panel. However, according to the official
> > > Chrome
> > > OS devices document, Elm refers to Acer Chromebook R13, which, as
> > > the
> > > name specifies, uses a 13.3" panel, which comes with EDID
> > > information.
> > > 
> > > As the kernel currently prioritizes the hardcoded timing
> > > parameters
> > > matched with the panel number compatible, a wrong timing will be
> > > applied
> > > to the 13.3" panel on Acer Chromebook R13, which leads to blank
> > > display.
> > > 
> > > Because the Elm DTSI is shared with Hana board, and Hana
> > > corresponds to
> > > multiple devices from 11" to 14", a certain panel model number
> > > shouldn't
> > > be present, and driving the panel according to its EDID
> > > information is
> > > necessary.
> > > 
> > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > ---
> > >   arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > We went through a bunch of back-and-forth here but in the end in
> > the
> > ChromeOS tree we have "edp-panel" as the "compatible" here in the
> > ChromeOS 5.15 tree and this makes sense.
> > 
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > 
> > ...in theory one would wish for a "Fixes" tag, but I think in
> > previous
> > discussions it was decided that it was too complicated. Hardcoding
> > the
> > other compatible string has always been technically wrong, but I
> > guess
> > it worked at some point in time. The more correct way (as you're
> > doing
> > here) needs the DP AUX bus support and the generic eDP panels, both
> > of
> > which are significantly newer than the elm dts. So I guess leaving
> > no
> > "Fixes" tag is OK, or perhaps you could do the somewhat weak:
> > 
> > Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move
> > display
> > to ps8640 auxiliary bus")
> 
> I remember I didn't change the compatible to panel-edp because it
> didn't
> work at that time, but it does now... I'm not sure what actually
> fixed that
> and if the commit(s) was/were backported to that suggested point, so
> I
> would leave the Fixes tag out, as that may break older kernel.

Well at least I developed this patch on v6.3.

(In fact the same kernel config do not boot to system at all on
v6.0/v6.1 when I do make olddefconfig then build)

> 
> Anyway, for this commit:
> 
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-29  8:45     ` Icenowy Zheng
@ 2023-05-29 16:28       ` Matthias Brugger
  2023-05-30  5:03         ` Icenowy Zheng
  0 siblings, 1 reply; 8+ messages in thread
From: Matthias Brugger @ 2023-05-29 16:28 UTC (permalink / raw)
  To: Icenowy Zheng, AngeloGioacchino Del Regno, Doug Anderson, Pin-yen Lin
  Cc: Rob Herring, Krzysztof Kozlowski, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, dri-devel



On 29/05/2023 10:45, Icenowy Zheng wrote:
> 在 2023-05-29星期一的 10:02 +0200,AngeloGioacchino Del Regno写道:
>> Il 26/05/23 16:24, Doug Anderson ha scritto:
>>> Hi,
>>>
>>> On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me>
>>> wrote:
>>>>
>>>> Currently a specific panel number is used in the Elm DTSI, which
>>>> is
>>>> corresponded to a 12" panel. However, according to the official
>>>> Chrome
>>>> OS devices document, Elm refers to Acer Chromebook R13, which, as
>>>> the
>>>> name specifies, uses a 13.3" panel, which comes with EDID
>>>> information.
>>>>
>>>> As the kernel currently prioritizes the hardcoded timing
>>>> parameters
>>>> matched with the panel number compatible, a wrong timing will be
>>>> applied
>>>> to the 13.3" panel on Acer Chromebook R13, which leads to blank
>>>> display.
>>>>
>>>> Because the Elm DTSI is shared with Hana board, and Hana
>>>> corresponds to
>>>> multiple devices from 11" to 14", a certain panel model number
>>>> shouldn't
>>>> be present, and driving the panel according to its EDID
>>>> information is
>>>> necessary.
>>>>
>>>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>>>> ---
>>>>    arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> We went through a bunch of back-and-forth here but in the end in
>>> the
>>> ChromeOS tree we have "edp-panel" as the "compatible" here in the
>>> ChromeOS 5.15 tree and this makes sense.
>>>
>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>
>>> ...in theory one would wish for a "Fixes" tag, but I think in
>>> previous
>>> discussions it was decided that it was too complicated. Hardcoding
>>> the
>>> other compatible string has always been technically wrong, but I
>>> guess
>>> it worked at some point in time. The more correct way (as you're
>>> doing
>>> here) needs the DP AUX bus support and the generic eDP panels, both
>>> of
>>> which are significantly newer than the elm dts. So I guess leaving
>>> no
>>> "Fixes" tag is OK, or perhaps you could do the somewhat weak:
>>>
>>> Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move
>>> display
>>> to ps8640 auxiliary bus")
>>
>> I remember I didn't change the compatible to panel-edp because it
>> didn't
>> work at that time, but it does now... I'm not sure what actually
>> fixed that
>> and if the commit(s) was/were backported to that suggested point, so
>> I
>> would leave the Fixes tag out, as that may break older kernel.
> 
> Well at least I developed this patch on v6.3.
> 
> (In fact the same kernel config do not boot to system at all on
> v6.0/v6.1 when I do make olddefconfig then build)
> 

I applied the patch without the fixes tag. Lets stay on the secure side to not 
break older kernels.

Regards,
Matthias

>>
>> Anyway, for this commit:
>>
>> Reviewed-by: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT
  2023-05-29 16:28       ` Matthias Brugger
@ 2023-05-30  5:03         ` Icenowy Zheng
  0 siblings, 0 replies; 8+ messages in thread
From: Icenowy Zheng @ 2023-05-30  5:03 UTC (permalink / raw)
  To: Matthias Brugger, AngeloGioacchino Del Regno, Doug Anderson, Pin-yen Lin
  Cc: Rob Herring, Krzysztof Kozlowski, devicetree, linux-kernel,
	linux-arm-kernel, linux-mediatek, dri-devel

在 2023-05-29星期一的 18:28 +0200,Matthias Brugger写道:
> 
> 
> On 29/05/2023 10:45, Icenowy Zheng wrote:
> > 在 2023-05-29星期一的 10:02 +0200,AngeloGioacchino Del Regno写道:
> > > Il 26/05/23 16:24, Doug Anderson ha scritto:
> > > > Hi,
> > > > 
> > > > On Fri, May 26, 2023 at 3:09 AM Icenowy Zheng <uwu@icenowy.me>
> > > > wrote:
> > > > > 
> > > > > Currently a specific panel number is used in the Elm DTSI,
> > > > > which
> > > > > is
> > > > > corresponded to a 12" panel. However, according to the
> > > > > official
> > > > > Chrome
> > > > > OS devices document, Elm refers to Acer Chromebook R13,
> > > > > which, as
> > > > > the
> > > > > name specifies, uses a 13.3" panel, which comes with EDID
> > > > > information.
> > > > > 
> > > > > As the kernel currently prioritizes the hardcoded timing
> > > > > parameters
> > > > > matched with the panel number compatible, a wrong timing will
> > > > > be
> > > > > applied
> > > > > to the 13.3" panel on Acer Chromebook R13, which leads to
> > > > > blank
> > > > > display.
> > > > > 
> > > > > Because the Elm DTSI is shared with Hana board, and Hana
> > > > > corresponds to
> > > > > multiple devices from 11" to 14", a certain panel model
> > > > > number
> > > > > shouldn't
> > > > > be present, and driving the panel according to its EDID
> > > > > information is
> > > > > necessary.
> > > > > 
> > > > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > > > ---
> > > > >    arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > We went through a bunch of back-and-forth here but in the end
> > > > in
> > > > the
> > > > ChromeOS tree we have "edp-panel" as the "compatible" here in
> > > > the
> > > > ChromeOS 5.15 tree and this makes sense.
> > > > 
> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > 
> > > > ...in theory one would wish for a "Fixes" tag, but I think in
> > > > previous
> > > > discussions it was decided that it was too complicated.
> > > > Hardcoding
> > > > the
> > > > other compatible string has always been technically wrong, but
> > > > I
> > > > guess
> > > > it worked at some point in time. The more correct way (as
> > > > you're
> > > > doing
> > > > here) needs the DP AUX bus support and the generic eDP panels,
> > > > both
> > > > of
> > > > which are significantly newer than the elm dts. So I guess
> > > > leaving
> > > > no
> > > > "Fixes" tag is OK, or perhaps you could do the somewhat weak:
> > > > 
> > > > Fixes: c2d94f72140a ("arm64: dts: mediatek: mt8173-elm: Move
> > > > display
> > > > to ps8640 auxiliary bus")
> > > 
> > > I remember I didn't change the compatible to panel-edp because it
> > > didn't
> > > work at that time, but it does now... I'm not sure what actually
> > > fixed that
> > > and if the commit(s) was/were backported to that suggested point,
> > > so
> > > I
> > > would leave the Fixes tag out, as that may break older kernel.
> > 
> > Well at least I developed this patch on v6.3.
> > 
> > (In fact the same kernel config do not boot to system at all on
> > v6.0/v6.1 when I do make olddefconfig then build)
> > 
> 
> I applied the patch without the fixes tag. Lets stay on the secure
> side to not 
> break older kernels.

Well I think this patch is at least meaningful to get backported to
v6.3.

Should we cc stable@vger.kernel.org ?

> 
> Regards,
> Matthias
> 
> > > 
> > > Anyway, for this commit:
> > > 
> > > Reviewed-by: AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@collabora.com>
> > 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-05-30  5:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20230526100801.16310-1-uwu@icenowy.me>
2023-05-26 14:24 ` [PATCH] arm64: dts: mediatek: mt8173-elm: remove panel model number in DT Doug Anderson
2023-05-26 15:29   ` Icenowy Zheng
2023-05-26 19:51     ` Doug Anderson
2023-05-29  4:19     ` Hsin-Yi Wang
2023-05-29  8:02   ` AngeloGioacchino Del Regno
2023-05-29  8:45     ` Icenowy Zheng
2023-05-29 16:28       ` Matthias Brugger
2023-05-30  5:03         ` Icenowy Zheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).