Re: [PATCH resend v2] drm: renesas: shmobile: Call drm_helper_force_disable_all() at shutdown time

[Doug Anderson]

Hi,

On Wed, May 29, 2024 at 5:16 AM Geert Uytterhoeven
 wrote:
>
> From: Douglas Anderson 
>
> Based on grepping through the source code, this driver appears to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown time.
> This is important because drm_helper_force_disable_all() will cause
> panels to get disabled cleanly which may be important for their power
> sequencing.  Future changes will remove any custom powering off in
> individual panel drivers so the DRM drivers need to start getting this
> right.
>
> The fact that we should call drm_atomic_helper_shutdown() in the case of
> OS shutdown comes straight out of the kernel doc "driver instance
> overview" in drm_drv.c.
>
> Suggested-by: Maxime Ripard 
> Signed-off-by: Douglas Anderson 
> Link: 
> https://lore.kernel.org/r/20230901164111.RFT.15.Iaf638a1d4c8b3c307a6192efabb4cbb06b195f15@changeid
> [geert: s/drm_helper_force_disable_all/drm_atomic_helper_shutdown/]
> [geert: shmob_drm_remove() already calls drm_atomic_helper_shutdown]
> Signed-off-by: Geert Uytterhoeven 
> Reviewed-by: Laurent Pinchart 
> ---
> v2:
>   - Add Reviewed-by.
>
> Tested on Atmark Techno Armadillo-800-EVA.
> ---
>  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 8 
>  1 file changed, 8 insertions(+)

FWIW: I've created a patch to list DRM modeset drivers that handle
shutdown properly [1]. For now "shmob-drm" is not part of that
patchset. Assuming my patch lands we'll have to later add it to the
list.

[1] 
https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid

I will also note that the subject/description of this patch could be
adjusted. They still reference "drm_helper_force_disable_all" which
should have been changed to "drm_atomic_helper_shutdown".

-Doug

Re: [PATCH] drm/panel-edp: Add panel CSOT MNB601LS1-1

[Doug Anderson]

Hi,

On Tue, May 28, 2024 at 9:27 AM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, May 6, 2024 at 8:54 AM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 6:55 PM Xuxin Xiong
> >  wrote:
> > >
> > > Hi Doug, thank you!
> > > We had reported this info to the CSOT to correct the vendor id.
> > > If they confirm to fix this with the same product ID, we will submit a
> > > patch to fix this.
> >
> > FYI, "top posting" like this is generally frowned upon on kernel
> > mailing lists. One such reference about this is [1]. Some folks are
> > very passionate about this topic, so please keep it in mind to avoid
> > upsetting people in the community.
> >
> > In any case: did you get any response from CSOT about the improper EDID?
>
> Just following up here. Was there any response from CSOT?

Continuing to follow up here. Did CSOT say anything about this?

-Doug

Re: [PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Doug Anderson]

Hi,

On Tue, Jun 11, 2024 at 7:05 AM Tejas Vipin  wrote:
>
> mipi_dsi_msleep expects struct mipi_dsi_multi_context to be passed as a
> value and not as a reference.
>
> Fixes: a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS 
> functions")
>
> Signed-off-by: Tejas Vipin 

Should be no blank line between "Fixes" and "Signed-off-by"

> ---
>
> Changes in v2:
> - Add Fixes tag
>
> v1: 
> https://lore.kernel.org/all/d9f4546f-c2f9-456d-ba75-85cc195dd...@gmail.com/
>
> ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I notice you didn't CC me, even though I authored the broken commit.
Presumably get_maintainer should have suggested you CC me?


> diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c 
> b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> index 6009a3fe1b8f..ab00fd92cce0 100644
> --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> @@ -479,7 +479,7 @@ static int hx83102_disable(struct drm_panel *panel)
> mipi_dsi_dcs_set_display_off_multi(_ctx);
> mipi_dsi_dcs_enter_sleep_mode_multi(_ctx);
>
> -   mipi_dsi_msleep(_ctx, 150);
> +   mipi_dsi_msleep(dsi_ctx, 150);

So while your fix is correct, it's not really enough. I swore that I
compile tested my change and, sure enough, the bad code compile tests
fine. This is because the macro mipi_dsi_msleep() fell into a macro
trap. :( Specifically, we have:

#define mipi_dsi_msleep(ctx, delay)\
do {   \
if (!ctx.accum_err)\
msleep(delay); \
} while (0)

Let's look at "if (!ctx.accum_err)". Before your patch, that translated to:

if (!_ctx.accum_err)

...adding extra parentheses for order of operations, that is:

 if (!&(dsi_ctx.accum_err))

...in other words it's testing whether the address of the "accum_err"
is NULL. That's not a syntax error, but _really_ not what was meant.

We really need to fix the macro trap by changing it like this:

-   if (!ctx.accum_err) \
+   if (!(ctx).accum_err)   \

When you do that, though, you find that half the users of the macro
were using it wrong since every other "_multi_" function passes the
address. IMO while fixing the macro trap we should just change this to
pass the address and then fix up all the callers.

This is a serious enough problem (thanks for noticing it) that I'm
happy to send out patches, but also I'm fine if you want to tackle it.
If I don't see anything from you in a day or two I'll send out
patches.

Thanks!

-Doug

Re: [PATCH v2] drm/panel : truly-nt35521: transition to mipi_dsi wrapped functions

[Doug Anderson]

Hi,

On Tue, Jun 11, 2024 at 7:44 AM Tejas Vipin  wrote:
>
> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi: Introduce
> mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> sony tulip truly nt35521 panel.
>
> Signed-off-by: Tejas Vipin 
> ---
>
> Changes in v2:
> - Fix patch format
> - Fix code style
>
> v1: 
> https://lore.kernel.org/all/485eef24-ddad-466a-a89f-f9f226801...@gmail.com/
>
> ---
>  .../panel/panel-sony-tulip-truly-nt35521.c| 435 +-
>  1 file changed, 209 insertions(+), 226 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c 
> b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> index 6d44970dccd9..5a050352c207 100644
> --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> @@ -44,248 +44,231 @@ static void truly_nt35521_reset(struct truly_nt35521 
> *ctx)
>  static int truly_nt35521_on(struct truly_nt35521 *ctx)
>  {
> struct mipi_dsi_device *dsi = ctx->dsi;
> -   struct device *dev = >dev;
> -   int ret;
> +
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };

It's not a huge deal, but normally in the kernel all the variable
declarations are cuddled together. AKA no blank line between the
declaration of "dsi" and the declaration of "dsi_ctx". It would be
awesome if you could send a v3 fixing that. When you send v3, feel
free to add this above your own Signed-off-by:

Reviewed-by: Douglas Anderson 

...with that, the patch will probably sit on the mailing lists for a
week or two and then get applied. Neil may want to apply it, but if
he's busy I can do it too.

I believe you were planning on tackling some more of the panels. Since
you're still getting started sending patches, maybe keep it to a
smaller batch for now and send another 10 or so? Probably best to keep
it as one panel driver per patch.

-Doug

Re: Subject: [PATCH] drm/panel : truly-nt35521: transition to mipi_dsi wrapped functions

[Doug Anderson]

Hi,

On Sat, Jun 8, 2024 at 3:57 AM Tejas Vipin  wrote:
>
> Use functions introduced in 966e397e4f603 ("drm/mipi-dsi: Introduce
> mipi_dsi_*_write_seq_multi()") and f79d6d28d8fe
> ("drm/mipi-dsi: wrap more functions for streamline handling") for the
> sony tulip truly nt35521 panel.

Running "scripts/checkpatch.pl" will yell about the above. You're
supposed to write the word "commit" before the git hash. AKA:

Use functions introduced in commit 966e397e4f603  ("drm/mipi-dsi:
Introduce mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
("drm/mipi-dsi: wrap more functions for streamline handling") for
the...


> Signed-off-by: Tejas Vipin 
> ---
>  .../panel/panel-sony-tulip-truly-nt35521.c| 383 +-
>  1 file changed, 183 insertions(+), 200 deletions(-)

The subject of your patch has an extra "Subject:" prefix. See:

https://lore.kernel.org/r/485eef24-ddad-466a-a89f-f9f226801...@gmail.com

...where you can see "Subject: Subject:". Maybe use "b4" or "patman"
to help you send your patch?


> diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c 
> b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> index 6d44970dccd9..13472c7c37f5 100644
> --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> @@ -44,248 +44,231 @@ static void truly_nt35521_reset(struct truly_nt35521 
> *ctx)
>  static int truly_nt35521_on(struct truly_nt35521 *ctx)
>  {
> struct mipi_dsi_device *dsi = ctx->dsi;
> -   struct device *dev = >dev;
> -   int ret;
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> -   mipi_dsi_generic_write_seq(dsi, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xff, 0xaa, 0x55, 0xa5, 0x80);
> -   mipi_dsi_generic_write_seq(dsi, 0x6f, 0x11, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xf7, 0x20, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0x6f, 0x01);
> -   mipi_dsi_generic_write_seq(dsi, 0xb1, 0x21);
> -   mipi_dsi_generic_write_seq(dsi, 0xbd, 0x01, 0xa0, 0x10, 0x08, 0x01);
> -   mipi_dsi_generic_write_seq(dsi, 0xb8, 0x01, 0x02, 0x0c, 0x02);
> -   mipi_dsi_generic_write_seq(dsi, 0xbb, 0x11, 0x11);
> -   mipi_dsi_generic_write_seq(dsi, 0xbc, 0x00, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xb6, 0x02);
> -   mipi_dsi_generic_write_seq(dsi, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x01);
> -   mipi_dsi_generic_write_seq(dsi, 0xb0, 0x09, 0x09);
> -   mipi_dsi_generic_write_seq(dsi, 0xb1, 0x09, 0x09);
> -   mipi_dsi_generic_write_seq(dsi, 0xbc, 0x8c, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xbd, 0x8c, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xca, 0x00);
> -   mipi_dsi_generic_write_seq(dsi, 0xc0, 0x04);
> -   mipi_dsi_generic_write_seq(dsi, 0xbe, 0xb5);
> -   mipi_dsi_generic_write_seq(dsi, 0xb3, 0x35, 0x35);
> -   mipi_dsi_generic_write_seq(dsi, 0xb4, 0x25, 0x25);
> -   mipi_dsi_generic_write_seq(dsi, 0xb9, 0x43, 0x43);
> -   mipi_dsi_generic_write_seq(dsi, 0xba, 0x24, 0x24);
> -   mipi_dsi_generic_write_seq(dsi, 0xf0, 0x55, 0xaa, 0x52, 0x08, 0x02);
> -   mipi_dsi_generic_write_seq(dsi, 0xee, 0x03);
> -   mipi_dsi_generic_write_seq(dsi, 0xb0,
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };

Please move the variable declaration above the line "dsi->mode_flags
|= MIPI_DSI_MODE_LPM;"


> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xf0, 0x55, 0xaa, 0x52, 
> 0x08, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xff, 0xaa, 0x55, 0xa5, 
> 0x80);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0x6f, 0x11, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xf7, 0x20, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0x6f, 0x01);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb1, 0x21);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbd, 0x01, 0xa0, 0x10, 
> 0x08, 0x01);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb8, 0x01, 0x02, 0x0c, 
> 0x02);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbb, 0x11, 0x11);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbc, 0x00, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb6, 0x02);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xf0, 0x55, 0xaa, 0x52, 
> 0x08, 0x01);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb0, 0x09, 0x09);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb1, 0x09, 0x09);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbc, 0x8c, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbd, 0x8c, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xca, 0x00);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xc0, 0x04);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xbe, 0xb5);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb3, 0x35, 0x35);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb4, 0x25, 0x25);
> +   mipi_dsi_generic_write_seq_multi(_ctx, 0xb9, 0x43, 0x43);
> +   

Re: [PATCH v3 0/3] drm/panel-edp: remove several legacy compatibles used by the driver

[Doug Anderson]

Hi,

On Fri, May 31, 2024 at 9:51 AM Jeffrey Hugo  wrote:
>
> On 5/31/2024 10:20 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 31, 2024 at 9:18 AM Jeffrey Hugo  wrote:
> >>
> >> On 5/30/2024 5:12 PM, Dmitry Baryshkov wrote:
> >>> There are two ways to describe an eDP panel in device tree. The
> >>> recommended way is to add a device on the AUX bus, ideally using the
> >>> edp-panel compatible. The legacy way is to define a top-level platform
> >>> device for the panel.
> >>>
> >>> Document that adding support for eDP panels in a legacy way is strongly
> >>> discouraged (if not forbidden at all).
> >>>
> >>> While we are at it, also drop legacy compatible strings and bindings for
> >>> five panels. These compatible strings were never used by a DT file
> >>> present in Linux kernel and most likely were never used with the
> >>> upstream Linux kernel.
> >>>
> >>> The following compatibles were never used by the devices supported by
> >>> the upstream kernel and are a subject to possible removal:
> >>>
> >>> - lg,lp097qx1-spa1
> >>> - samsung,lsn122dl01-c01
> >>> - sharp,ld-d5116z01b
> >>
> >> Ok to drop the sharp one I added.  It should be able to be handled by
> >> the (newish) edp-panel, but I think the TI bridge driver needs some work
> >> for the specific platform (no I2C connection) to verify.
> >
> > Is the platform supported upstream? If so, which platform is it? Is
> > the TI bridge chip the ti-sn65dsi86? If so, I'm confused how you could
> > use that bridge chip without an i2c connection, but perhaps I'm
> > misunderstanding. :-P
>
> Yes, the platform is upstream.  The 8998 laptops (clamshell).  It is the
> ti-sn65si86.  I suspect the I2C connection was not populated for cost
> reasons, then determined its much more convenient to have it as every
> generation after that I've seen has the I2C.
>
> If you check the datasheet closely, the I2C connection is optional.  You
> can also configure the bridge inband using DSI commands.  This is what
> the FW and Windows does.
>
> So, the DT binding needs to make the I2C property optional (this should
> be backwards compatible).  The driver needs to detect that the I2C
> connection is not provided, and fall back to DSI commands.  Regmap would
> be nice for this, but I got pushback on the proposal.  Then I got
> sidetracked looking at other issues.

Crazy! I'm sure I've skimmed over that part of the ti-sn65dsi86
datasheet before but I don't think I internalized it. I guess if you
did it this way then you'd instantiate it as a platform device instead
of an i2c device and that would be how you'd detect the difference. I
could imagine this being a bit of a challenge to get working in the
driver.

Re: [PATCH v3 0/3] drm/panel-edp: remove several legacy compatibles used by the driver

[Doug Anderson]

Hi,

On Fri, May 31, 2024 at 9:18 AM Jeffrey Hugo  wrote:
>
> On 5/30/2024 5:12 PM, Dmitry Baryshkov wrote:
> > There are two ways to describe an eDP panel in device tree. The
> > recommended way is to add a device on the AUX bus, ideally using the
> > edp-panel compatible. The legacy way is to define a top-level platform
> > device for the panel.
> >
> > Document that adding support for eDP panels in a legacy way is strongly
> > discouraged (if not forbidden at all).
> >
> > While we are at it, also drop legacy compatible strings and bindings for
> > five panels. These compatible strings were never used by a DT file
> > present in Linux kernel and most likely were never used with the
> > upstream Linux kernel.
> >
> > The following compatibles were never used by the devices supported by
> > the upstream kernel and are a subject to possible removal:
> >
> > - lg,lp097qx1-spa1
> > - samsung,lsn122dl01-c01
> > - sharp,ld-d5116z01b
>
> Ok to drop the sharp one I added.  It should be able to be handled by
> the (newish) edp-panel, but I think the TI bridge driver needs some work
> for the specific platform (no I2C connection) to verify.

Is the platform supported upstream? If so, which platform is it? Is
the TI bridge chip the ti-sn65dsi86? If so, I'm confused how you could
use that bridge chip without an i2c connection, but perhaps I'm
misunderstanding. :-P

Thanks!

-Doug

Re: [PATCH v3 0/3] drm/panel-edp: remove several legacy compatibles used by the driver

[Doug Anderson]

Hi,

On Thu, May 30, 2024 at 4:13 PM Dmitry Baryshkov
 wrote:
>
> There are two ways to describe an eDP panel in device tree. The
> recommended way is to add a device on the AUX bus, ideally using the
> edp-panel compatible. The legacy way is to define a top-level platform
> device for the panel.
>
> Document that adding support for eDP panels in a legacy way is strongly
> discouraged (if not forbidden at all).
>
> While we are at it, also drop legacy compatible strings and bindings for
> five panels. These compatible strings were never used by a DT file
> present in Linux kernel and most likely were never used with the
> upstream Linux kernel.
>
> The following compatibles were never used by the devices supported by
> the upstream kernel and are a subject to possible removal:
>
> - lg,lp097qx1-spa1
> - samsung,lsn122dl01-c01
> - sharp,ld-d5116z01b
>
> I'm considering dropping them later, unless there is a good reason not
> to do so.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
> Changes in v3:
> - Rephrased the warning comment, following some of Doug's suggestions.
> - Link to v2: 
> https://lore.kernel.org/r/20240529-edp-panel-drop-v2-0-fcfc457fc...@linaro.org
>
> Changes in v2:
> - Actually drop support for five panels acked by Doug Andersson
> - Link to v1: 
> https://lore.kernel.org/r/20240523-edp-panel-drop-v1-1-045d62511...@linaro.org
>
> ---
> Dmitry Baryshkov (3):
>   drm/panel-edp: add fat warning against adding new panel compatibles
>   dt-bindings: display: panel-simple: drop several eDP panels
>   drm/panel-edp: drop several legacy panels
>
>  .../bindings/display/panel/panel-simple.yaml   |  10 --
>  drivers/gpu/drm/panel/panel-edp.c  | 192 
> +++--
>  2 files changed, 25 insertions(+), 177 deletions(-)

Thanks! I'm happy to apply this series or also happy if some other
drm-misc committer member wants to apply it. Probably we should wait
for a DT person to make sure that they don't have any problems with
the bindings change.


-Doug

Re: [PATCH v2 2/3] dt-bindings: display: panel-simple: drop several eDP panels

[Doug Anderson]

Hi,

On Tue, May 28, 2024 at 4:53 PM Dmitry Baryshkov
 wrote:
>
> The panel-simple.yaml includes legacy bindings for several eDP panels
> which were never used in DT files present in Linux tree and most likely
> have never been used with the upstream kernel. Drop compatibles for
> these panels in favour of using a generic "edp-panel" device on the AUX
> bus.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  .../devicetree/bindings/display/panel/panel-simple.yaml| 10 
> --
>  1 file changed, 10 deletions(-)

All of these are fine as per my research [1].

Reviewed-by: Douglas Anderson 

[1] 
https://lore.kernel.org/r/CAD=FV=Xz1VsF8jG0q-Ldk+p=NY-ChJkTk0iW8fq2bO=orfv...@mail.gmail.com

Re: [PATCH v2 3/3] drm/panel-edp: drop several legacy panels

[Doug Anderson]

Hi,

On Tue, May 28, 2024 at 4:53 PM Dmitry Baryshkov
 wrote:
>
> The panel-edp driver supports legacy compatible strings for several eDP
> panels which were never used in DT files present in Linux tree and most
> likely have never been used with the upstream kernel. Drop compatibles
> for these panels in favour of using a generic "edp-panel" device on the
> AUX bus.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 173 
> ++
>  1 file changed, 7 insertions(+), 166 deletions(-)

All of these are fine as per my research [1].

Reviewed-by: Douglas Anderson 

[1] 
https://lore.kernel.org/r/CAD=FV=Xz1VsF8jG0q-Ldk+p=NY-ChJkTk0iW8fq2bO=orfv...@mail.gmail.com

Re: [PATCH v2 1/3] drm/panel-edp: add fat warning against adding new panel compatibles

[Doug Anderson]

Hi,

On Tue, May 28, 2024 at 4:52 PM Dmitry Baryshkov
 wrote:
>
> Add a fat warning against adding new panel compatibles to the panel-edp
> driver. All new users of the eDP panels are supposed to use the generic
> "edp-panel" compatible device on the AUX bus. The remaining compatibles
> are either used by the existing DT or were used previously and are
> retained for backwards compatibility.
>
> Suggested-by: Doug Anderson 
> Reviewed-by: Neil Armstrong 
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c 
> b/drivers/gpu/drm/panel/panel-edp.c
> index 6db277efcbb7..95b25ec67168 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = 
> {
> {
> /* Must be first */
> .compatible = "edp-panel",
> -   }, {
> +   },
> +   /*
> +* Do not add panels to the list below unless they cannot be handled 
> by
> +* the generic edp-panel compatible.
> +*
> +* The only two valid reasons are:
> +* - because of the panel issues (e.g. broken EDID or broken
> +*   identification),
> +* - because the platform which uses the panel didn't wire up the AUX
> +*   bus properly.
> +*
> +* In all other cases the platform should use the aux-bus and declare
> +* the panel using the 'edp-panel' compatible as a device on the AUX
> +* bus. The lack of the aux-bus support is not a valid case. Platforms
> +* are urged to be converted to declaring panels in a proper way.

I'm still at least slightly confused by the wording. What is "the lack
of the aux-bus support". I guess this is different from the valid
reason of "the platform which uses the panel didn't wire up the AUX
bus properly" but I'm not sure how. Maybe you can explain?

I guess I'd write it like this:

/*
 * Do not add panels to the list below unless they cannot be handled by
 * the generic edp-panel compatible.
 *
 * The only two valid reasons are:
 * - because of the panel issues (e.g. broken EDID or broken
 *   identification).
 * - because the platform which uses the panel didn't wire up the AUX
 *   bus properly. NOTE that, though this is a marginally valid reason,
 *   some justification needs to be made for why the platform can't
 *   wire up the AUX bus properly.
 *
 * In all other cases the platform should use the aux-bus and declare
 * the panel using the 'edp-panel' compatible as a device on the AUX
 * bus.
 */

What do you think? In any case, it probably doesn't matter much. The
important thing is some sort of warning here telling people not to add
to the table. In that sense:

Reviewed-by: Douglas Anderson 

Re: [PATCH 2/2] drm/panel-edp: Add more panels with conservative timings

[Doug Anderson]

Hi,

On Mon, May 27, 2024 at 2:56 AM Pin-yen Lin  wrote:
>
> Same as commit 7c8690d8fc80 ("drm/panel-edp: Add some panels with
> conservative timings"), the 3 panels added in this patch are used by
> Mediatek MT8173 Chromebooks and they used to work with the downstream
> v4.19 kernel without any specified delay.
>
> These panel IDs were found from in-field reports, but their datahseets
> are not available. For BOE 0x0623 and SHP 0x153a, their product names
> are retrieved from the EDIDs. The EDID of AUO 0x1999 does not contain
> such information, so list as "Unknown" in this patch.
>
> Update these entries with less-conservative timings from other panels of
> the same vendor.
>
> Signed-off-by: Pin-yen Lin 
>
> ---
>
>  drivers/gpu/drm/panel/panel-edp.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Douglas Anderson 

...and as usual with small updates to this table I've pushed it to
drm-misc-next without a big wait.

Re: [PATCH 1/2] drm/panel-edp: Add support for several panels

[Doug Anderson]

Hi,

On Mon, May 27, 2024 at 2:56 AM Pin-yen Lin  wrote:
>
> Add support for the following models:
> AUO B140HTN02.0
> BOE NT116WHM-N21 V4.1
> BOE NT116WHM-N21
>
> Signed-off-by: Pin-yen Lin 
> ---
>
>  drivers/gpu/drm/panel/panel-edp.c | 3 +++
>  1 file changed, 3 insertions(+)

Ideally the subject would be slightly less generic, but I guess it's fine.

Reviewed-by: Douglas Anderson 

...and as usual with small updates to this table I've pushed it to
drm-misc-next without a big wait.

Re: [RFT PATCH v2 00/48] drm/panel: Remove most store/double-check of prepared/enabled state

[Doug Anderson]

Hi,

On Wed, May 8, 2024 at 2:14 PM Doug Anderson  wrote:
>
> > This is the right thing to do, thanks for looking into this!
> >
> > As for the behaviour of .remove() I doubt whether in many cases
> > the original driver authors have even tested this themselves.
>
> Yeah, I'd tend to agree.
>
>
> > I would say we should just apply the series as soon as it's non-RFC
>
> It's not actually RFC now, but "RFT" (request for testing). I don't
> _think_ there's any need to send a version without the RFT tag before
> landing unless someone really feels strongly about it.
>
>
> > after the next merge window
>
> With drm-misc there's not really any specific reason to wait for the
> merge window to open/close as we can land in drm-misc-next at any time
> regardless of the merge window. drm-misc-next will simply stop feeding
> linuxnext for a while.
>
> That all being said, I'm happy to delay landing this until after the
> next -rc1 comes out if people would prefer that. If I don't hear
> anything, I guess I'll just wait until -rc1 before landing any of
> these.
>
>
> > and see what happens. I doubt it
> > will cause much trouble.
>
> I can land the whole series if that's what everyone agrees on. As I
> mentioned above, I'm at least slightly worried that I did something
> stupid _somewhere_ in this series since no automation was possible and
> with repetitive tasks like this it's super easy to flub something up.
> It's _probably_ fine, but I guess I still have the worry in the back
> of my mind.
>
> If folks think I should just apply the whole series then I'm happy to
> do that. If folks think I should just land parts of the series as they
> are reviewed/tested I can do that as well. Let me know. If I don't
> hear anything I'd tend to just land patches that are reviewed/tested.
> Then after a month or so (hopefully) I'd send out a v2 with anything
> left.

Nobody said anything, so I did what I indicated above:

1. I've applied all patches that someone responded to with Linus +
Maxime's Acks + any given tags. This includes the st7703 panels which
Ondřej replied to the cover letter about but didn't officially get any
tags.

2. I also applied patches for panels that I was personally involved
with. This includes panel-edp, panel-simple, samsung-atna33xc20,
boe-tv101wum-nl6.

Anything totally unresponded to I've left unapplied. I'll wait a
little while (at least a week) and then plan to send a v2 with
anything still outstanding. If someone sends Tested-by/Reviewed-by for
some panels in the meantime I'll apply them.

Here are the 25 patches applied to drm-misc-next:

[01/48] drm/panel: raydium-rm692e5: Stop tracking prepared
commit: 598dc42f25cc3060fd350db0f52af1075af3f500

[04/48] drm/panel: boe-tv101wum-nl6: Stop tracking prepared
commit: 3c24e31c908eb12e99420ff33b74c01f045253fe
[05/48] drm/panel: boe-tv101wum-nl6: Don't call unprepare+disable at
shutdown/remove
commit: 1985e3512b5a3777f6a18c36e40f3926037120bb
[06/48] drm/panel: edp: Stop tracking prepared/enabled
commit: 3904f317fd977533f6d7d3c4bfd75e0ac6169bb7
[07/48] drm/panel: edp: Add a comment about unprepare+disable at shutdown/remove
commit: ec7629859331fb67dbfb6bcd47f887a402e390ff
[08/48] drm/panel: innolux-p079zca: Stop tracking prepared/enabled
commit: f9055051292442d52092f17e191cf0a58d23d4ed
[09/48] drm/panel: innolux-p079zca: Don't call unprepare+disable at
shutdown/remove
commit: eeb133ff78476eb1e6e88154dfb75a741e8a034a

[12/48] drm/panel: kingdisplay-kd097d04: Stop tracking prepared/enabled
commit: 157c1381780a453e06430f8b35bb8c5d439eb8c6
[13/48] drm/panel: kingdisplay-kd097d04: Don't call unprepare+disable
at shutdown/remove
commit: 68c205ef3c39edce4a3346b8a53fd2b700394a0c
[14/48] drm/panel: ltk050h3146w: Stop tracking prepared
commit: f124478dd18c519544489caddce78e7c5796a758
[15/48] drm/panel: ltk050h3146w: Don't call unprepare+disable at shutdown/remove
commit: b7ca446ecb53205944968617b158f073bcacaedc
[16/48] drm/panel: ltk500hd1829: Stop tracking prepared
commit: 2b8c19b9d7bc9d03e8c44bd391d21e95c07a2c83
[17/48] drm/panel: ltk500hd1829: Don't call unprepare+disable at shutdown/remove
commit: 3357f6f465e62c0bc5e906365063734740c9f6d4
[18/48] drm/panel: novatek-nt36672a: Stop tracking prepared
commit: b605f257f386b7f4b6fc9c0f82b86b75d0579287
[19/48] drm/panel: novatek-nt36672a: Don't call unprepare+disable at
shutdown/remove
commit: 2a9487b5aa55753993fde80e4841128c8da4df71

[24/48] drm/panel: samsung-atna33xc20: Stop tracking prepared/enabled
commit: 5a847750aac8454a1604070ab99d689c0a6e4290
[25/48] drm/panel: samsung-atna33xc20: Don't call unprepare+disable at
shutdown/remove
commit: 49869668ff0e3f380858b4c20b8d0cb02b933f48
[26/48] drm/panel: simple: St

Re: [PATCH] drm/panel-edp: Add panel CSOT MNB601LS1-1

[Doug Anderson]

Hi,

On Mon, May 6, 2024 at 8:54 AM Doug Anderson  wrote:
>
> Hi,
>
> On Tue, Apr 23, 2024 at 6:55 PM Xuxin Xiong
>  wrote:
> >
> > Hi Doug, thank you!
> > We had reported this info to the CSOT to correct the vendor id.
> > If they confirm to fix this with the same product ID, we will submit a
> > patch to fix this.
>
> FYI, "top posting" like this is generally frowned upon on kernel
> mailing lists. One such reference about this is [1]. Some folks are
> very passionate about this topic, so please keep it in mind to avoid
> upsetting people in the community.
>
> In any case: did you get any response from CSOT about the improper EDID?

Just following up here. Was there any response from CSOT?

-Doug

Re: [PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Doug Anderson]

Hi,

On Wed, May 22, 2024 at 3:07 PM Dmitry Baryshkov
 wrote:
>
> Add a fat warning against adding new panel compatibles to the panel-edp
> driver. All new users of the eDP panels are supposed to use the generic
> "edp-panel" compatible device on the AUX bus. The remaining compatibles
> are either used by the existing DT or were used previously and are
> retained for backwards compatibility.
>
> Suggested-by: Doug Anderson 
> Signed-off-by: Dmitry Baryshkov 
> ---
> The following compatibles were never used by the devices supported by
> the upstream kernel and are a subject to possible removal:
>
> - auo,b133han05

Ack. Looks like this was added by Bjorn but by the time the dts for
the board (from context, sc8180x-primus) using it made it upstream it
was using "edp-panel".


> - auo,b140han06

Ack. Same as above, but this time the board was "sc8180x-lenovo-flex-5g".


> - ivo,m133nwf4-r0

Ack. Also added by Bjorn but not easy to tell from context which board
it was from. "m133nwf4" never shows up in the history of arm64 qcom
dts files.


> - lg,lp097qx1-spa1

Maybe. Added by Yakir at Rockchip. I don't think this was ChromeOS
related so I don't have any inside knowledge. Presumably this is for
some other hardware they were using. Probably worth CCing Rockchip
mailing list. It's entirely possible that they have downstream dts
files using this and there's no requirement to upstream dts files that
I'm aware of.


> - lg,lp129qe

NAK. See "arch/arm/boot/dts/nvidia/tegra124-venice2.dts"


> - samsung,lsn122dl01-c01

Maybe. Also by Yakir at Rockchip and also doesn't appear to be
ChromeOS, so you should ask them.


> - samsung,ltn140at29-301

NAK. arch/arm/boot/dts/nvidia/tegra124-nyan-blaze.dts


> - sharp,ld-d5116z01b

Added by Jeffrey Hugo. Maybe Cc him to make sure it's OK to delete?


> - sharp,lq140m1jw46

Ack. Feel free to delete. This was used in the sc7280 reference board
(hoglin/zoglin) and by the time the dts made it upstream it was
already using generic edp-panel.


> - starry,kr122ea0sra

Ack. From my previous notes: "starry,kr122ea0sra - rk3399-gru-gru
(reference board, not upstream)". Nobody needs this.


> I'm considering dropping them, unless there is a good reason not to do
> so.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c 
> b/drivers/gpu/drm/panel/panel-edp.c
> index 6db277efcbb7..95b25ec67168 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = 
> {
> {
> /* Must be first */
> .compatible = "edp-panel",
> -   }, {
> +   },
> +   /*
> +* Do not add panels to the list below unless they cannot be handled 
> by
> +* the generic edp-panel compatible.
> +*
> +* The only two valid reasons are:
> +* - because of the panel issues (e.g. broken EDID or broken
> +*   identification),
> +* - because the platform which uses the panel didn't wire up the AUX
> +*   bus properly.

You mean that they physically didn't wire up the AUX bus? I don't
think that's actually possible. I don't believe you can use an eDP
panel without this working. Conceivably a reason to add here is if
there's some old platform that hasn't coded up support for AUX bus.
...but it would be much preferred to update the platform driver to
support it.

-Doug

Re: [PATCH] drm/panel-edp: Add CMN N116BCJ-EAK

[Doug Anderson]

Hi,

On Wed, May 22, 2024 at 4:39 AM Haikun Zhou
 wrote:
>
> Add support for the CMN N116BCJ-EAK, place the raw EDID here for
> subsequent reference.
> 00 ff ff ff ff ff ff 00 0d ae 60 11 00 00 00 00
> 04 22 01 04 95 1a 0e 78 02 67 75 98 59 53 90 27
> 1c 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 da 1d 56 e2 50 00 20 30 30 20
> a6 00 00 90 10 00 00 18 00 00 00 fe 00 4e 31 31
> 36 42 43 4a 2d 45 41 4b 0a 20 00 00 00 fe 00 43
> 4d 4e 0a 20 20 20 20 20 20 20 20 20 00 00 00 fe
> 00 4e 31 31 36 42 43 4a 2d 45 41 4b 0a 20 00 98
>
> Signed-off-by: Haikun Zhou 
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson 

...pushing to drm-misc-next...

Re: [PATCH] drm/msm: remove python 3.9 dependency for compiling msm

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 4:05 PM Abhinav Kumar  wrote:
>
> Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
> compilation is broken on machines having python versions older than 3.9
> due to dependency on argparse.BooleanOptionalAction.
>
> Switch to use simple bool for the validate flag to remove the dependency.
>
> Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

FWIW, it looks like the commit this is fixing is now present in
Linus's tree. Is there any plan to land this fix? It would be nifty if
it could somehow make it in time for -rc1 so I don't need to track
down this patch every time I need to build a subsystem tree for the
next several weeks...

-Doug

Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid

[Doug Anderson]

Hi,

On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov
 wrote:
>
> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
> > Prefer the struct drm_edid based functions for reading the EDID and
> > updating the connector.
> >
> > Simplify the flow by updating the EDID property when the EDID is read
> > instead of at .get_modes.
> >
> > Signed-off-by: Jani Nikula 
> >
> > ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov 
>
> What is the merge strategy for the series? Do you plan to pick up all
> the patches to drm-misc or should we pick up individual patches into
> driver trees?

I'm not sure I have too much to add here aside from what you guys have
already talked about. I'm OK with:

Reviewed-by: Douglas Anderson 

Re: [v7 3/7] arm64: defconfig: Enable HIMAX_HX83102 panel

[Doug Anderson]

Hi,

On Thu, May 16, 2024 at 6:43 AM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, May 15, 2024 at 11:55 PM  wrote:
> >
> > On 16/05/2024 08:43, cong yang wrote:
> > > Hi:
> > >
> > > If it is determined that a separately patch needs to be sent, then I
> > > will remove this patch in V8 series?
> > >
> > > Doug Anderson  于2024年5月16日周四 05:28写道：
> > >
> > >>
> > >> Hi,
> > >>
> > >> On Wed, May 15, 2024 at 2:16 PM  wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> On 15/05/2024 03:46, Cong Yang wrote:
> > >>>> DRM_PANEL_HIMAX_HX83102 is being split out from 
> > >>>> DRM_PANEL_BOE_TV101WUM_NL6.
> > >>>> Since the arm64 defconfig had the BOE panel driver enabled, let's also
> > >>>> enable the himax driver.
> > >>>>
> > >>>> Signed-off-by: Cong Yang 
> > >>>> Reviewed-by: Douglas Anderson 
> > >>>> ---
> > >>>>arch/arm64/configs/defconfig | 1 +
> > >>>>1 file changed, 1 insertion(+)
> > >>>>
> > >>>> diff --git a/arch/arm64/configs/defconfig 
> > >>>> b/arch/arm64/configs/defconfig
> > >>>> index 2c30d617e180..687c86ddaece 100644
> > >>>> --- a/arch/arm64/configs/defconfig
> > >>>> +++ b/arch/arm64/configs/defconfig
> > >>>> @@ -864,6 +864,7 @@ CONFIG_DRM_PANEL_BOE_TV101WUM_NL6=m
> > >>>>CONFIG_DRM_PANEL_LVDS=m
> > >>>>CONFIG_DRM_PANEL_SIMPLE=m
> > >>>>CONFIG_DRM_PANEL_EDP=m
> > >>>> +CONFIG_DRM_PANEL_HIMAX_HX83102=m
> > >>>>CONFIG_DRM_PANEL_ILITEK_ILI9882T=m
> > >>>>CONFIG_DRM_PANEL_MANTIX_MLAF057WE51=m
> > >>>>CONFIG_DRM_PANEL_RAYDIUM_RM67191=m
> > >>>
> > >>> You should probably sent this one separately since only an ARM SoC 
> > >>> maintainer
> > >>> can apply this, probably via the qcom tree.
> > >>
> > >> Really? I always kinda figured that this was a bit like MAINTAINERS
> > >> where it can come through a bunch of different trees. Certainly I've
> > >> landed changes to it before through the drm-misc tree. If that was
> > >> wrong then I'll certainly stop doing it, of course.
> >
> > Yeah we usually don't mess with arch specific defconfig from drm tree
>
> In general I agree that makes sense. In this case, though, the new
> config symbol was introduced in the previous patch and split off an
> existing symbol. Updating "all" of the configs (AKA just arm64) that
> had the old symbol to also have the new symbol seems like the nice
> thing to do and it feels like it makes sense to land in the same tree
> that did the "split" just to cause the least confusion to anyone
> affected.
>
> In any case, if it's going to land in some other tree then I guess the
> question is whether it needs to wait a few revisions to land there or
> if it should land right away. Nobody would get a compile error if it
> landed in a different tree right away since unknown config symbols are
> silently ignored, but it feels a little weird to me.
>
> ...of course, I'm also OK just dropping the config patch. I personally
> don't use the upstream "defconfig". It just seemed courteous to update
> it for those who do.

Hmmm, probably should have put Arnd on this thread. Added now in case
he has any opinions. I also did manage to find when this last came up
where I was involved. At that time Will Deacon (who get_maintainer.pl
reports is the official maintainer of this file) said [1]:

> But yes, although there are a few things I really care about
> in defconfig (e.g. things like page size!), generally speaking we don't
> need to Ack everything that changes in there.

[1] 
https://lore.kernel.org/linux-arm-kernel/20201112004130.17290-1-diand...@chromium.org/T/

Re: [v7 3/7] arm64: defconfig: Enable HIMAX_HX83102 panel

[Doug Anderson]

Hi,

On Wed, May 15, 2024 at 11:55 PM  wrote:
>
> On 16/05/2024 08:43, cong yang wrote:
> > Hi:
> >
> > If it is determined that a separately patch needs to be sent, then I
> > will remove this patch in V8 series?
> >
> > Doug Anderson  于2024年5月16日周四 05:28写道：
> >
> >>
> >> Hi,
> >>
> >> On Wed, May 15, 2024 at 2:16 PM  wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 15/05/2024 03:46, Cong Yang wrote:
> >>>> DRM_PANEL_HIMAX_HX83102 is being split out from 
> >>>> DRM_PANEL_BOE_TV101WUM_NL6.
> >>>> Since the arm64 defconfig had the BOE panel driver enabled, let's also
> >>>> enable the himax driver.
> >>>>
> >>>> Signed-off-by: Cong Yang 
> >>>> Reviewed-by: Douglas Anderson 
> >>>> ---
> >>>>arch/arm64/configs/defconfig | 1 +
> >>>>1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> >>>> index 2c30d617e180..687c86ddaece 100644
> >>>> --- a/arch/arm64/configs/defconfig
> >>>> +++ b/arch/arm64/configs/defconfig
> >>>> @@ -864,6 +864,7 @@ CONFIG_DRM_PANEL_BOE_TV101WUM_NL6=m
> >>>>CONFIG_DRM_PANEL_LVDS=m
> >>>>CONFIG_DRM_PANEL_SIMPLE=m
> >>>>CONFIG_DRM_PANEL_EDP=m
> >>>> +CONFIG_DRM_PANEL_HIMAX_HX83102=m
> >>>>CONFIG_DRM_PANEL_ILITEK_ILI9882T=m
> >>>>CONFIG_DRM_PANEL_MANTIX_MLAF057WE51=m
> >>>>CONFIG_DRM_PANEL_RAYDIUM_RM67191=m
> >>>
> >>> You should probably sent this one separately since only an ARM SoC 
> >>> maintainer
> >>> can apply this, probably via the qcom tree.
> >>
> >> Really? I always kinda figured that this was a bit like MAINTAINERS
> >> where it can come through a bunch of different trees. Certainly I've
> >> landed changes to it before through the drm-misc tree. If that was
> >> wrong then I'll certainly stop doing it, of course.
>
> Yeah we usually don't mess with arch specific defconfig from drm tree

In general I agree that makes sense. In this case, though, the new
config symbol was introduced in the previous patch and split off an
existing symbol. Updating "all" of the configs (AKA just arm64) that
had the old symbol to also have the new symbol seems like the nice
thing to do and it feels like it makes sense to land in the same tree
that did the "split" just to cause the least confusion to anyone
affected.

In any case, if it's going to land in some other tree then I guess the
question is whether it needs to wait a few revisions to land there or
if it should land right away. Nobody would get a compile error if it
landed in a different tree right away since unknown config symbols are
silently ignored, but it feels a little weird to me.

...of course, I'm also OK just dropping the config patch. I personally
don't use the upstream "defconfig". It just seemed courteous to update
it for those who do.

-Doug

Re: [PATCH v8 0/6] Break out as separate driver and add BOE nv110wum-l60 IVO t109nw41 MIPI-DSI panel

[Doug Anderson]

Hi,

On Thu, May 16, 2024 at 12:21 AM Cong Yang
 wrote:
>
> Discussion with Doug and Linus in V1, we need a
> separate driver to enable the hx83102 controller.
>
> So this series this series mainly Break out as separate driver
> for Starry-himax83102-j02 panels from boe tv101wum driver.
>
> Then add BOE nv110wum-l60 and IVO t109nw41 in himax-hx83102 driver.
>
> Add compatible for BOE nv110wum-l60 and IVO t109nw41
> in dt-bindings
>
> Note:this series depend Dous'series [1]
> [1]: 
> https://lore.kernel.org/all/20240501154251.3302887-1-diand...@chromium.org/
>
> Changes in v8:
> - Neil think need sent separately to ARM SoC maintainer with "arm64: 
> defconfig: Enable HIMAX_HX83102 panel patch ", so remove it.
> - PATCH 1/6: No change.
> - PATCH 2/6: Fix Doug comment "return ret" change to "goto poweroff".
> - PATCH 3/6: No change.
> - PATCH 4/6: No change.
> - PATCH 5/6: No change.
> - PATCH 6/6: No change.
> - Link to 
> v7:https://lore.kernel.org/all/20240515014643.2715010-1-yangco...@huaqin.corp-partner.google.com/

This all looks good to me now.

Neil: do you want to apply this series plus Dmitry's [1] atop it, or
would you like me to? Dmitry's series has a fix in it but I don't
think it's critical enough to warrant the merge conflict that would
come with putting it through drm-misc-fixes.

[1] 
https://lore.kernel.org/r/20240512-dsi-panels-upd-api-v2-0-e31ca14d1...@linaro.org/

Re: [v7 3/7] arm64: defconfig: Enable HIMAX_HX83102 panel

[Doug Anderson]

Hi,

On Wed, May 15, 2024 at 2:16 PM  wrote:
>
> Hi,
>
> On 15/05/2024 03:46, Cong Yang wrote:
> > DRM_PANEL_HIMAX_HX83102 is being split out from DRM_PANEL_BOE_TV101WUM_NL6.
> > Since the arm64 defconfig had the BOE panel driver enabled, let's also
> > enable the himax driver.
> >
> > Signed-off-by: Cong Yang 
> > Reviewed-by: Douglas Anderson 
> > ---
> >   arch/arm64/configs/defconfig | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 2c30d617e180..687c86ddaece 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -864,6 +864,7 @@ CONFIG_DRM_PANEL_BOE_TV101WUM_NL6=m
> >   CONFIG_DRM_PANEL_LVDS=m
> >   CONFIG_DRM_PANEL_SIMPLE=m
> >   CONFIG_DRM_PANEL_EDP=m
> > +CONFIG_DRM_PANEL_HIMAX_HX83102=m
> >   CONFIG_DRM_PANEL_ILITEK_ILI9882T=m
> >   CONFIG_DRM_PANEL_MANTIX_MLAF057WE51=m
> >   CONFIG_DRM_PANEL_RAYDIUM_RM67191=m
>
> You should probably sent this one separately since only an ARM SoC maintainer
> can apply this, probably via the qcom tree.

Really? I always kinda figured that this was a bit like MAINTAINERS
where it can come through a bunch of different trees. Certainly I've
landed changes to it before through the drm-misc tree. If that was
wrong then I'll certainly stop doing it, of course.

-Doug

Re: [v7 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, May 14, 2024 at 6:47 PM Cong Yang
 wrote:
>
> +static int hx83102_prepare(struct drm_panel *panel)
> +{
> +   struct hx83102 *ctx = panel_to_hx83102(panel);
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = >dev;
> +   int ret;
> +
> +   gpiod_set_value(ctx->enable_gpio, 0);
> +   usleep_range(1000, 1500);
> +
> +   ret = regulator_enable(ctx->pp1800);
> +   if (ret < 0)
> +   return ret;
> +
> +   usleep_range(3000, 5000);
> +
> +   ret = regulator_enable(ctx->avdd);
> +   if (ret < 0)
> +   goto poweroff1v8;
> +   ret = regulator_enable(ctx->avee);
> +   if (ret < 0)
> +   goto poweroffavdd;
> +
> +   usleep_range(1, 11000);
> +
> +   mipi_dsi_dcs_nop(ctx->dsi);
> +   usleep_range(1000, 2000);
> +
> +   gpiod_set_value(ctx->enable_gpio, 1);
> +   usleep_range(1000, 2000);
> +   gpiod_set_value(ctx->enable_gpio, 0);
> +   usleep_range(1000, 2000);
> +   gpiod_set_value(ctx->enable_gpio, 1);
> +   usleep_range(6000, 1);
> +
> +   ret = ctx->desc->init(ctx);
> +   if (ret < 0)
> +   goto poweroff;
> +
> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> +   return ret;
> +   }

The above "return ret" should be "goto poweroff". Please send a v8.

-Doug

Re: [PATCH v2 2/7] drm/mipi-dsi: wrap more functions for streamline handling

[Doug Anderson]

Hi,

On Sat, May 11, 2024 at 4:00 PM Dmitry Baryshkov
 wrote:
>
> Follow the pattern of mipi_dsi_dcs_*_multi() and wrap several existing
> MIPI DSI functions to use the context for processing. This simplifies
> and streamlines driver code to use simpler code pattern.
>
> Note, msleep function is also wrapped in this way as it is frequently
> called inbetween other mipi_dsi_dcs_*() functions.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 210 
> +
>  include/drm/drm_mipi_dsi.h |  21 +
>  2 files changed, 231 insertions(+)

Reviewed-by: Douglas Anderson 

Re: [PATCH v6 5/7] drm/panel: himax-hx83102: Support for BOE nv110wum-l60 MIPI-DSI panel

[Doug Anderson]

Hi,

On Fri, May 10, 2024 at 7:13 PM Cong Yang
 wrote:
>
> The BOE nv110wum-l60 is a 11.0" WUXGA TFT LCD panel, use hx83102 controller
> which fits in nicely with the existing panel-himax-hx83102 driver. Hence,
> we add a new compatible with panel specific config.
>
> Signed-off-by: Cong Yang 
> ---
> Chage since V6:
>
> - No change.
>
> V5: 
> https://lore.kernel.org/all/20240509015207.3271370-6-yangco...@huaqin.corp-partner.google.com
>
> Chage since V5:
>
> - Adjust inital cmds indentation and check accum_err before calling mdelay in 
> init()..
>
> V4: 
> https://lore.kernel.org/all/20240507135234.1356855-6-yangco...@huaqin.corp-partner.google.com
>
> Chage since V4:
>
> - Depend Dous'series [1].
> [1]: 
> https://lore.kernel.org/all/20240501154251.3302887-1-diand...@chromium.org
>
> V3: 
> https://lore.kernel.org/all/20240424023010.2099949-6-yangco...@huaqin.corp-partner.google.com
>
> Chage since V3:
>
> - inital cmds use lowercasehex.
>
> V2: 
> https://lore.kernel.org/all/20240422090310.3311429-6-yangco...@huaqin.corp-partner.google.com
>
> ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c | 133 
>  1 file changed, 133 insertions(+)

Reviewed-by: Douglas Anderson 

Re: [PATCH v6 7/7] drm/panel: himax-hx83102: Support for IVO t109nw41 MIPI-DSI panel

[Doug Anderson]

Hi,

On Fri, May 10, 2024 at 7:14 PM Cong Yang
 wrote:
>
> The IVO t109nw41 is a 11.0" WUXGA TFT LCD panel, use hx83102 controller
> which fits in nicely with the existing panel-himax-hx83102 driver. Hence,
> we add a new compatible with panel specific config.
>
> Signed-off-by: Cong Yang 
> ---
> Chage since V6:
>
> - Add hx83102_enable_extended_cmds(_ctx, false) at end of inital cmds.
>
> V5: 
> https://lore.kernel.org/all/20240509015207.3271370-8-yangco...@huaqin.corp-partner.google.com
>
> Chage since V5:
>
> - Adjust inital cmds indentation and check accum_err before calling mdelay in 
> init().
> - Adjust somes inital cmds to Optimize gamma.
>
> V4: 
> https://lore.kernel.org/all/20240507135234.1356855-8-yangco...@huaqin.corp-partner.google.com
>
> Chage since V4:
>
> - inital cmds use lowercasehex.
>
> V3: 
> https://lore.kernel.org/all/20240424023010.2099949-8-yangco...@huaqin.corp-partner.google.com
>
> Chage since V3:
>
> - Depend Dous'series [1].
> [1]: 
> https://lore.kernel.org/all/20240501154251.3302887-1-diand...@chromium.org
>
> V2: 
> https://lore.kernel.org/all/20240422090310.3311429-8-yangco...@huaqin.corp-partner.google.com
>
> ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c | 131 
>  1 file changed, 131 insertions(+)

Reviewed-by: Douglas Anderson 

Re: [PATCH v6 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Fri, May 10, 2024 at 7:13 PM Cong Yang
 wrote:
>
> +static int hx83102_prepare(struct drm_panel *panel)
> +{
> +   struct hx83102 *ctx = panel_to_hx83102(panel);
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = >dev;
> +   int ret;
> +
> +   gpiod_set_value(ctx->enable_gpio, 0);
> +   usleep_range(1000, 1500);
> +
> +   ret = regulator_enable(ctx->pp1800);
> +   if (ret < 0)
> +   return ret;
> +
> +   usleep_range(3000, 5000);
> +
> +   ret = regulator_enable(ctx->avdd);
> +   if (ret < 0)
> +   goto poweroff1v8;
> +   ret = regulator_enable(ctx->avee);
> +   if (ret < 0)
> +   goto poweroffavdd;
> +
> +   usleep_range(1, 11000);
> +
> +   mipi_dsi_dcs_nop(ctx->dsi);
> +   usleep_range(1000, 2000);
> +
> +   gpiod_set_value(ctx->enable_gpio, 1);
> +   usleep_range(1000, 2000);
> +   gpiod_set_value(ctx->enable_gpio, 0);
> +   usleep_range(1000, 2000);
> +   gpiod_set_value(ctx->enable_gpio, 1);
> +   usleep_range(6000, 1);
> +
> +   ret = ctx->desc->init(ctx);
> +   if (ret < 0)
> +   goto poweroff;
> +
> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> +   return ret;
> +   }

The above should have been "goto poweroff", not "return ret".


> +   msleep(120);
> +
> +   ret = mipi_dsi_dcs_set_display_on(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to turn on the display: %d\n", ret);
> +   return ret;
> +   }

The above should have been "goto poweroff", not "return ret".


Other than that:

Reviewed-by: Douglas Anderson 

Re: [PATCH v4 4/9] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Mon, May 13, 2024 at 2:30 AM Maxime Ripard  wrote:
>
> Hi,
>
> On Wed, May 08, 2024 at 01:51:46PM -0700, Douglas Anderson wrote:
> > Through a cooperative effort between Hsin-Yi Wang and Dmitry
> > Baryshkov, we have realized the dev_err() in the
> > mipi_dsi_*_write_seq() macros was causing quite a bit of bloat to the
> > kernel. Let's hoist this call into drm_mipi_dsi.c by adding a "chatty"
> > version of the functions that includes the print. While doing this,
> > add a bit more comments to these macros making it clear that they
> > print errors and also that they return out of _the caller's_ function.
> >
> > Without any changes to clients this gives a nice savings. Specifically
> > the macro was inlined and thus the error report call was inlined into
> > every call to mipi_dsi_dcs_write_seq() and
> > mipi_dsi_generic_write_seq(). By using a call to a "chatty" function,
> > the usage is reduced to one call in the chatty function and a function
> > call at the invoking site.
> >
> > Building with my build system shows one example:
> >
> > $ scripts/bloat-o-meter \
> >   .../before/panel-novatek-nt36672e.ko \
> >   .../after/panel-novatek-nt36672e.ko
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-4404 (-4404)
> > Function old new   delta
> > nt36672e_1080x2408_60hz_init   106406236   -4404
> > Total: Before=15055, After=10651, chg -29.25%
> >
> > Note that given the change in location of the print it's harder to
> > include the "cmd" in the printout for mipi_dsi_dcs_write_seq() since,
> > theoretically, someone could call the new chatty function with a
> > zero-size array and it would be illegal to dereference data[0].
> > There's a printk format to print the whole buffer and this is probably
> > more useful for debugging anyway. Given that we're doing this for
> > mipi_dsi_dcs_write_seq(), let's also print the buffer for
> > mipi_dsi_generic_write_seq() in the error case.
> >
> > It should be noted that the current consensus of DRM folks is that the
> > mipi_dsi_*_write_seq() should be deprecated due to the non-intuitive
> > return behavior. A future patch will formally mark them as deprecated
> > and provide an alternative.
> >
> > Reviewed-by: Dmitry Baryshkov 
> > Reviewed-by: Neil Armstrong 
> > Reviewed-by: Linus Walleij 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v4:
> > - Update wording as per Linus W.
> >
> > Changes in v3:
> > - Rebased upon patch to remove ratelimit of prints.
> >
> > Changes in v2:
> > - Add some comments to the macros about printing and returning.
> > - Change the way err value is handled in prep for next patch.
> > - Modify commit message now that this is part of a series.
> > - Rebased upon patches to avoid theoretical int overflow.
> >
> >  drivers/gpu/drm/drm_mipi_dsi.c | 56 ++
> >  include/drm/drm_mipi_dsi.h | 47 +++-
> >  2 files changed, 82 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 795001bb7ff1..8593d9ed5891 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -764,6 +764,34 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device 
> > *dsi, const void *payload,
> >  }
> >  EXPORT_SYMBOL(mipi_dsi_generic_write);
> >
> > +/**
> > + * mipi_dsi_generic_write_chatty() - mipi_dsi_generic_write() w/ an error 
> > log
> > + * @dsi: DSI peripheral device
> > + * @payload: buffer containing the payload
> > + * @size: size of payload buffer
> > + *
> > + * Like mipi_dsi_generic_write() but includes a dev_err_ratelimited()
>
> You mention in both functions that it's calling dev_err_ratelimited() ...
>
> > + * call for you and returns 0 upon success, not the number of bytes sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
> > +   const void *payload, size_t size)
> > +{
> > + struct device *dev = >dev;
> > + ssize_t ret;
> > +
> > + ret = mipi_dsi_generic_write(dsi, payload, size);
> > + if (ret < 0) {
> > + dev_err(dev, "sending generic data %*ph failed: %zd\n",
> > + (int)size, payload, ret);
>
> ... but it doesn't.

Whoops, thanks for catching this! I'll plan to send a v5 tomorrow to
fix this and then I'll still plan to land the series on Thursday
unless anything major comes up.

-Doug

Re: [PATCH RFC 6/7] drm/panel: lg-sw43408: add missing error handling

[Doug Anderson]

Hi,

On Fri, May 10, 2024 at 3:25 PM Dmitry Baryshkov
 wrote:
>
> On Fri, May 10, 2024 at 02:47:05PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
> >  wrote:
> > >
> > > Add missing error handling for the mipi_dsi_ functions that actually
> > > return error code instead of silently ignoring it.
> > >
> > > Fixes: 069a6c0e94f9 ("drm: panel: Add LG sw43408 panel driver")
> > > Signed-off-by: Dmitry Baryshkov 
> > > ---
> > >  drivers/gpu/drm/panel/panel-lg-sw43408.c | 33 
> > > ++--
> > >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > Looks right to me. Only slight nit would be that I'd put this as the
> > first patch in the series to make it obvious to anyone backporting it
> > to older kernels that it doesn't have any dependencies on the earlier
> > patches in the series. It's fairly obvious so this isn't a huge deal,
> > but still could be nice.
>
> Yes. I wanted to emphasise the _multi stuff rather than this fix. I'll
> reorder patches for v2. Maybe I should also rebase the series on top of
> patches by Cong Yang. WDYT?

Sounds good. I think Cong is planning on a V6 to fix the last nit I
had on his patch series [1] but otherwise this plan sounds fine to me.

[1] 
https://lore.kernel.org/r/cahwb_nktw0aymgfb4rmfngr4wf10o9_0pyvbkpndm45ayma...@mail.gmail.com

-Doug

Re: [PATCH RFC 7/7] drm/panel: lg-sw43408: use new streamlined MIPI DSI API

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> Use newer mipi_dsi_*_multi() functions in order to simplify and cleanup
> panel's prepare() and unprepare() functions.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-lg-sw43408.c | 95 
> +---
>  1 file changed, 37 insertions(+), 58 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 6/7] drm/panel: lg-sw43408: add missing error handling

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> Add missing error handling for the mipi_dsi_ functions that actually
> return error code instead of silently ignoring it.
>
> Fixes: 069a6c0e94f9 ("drm: panel: Add LG sw43408 panel driver")
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-lg-sw43408.c | 33 
> ++--
>  1 file changed, 27 insertions(+), 6 deletions(-)

Looks right to me. Only slight nit would be that I'd put this as the
first patch in the series to make it obvious to anyone backporting it
to older kernels that it doesn't have any dependencies on the earlier
patches in the series. It's fairly obvious so this isn't a huge deal,
but still could be nice.

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 5/7] drm/panel: novatek-nt36672e: use wrapped MIPI DCS functions

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> Remove conditional code and always use mipi_dsi_dcs_*multi() wrappers to
> simplify driver's init/exit code. This also includes passing context to
> the init_sequence() function instead of passing the DSI device.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-novatek-nt36672e.c | 587 
> -
>  1 file changed, 281 insertions(+), 306 deletions(-)

One note is that we might not be able to land ${SUBJECT} patch since
the patch it's based on [1] doesn't have any Reviewed-by tags. Just
sayin'. ;-)

[1] 
https://lore.kernel.org/r/20240508135148.v4.6.I3c08a7d02c467d2bc88da14e513ea4c8649fce45@changeid


> @@ -381,61 +377,40 @@ static int nt36672e_power_off(struct nt36672e_panel 
> *ctx)
> return ret;
>  }
>
> -static int nt36672e_on(struct nt36672e_panel *ctx)
> +static int nt36672e_on(struct nt36672e_panel *panel)

Small nit that I think of the variable "panel" as referring to a
"struct drm_panel". I'd personally rather this be named something
else, like "nt36672e".


>  {
> -   struct mipi_dsi_device *dsi = ctx->dsi;
> -   const struct panel_desc *desc = ctx->desc;
> -   int ret = 0;
> +   struct mipi_dsi_multi_context ctx = { .dsi = panel->dsi };
> +   struct mipi_dsi_device *dsi = panel->dsi;
> +   const struct panel_desc *desc = panel->desc;
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> -   if (desc->init_sequence) {
> -   ret = desc->init_sequence(dsi);
> -   if (ret < 0) {
> -   dev_err(>dev, "panel init sequence failed: 
> %d\n", ret);
> -   return ret;
> -   }
> -   }
> +   if (desc->init_sequence)
> +   desc->init_sequence();
>
> -   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> -   if (ret < 0) {
> -   dev_err(>dev, "Failed to exit sleep mode: %d\n", ret);
> -   return ret;
> -   }
> -   msleep(120);
> +   mipi_dsi_dcs_exit_sleep_mode_multi();
> +   mipi_dsi_msleep(, 120);
>
> -   ret = mipi_dsi_dcs_set_display_on(dsi);
> -   if (ret < 0) {
> -   dev_err(>dev, "Failed to set display on: %d\n", ret);
> -   return ret;
> -   }
> -   msleep(100);
> +   mipi_dsi_dcs_set_display_on_multi();
>
> -   return 0;
> +   mipi_dsi_msleep(, 100);
> +
> +   return ctx.accum_err;
>  }
>
> -static int nt36672e_off(struct nt36672e_panel *ctx)
> +static int nt36672e_off(struct nt36672e_panel *panel)
>  {
> -   struct mipi_dsi_device *dsi = ctx->dsi;
> -   int ret = 0;
> +   struct mipi_dsi_multi_context ctx = { .dsi = panel->dsi };
>
> -   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +   panel->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> -   ret = mipi_dsi_dcs_set_display_off(dsi);
> -   if (ret < 0) {
> -   dev_err(>dev, "Failed to set display off: %d\n", ret);
> -   return ret;
> -   }
> -   msleep(20);
> +   mipi_dsi_dcs_set_display_off_multi();
> +   mipi_dsi_msleep(, 20);
>
> -   ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> -   if (ret < 0) {
> -   dev_err(>dev, "Failed to enter sleep mode: %d\n", ret);
> -   return ret;
> -   }
> -   msleep(60);
> +   mipi_dsi_dcs_enter_sleep_mode_multi();
> +   mipi_dsi_msleep(, 60);
>
> -   return 0;
> +   return ctx.accum_err;
>  }

nit: similar to other patches in the series, the callers of
nt36672e_on() and nt36672e_off() should be able to get rid of their
error prints.

In any case:

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 4/7] drm/panel: innolux-p079zca: use mipi_dsi_dcs_nop_multi()

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> Remove conditional code and use mipi_dsi_dcs_nop_multi() wrapper to
> simplify driver code.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 3/7] drm/panel: ilitek-ili9882t: use wrapped MIPI DCS functions

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> @@ -424,20 +420,14 @@ static inline struct ili9882t *to_ili9882t(struct 
> drm_panel *panel)
>
>  static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
>  {
> -   struct mipi_dsi_device *dsi = ili->dsi;
> -   int ret;
> -
> -   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +   struct mipi_dsi_multi_context ctx = { .dsi = ili->dsi };
>
> -   ret = mipi_dsi_dcs_set_display_off(dsi);
> -   if (ret < 0)
> -   return ret;
> +   ili->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> -   ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> -   if (ret < 0)
> -   return ret;
> +   mipi_dsi_dcs_set_display_off_multi();
> +   mipi_dsi_dcs_enter_sleep_mode_multi();
>
> -   return 0;
> +   return ctx.accum_err;
>  }

nit: Same comments I had on patch #2 (boe-tv101wum-nl6) about inlining
this to the caller. Here it's even better since the caller already has
a multi_context...

In any case:

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 2/7] drm/panel: boe-tv101wum-nl6: use wrapped MIPI DCS functions

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> Remove conditional code and always use mipi_dsi_dcs_*multi() wrappers to
> simplify driver's init/exit code.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 73 
> --
>  1 file changed, 21 insertions(+), 52 deletions(-)

Please CC Cong Yang (CCed here) on your next version since he's also
touching this file and your changes will conflict with his.


> @@ -1508,20 +1483,14 @@ static inline struct boe_panel *to_boe_panel(struct 
> drm_panel *panel)
>
>  static int boe_panel_enter_sleep_mode(struct boe_panel *boe)
>  {
> -   struct mipi_dsi_device *dsi = boe->dsi;
> -   int ret;
> -
> -   dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +   struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
>
> -   ret = mipi_dsi_dcs_set_display_off(dsi);
> -   if (ret < 0)
> -   return ret;
> +   boe->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> -   ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> -   if (ret < 0)
> -   return ret;
> +   mipi_dsi_dcs_set_display_off_multi();
> +   mipi_dsi_dcs_enter_sleep_mode_multi();
>
> -   return 0;
> +   return ctx.accum_err;
>  }

nit: since the "multi" functions print errors for you, you can get rid
of the (now duplicate) error print in the caller. I guess if you
really wanted to you could actually just inline
boe_panel_enter_sleep_mode() into its one caller and that would make
it easy to use the "_multi" version of msleep() there...

In any case, I think this looks nice:

Reviewed-by: Douglas Anderson 

Re: [PATCH RFC 1/7] drm/mipi-dsi: wrap more functions for streamline handling

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov
 wrote:
>
> +/**
> + * mipi_dsi_compression_mode_ext() - enable/disable DSC on the peripheral
> + * @ctx: Context for multiple DSI transactions
> + * @enable: Whether to enable or disable the DSC
> + * @algo: Selected compression algorithm
> + * @pps_selector: Select PPS from the table of pre-stored or uploaded PPS 
> entries
> + *
> + * Like mipi_dsi_compression_mode_ext_multi() but deals with errors in a way 
> that
> + * makes it convenient to make several calls in a row.

Your comment is backward. The name of the function is
mipi_dsi_compression_mode_ext_multi() not
mipi_dsi_compression_mode_ext(). ...and it's like
mipi_dsi_compression_mode_ext() not like
mipi_dsi_compression_mode_ext_multi().


> @@ -338,6 +345,18 @@ int mipi_dsi_dcs_set_display_brightness_large(struct 
> mipi_dsi_device *dsi,
>  int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
>  u16 *brightness);
>
> +void mipi_dsi_dcs_nop_multi(struct mipi_dsi_multi_context *ctx);
> +void mipi_dsi_dcs_enter_sleep_mode_multi(struct mipi_dsi_multi_context *ctx);
> +void mipi_dsi_dcs_exit_sleep_mode_multi(struct mipi_dsi_multi_context *ctx);
> +void mipi_dsi_dcs_set_display_off_multi(struct mipi_dsi_multi_context *ctx);
> +void mipi_dsi_dcs_set_display_on_multi(struct mipi_dsi_multi_context *ctx);
> +void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
> +   enum mipi_dsi_dcs_tear_mode mode);
> +
> +#define mipi_dsi_msleep(ctx, delay)\
> +   if (!ctx.accum_err) \
> +   msleep(delay)   \

Please enclose the above in a "do { ... } while (0)" as typical for
macros. Otherwise you could possibly get some very surprising
behavior:

if (needs_big_delay)
  mipi_dsi_msleep(ctx, 50)
else
  mipi_dsi_msleep(ctx, 10)

...with your macro as it is I think the "else" will match up against
the "if !(ctx.accum_err)" inside the macro and not against the "if
(needs_big_delay)"

Also: nit that the mipi_dsi_msleep() should probably be defined above
the "mipi_dsi_dcs" section.


-Doug

Re: [PATCH v5 7/7] drm/panel: himax-hx83102: Support for IVO t109nw41 MIPI-DSI panel

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 11:05 PM cong yang
 wrote:
>
> Hi,
>
> Doug Anderson  于2024年5月10日周五 00:49写道：
> >
> > Hi,
> >
> > On Wed, May 8, 2024 at 6:53 PM Cong Yang
> >  wrote:
> > >
> > > +static int ivo_t109nw41_init(struct hx83102 *ctx)
> > > +{
> > > +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > > +
> > > +   msleep(60);
> > > +
> > > +   hx83102_enable_extended_cmds(_ctx, true);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPOWER, 0x2c, 
> > > 0xed, 0xed, 0x0f, 0xcf, 0x42,
> > > +0xf5, 0x39, 0x36, 0x36, 0x36, 0x36, 
> > > 0x32, 0x8b, 0x11, 0x65, 0x00, 0x88,
> > > +0xfa, 0xff, 0xff, 0x8f, 0xff, 0x08, 
> > > 0xd6, 0x33);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETDISP, 0x00, 
> > > 0x47, 0xb0, 0x80, 0x00, 0x12,
> > > +0x71, 0x3c, 0xa3, 0x22, 0x20, 0x00, 
> > > 0x00, 0x88, 0x01);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCYC, 0x35, 
> > > 0x35, 0x43, 0x43, 0x35, 0x35,
> > > +0x30, 0x7a, 0x30, 0x7a, 0x01, 0x9d);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcd);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETMIPI, 0x84);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETVDC, 0x1b, 
> > > 0x04);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_UNKNOWN_BE, 0x20);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPTBA, 0xfc, 
> > > 0xc4);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSTBA, 0x34, 
> > > 0x34, 0x22, 0x11, 0x22, 0xa0,
> > > +0x31, 0x08, 0xf5, 0x03);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcc);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTCON, 0x80);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xd3);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTCON, 0x22);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc6);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETRAMDMY, 0x97);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPWM, 0x00, 
> > > 0x1e, 0x13, 0x88, 0x01);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCLOCK, 0x08, 
> > > 0x13, 0x07, 0x00, 0x0f, 0x34);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPANEL, 0x02, 
> > > 0x03, 0x44);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc4);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCASCADE, 0x03);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPCTRL, 0x07, 
> > > 0x06, 0x00, 0x02, 0x04, 0x2c,
> > > +0xff);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP0, 0x06, 
> > > 0x00, 0x00, 0x00, 0x00, 0x08,
> > > +0x08, 0x08, 0x08, 0x37, 0x07, 0x64, 
> > > 0x7c, 0x11, 0x11, 0x03, 0x03, 0x32,
> > > +0x10, 0x0e, 0x00, 0x0e, 0x32, 0x17, 
> > > 0x97, 0x07, 0x97, 0x32, 0x00, 0x02,
> > > +0x00, 0x02, 0x00, 0x00);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP1, 0x25, 
> > > 0x24, 0x25, 0x24, 0x18, 0x18,
> > > +0x18, 0x18, 0x07, 0x06, 0x07, 0x06, 
> > > 0x05, 0x04, 0x05, 0x04, 0x03, 0x02,
> > > +0x03, 0x02, 0x01, 0x00, 0x01, 0x00, 
> > > 0x1e, 0x1e, 0x1e, 0x1e, 0x1f, 0x1f,
> > > +0x1f, 0x1f, 0x21, 0x20, 0x21, 0x20, 
> > > 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> > > +0x18, 0x18);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP3, 0xaa, 
> > > 0xaa, 0xaa, 0xaa, 0xaa, 0xa0,
> > > +0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 

Re: [PATCH v5 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Wed, May 8, 2024 at 6:53 PM Cong Yang
 wrote:
>
> +static int hx83102_enable(struct drm_panel *panel)
> +{
> +   struct hx83102 *ctx = panel_to_hx83102(panel);
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = >dev;
> +   int ret;
> +
> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> +   return ret;
> +   }
> +
> +   msleep(120);
> +
> +   ret = mipi_dsi_dcs_set_display_on(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to turn on the display: %d\n", ret);
> +   return ret;
> +   }

FWIW, I think that the mipi_dsi_dcs_exit_sleep_mode(), msleep(120),
and mipi_dsi_dcs_set_display_on() should also be in the prepare() to
match how they were in the boe-tv101wum-nl6.c driver, right? Then the
enable() would be left with just the simple "msleep(130)".

I know it doesn't make much difference and it probably doesn't matter
and maybe I'm just being a little nitpicky, but given that the
prepare() and enable() functions are unique phases I'd rather be
explicit if we've moving something from one phase to the other.


-Doug

Re: [PATCH v5 7/7] drm/panel: himax-hx83102: Support for IVO t109nw41 MIPI-DSI panel

[Doug Anderson]

Hi,

On Wed, May 8, 2024 at 6:53 PM Cong Yang
 wrote:
>
> +static int ivo_t109nw41_init(struct hx83102 *ctx)
> +{
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +   msleep(60);
> +
> +   hx83102_enable_extended_cmds(_ctx, true);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPOWER, 0x2c, 0xed, 
> 0xed, 0x0f, 0xcf, 0x42,
> +0xf5, 0x39, 0x36, 0x36, 0x36, 0x36, 
> 0x32, 0x8b, 0x11, 0x65, 0x00, 0x88,
> +0xfa, 0xff, 0xff, 0x8f, 0xff, 0x08, 
> 0xd6, 0x33);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETDISP, 0x00, 0x47, 
> 0xb0, 0x80, 0x00, 0x12,
> +0x71, 0x3c, 0xa3, 0x22, 0x20, 0x00, 
> 0x00, 0x88, 0x01);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCYC, 0x35, 0x35, 
> 0x43, 0x43, 0x35, 0x35,
> +0x30, 0x7a, 0x30, 0x7a, 0x01, 0x9d);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcd);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETMIPI, 0x84);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETVDC, 0x1b, 0x04);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_UNKNOWN_BE, 0x20);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPTBA, 0xfc, 0xc4);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSTBA, 0x34, 0x34, 
> 0x22, 0x11, 0x22, 0xa0,
> +0x31, 0x08, 0xf5, 0x03);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcc);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTCON, 0x80);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xd3);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTCON, 0x22);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc6);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETRAMDMY, 0x97);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPWM, 0x00, 0x1e, 
> 0x13, 0x88, 0x01);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCLOCK, 0x08, 0x13, 
> 0x07, 0x00, 0x0f, 0x34);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPANEL, 0x02, 0x03, 
> 0x44);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc4);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCASCADE, 0x03);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPCTRL, 0x07, 0x06, 
> 0x00, 0x02, 0x04, 0x2c,
> +0xff);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP0, 0x06, 0x00, 
> 0x00, 0x00, 0x00, 0x08,
> +0x08, 0x08, 0x08, 0x37, 0x07, 0x64, 
> 0x7c, 0x11, 0x11, 0x03, 0x03, 0x32,
> +0x10, 0x0e, 0x00, 0x0e, 0x32, 0x17, 
> 0x97, 0x07, 0x97, 0x32, 0x00, 0x02,
> +0x00, 0x02, 0x00, 0x00);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP1, 0x25, 0x24, 
> 0x25, 0x24, 0x18, 0x18,
> +0x18, 0x18, 0x07, 0x06, 0x07, 0x06, 
> 0x05, 0x04, 0x05, 0x04, 0x03, 0x02,
> +0x03, 0x02, 0x01, 0x00, 0x01, 0x00, 
> 0x1e, 0x1e, 0x1e, 0x1e, 0x1f, 0x1f,
> +0x1f, 0x1f, 0x21, 0x20, 0x21, 0x20, 
> 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +0x18, 0x18);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP3, 0xaa, 0xaa, 
> 0xaa, 0xaa, 0xaa, 0xa0,
> +0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xa0, 
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGMA, 0x04, 0x04, 
> 0x06, 0x0a, 0x0a, 0x05,
> +0x12, 0x14, 0x17, 0x13, 0x2c, 0x33, 
> 0x39, 0x4b, 0x4c, 0x56, 0x61, 0x78,
> +0x7a, 0x41, 0x50, 0x68, 0x73, 0x04, 
> 0x04, 0x06, 0x0a, 0x0a, 0x05, 0x12,
> +0x14, 0x17, 0x13, 0x2c, 0x33, 0x39, 
> 0x4b, 0x4c, 0x56, 0x61, 0x78, 0x7a,
> +0x41, 0x50, 0x68, 0x73);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTP1, 0x07, 0x10, 
> 0x10, 0x1a, 0x26, 0x9e,
> +0x00, 0x4f, 0xa0, 0x14, 0x14, 0x00, 
> 0x00, 0x00, 0x00, 0x12, 0x0a, 0x02,
> +0x02, 0x00, 0x33, 0x02, 0x04, 0x18, 
> 0x01);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETBANK, 0x01);
> +

Re: [PATCH v1 2/2] HID: i2c-hid: elan: Add ili2900 timing

[Doug Anderson]

Hi,

On Wed, May 8, 2024 at 11:43 PM Zhaoxiong Lv
 wrote:
>
> From: lvzhaoxiong 
>
> ILI2900 requires reset to pull down time greater than 10ms,
> so the configuration post_power_delay_ms is 10, and the chipset
> initial time is required to be greater than 100ms,
> so the post_gpio_reset_on_delay_ms is set to 100.
>
> Signed-off-by: lvzhaoxiong 
> ---
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 8 
>  1 file changed, 8 insertions(+)

You silently ignored pretty much all of the feedback from the previous
version [1], so I'm not planning to review this version.

[1] 
https://lore.kernel.org/r/CAD=FV=x5tk0tccda+vlnu0aoas1tdwuqvkmzm-278docx8k...@mail.gmail.com

Re: [PATCH v2 0/2] Add starry bindings and driver

[Doug Anderson]

Hi,

On Thu, May 9, 2024 at 1:35 AM Zhaoxiong Lv
 wrote:
>
> Add bindings and driver for starry.
> ---
> Modifications between V1 and V2:
> Kconfig and Makefile configurations added for starry driver
>
> ---
>
> Zhaoxiong Lv (2):
>   dt-bindings: display: panel: Add Starry-er88577 support
>   drm/panel: starry: add new panel driver
>
>  .../display/panel/starry,er88577.yaml |  59 +++
>  drivers/gpu/drm/panel/Kconfig |   9 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  drivers/gpu/drm/panel/panel-starry-er88577.c  | 444 ++
>  4 files changed, 513 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/starry,er88577.yaml
>  create mode 100644 drivers/gpu/drm/panel/panel-starry-er88577.c

I don't think anyone is going to look at this series since it's not
taking into account previous feedback. Please talk to Cong Yang (CCed)
who is also working at Huaquin and is also sending MIPI panel patches.
Hopefully he should be able to pre-review your patches with you so
that you can learn from what he learned. If for some reason you are
unable to work with Cong Yang then let me know and we can figure out
the next steps here.

-Doug

Re: [PATCH] drm/msm: remove python 3.9 dependency for compiling msm

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 4:05 PM Abhinav Kumar  wrote:
>
> Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
> compilation is broken on machines having python versions older than 3.9
> due to dependency on argparse.BooleanOptionalAction.
>
> Switch to use simple bool for the validate flag to remove the dependency.
>
> Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

No idea if we're supposed to allow python as a build dependency. That
being said, I can confirm that this fixes the problem for me since I
ran into it too [1].

Tested-by: Douglas Anderson 

[1] 
https://lore.kernel.org/r/CAD=FV=XnpS-=cookkxzfm8og9wcsemxfesmftyh811438qg...@mail.gmail.com

Re: [PATCH v2 1/2] drm/msm/gen_header: allow skipping the validation

[Doug Anderson]

Hi,

On Fri, May 3, 2024 at 11:15 AM Dmitry Baryshkov
 wrote:
>
> @@ -941,6 +948,7 @@ def main():
> parser = argparse.ArgumentParser()
> parser.add_argument('--rnn', type=str, required=True)
> parser.add_argument('--xml', type=str, required=True)
> +   parser.add_argument('--validate', 
> action=argparse.BooleanOptionalAction)

FWIW, the above (argparse.BooleanOptionalAction) appears to be a
python 3.9 thing. My own build environment happens to have python3
default to python 3.8 and thus I get a build error related to this. I
have no idea what the kernel usually assumes for a baseline, but
others might get build errors too. I don't even see python listed in:

https://docs.kernel.org/process/changes.html

...in any case, if it's easy to change this to not require python3.9
that would at least help for my build environment. :-P

-Doug

Re: [RFT PATCH v2 00/48] drm/panel: Remove most store/double-check of prepared/enabled state

[Doug Anderson]

Hi,

On Sun, May 5, 2024 at 11:52 PM Linus Walleij  wrote:
>
> On Fri, May 3, 2024 at 11:36 PM Douglas Anderson  
> wrote:
>
> > As talked about in commit d2aacaf07395 ("drm/panel: Check for already
> > prepared/enabled in drm_panel"), we want to remove needless code from
> > panel drivers that was storing and double-checking the
> > prepared/enabled state. Even if someone was relying on the
> > double-check before, that double-check is now in the core and not
> > needed in individual drivers.
> >
> > This series attempts to do just that. While the original grep, AKA:
> >   git grep 'if.*>prepared' -- drivers/gpu/drm/panel
> >   git grep 'if.*>enabled' -- drivers/gpu/drm/panel
> > ...still produces a few hits after my series, they are _mostly_ all
> > gone. The ones that are left are less trivial to fix.
> >
> > One of the main reasons that many panels probably needed to store and
> > double-check their prepared/enabled appears to have been to handle
> > shutdown and/or remove. Panels drivers often wanted to force the power
> > off for panels in these cases and this was a good reason for the
> > double-check.
> >
> > In response to my V1 series [1] we had much discussion of what to
> > do. The conclusion was that as long as DRM modeset drivers properly
> > called drm_atomic_helper_shutdown() that we should be able to remove
> > the explicit shutdown/remove handling in the panel drivers. Most of
> > the patches to improve DRM modeset drivers [2] [3] [4] have now
> > landed.
> >
> > In contrast to my V1 series, I broke the V2 series up a lot
> > more. Since a few of the panel drivers in V1 already landed, we had
> > fewer total drivers and so we could devote a patch to each panel.
> > Also, since we were now relying on DRM modeset drivers I felt like we
> > should split the patches for each panel into two: one that's
> > definitely safe and one that could be reverted if we found a
> > problematic DRM modeset driver that we couldn't fix.
> >
> > Sorry for the large number of patches. I've set things to mostly just
> > CC people on the cover letter and the patches that are relevant to
> > them. I've tried to CC people on the whole series that have shown
> > interest in this TODO item.
> >
> > As patches in this series are reviewed and/or tested they could be
> > landed. There's really no ordering requirement for the series unless
> > patches touch the same driver.
> >
> > NOTE: this touches _a lot_ of drivers, is repetitive, and is not
> > really possible to generate automatically. That means it's entirely
> > possible that my eyes glazed over and I did something wrong. Please
> > double-check me and don't assume that I got everything perfect, though
> > I did my best. I have at least confirmed that "allmodconfig" for arm64
> > doesn't fall on its face with this series. I haven't done a ton of
> > other testing.
> >
> > [1] 
> > https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
> > [2] https://lore.kernel.org/r/20230901234015.566018-1-diand...@chromium.org
> > [3] https://lore.kernel.org/r/20230901234202.566951-1-diand...@chromium.org
> > [4] https://lore.kernel.org/r/20230921192749.1542462-1-diand...@chromium.org
>
> This is the right thing to do, thanks for looking into this!
>
> As for the behaviour of .remove() I doubt whether in many cases
> the original driver authors have even tested this themselves.

Yeah, I'd tend to agree.


> I would say we should just apply the series as soon as it's non-RFC

It's not actually RFC now, but "RFT" (request for testing). I don't
_think_ there's any need to send a version without the RFT tag before
landing unless someone really feels strongly about it.


> after the next merge window

With drm-misc there's not really any specific reason to wait for the
merge window to open/close as we can land in drm-misc-next at any time
regardless of the merge window. drm-misc-next will simply stop feeding
linuxnext for a while.

That all being said, I'm happy to delay landing this until after the
next -rc1 comes out if people would prefer that. If I don't hear
anything, I guess I'll just wait until -rc1 before landing any of
these.


> and see what happens. I doubt it
> will cause much trouble.

I can land the whole series if that's what everyone agrees on. As I
mentioned above, I'm at least slightly worried that I did something
stupid _somewhere_ in this series since no automation was possible and
with repetitive tasks like this it's super easy to flub something up.
It's _probably_ fine, but I guess I still have the worry in the back
of my mind.

If folks think I should just apply the whole series then I'm happy to
do that. If folks think I should just land parts of the series as they
are reviewed/tested I can do that as well. Let me know. If I don't
hear anything I'd tend to just land patches that are reviewed/tested.
Then after a month or so (hopefully) I'd send out a v2 with anything
left.


> The series:
> Acked-by: Linus Walleij 

Re: [PATCH v4 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Wed, May 8, 2024 at 4:52 AM cong yang
 wrote:
>
> > > +static int starry_himax83102_j02_init(struct hx83102 *ctx)
> > > +{
> > > +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > > +
> > > +   hx83102_enable_extended_cmds(ctx, true);
> > > +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPOWER, 0x2c, 
> > > 0xb5, 0xb5, 0x31, 0xf1,
> > > +0x31, 0xd7, 0x2f, 0x36, 0x36, 
> > > 0x36, 0x36, 0x1a, 0x8b, 0x11,
> > > +0x65, 0x00, 0x88, 0xfa, 0xff, 
> > > 0xff, 0x8f, 0xff, 0x08, 0x74,
> > > +0x33);
> >
> > The indentation is still off here. You have 5 tabs followed by a
> > space. To make things line up with the opening brace I think it should
> > be 4 tabs followed by 5 spaces.
>
> Sorry, my  editor 'Visual Studio Code' It seems that the correct indentation
> is not recognized. I have checked it through the 'vim' editor in the V4 
> version.
> Thanks.

FWIW, I use VS Code and it looks fine to me. Maybe check your VS Code
settings? Tab size should be 8.


> > > +static int hx83102_enable(struct drm_panel *panel)
> > > +{
> > > +   struct hx83102 *ctx = panel_to_hx83102(panel);
> > > +   struct mipi_dsi_device *dsi = ctx->dsi;
> > > +   struct device *dev = >dev;
> > > +   int ret;
> > > +
> > > +   ret = ctx->desc->init(ctx);
> > > +   if (ret)
> > > +   return ret;
> >
> > You're still changing behavior here. In the old boe-tv101wum-nl6
> > driver the init() function was invoked at the end of prepare(). Now
> > you've got it at the beginning of enable(). If this change is
> > important it should be in a separate commit and explained.
> >
> >
> > > +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > > +   if (ret) {
> > > +   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > > +   return ret;
> > > +   }
> > > +
> > > +   msleep(120);
> > > +
> > > +   ret = mipi_dsi_dcs_set_display_on(dsi);
> > > +   if (ret) {
> > > +   dev_err(dev, "Failed to turn on the display: %d\n", ret);
> > > +   }
> >
> > The old boe-tv101wum-nl6 driver didn't call
> > mipi_dsi_dcs_exit_sleep_mode() nor mipi_dsi_dcs_set_display_on() in
> > its enable routine, did it? If this change is important please put it
> > in a separate change and justify it.
>
> In the old boe-tv101wum-nl6 driver inital cmds was invoked at the end of
> prepare() function , and call 0x11 and 0x29 at end of inital. For
> himax-hx83102 driver, we move inital cmds invoked at enable() function.
> For panel timing, I think there is no much difference. They are
> all initial cmds executed after meeting the power-on sequence.
> I will update these in the v4 commit message.

Ah, I see! So the mipi_dsi_dcs_exit_sleep_mode() was the 0x11 in the
old code and the mipi_dsi_dcs_set_display_on() was the 0x29 in the old
code. OK, I agree that it's better like you've done it where those
functions are moved out of the "->init()" function and into the
caller, so please keep that as you have it.

The only thing I would request is to keep the ->init() call to be made
at the end of prepare() instead of the beginning of enable(). It may
not matter too much, but in that case I'd rather keep it how it was or
make it an explicit change and not an implicit part of the refactor.

-Doug

Re: [PATCH v4 5/7] drm/panel: himax-hx83102: Support for BOE nv110wum-l60 MIPI-DSI panel

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 6:53 AM Cong Yang
 wrote:
>
> +static int boe_nv110wum_init(struct hx83102 *ctx)
> +{
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +   msleep(60);
> +
> +   hx83102_enable_extended_cmds(ctx, true);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPOWER, 0x2c, 0xaf, 
> 0xaf, 0x2b, 0xeb, 0x42,
> +0xe1, 0x4d, 0x36, 0x36, 0x36, 0x36, 
> 0x1a, 0x8b, 0x11, 0x65, 0x00,
> +0x88, 0xfa, 0xff, 0xff, 0x8f, 0xff, 
> 0x08, 0x9a, 0x33);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETDISP, 0x00, 0x47, 
> 0xb0, 0x80, 0x00, 0x12,
> +0x71, 0x3c, 0xa3, 0x11, 0x00, 0x00, 
> 0x00, 0x88, 0xf5, 0x22, 0x8f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCYC, 0x49, 0x49, 
> 0x32, 0x32, 0x14, 0x32,
> +0x84, 0x6e, 0x84, 0x6e, 0x01, 0x9c);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcd);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETMIPI, 0x84);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETVDC, 0x1b, 0x04);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_UNKNOWN_BE, 0x20);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPTBA, 0xfc, 0x84);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSTBA, 0x36, 0x36, 
> 0x22, 0x00, 0x00, 0xa0,
> +0x61, 0x08, 0xf5, 0x03);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xcc);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTCON, 0x80);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc6);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETRAMDMY, 0x97);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPWM, 0x00, 0x1e, 
> 0x30, 0xd4, 0x01);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCLOCK, 0x08, 0x13, 
> 0x07, 0x00, 0x0f, 0x34);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPANEL, 0x02, 0x03, 
> 0x44);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0xc4);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETCASCADE, 0x03);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETSPCCMD, 0x3f);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPCTRL, 0x37, 0x06, 
> 0x00, 0x02, 0x04, 0x0c, 0xff);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_UNKNOWN_D2, 0x1f, 
> 0x11, 0x1f, 0x11);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP0, 0x06, 0x00, 
> 0x00, 0x00, 0x00, 0x04,
> +0x08, 0x04, 0x08, 0x37, 0x37, 0x64, 
> 0x4b, 0x11, 0x11, 0x03, 0x03, 0x32,
> +0x10, 0x0e, 0x00, 0x0e, 0x32, 0x10, 
> 0x0a, 0x00, 0x0a, 0x32, 0x17, 0x98,
> +0x07, 0x98, 0x00, 0x00);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP1, 0x18, 0x18, 
> 0x18, 0x18, 0x1e, 0x1e,
> +0x1e, 0x1e, 0x1f, 0x1f, 0x1f, 0x1f, 
> 0x24, 0x24, 0x24, 0x24, 0x07, 0x06,
> +0x07, 0x06, 0x05, 0x04, 0x05, 0x04, 
> 0x03, 0x02, 0x03, 0x02, 0x01, 0x00,
> +0x01, 0x00, 0x21, 0x20, 0x21, 0x20, 
> 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +0x18, 0x18);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGIP3, 0xaf, 0xaa, 
> 0xaa, 0xaa, 0xaa, 0xa0,
> +0xaf, 0xaa, 0xaa, 0xaa, 0xaa, 0xa0);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETGMA, 0x00, 0x05, 
> 0x0d, 0x14, 0x1b, 0x2c,
> +0x44, 0x49, 0x51, 0x4c, 0x67, 0x6c, 
> 0x71, 0x80, 0x7d, 0x84, 0x8d, 0xa0,
> +0xa0, 0x4f, 0x58, 0x64, 0x73, 0x00, 
> 0x05, 0x0d, 0x14, 0x1b, 0x2c, 0x44,
> +0x49, 0x51, 0x4c, 0x67, 0x6c, 0x71, 
> 0x80, 0x7d, 0x84, 0x8d, 0xa0, 0xa0,
> +0x4f, 0x58, 0x64, 0x73);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETTP1, 0x07, 0x10, 
> 0x10, 0x1a, 0x26, 0x9e,
> +0x00, 0x53, 0x9b, 0x14, 0x14);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_UNKNOWN_E1, 0x11, 
> 0x00, 0x00, 0x89, 0x30, 0x80,
> +0x07, 0x80, 0x02, 0x58, 0x00, 0x14, 
> 0x02, 0x58, 0x02, 0x58, 0x02, 0x00,
> +0x02, 0x2c, 0x00, 0x20, 0x02, 0x02, 
> 0x00, 0x08, 0x00, 0x0c, 0x05, 0x0e,
> +0x04, 0x94, 0x18, 0x00, 0x10, 0xf0, 
> 0x03, 0x0c, 0x20, 0x00, 0x06, 0x0b,
> +0x0b, 

Re: [PATCH v4 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 6:53 AM Cong Yang
 wrote:
>
> +static int hx83102_enable_extended_cmds(struct hx83102 *ctx, bool enable)
> +{
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +   if (enable)
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETEXTC, 0x83, 
> 0x10, 0x21, 0x55, 0x00);
> +   else
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETEXTC, 0x00, 
> 0x00, 0x00);
> +
> +   return 0;

You're throwing away the error codes returned by the
mipi_dsi_dcs_write_seq_multi(), which you shouldn't do. You have two
options:

Option #1: return dsi_ctx.accum_err here and then check the return
value in callers.

Option #2: instead of having this function take "struct hx83102 *ctx",
just have it take "struct mipi_dsi_multi_context *dsi_ctx". Then it
can return void and everything will be fine.

I'd prefer option #2 but either is OK w/ me.


> +static int starry_himax83102_j02_init(struct hx83102 *ctx)
> +{
> +   struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +   hx83102_enable_extended_cmds(ctx, true);
> +   mipi_dsi_dcs_write_seq_multi(_ctx, HX83102_SETPOWER, 0x2c, 0xb5, 
> 0xb5, 0x31, 0xf1,
> +0x31, 0xd7, 0x2f, 0x36, 0x36, 0x36, 
> 0x36, 0x1a, 0x8b, 0x11,
> +0x65, 0x00, 0x88, 0xfa, 0xff, 0xff, 
> 0x8f, 0xff, 0x08, 0x74,
> +0x33);

The indentation is still off here. You have 5 tabs followed by a
space. To make things line up with the opening brace I think it should
be 4 tabs followed by 5 spaces.


> +static int hx83102_enable(struct drm_panel *panel)
> +{
> +   struct hx83102 *ctx = panel_to_hx83102(panel);
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +   struct device *dev = >dev;
> +   int ret;
> +
> +   ret = ctx->desc->init(ctx);
> +   if (ret)
> +   return ret;

You're still changing behavior here. In the old boe-tv101wum-nl6
driver the init() function was invoked at the end of prepare(). Now
you've got it at the beginning of enable(). If this change is
important it should be in a separate commit and explained.


> +   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> +   return ret;
> +   }
> +
> +   msleep(120);
> +
> +   ret = mipi_dsi_dcs_set_display_on(dsi);
> +   if (ret) {
> +   dev_err(dev, "Failed to turn on the display: %d\n", ret);
> +   }

The old boe-tv101wum-nl6 driver didn't call
mipi_dsi_dcs_exit_sleep_mode() nor mipi_dsi_dcs_set_display_on() in
its enable routine, did it? If this change is important please put it
in a separate change and justify it.


-Doug

Re: [PATCH v4 1/7] dt-bindings: display: panel: Add himax hx83102 panel bindings

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 8:14 AM Rob Herring (Arm)  wrote:
>
>
> On Tue, 07 May 2024 21:52:28 +0800, Cong Yang wrote:
> > In V1, discussed with Doug and Linus [1], we need break out as separate
> > driver for the himax83102-j02 controller. Beacuse "starry,himax83102-j02"
> > and in this series "BOE nv110wum-l60" "IVO t109nw41" panels use same
> > controller, they have some common CMDS. So add new documentation for
> > this panels.
> >
> > For himax83102-j02 controller, no need 3v3 supply, so remove it.
> >
> > [1]: 
> > https://lore.kernel.org/all/CACRpkdbzYZAS0=zbqjuc4cb2wj4s1h6n6asazqvdmv95r3z...@mail.gmail.com
> >
> > Signed-off-by: Cong Yang 
> > ---
> > Chage since V4:
> >
> > - Update commit message and add fallback compatible.
> >
> > V3: 
> > https://lore.kernel.org/all/20240424023010.2099949-2-yangco...@huaqin.corp-partner.google.com
> >
> > Chage since V3:
> >
> > - Update commit message.
> >
> > V2: 
> > https://lore.kernel.org/all/20240422090310.3311429-2-yangco...@huaqin.corp-partner.google.com
> >
> > ---
> >  .../display/panel/boe,tv101wum-nl6.yaml   |  2 -
> >  .../bindings/display/panel/himax,hx83102.yaml | 73 +++
> >  2 files changed, 73 insertions(+), 2 deletions(-)
> >  create mode 100644 
> > Documentation/devicetree/bindings/display/panel/himax,hx83102.yaml
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/panel/himax,hx83102.example.dtb:
>  panel@0: compatible:0: 'starry,himax83102-j02, himax,hx83102' does not match 
> '^[a-zA-Z0-9][a-zA-Z0-9,+\\-._/]+$'
> from schema $id: http://devicetree.org/schemas/dt-core.yaml#
> Documentation/devicetree/bindings/display/panel/himax,hx83102.example.dtb: 
> /example-0/dsi/panel@0: failed to match any schema with compatible: 
> ['starry,himax83102-j02, himax,hx83102']
>
> doc reference errors (make refcheckdocs):
>
> See 
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240507135234.1356855-2-yangco...@huaqin.corp-partner.google.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

I think several of your bindings patches have triggered Rob's bot.
Please make sure you're set up to test this yourself and make sure you
run it locally before sending out the next version of your patches. In
general you should get in the habit of running 'make dt_binding_check'
locally before you post any bindings changes.

Thanks!

-Doug

Re: [PATCH v3 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, May 7, 2024 at 6:44 AM cong yang
 wrote:
>
> > Speaking of which, some of the panels that you add to this driver seem
> > to disable extended commands at the end of their init sequence, but
> > this one doesn't. Should it?
>
> I  have confirmed with himax that disable extended commands  used
> at the end to prevent accidental writing. And our inital code has been
> confirmed by himax before FSI. Considering that this has been on the
> market for a long time and there are no feedback issues, I think it should
> remain the same as `panel-boe-tv101wum-nl6.c`.  What do you think?

It's fine with me to leave it the same as `panel-boe-tv101wum-nl6.c`
had it. At least it should be more obvious to people that there's a
difference now. ;-)

-Doug

Re: [PATCH] drm/connector: Add \n to message about demoting connector force-probes

[Doug Anderson]

Hi,

On Thu, May 2, 2024 at 3:33 PM Douglas Anderson  wrote:
>
> The debug print clearly lacks a \n at the end. Add it.
>
> Fixes: 8f86c82aba8b ("drm/connector: demote connector force-probes for 
> non-master clients")
> Signed-off-by: Douglas Anderson 
> ---
>
>  drivers/gpu/drm/drm_connector.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Pushed to drm-misc-fixes:

6897204ea3df drm/connector: Add \n to message about demoting connector
force-probes

Re: [PATCH] drm/panel-edp: Add ID for KD KD116N09-30NH-A016

[Doug Anderson]

Hi,

On Thu, May 2, 2024 at 4:55 PM Hsin-Yi Wang  wrote:
>
> On Thu, May 2, 2024 at 4:48 PM Douglas Anderson  wrote:
> >
> > As evidenced by in-field reports, this panel shipped on pompom but we
> > never added the ID and thus we're stuck w/ conservative timings. The
> > panel was part of early patches but somehow got left off in the
> > end. :( Add it in now.
> >
> > For future reference, EDID from this panel is:
> > 00002c821212
> > 321e0104951a0e780ae511965e55932c
> > 1950540001010101010101010101
> > 010101010101a41f5686500084302820
> > 55901018
> > 
> > 00fe
> > 004b443131364e3039333041313600f6
> >
> > We use the ASCII string from decoding the EDID ("KD116N0930A16") as
> > the panel name.
> >
> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Hsin-Yi Wang 

Pushed to drm-misc-next:

a6cd27d92a96 drm/panel-edp: Add ID for KD KD116N09-30NH-A016

Re: [PATCH] drm/panel-edp: Add panel CSOT MNB601LS1-1

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 6:55 PM Xuxin Xiong
 wrote:
>
> Hi Doug, thank you!
> We had reported this info to the CSOT to correct the vendor id.
> If they confirm to fix this with the same product ID, we will submit a
> patch to fix this.

FYI, "top posting" like this is generally frowned upon on kernel
mailing lists. One such reference about this is [1]. Some folks are
very passionate about this topic, so please keep it in mind to avoid
upsetting people in the community.

In any case: did you get any response from CSOT about the improper EDID?

[1] https://subspace.kernel.org/etiquette.html

-Doug

Re: [RFT PATCH v2 15/48] drm/panel: ltk050h3146w: Don't call unprepare+disable at shutdown/remove

[Doug Anderson]

Hi,

On Mon, May 6, 2024 at 7:04 AM Quentin Schulz  wrote:
>
> Hi Douglas,
>
> On 5/3/24 11:32 PM, Douglas Anderson wrote:
> > [You don't often get email from diand...@chromium.org. Learn why this is 
> > important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > It's the responsibility of a correctly written DRM modeset driver to
> > call drm_atomic_helper_shutdown() at shutdown time and that should be
> > disabling / unpreparing the panel if needed. Panel drivers shouldn't
> > be calling these functions themselves.
> >
> > A recent effort was made to fix as many DRM modeset drivers as
> > possible [1] [2] [3] and most drivers are fixed now.
> >
> > Unfortunately, grepping mainline for this panel's compatible string
> > shows no hits, so we can't be 100% sure if the DRM modeset driver used
> > with this panel has been fixed. If it is found that the DRM modeset
> > driver hasn't been fixed then this patch could be temporarily reverted
> > until it is.
> >
> > [1] https://lore.kernel.org/r/20230901234015.566018-1-diand...@chromium.org
> > [2] https://lore.kernel.org/r/20230901234202.566951-1-diand...@chromium.org
> > [3] https://lore.kernel.org/r/20230921192749.1542462-1-diand...@chromium.org
> >
> > Cc: "Heiko Stübner" 
> > Cc: Quentin Schulz 
> > Signed-off-by: Douglas Anderson 
>
> I get:
>
> """
> [   27.239362] [ cut here ]
> [   27.244549] refcount_t: addition on 0; use-after-free.
> [   27.250308] WARNING: CPU: 4 PID: 1 at lib/refcount.c:25
> refcount_warn_saturate+0x120/0x144
> [   27.259556] Modules linked in: snd_soc_simple_card crct10dif_ce
> snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
> [   27.273470] CPU: 4 PID: 1 Comm: systemd-shutdow Not tainted
> 6.9.0-rc7-2-g4a8eaebfcfde-dirty #63
> [   27.283584] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou
> devkit with Video Demo adapter (DT)
> [   27.294180] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [   27.301962] pc : refcount_warn_saturate+0x120/0x144
> [   27.307411] lr : refcount_warn_saturate+0x120/0x144
> [   27.312860] sp : 800081babb10
> [   27.316559] x29: 800081babb10 x28: 0064 x27:
> 
> [   27.324539] x26: 8000814847f8 x25: 0001 x24:
> 800081adb028
> [   27.332519] x23: 00e13090 x22: 800081b3c280 x21:
> 006c8680
> [   27.340489] x20: 006c8680 x19: 0a6f8000 x18:
> 0006
> [   27.348468] x17: 00040044 x16: 00500074b5503510 x15:
> 800081bab580
> [   27.356447] x14:  x13: 8000819944d0 x12:
> 0a2f
> [   27.364426] x11: 0365 x10: 8000819ec4d0 x9 :
> 8000819944d0
> [   27.372396] x8 : efff x7 : 8000819ec4d0 x6 :
> 8000f000
> [   27.380375] x5 : 0366 x4 :  x3 :
> 
> [   27.388353] x2 :  x1 :  x0 :
> 0064
> [   27.396332] Call trace:
> [   27.399061]  refcount_warn_saturate+0x120/0x144
> [   27.404122]  drm_dev_get+0x78/0x7c
> [   27.407924]  drm_atomic_state_init+0x78/0xd0
> [   27.412695]  drm_atomic_state_alloc+0x6c/0x9c
> [   27.417563]  drm_atomic_helper_disable_all+0x20/0x214
> [   27.423208]  drm_atomic_helper_shutdown+0xa4/0x148
> [   27.428560]  rockchip_drm_platform_shutdown+0x18/0x28
> [   27.434207]  platform_shutdown+0x24/0x34
> [   27.438589]  device_shutdown+0x150/0x258
> [   27.442973]  kernel_power_off+0x38/0x7c
> [   27.447260]  __do_sys_reboot+0x204/0x24c
> [   27.451643]  __arm64_sys_reboot+0x24/0x30
> [   27.456122]  invoke_syscall+0x48/0x114
> [   27.460311]  el0_svc_common.constprop.0+0x40/0xe0
> [   27.465567]  do_el0_svc+0x1c/0x28
> [   27.469269]  el0_svc+0x34/0xd8
> [   27.472681]  el0t_64_sync_handler+0x120/0x12c
> [   27.477548]  el0t_64_sync+0x190/0x194
> [   27.481639] ---[ end trace  ]---
> [   27.486831] [ cut here ]
> [   27.491995] refcount_t: underflow; use-after-free.
> [   27.497414] WARNING: CPU: 4 PID: 1 at lib/refcount.c:28
> refcount_warn_saturate+0xf4/0x144
> [   27.506558] Modules linked in: snd_soc_simple_card crct10dif_ce
> snd_soc_simple_card_utils fuse [last unloaded: panel_leadtek_ltk050h3146w]
> [   27.520468] CPU: 4 PID: 1 Comm: systemd-shutdow Tainted: GW
> 6.9.0-rc7-2-g4a8eaebfcfde-dirty #63
> [   27.532230] Hardware name: Theobroma Systems RK3399-Q7 SoM on Haikou
> devkit with Video Demo adapter (DT)
> [   27.542826] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [   27.550607] pc : refcount_warn_saturate+0xf4/0x144
> [   27.555958] lr : refcount_warn_saturate+0xf4/0x144
> [   27.561309] sp : 800081babb10
> [   27.565008] x29: 800081babb10 x28: 0064 x27:
> 
> [   27.572988] x26: 8000814847f8 x25: 0001 x24:
> 800081adb028
> [   27.580958] x23: 00e13090 x22: 0a6f82e0 

Re: [PATCH] drm: renesas: shmobile: Call drm_helper_force_disable_all() at shutdown time

[Doug Anderson]

Hi,

On Tue, Dec 5, 2023 at 9:35 AM Doug Anderson  wrote:
>
> Hi,
>
> On Tue, Dec 5, 2023 at 5:40 AM Laurent Pinchart
>  wrote:
> >
> > On Tue, Dec 05, 2023 at 02:31:24PM +0100, Geert Uytterhoeven wrote:
> > > On Tue, Dec 5, 2023 at 1:16 PM Laurent Pinchart wrote:
> > > > On Tue, Dec 05, 2023 at 12:30:02PM +0100, Geert Uytterhoeven wrote:
> > > > > From: Douglas Anderson 
> > > > >
> > > > > Based on grepping through the source code, this driver appears to be
> > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown 
> > > > > time.
> > > > > This is important because drm_helper_force_disable_all() will cause
> > > > > panels to get disabled cleanly which may be important for their power
> > > > > sequencing.  Future changes will remove any custom powering off in
> > > > > individual panel drivers so the DRM drivers need to start getting this
> > > > > right.
> > > > >
> > > > > The fact that we should call drm_atomic_helper_shutdown() in the case 
> > > > > of
> > > > > OS shutdown comes straight out of the kernel doc "driver instance
> > > > > overview" in drm_drv.c.
> > > > >
> > > > > Suggested-by: Maxime Ripard 
> > > > > Signed-off-by: Douglas Anderson 
> > > > > Link: 
> > > > > https://lore.kernel.org/r/20230901164111.RFT.15.Iaf638a1d4c8b3c307a6192efabb4cbb06b195f15@changeid
> > > > > [geert: s/drm_helper_force_disable_all/drm_atomic_helper_shutdown/]
> > > > > [geert: shmob_drm_remove() already calls drm_atomic_helper_shutdown]
> > > > > Signed-off-by: Geert Uytterhoeven 
> > > >
> > > > Reviewed-by: Laurent Pinchart 
> > >
> > > Thanks!
> > >
> > > > > Panel-simple does print two new warnings:
> > > > >
> > > > > +panel-simple panel: Skipping disable of already disabled panel
> > > > > +panel-simple panel: Skipping unprepare of already unprepared 
> > > > > panel
> > > >
> > > > Have you investigated where this comes from ?
> > >
> > > Meh, I knew I forgot something ;-)
> > >
> > > The panel is unprepared and disabled a first time from shmob_drm's
> > > .shutdown() callback:
> > >
> > >   shmob_drm_shutdown
> > > drm_atomic_helper_shutdown
> > >   drm_atomic_helper_disable_all
> > > drm_atomic_commit
> > >   drm_atomic_helper_commit
> > > commit_tail
> > >   drm_atomic_helper_commit_tail
> > > drm_atomic_helper_commit_modeset_disables
> > >   disable_outputs
> > > drm_atomic_bridge_chain_disable
> > > drm_panel_disable
> > > drm_atomic_bridge_chain_post_disable
> > > drm_panel_unprepare
> > >
> > > And a second time from simple_panel's .shutdown() callback():
> > >
> > >   panel_simple_platform_shutdown
> > > panel_simple_shutdown
> > >   drm_panel_disable
> > >   drm_panel_unprepare
> >
> > That looks like what Doug mentioned should be removed in the commit
> > message of this patch (a confirmation would be nice). It should be fine
> > for now.
>
> Yup, this is completely expected right now and is actually a _good_
> sign that your patch is doing what it should be. We unfortunately
> can't remove the panel_simple_shutdown() until all DRM modeset drivers
> (or at least all the ones that could be used w/ panel_simple) are
> properly calling drm_helper_force_disable_all(), though.

FWIW, I've sent out a new version of the series that removes most
panel-specific enable/prepare tracking and also adjusts the TODO to
make it clear that these warnings are expected for panel-simple and
panel-edp [1]. For all other panels my series removes the extra
drm_panel_disable() and drm_panel_unprepare().

I noticed that ${SUBJECT} patch hasn't landed yet, but from grepping I
couldn't find it used with any panels that I was touching... In any
case, it seems like it would be nice if it could land...

I'm not sure the best way to deal with panel-simple / panel-edp. I'm a
little scared to just remove the disable/unprepare since they are used
across so many different DRM modeset drivers, but the annoying thing
is that the warning shows up only on DRM modeset drivers that have
been fixed and _not_ on ones that weren't fixed. :( To get a warning
at the right times we'd need something that runs _after_ driver
shutdown to check whether the DRM modeset driver forgot to call
drm_atomic_helper_shutdown()...

[1] https://lore.kernel.org/r/20240503213441.177109-1-diand...@chromium.org

Re: [PATCH 5/6] drm/panel: novatek-nt36672a: stop calling regulator_set_load manually

[Doug Anderson]

Hi,

On Thu, Apr 4, 2024 at 3:08 AM Dmitry Baryshkov
 wrote:
>
> Use .init_load_uA part of the bulk regulator API instead of calling
> register_set_load() manually.
>
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/panel/panel-novatek-nt36672a.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c 
> b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> index 33fb3d715e54..3886372415c2 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> @@ -605,21 +605,16 @@ static int nt36672a_panel_add(struct nt36672a_panel 
> *pinfo)
> struct device *dev = >link->dev;
> int i, ret;
>
> -   for (i = 0; i < ARRAY_SIZE(pinfo->supplies); i++)
> +   for (i = 0; i < ARRAY_SIZE(pinfo->supplies); i++) {
> pinfo->supplies[i].supply = nt36672a_regulator_names[i];
> +   pinfo->supplies[i].init_load_uA = 
> nt36672a_regulator_enable_loads[i];
> +   }
>
> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(pinfo->supplies),
>   pinfo->supplies);
> if (ret < 0)
> return dev_err_probe(dev, ret, "failed to get regulators\n");
>
> -   for (i = 0; i < ARRAY_SIZE(pinfo->supplies); i++) {
> -   ret = regulator_set_load(pinfo->supplies[i].consumer,
> -nt36672a_regulator_enable_loads[i]);
> -   if (ret)
> -   return dev_err_probe(dev, ret, "failed to set 
> regulator enable loads\n");
> -   }

Thanks for the cleanup! I happened to notice this commit and I'm
curious why you didn't take the next step and move to
devm_regulator_bulk_get_const(). That would have allowed you to avoid
the "for" loop above that copies the data ;-)

-Doug

Re: [PATCH v3 9/9] drm/panel: innolux-p079zca: Don't use a table for initting panels

[Doug Anderson]

Hi,

On Wed, May 1, 2024 at 8:43 AM Douglas Anderson  wrote:
>
> @@ -132,33 +125,9 @@ static int innolux_panel_prepare(struct drm_panel *panel)
> /* p079zca: t4, p097pfg: t5 */
> usleep_range(2, 21000);
>
> -   if (innolux->desc->init_cmds) {
> -   const struct panel_init_cmd *cmds =
> -   innolux->desc->init_cmds;
> -   unsigned int i;
> -
> -   for (i = 0; cmds[i].len != 0; i++) {
> -   const struct panel_init_cmd *cmd = [i];
> -
> -   err = mipi_dsi_generic_write(innolux->link, cmd->data,
> -cmd->len);
> -   if (err < 0) {
> -   dev_err(panel->dev, "failed to write command 
> %u\n", i);
> -   goto poweroff;
> -   }
> -
> -   /*
> -* Included by random guessing, because without this
> -* (or at least, some delay), the panel sometimes
> -* didn't appear to pick up the command sequence.
> -*/
> -   err = mipi_dsi_dcs_nop(innolux->link);
> -   if (err < 0) {
> -   dev_err(panel->dev, "failed to send DCS nop: 
> %d\n", err);
> -   goto poweroff;
> -   }
> -   }
> -   }
> +   err = innolux->desc->init(innolux);
> +   if (err < 0)
> +   goto poweroff;

FWIW, I happened to notice a bug in the above by code inspection. The
old code checked "if (innolux->desc->init_cmds)" and thus handled
init_cmds being NULL. The new code doesn't handle the init function
being NULL. One of the two panels in this file (which seems to have no
users in mainline) doesn't specify an init sequence.

I'll spin this next week with the extra "if" test.

-Doug

Re: [PATCH v3 4/9] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Thu, May 2, 2024 at 1:23 AM Linus Walleij  wrote:
>
> On Wed, May 1, 2024 at 5:43 PM Douglas Anderson  wrote:
>
> > Through a cooperative effort between Hsin-Yi Wang and Dmitry
> > Baryshkov, we have realized the dev_err() in the
> > mipi_dsi_*_write_seq() macros was causing quite a bit of bloat to the
> > kernel. Let's hoist this call into drm_mipi_dsi.c by adding a "chatty"
> > version of the functions that includes the print. While doing this,
> > add a bit more comments to these macros making it clear that they
> > print errors and also that they return out of _the caller's_ function.
>
> This doesn't really explain why this takes so much less space.
>
> Please add some explanation like that "the macro gets inlined
> and thus the error report string gets inlined into every call to
> mipi_dsi_dcs_write_seq() and mipi_dsi_generic_write_seq(),
> by using a call to a "chatty" function, the usage is reduced to one
> string in the chatty function and a function call at the invoking
> site".
>
> With some explanation like that +/- added in:
> Reviewed-by: Linus Walleij 

Sure. I'll plan on not sending out a v4 (unless I need to for some
other reason) and I can just add your wording in when applying. Yell
if you want me to do something different.

-Doug

Re: [PATCH v3 7/9] drm/panel: boe-tv101wum-nl6: Don't use a table for initting panels

[Doug Anderson]

Hi,

On Thu, May 2, 2024 at 6:42 AM Linus Walleij  wrote:
>
> On Wed, May 1, 2024 at 5:43 PM Douglas Anderson  wrote:
>
> > Consensus on the mailing lists is that panels shouldn't use a table of
> > init commands but should instead use init functions. With the recently
> > introduced mipi_dsi_dcs_write_seq_multi() this is not only clean/easy
> > but also saves space. Measuring before/after this change:
> >
> > $ scripts/bloat-o-meter \
> >   .../before/panel-boe-tv101wum-nl6.ko \
> >   .../after/panel-boe-tv101wum-nl6.ko
> > add/remove: 14/8 grow/shrink: 0/1 up/down: 27062/-31433 (-4371)
> > Function old new   delta
> > inx_hj110iz_init   -7040   +7040
> > boe_tv110c9m_init  -6440   +6440
> > boe_init   -5916   +5916
> > starry_qfh032011_53g_init  -1944   +1944
> > starry_himax83102_j02_init -1228   +1228
> > inx_hj110iz_init.d -1040   +1040
> > boe_tv110c9m_init.d- 982+982
> > auo_b101uan08_3_init   - 944+944
> > boe_init.d - 580+580
> > starry_himax83102_j02_init.d   - 512+512
> > starry_qfh032011_53g_init.d- 180+180
> > auo_kd101n80_45na_init - 172+172
> > auo_b101uan08_3_init.d -  82 +82
> > auo_kd101n80_45na_init.d   -   2  +2
> > auo_kd101n80_45na_init_cmd   144   --144
> > boe_panel_prepare592 440-152
> > auo_b101uan08_3_init_cmd1056   -   -1056
> > starry_himax83102_j02_init_cmd  1392   -   -1392
> > starry_qfh032011_53g_init_cmd   2256   -   -2256
> > .compoundliteral3393   -   -3393
> > boe_init_cmd7008   -   -7008
> > boe_tv110c9m_init_cmd   7656   -   -7656
> > inx_hj110iz_init_cmd8376   -   -8376
> > Total: Before=37297, After=32926, chg -11.72%
> >
> > Let's do the conversion.
> >
> > Since we're touching all the tables, let's also convert hex numbers to
> > lower case as per kernel conventions.
> >
> > Signed-off-by: Douglas Anderson 
>
> Wow that's a *VERY* nice patch.
> Reviewed-by: Linus Walleij 

Thanks!


> The metrics surprisingly reports more compact object code,
> I wasn't expecting this, but it's nice.

I think it has to do with the design of the table structure in this
driver. Each "struct panel_init_cmd" was 24-bytes big. That means that
to represent one 1-byte sequence we needed 24 bytes + 1 bytes cmd + 1
byte seq = 26 bytes. Lots of overhead for 2 bytes of content. The old
table stuff could certainly have been made _a lot_ less overhead, but
since it wasn't then it also wasn't hard to be better than it with it
via the new style.

FWIW, it also wouldn't be terribly hard to save a tiny bit more space
with the new style if we wanted. It gets a little ugly, but it doesn't
affect callers of the macro. Specifically, if you assume people aren't
going to pass more than 256-byte sequences, you could stuff the length
in the data:

 #define mipi_dsi_dcs_write_seq_multi(ctx, cmd, seq...)  \
-   do {\
-   static const u8 d[] = { cmd, seq }; \
-   mipi_dsi_dcs_write_buffer_multi(ctx, d, ARRAY_SIZE(d)); \
+   do { \
+   static const u8 d[] = { \
+   (sizeof((u8[]){seq})/sizeof(u8)) + 1, cmd, seq }; \
+   mipi_dsi_dcs_write_buffer_multi(ctx, d); \
} while (0)


...and then snag the length out of the first byte of the data in
mipi_dsi_dcs_write_buffer_multi(). If you do this, you actually get
another 10% space savings on this driver. :-P

add/remove: 0/0 grow/shrink: 7/7 up/down: 1140/-4560 (-3420)
Function old new   delta
inx_hj110iz_init.d  10401385+345
boe_tv110c9m_init.d  9821297+315
boe_init.d   580 870+290
starry_qfh032011_53g_init.d  180 271 +91
starry_himax83102_j02_init.d 512 568 +56
auo_b101uan08_3_init.d82 123 +41
auo_kd101n80_45na_init.d   2   4  +2
auo_kd101n80_45na_init   172 164  -8
auo_b101uan08_3_init 944 780-164
starry_himax83102_j02_init  12281004-224
starry_qfh032011_53g_init   19441580-364
boe_init5916

Re: [PATCH v3 0/9] drm/mipi-dsi: Reduce bloat and add funcs for cleaner init seqs

[Doug Anderson]

Hi,

On Thu, May 2, 2024 at 12:48 AM Neil Armstrong
 wrote:
>
> Hi,
>
> On 01/05/2024 17:41, Douglas Anderson wrote:
> > The consensus of many DRM folks is that we want to move away from DSI
> > drivers defining tables of init commands. Instead, we want to move to
> > init functions that can use common DRM functions. The issue thus far
> > has been that using the macros mipi_dsi_generic_write_seq() and
> > mipi_dsi_dcs_write_seq() bloats the driver using them.
> >
> > While trying to solve bloat, we realized that the majority of the it
> > was easy to solve. This series solves the bloat for existing drivers
> > by moving the printout outside of the macro.
> >
> > During discussion of my v1 patch to fix the bloat [1], we also decided
> > that we really want to change the way that drivers deal with init
> > sequences to make it clearer. In addition to being cleaner, a side
> > effect of moving drivers to the new style reduces bloat _even more_.
> >
> > This series also contains a few minor fixes / cleanups that I found
> > along the way.
> >
> > This series converts four drivers over to the new
> > mipi_dsi_dcs_write_seq_multi() function. Not all conversions have been
> > tested, but hopefully they are straightforward enough. I'd appreciate
> > testing.
> >
> > NOTE: In v3 I tried to incorporate the feedback from v2. I also
> > converted the other two panels I could find that used table-based
> > initialization.
> >
> > [1] 
> > https://lore.kernel.org/r/20240424172017.1.Id15fae80582bc74a0d4f1338987fa375738f45b9@changeid
> >
> > Changes in v3:
> > - ("mipi_dsi_*_write functions don't need to ratelimit...") moved earlier.
> > - Add a TODO item for cleaning up the deprecated macros/functions.
> > - Fix spacing of init function.
> > - Inline kerneldoc comments for struct mipi_dsi_multi_context.
> > - Rebased upon patch to remove ratelimit of prints.
> > - Remove an unneeded error print.
> > - Squash boe-tv101wum-nl6 lowercase patch into main patch
> > - Use %zd in print instead of casting errors to int.
> > - drm/panel: ili9882t: Don't use a table for initting panels
> > - drm/panel: innolux-p079zca: Don't use a table for initting panels
> >
> > Changes in v2:
> > - Add some comments to the macros about printing and returning.
> > - Change the way err value is handled in prep for next patch.
> > - Modify commit message now that this is part of a series.
> > - Rebased upon patches to avoid theoretical int overflow.
> > - drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_dcs_write_seq()
> > - drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_generic_write_seq()
> > - drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
> > - drm/mipi-dsi: mipi_dsi_*_write functions don't need to ratelimit prints
> > - drm/panel: boe-tv101wum-nl6: Convert hex to lowercase
> > - drm/panel: boe-tv101wum-nl6: Don't use a table for initting commands
> > - drm/panel: novatek-nt36672e: Switch to mipi_dsi_dcs_write_seq_multi()
> >
> > Douglas Anderson (9):
> >drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_dcs_write_seq()
> >drm/mipi-dsi: Fix theoretical int overflow in
> >  mipi_dsi_generic_write_seq()
> >drm/mipi-dsi: mipi_dsi_*_write functions don't need to ratelimit
> >  prints
> >drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
> >drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
> >drm/panel: novatek-nt36672e: Switch to mipi_dsi_dcs_write_seq_multi()
> >drm/panel: boe-tv101wum-nl6: Don't use a table for initting panels
> >drm/panel: ili9882t: Don't use a table for initting panels
> >drm/panel: innolux-p079zca: Don't use a table for initting panels
>
> Thanks Doug!
>
> I think we all agree on the core changes, now I think we can wait a few weeks
> and try to get some test feedbacks on the indirectly and directly affected
> panels, drm-misc-next won't be merged into linux-next until v6.10-rc1 anyway
> so we have some time to test on our boards.

Great!

Just to be clear, are you suggesting that we leave these patches on
the lists for a few weeks before landing in drm-misc-next, or are you
suggesting that it's safe to land them in drm-misc-next because it
won't make it to linuxnext for a while anyway? I assume the former
(AKA leave it on the lists for a while) but just want to be clear. ;-)

There's nothing terribly urgent about these patches except that they
are blocking Cong Yang's patch series [0] and lvzhaoxiong's patch
series [1]. I think it would be fine for them to send out their patch
series with mine marked as a dependency so we can finish reviewing
their series and then when mine lands theirs will be good to go too.

Maybe we can aim for 2 weeks of stewing on the list if there are no
issues during that time? I know landing in drm-misc during this time
won't help get into mainline faster, but with ChromeOS's "upstream
first" policy it saves us a bunch of headache if we can land things in
our tree from a maintainer tree with stable git hashes (like

Re: [PATCH v2] drm/debugfs: Drop conditionals around of_node pointers

[Doug Anderson]

Hi,

On Tue, Apr 30, 2024 at 10:13 PM Sui Jingfeng  wrote:
>
> Having conditional around the of_node pointer of the drm_bridge structure
> is not necessary anymore, since drm_bridge structure always has the of_node
> member since the commit d8dfccde2709 ("drm/bridge: Drop conditionals around
> of_node pointers").
>
> So drop the conditional, please also note that this patch is following the
> convention used by driver core, see commit c9e358dfc4a8 ("driver-core:
> remove conditionals around devicetree pointers").
>
> Reviewed-by: Dmitry Baryshkov 
> Signed-off-by: Sui Jingfeng 
> ---
> v2: Update commit message
> ---
>  drivers/gpu/drm/drm_debugfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Pushed to drm-misc-next:

235e60653f8d drm/debugfs: Drop conditionals around of_node pointers

-Doug

Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()

[Doug Anderson]

Hi,

On Mon, Apr 29, 2024 at 8:39 AM Jani Nikula  wrote:
>
> On Mon, 29 Apr 2024, Doug Anderson  wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
> >  wrote:
> >>
> >> > +/**
> >> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI 
> >> > funcs in a row
> >> > + * @dsi: Pointer to the MIPI DSI device
> >> > + * @accum_err: Storage for the accumulated error over the multiple 
> >> > calls. Init
> >> > + *   to 0. If a function encounters an error then the error code will be
> >> > + *   stored here. If you call a function and this points to a non-zero
> >> > + *   value then the function will be a noop. This allows calling a 
> >> > function
> >> > + *   many times in a row and just checking the error at the end to see 
> >> > if
> >> > + *   any of them failed.
> >> > + */
> >> > +
> >> > +struct mipi_dsi_multi_context {
> >> > + struct mipi_dsi_device *dsi;
> >> > + int accum_err;
> >> > +};
> >>
> >> I like the design, but having a context struct seems over-engineered while 
> >> we could pass
> >> a single int over without encapsulating it with mipi_dsi_multi_context.
> >>
> >> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
> >>  const void *data, size_t len);
> >> vs
> >> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int 
> >> *accum_err,
> >>  const void *data, size_t len);
> >>
> >> is the same, and it avoids having to declare a mipi_dsi_multi_context and 
> >> set ctx->dsi,
> >> and I'll find it much easier to migrate, just add a  and make sure ret 
> >> is initialized to 0.
> >
> > Yeah, I had the same reaction when Jani suggested the context style
> > [1] and I actually coded it up exactly as you suggest above. I then
> > changed my mind and went with the context. My motivation was that when
> > I tested it I found that using the context produced smaller code.
> > Specifically, from the description of this patch we see we end up
> > with:
> >
> > Total: Before=10651, After=9663, chg -9.28%
> >
> > ...when I didn't have the context and I had the accum_err then instead
> > of getting ~9% smaller I believe it actually got ~0.5% bigger. This
> > makes some sense as the caller has to pass 4 parameters to each call
> > instead of 3.
> >
> > It's not a giant size difference but it was at least some motivation
> > that helped push me in this direction. I'd also say that when I looked
> > at the code in the end the style grew on me. It's really not too
> > terrible to have one line in your functions that looks like:
> >
> > struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
> >
> > ...and if that becomes the canonical way to do it then it's really
> > hard to accidentally forget to initialize the error value. With the
> > other API it's _very_ easy to forget to initialize the error value and
> > the compiler won't yell at you. It also makes it very obvious to the
> > caller that this function is doing something a little different than
> > most Linux APIs with that error return.
> >
> > So I guess I'd say that I ended up being pretty happy with the
> > "context" even if it does feel a little over engineered and I'd argue
> > to keep it that way. That being said, if you feel strongly about it
> > then we can perhaps get others to chime in to see which style they
> > prefer? Let me know what you think.
> >
> >
> > [1] https://lore.kernel.org/r/8734r85tcf@intel.com
>
> FWIW, I don't feel strongly about this, and I could be persuaded either
> way, but I've got this gut feeling that an extensible context parameter
> might be benefitial future proofing in this case.

I've gone ahead and sent out a v3 where I still leave it as taking
`struct mipi_dsi_multi_context`. Neil: if you feel strongly about it,
I have no problem sending out a v4. I still think that the size
savings of using the context are a good "tiebreaking" factor in
choosing between the two styles... ;-)

-Doug

Re: Re: drm/debugfs: Drop conditionals around of_node pointers

[Doug Anderson]

Hi,

On Mon, Apr 29, 2024 at 6:16 PM 隋景峰  wrote:
>
> Hi,
>
>
> > -原始邮件-
> > 发件人: "Maxime Ripard" 
> > 发送时间: 2024-04-29 19:30:24 (星期一)
> > 收件人: "Sui Jingfeng" 
> > 抄送: "Sui Jingfeng" , "Maarten Lankhorst" 
> > , "Thomas Zimmermann" 
> > , "David Airlie" , "Daniel Vetter" 
> > , "Douglas Anderson" , "Laurent 
> > Pinchart" , "Biju Das" 
> > , dri-devel@lists.freedesktop.org, 
> > linux-ker...@vger.kernel.org
> > 主题: Re: drm/debugfs: Drop conditionals around of_node pointers
> >
> > On Sun, Apr 28, 2024 at 04:52:13PM +0800, Sui Jingfeng wrote:
> > > ping
> > >
> > > 在 2024/3/22 06:22, Sui Jingfeng 写道:
> > > > Having conditional around the of_node pointer of the drm_bridge 
> > > > structure
> > > > turns out to make driver code use ugly #ifdef blocks.
> >
> > The code being ugly is an opinion, what problem is it causing exactly?
> >
> > > Drop the conditionals to simplify debugfs.
> >
> > What does it simplifies?
> >
> > > >
> > > > Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node 
> > > > pointers")
> > > > Signed-off-by: Sui Jingfeng 
> >
> > Why do we want to backport that patch to stable?

Technically it's not CCing stable and so it's not really incorrect.
...but I agree that this is a bit of a stretch to call it a "Fix".
Maybe drop the "Fixes" line?


> My commit message is written based on commit of d8dfccde2709
>
> $ git show c9e358dfc4a8
>
> This patch is based on commit c9e358dfc4a8 ("driver-core: remove
> conditionals around devicetree pointers").
>
> Having conditional around the of_node pointer of the drm_bridge
> structure turns out to make driver code use ugly #ifdef blocks. Drop the
> conditionals to simplify drivers. While this slightly increases the size
> of struct drm_bridge on non-OF system, the number of bridges used today
> and foreseen tomorrow on those systems is very low, so this shouldn't be
> an issue.
>
> So drop #if conditionals by adding struct device_node forward declaration.
>
> > Maxime
>
> I'm just start to contribute by mimic other people's tone, there seems no need
> to over read.

I think the fact that you skipped the reference to commit c9e358dfc4a8
("driver-core: remove conditionals around devicetree pointers") was
relevant here. Referencing that commit makes it easy for the reader to
see that you are following convention used throughout the kernel and
not just asserting your own opinion about style.

If you add that reference into your commit message and send a v2, I'm
happy to apply it.

-Doug

Re: [PATCH v3 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 7:30 PM Cong Yang
 wrote:
>
> The Starry HX83102 based mipi panel should never have been part of the boe
> tv101wum driver. Discussion with Doug and Linus in V1 [1], we need a
> separate driver to enable the hx83102 controller.
>
> In hx83102 driver, add DSI commands as macros. So it can add some panels
> with same control model in the future.
>
> [1]: 
> https://lore.kernel.org/all/CACRpkdbzYZAS0=zbqjuc4cb2wj4s1h6n6asazqvdmv95r3z...@mail.gmail.com
>
> Signed-off-by: Cong Yang 
> ---
> Chage since V3:
>
> -  Drop excess flags and function, inital cmds use lowercasehex.
>
> V2: 
> https://lore.kernel.org/all/20240422090310.3311429-3-yangco...@huaqin.corp-partner.google.com
>
> ---
>  drivers/gpu/drm/panel/Kconfig |   9 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c|  99 
>  drivers/gpu/drm/panel/panel-himax-hx83102.c   | 525 ++
>  4 files changed, 535 insertions(+), 99 deletions(-)

It probably makes sense to base your series upon mine that reduces
bloat / introduces a better way to do these init sequences. I'm going
to wait one more day in case anyone else has any more comments on my
v2 and then I'll post my v3. So far everyone has been on-board with
the overall goal and so all we need to do is iron out the small
details, so I don't expect it to take too long.

If you want to wait a day or two and then post your patches atop my v3
(once I post it) then that would be OK by me.


> @@ -0,0 +1,525 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for panels based on Himax HX83102 controller, such as:
> + *
> + * - Starry 10.51" WUXGA MIPI-DSI panel
> + *
> + * Based on drivers/gpu/drm/panel/panel-boe-tv101wum.c

The above file doesn't exist? Maybe you forgot the "-nl6" suffix? I
would also say that this driver appears to be more similar to
`panel-himax-hx8394.c` even if the data came from
`panel-boe-tv101wum-nl6.c`.

...also, since this is based on `panel-himax-hx8394.c`, it seems like
you're making pretty significant changes here. For instance, when this
code was part of `panel-boe-tv101wum-nl6.c` it used to do the "init
commands" as part of prepare. With the new driver it does it as part
of "enable". IMO even if the new code based on `panel-himax-hx8394.c`
is more correct, I'd rather see you change that in a separate change.
In this change, which is supposed to be more about code refactoring, I
think you should focus on keeping the behavior before and after your
patch identical.


> +/* Manufacturer specific DSI commands */
> +#define HX83102_SETPOWER   0xb1
> +#define HX83102_SETDISP0xb2
> +#define HX83102_SETCYC 0xb4
> +#define HX83102_SETEXTC0xb9
> +#define HX83102_SETMIPI0xba
> +#define HX83102_SETVDC 0xbc
> +#define HX83102_SETBANK0xbd
> +#define HX83102_UNKNOWN1   0xbe

I'm not sure that the "unknown" define helps much, but I guess it's
fine. One nit would be to call this UNKNOWN_BE based on the address so
that if we can later replace some of the unknowns then there won't be
gaps in the numbering.


> +#define HX83102_SETPTBA0xbf
> +#define HX83102_SETSTBA0xc0
> +#define HX83102_SETTCON0xc7
> +#define HX83102_SETRAMDMY  0xc8
> +#define HX83102_SETPWM 0xc9
> +#define HX83102_SETCLOCK   0xcb
> +#define HX83102_SETPANEL   0xcc
> +#define HX83102_SETCASCADE 0xd0
> +#define HX83102_SETPCTRL   0xd1
> +#define HX83102_UNKNOWN2   0xd2
> +#define HX83102_SETGIP00xd3
> +#define HX83102_SETGIP10xd5
> +#define HX83102_UNKNOWN3   0xd6

Given everything surrounding it and given a datasheet I have for a
similar panel, I'm going to guess UNKNOWN3 is "GIP2".


> +#define HX83102_SETGIP30xd8
> +#define HX83102_UNKNOWN4   0xe0

I think UNKNOWN4 is SETGMA to set the gamma curve.


> +static int starry_init_cmd(struct hx83102 *ctx)
> +{
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +
> +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 
> 0x00);

As far as I can tell from looking at a different (but similar)
datasheet, the above means "enable extended command set". Assuming I'm
correct, maybe you could abstract out:

static int hx83102_enable_extended_cmds(struct hx83102 *ctx, bool enable)
{
  if (enable)
mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
  else
mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x00, 0x00, 0x00);
}

Then your panel-specific init functions could call that?

Speaking of which, some of the panels that you add to this driver seem
to disable extended commands at the end of their init sequence, but
this one doesn't. Should it?


> +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2c, 0xb5, 0xb5, 0x31, 
> 0xf1, 0x31, 0xd7, 0x2f,
> +

Re: [PATCH v2 1/8] drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_dcs_write_seq()

[Doug Anderson]

Hi,

On Fri, Apr 26, 2024 at 11:22 PM Sam Ravnborg  wrote:
>
> On Sat, Apr 27, 2024 at 04:44:33AM +0300, Dmitry Baryshkov wrote:
> > On Sat, 27 Apr 2024 at 02:59, Douglas Anderson  
> > wrote:
> > >
> > > The mipi_dsi_dcs_write_seq() macro makes a call to
> > > mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro
> > > then stores it in an int and checks to see if it's negative. This
> > > could theoretically be a problem if "ssize_t" is larger than "int".
> > >
> > > To see the issue, imagine that "ssize_t" is 32-bits and "int" is
> > > 16-bits, you could see a problem if there was some code out there that
> > > looked like:
> > >
> > >   mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>);
> > >
> > > ...since we'd get back that 32768 bytes were transferred and 32768
> > > stored in a 16-bit int would look negative.
> > >
> > > Though there are no callsites where we'd actually hit this (even if
> > > "int" was only 16-bit), it's cleaner to make the types match so let's
> > > fix it.
> > >
> > > Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq 
> > > macro")
> > > Signed-off-by: Douglas Anderson 
> > > ---
> > >
> > > Changes in v2:
> > > - New
> > >
> > >  include/drm/drm_mipi_dsi.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > > index 82b1cc434ea3..b3576be22bfa 100644
> > > --- a/include/drm/drm_mipi_dsi.h
> > > +++ b/include/drm/drm_mipi_dsi.h
> > > @@ -337,12 +337,12 @@ int 
> > > mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> > > do {  
> > >  \
> > > static const u8 d[] = { cmd, seq };   
> > >  \
> > > struct device *dev = >dev;   
> > >  \
> > > -   int ret;  
> > >  \
> > > +   ssize_t ret;  
> > >  \
> > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));   
> > >  \
> > > if (ret < 0) {
> > >  \
> > > dev_err_ratelimited(  
> > >  \
> > > dev, "sending command %#02x failed: 
> > > %d\n", \
> > > -   cmd, ret);
> > >  \
> > > +   cmd, (int)ret);   
> > >  \
> >
> > Please consider using %zd instead
>
> Hi Douglas,
> please consider the above for all the pathces, there are more places
> where a cast can be dropped.

Sure, I'll change in the next version. I personally prefer the %d with
an "int" type because technically we're printing an error code and
errors are int-sized. ...but I don't feel strongly and I guess there's
a tiny chance some bug in the code could lead to a negative value
that's more useful as 64-bits than 32-bits. ;-)

I will note that I will still need a cast in some of the later patches
for "%*ph" since, I believe, the size passed for the "*" in a printf
format string is defined to be an int, not a size_t or ssize_t.

-Doug

Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()

[Doug Anderson]

Hi,

On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
 wrote:
>
> > +/**
> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs 
> > in a row
> > + * @dsi: Pointer to the MIPI DSI device
> > + * @accum_err: Storage for the accumulated error over the multiple calls. 
> > Init
> > + *   to 0. If a function encounters an error then the error code will be
> > + *   stored here. If you call a function and this points to a non-zero
> > + *   value then the function will be a noop. This allows calling a function
> > + *   many times in a row and just checking the error at the end to see if
> > + *   any of them failed.
> > + */
> > +
> > +struct mipi_dsi_multi_context {
> > + struct mipi_dsi_device *dsi;
> > + int accum_err;
> > +};
>
> I like the design, but having a context struct seems over-engineered while we 
> could pass
> a single int over without encapsulating it with mipi_dsi_multi_context.
>
> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
>  const void *data, size_t len);
> vs
> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int 
> *accum_err,
>  const void *data, size_t len);
>
> is the same, and it avoids having to declare a mipi_dsi_multi_context and set 
> ctx->dsi,
> and I'll find it much easier to migrate, just add a  and make sure ret is 
> initialized to 0.

Yeah, I had the same reaction when Jani suggested the context style
[1] and I actually coded it up exactly as you suggest above. I then
changed my mind and went with the context. My motivation was that when
I tested it I found that using the context produced smaller code.
Specifically, from the description of this patch we see we end up
with:

Total: Before=10651, After=9663, chg -9.28%

...when I didn't have the context and I had the accum_err then instead
of getting ~9% smaller I believe it actually got ~0.5% bigger. This
makes some sense as the caller has to pass 4 parameters to each call
instead of 3.

It's not a giant size difference but it was at least some motivation
that helped push me in this direction. I'd also say that when I looked
at the code in the end the style grew on me. It's really not too
terrible to have one line in your functions that looks like:

struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };

...and if that becomes the canonical way to do it then it's really
hard to accidentally forget to initialize the error value. With the
other API it's _very_ easy to forget to initialize the error value and
the compiler won't yell at you. It also makes it very obvious to the
caller that this function is doing something a little different than
most Linux APIs with that error return.

So I guess I'd say that I ended up being pretty happy with the
"context" even if it does feel a little over engineered and I'd argue
to keep it that way. That being said, if you feel strongly about it
then we can perhaps get others to chime in to see which style they
prefer? Let me know what you think.


[1] https://lore.kernel.org/r/8734r85tcf@intel.com

Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()

[Doug Anderson]

Hi,

On Fri, Apr 26, 2024 at 11:33 PM Sam Ravnborg  wrote:
>
> > ---
> > Right now this patch introduces two new functions in
> > drm_mipi_dsi.c. Alternatively we could have changed the prototype of
> > the "chatty" functions and made the deprecated macros adapt to the new
> > prototype. While this sounds nice, it bloated callers of the
> > deprecated functioin a bit because it caused the compiler to emit less
> > optimal code. It doesn't seem terrible to add two more functions, so I
> > went that way.
> The concern here is when it will be cleaned up. As a minimum please
> consider adding an item to todo.rst that details what should be done
> to get rid of the now obsolete chatty functions so someone can pick it
> up.

Sure, I can add something to do TODO. Do folks agree that's the
preferred way to do things? A few thoughts I had:

1. In theory it could be useful to keep _both_ the "chatty" and
"multi" variants of the functions. In at least a few places the
"multi" variant was awkward. The resulting auo_kd101n80_45na_init()
[1] looked awkward. If I was writing just this function I would have
chosen an API more like the "chatty" one, but since I was trying to
make all the init functions similar I kept them all using the "multi"
API. Does it make sense to keep both long term?

[1] 
https://lore.kernel.org/all/20240426165839.v2.7.Ib5030ab5cd41b4e08b1958bd7e51571725723008@changeid/

2. Going completely the opposite direction, we could also not bother
saving as much space today and _force_ everyone to move to the new
"multi" API to get the full space savings.


So I guess three options: a) leave my patches the way they are and add
a TODO, b) Keep the "chatty" variants long term and remove my
"after-the-cut", or c) Don't get as much space savings today but don't
introduce a new exported function. Opinions?


> > @@ -792,6 +792,34 @@ int mipi_dsi_generic_write_chatty(struct 
> > mipi_dsi_device *dsi,
> >  }
> >  EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
> >
> > +/**
> > + * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write_chatty() w/ 
> > accum_err
> > + * @ctx: Context for multiple DSI transactions
> > + * @payload: buffer containing the payload
> > + * @size: size of payload buffer
> > + *
> > + * Like mipi_dsi_generic_write_chatty() but deals with errors in a way that
> > + * makes it convenient to make several calls in a row.
> > + */
> > +void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> > +   const void *payload, size_t size)
> > +{
> > + struct mipi_dsi_device *dsi = ctx->dsi;
> > + struct device *dev = >dev;
> > + ssize_t ret;
> > +
> > + if (ctx->accum_err)
> > + return;
> > +
> > + ret = mipi_dsi_generic_write(dsi, payload, size);
> > + if (ret < 0) {
> > + ctx->accum_err = ret;
> > + dev_err_ratelimited(dev, "sending generic data %*ph failed: 
> > %d\n",
> > + (int)size, payload, ctx->accum_err);
> > + }
> I see no point in using ratelimited and then change it in the next
> patch.

Sure, I'll re-order patches.


> > @@ -197,6 +197,22 @@ struct mipi_dsi_device {
> >   struct drm_dsc_config *dsc;
> >  };
> >
> > +/**
> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs 
> > in a row
> > + * @dsi: Pointer to the MIPI DSI device
> > + * @accum_err: Storage for the accumulated error over the multiple calls. 
> > Init
> > + *   to 0. If a function encounters an error then the error code will be
> > + *   stored here. If you call a function and this points to a non-zero
> > + *   value then the function will be a noop. This allows calling a function
> > + *   many times in a row and just checking the error at the end to see if
> > + *   any of them failed.
> > + */
> > +
> > +struct mipi_dsi_multi_context {
> > + struct mipi_dsi_device *dsi;
> > + int accum_err;
> > +};
> Inline comments is - I think - preferred.

Sure, I'll update in the next version.

Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Fri, Apr 26, 2024 at 8:28 AM Doug Anderson  wrote:
>
> > I don't find this unintuitive, but if it helps, you could conceivably
> > add a context parameter:
> >
> > struct mipi_dsi_seq_context context = {
> > .dsi = dsi,
> > };
> >
> > mipi_dsi_dcs_write_seq(, HX83102_SETSPCCMD, 0xcd);
> > ...
> >
> > if (context.ret)
> > ...
> >
> > And even have further control in the context whether to log or keep
> > going or whatever.
>
> I agree there are some benefits of adding the extra "context"
> abstraction and we can go that way if you want, but I lean towards the
> simplicity of just passing in the accumulated return value like I did
> in my example.
>
>
> I'll try to write up patches and see if I can post them later today.

FWIW, I went with your "context" idea. In the end, I liked how it
looked and the icing on the cake was that it generated even smaller
code! :-)

My v2 series (now 8 patches long) is at:

https://lore.kernel.org/r/20240426235857.3870424-1-diand...@chromium.org


-Doug

Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Thu, Apr 25, 2024 at 8:03 PM Dmitry Baryshkov
 wrote:
>
> On Thu, Apr 25, 2024 at 10:04:49AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula  
> > wrote:
> > >
> > > > @@ -279,6 +281,8 @@ enum mipi_dsi_dcs_tear_mode {
> > > >
> > > >  ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> > > > const void *data, size_t len);
> > > > +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> > > > +  const void *data, size_t len);
> > > >  ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> > > >  const void *data, size_t len);
> > > >  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void 
> > > > *data,
> > > > @@ -317,14 +321,10 @@ int 
> > > > mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> > > >  #define mipi_dsi_generic_write_seq(dsi, seq...)
> > > > \
> > > >   do {  
> > > >  \
> > > >   static const u8 d[] = { seq };
> > > >  \
> > > > - struct device *dev = >dev;   
> > > >  \
> > > >   int ret;  
> > > >  \
> > > > - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));  
> > > >  \
> > > > - if (ret < 0) {
> > > >  \
> > > > - dev_err_ratelimited(dev, "transmit data failed: 
> > > > %d\n", \
> > > > - ret); 
> > > >  \
> > > > + ret = mipi_dsi_generic_write_chatty(dsi, d, 
> > > > ARRAY_SIZE(d));\
> > > > + if (ret < 0)  
> > > >  \
> > > >   return ret;   
> > > >  \
> > > > - } 
> > > >  \
> > > >   } while (0)
>
>
> Reading the thread makes me wonder whether we should be going into
> slightly other direction:
>
> Add __must_check() to mipi_dsi_ writing functions,
>
> #define mipi_dsi_dcs_whatever_write(dsi, cmd, seq...)   \
> ({  \
> static const u8 d[] = { cmd, seq }; \
> mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));\
> })
>
> Then in panel drivers we actually have to explicitly handle the return
> code (either by dropping to the error label or by just returning an
> error).

Given the sheer number of init commands needed by some panels (see
j606f_boe_init_sequence() for instance) I'm still convinced that we
want something that allows people to write their init code in a way
that's not quite so verbose. It sounds as if Jani is OK w/ the
proposal of using the "accumulated return value" (proposal #2 I had).
I'm hoping you're OK w/ that too...

-Doug

Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Fri, Apr 26, 2024 at 3:09 AM Jani Nikula  wrote:
>
> > 2. Accept that a slightly less efficient handling of the error case
> > and perhaps a less intuitive API, but avoid the goto.
> >
> > Essentially you could pass in "ret" and have the function be a no-op
> > if an error is already present. Something like this:
> >
> > void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi,
> > const void *data, size_t len, int *accum_ret)
> > {
> >   if (*accum_ret)
> > return;
> >
> >   *accum_ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
>
> No reason you couldn't do error logging here
>
> if (*accum_ret)
> dev_err(...)

Yup, exactly. This is probably best.


> > }
> >
> > ...and then the caller:
> >
> > int ret;
> >
> > ret = 0;
> > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0xcd, );
> > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETMIPI, 0x84, );
> > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0x3f, );
> > mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETVDC, 0x1b, 0x04, );
> > if (ret)
> >   goto some_cmd_failed;
> >
> > This has similar properties to solution #1.
>
> I like this option the best, for the simple reason that the caller side
> is aware of what's going on, there's no magic control flow happening,
> and they can add error handling in the middle if they so choose.

Sounds good to me. I went back and forth a bit between solution #1 and
this and I see the benefits of both. If folks like this one I think we
should run with it. Certainly it's better than the current hidden
return.



> I don't find this unintuitive, but if it helps, you could conceivably
> add a context parameter:
>
> struct mipi_dsi_seq_context context = {
> .dsi = dsi,
> };
>
> mipi_dsi_dcs_write_seq(, HX83102_SETSPCCMD, 0xcd);
> ...
>
> if (context.ret)
> ...
>
> And even have further control in the context whether to log or keep
> going or whatever.

I agree there are some benefits of adding the extra "context"
abstraction and we can go that way if you want, but I lean towards the
simplicity of just passing in the accumulated return value like I did
in my example.


I'll try to write up patches and see if I can post them later today.

-Doug

Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()

[Doug Anderson]

Hi,

On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula  wrote:
>
> > @@ -279,6 +281,8 @@ enum mipi_dsi_dcs_tear_mode {
> >
> >  ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> > const void *data, size_t len);
> > +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> > +  const void *data, size_t len);
> >  ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> >  const void *data, size_t len);
> >  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> > @@ -317,14 +321,10 @@ int mipi_dsi_dcs_get_display_brightness_large(struct 
> > mipi_dsi_device *dsi,
> >  #define mipi_dsi_generic_write_seq(dsi, seq...)
> > \
> >   do {  
> >  \
> >   static const u8 d[] = { seq };
> >  \
> > - struct device *dev = >dev;   
> >  \
> >   int ret;  
> >  \
> > - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));  
> >  \
> > - if (ret < 0) {
> >  \
> > - dev_err_ratelimited(dev, "transmit data failed: 
> > %d\n", \
> > - ret); 
> >  \
> > + ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d));   
> >  \
> > + if (ret < 0)  
> >  \
> >   return ret;   
> >  \
> > - } 
> >  \
> >   } while (0)
>
> The one thing that I've always disliked about these macros (even if I've
> never actually used them myself) is that they hide control flow from the
> caller, i.e. return directly. You don't see that in the code, it's not
> documented, and if you wanted to do better error handling yourself,
> you're out of luck.

Yeah, I agree that it's not the cleanest. That being said, it is
existing code and making the existing code less bloated seems worth
doing.

I'd also say that it feels worth it to have _some_ solution so that
the caller doesn't need to write error handling after every single cmd
sent. If we get rid of / discourage these macros that's either going
to end us up with ugly/verbose code or it's going to encourage people
to totally skip error handling. IMO neither of those are wonderful
solutions.

While thinking about this there were a few ideas I came up with. None
of them are amazing, but probably they are better than the hidden
"return" like this. Perhaps we could mark the current function as
"deprecated" and pick one of these depending on what others opinions
are:

1. Use "goto" and force the caller to give a goto target for error handling.

This is based on an idea that Dmitry came up with, but made a little
more explicit. Example usage:

int ret;

ret = 0;
mipi_dsi_dcs_write_seq_goto(dsi, , HX83102_SETSPCCMD, 0xcd,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, , HX83102_SETMIPI, 0x84,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, , HX83102_SETSPCCMD, 0x3f,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, , HX83102_SETVDC, 0x1b, 0x04,
some_cmd_failed);

...

some_cmd_failed:
  pr_err("Commands failed to write: %d", ret);
  return ret;
}

One downside here is that you can't easily tell which command failed
to put it in the error message. A variant of this idea (1a?) could be
to hoist the print back into the write command. I'd want to pick one
or the other. I guess my preference would be to hoist the print into
the write command and if someone really doesn't want the print then
they call mipi_dsi_dcs_write_buffer() directly.

---

2. Accept that a slightly less efficient handling of the error case
and perhaps a less intuitive API, but avoid the goto.

Essentially you could pass in "ret" and have the function be a no-op
if an error is already present. Something like this:

void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi,
const void *data, size_t len, int *accum_ret)
{
  if (*accum_ret)
return;

  *accum_ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
}

...and then the caller:

int ret;

ret = 0;
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0xcd, );
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETMIPI, 0x84, );
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0x3f, );
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETVDC, 0x1b, 0x04, );
if (ret)
  goto some_cmd_failed;

This has similar properties to solution #1.

---

3. Accept that callers don't want to error handling but just need a print.

I'm not 100% 

Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Doug Anderson]

Hi,

On Wed, Apr 24, 2024 at 4:37 PM Dmitry Baryshkov
 wrote:
>
> On Thu, 25 Apr 2024 at 02:25, Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov
> >  wrote:
> > >
> > > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang  wrote:
> > > >
> > > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> > > >  wrote:
> > > > >
> > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson  
> > > > > wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang 
> > > > > > > >  wrote:
> > > > > > > > >
> > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > >
> > > > > > > > > > > > This is the third panel driver using the same appoach. 
> > > > > > > > > > > > Can you use
> > > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? 
> > > > > > > > > > > > Or if you prefer
> > > > > > > > > > > > the table, we should extract this framework to a common 
> > > > > > > > > > > > helper.
> > > > > > > > > > > > (my preference is shifted towards 
> > > > > > > > > > > > mipi_dsi_generic_write_seq()).
> > > > > > > > > > > >
> > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it 
> > > > > > > > > > > can cause the
> > > > > > > > > > > kernel size grows a lot since every sequence will be 
> > > > > > > > > > > expanded.
> > > > > > > > > > >
> > > > > > > > > > > Similar discussion in here:
> > > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_js7tf1tsaazbb...@mail.gmail.com/
> > > > > > > > > > >
> > > > > > > > > > > This patch would increase the module size from 157K to 
> > > > > > > > > > > 572K.
> > > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > > > > >
> > > > > > > > > > > So maybe the common helper is better regarding the kernel 
> > > > > > > > > > > module size?
> > > > > > > > > >
> > > > > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and 
> > > > > > > > > > usleep_range() should be
> > > > > > > > > > used instead (and it's up to the developer to select 
> > > > > > > > > > correct delay
> > > > > > > > > > function).
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +static const struct panel_init_cmd 
> >

Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Doug Anderson]

Hi,

On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov
 wrote:
>
> On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang  wrote:
> >
> > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> >  wrote:
> > >
> > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson  wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > >  wrote:
> > > > >
> > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang  
> > > > > > wrote:
> > > > > > >
> > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > +
> > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > >
> > > > > > > > > > This is the third panel driver using the same appoach. Can 
> > > > > > > > > > you use
> > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or 
> > > > > > > > > > if you prefer
> > > > > > > > > > the table, we should extract this framework to a common 
> > > > > > > > > > helper.
> > > > > > > > > > (my preference is shifted towards 
> > > > > > > > > > mipi_dsi_generic_write_seq()).
> > > > > > > > > >
> > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can 
> > > > > > > > > cause the
> > > > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > > > >
> > > > > > > > > Similar discussion in here:
> > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_js7tf1tsaazbb...@mail.gmail.com/
> > > > > > > > >
> > > > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > > >
> > > > > > > > > So maybe the common helper is better regarding the kernel 
> > > > > > > > > module size?
> > > > > > > >
> > > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() 
> > > > > > > > should be
> > > > > > > > used instead (and it's up to the developer to select correct 
> > > > > > > > delay
> > > > > > > > function).
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +static const struct panel_init_cmd 
> > > > > > > > > > > kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > >
> > > > > > > > [skipped the body of the table]
> > > > > > > >
> > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > > > +
> > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > >
> > > > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > > > >
> > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > > > >
> > > 

Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 7:42 PM cong yang
 wrote:
>
> Hi,
>  Thanks reply.
>
> Doug Anderson  于2024年4月24日周三 00:26写道：
> >
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 2:37 AM cong yang
> >  wrote:
> > >
> > > > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > > > +{
> > > > > +   struct mipi_dsi_device *dsi = ctx->dsi;
> > > > > +
> > > > > +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 
> > > > > 0x21, 0x55, 0x00);
> > > > > +
> > > > > +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 
> > > > > 0xB5, 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > > > + 0x36, 0x36, 0x36, 
> > > > > 0x36, 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > > > + 0xFF, 0x8F, 0xFF, 
> > > > > 0x08, 0x74, 0x33);
> > > >
> > > > I know this is a sticking point between Linus W. and me, but I'm
> > > > really not a fan of all the hardcoded function calls since it bloats
> > > > the code so much. I think we need to stick with something more table
> > > > based at least for the majority of the commands. If I understand
> > > > correctly, Linus was OK w/ something table based as long as it was in
> > > > common code [1]. I think he also wanted the "delay" out of the table,
> > > > but since those always seem to be at the beginning or the end it seems
> > > > like we could still have the majority of the code as table based. Do
> > > > you want to make an attempt at that? If not I can try to find some
> > > > time to write up a patch in the next week or so.
> > >
> > > Do you mean not add "delay" in the table?  However, the delay
> > > required by each panel may be different. How should this be handled?
> >
> > In the case of the "himax-hx83102" driver, it looks as if all the
> > delays are at the beginning or end of the init sequence. That means
> > you could just make those extra parameters that are set per-panel and
> > you're back to having a simple sequence without delays.
>
> Do you mean add msleep  in hx83102_enable()?
>
> @@ -612,12 +604,15 @@ static int hx83102_enable(struct drm_panel *panel)
> struct device *dev = >dev;
> int ret;
>
> +   msleep(60);
> ret = ctx->desc->init_cmds(ctx);
> if (ret) {
> dev_err(dev, "Panel init cmds failed: %d\n", ret);
> return ret;
> }
>
> +   msleep(60);
> +
> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>
> >
> > If you had panels that needed delays in a more complicated way, you
> > could keep the per-panel functions but just make the bulk of the
> > function calls apply a sequence. For instance:
> >
> > static int my_panel_init_cmd(...)
> > {
> >   ret = mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
> >   if (ret)
> > return ret;
> >   mdelay(100);
> >   ret = mipi_dsi_dcs_write(dsi, ...);
> >   if (ret)
> > return ret;
> >   mdelay(50);
> >   ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> >   if (ret)
> > return ret;
> > }
> >
> > The vast majority of the work is still table driven so it doesn't
> > bloat the code, but you don't have the "delay" in the command sequence
> > since Linus didn't like it. I think something like the above would
> > make Linus happy and I'd be OK w/ it as well. Ideally you should still
> > make your command sequence as easy to understand as possible, kind of
> > like how we did with _INIT_SWITCH_PAGE_CMD() in
> > "panel-ilitek-ili9882t.c"
> >
> > As part of this, you'd have to add a patch to create
> > mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
> > complicated?
> >
> >
> > > It would be great if you could help provide a patch. Thank you so much.
> >
> > Sure, I can, though maybe you want to give it a shot with the above 
> > description?
>
> Sorry, I still don't seem to understand. How to encapsulate the parameters of
> "HX83102_SETDISP, HX83102_SETCYC,."? Different parameters for each panel.
> I have sent my V3 but it does not contain the patch you want.

It sounds as if Dmitry has come up with a solution that allows us to
keep using the functions like you've been using but avoid the code
bloat problems. ...so let's go with that. It sounds as if he's going
to send a patch before too long and then it should be pretty easy to
convert your patches over to use his new function.

[1] 
https://lore.kernel.org/r/CAA8EJprv3qBd1hfdWHrfhY=s0w2o70dznyb6tvss6agrpxs...@mail.gmail.com

Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Doug Anderson]

Hi,

On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
 wrote:
>
> > The problem is that with these panels that need big init sequences the
> > code based solution is _a lot_ bigger. If it were a few bytes or a
> > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > from a table to code it was 100K difference in code [1]. I would also
> > say that having these long init sequences done as separate commands
> > encourages people to skip checking the return values of each of the
> > transfer functions and I don't love that idea.
> >
> > It would be ideal if these panels didn't need these long init
> > sequences, but I don't have any inside knowledge here saying that they
> > could be removed. So assume we can't get rid of the init sequences it
> > feels like we have to find some way to make the tables work for at
> > least the large chunks of init code and encourage people to make the
> > tables readable...
>
>
> I did a quick check on the boe-tv101wum-nl6 driver by converting the
> writes to use the following macro:
>
> #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
>  \
> do {  
>  \
> static const u8 d[] = { cmd, seq };\
> ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));\
> if (ret < 0)  
>  \
> goto err; 
>  \
> } while (0)
>
> And then at the end of the init funciton having
>
> err:
> dev_err(panel->dev,
> "failed to write command %d\n", ret);
> return ret;
> }
>
> Size comparison:
>textdata bss dec hex filename
> before
>   34109   10410  18   44537adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> making init data const
>   44359 184   0   44543adff
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> with new macros
>   44353 184   0   44537adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
>
> As you can see, there is literally no size difference with this macro in 
> place.
> The only drawback is that the init stops on the first write rather
> than going through the sequence.
>
> WDYT? I can turn this into a proper patch if you think this makes sense.

Ah, so what you're saying is that the big bloat from using the
existing mipi_dsi_dcs_write_seq() is the error printing. That makes
sense. ...and by relying on the caller to provide an error handling
label we can get rid of the overhead and still get the error prints.

Yes, that seems pretty reasonable to me. I guess I'd perhaps make the
error label a parameter to the macro (so it's obvious that the caller
needs to define it) and maybe name it in such a way to make it obvious
the difference between this macro and mipi_dsi_dcs_write_seq().

With that and your measurements then this seems perfectly reasonable
to me and I'm good with fully moving away from the table-based
approach. I'd be happy if you sent a patch for it and happy to review
it.

-Doug

Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
 wrote:
>
> On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang  wrote:
> > >
> > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > +
> > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > + .type = DELAY_CMD,\
> > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > >
> > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you 
> > > > > > prefer
> > > > > > the table, we should extract this framework to a common helper.
> > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > >
> > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > kernel size grows a lot since every sequence will be expanded.
> > > > >
> > > > > Similar discussion in here:
> > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_js7tf1tsaazbb...@mail.gmail.com/
> > > > >
> > > > > This patch would increase the module size from 157K to 572K.
> > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > >
> > > > > So maybe the common helper is better regarding the kernel module size?
> > > >
> > > > Yes, let's get a framework done in a useful way.
> > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > used instead (and it's up to the developer to select correct delay
> > > > function).
> > > >
> > > > >
> > > > > > > +
> > > > > > > +static const struct panel_init_cmd 
> > > > > > > kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > >
> > > > [skipped the body of the table]
> > > >
> > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > +
> > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > >
> > > > > > > + _INIT_DCS_CMD(0X11),
> > > >
> > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > >
> > > > > > > + /* T6: 120ms */
> > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > + _INIT_DCS_CMD(0X29),
> > > >
> > > > And this is mipi_dsi_dcs_set_display_on().
> > > >
> > > > Having a single table enourages people to put known commands into the
> > > > table, the practice that must be frowned upon and forbidden.
> > > >
> > > > We have functions for some of the standard DCS commands. So, maybe
> > > > instead of adding a single-table based approach we can improve
> > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > error handling to a common part of enable() / prepare() function.
> > > >
> > >
> > > For this panel, I think it can also refer to how
> > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > looping through the table.
> >
> > Even more similar discussion:
> >
> > https://lore.kernel.org/r/CAD=FV=ugdbnvamjzwsovxybgikqcvw9jsrtbxhvg8_97ype...@mail.gmail.com
>
> It seems I skipped that thread.
>
> I'd still suggest a code-based solution compared to table-based one, for
> the reasons I've outlined before. Having a tables puts a pressure on the
> developer to put commands there for which we already have a
> command-specific function.

The problem is that with these panels that need big init sequences the
code based solution is _a lot_ bigger. If it were a few bytes or a
1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
from a table to code it was 100K difference in code [1]. I would also
say that having these long init sequences done as separate commands
encourages people to skip checking the return values of each of the
transfer functions and I don't love that idea.

It would be ideal if these panels didn't need these long init
sequences, but I don't have any inside knowledge here saying that they
could be removed. So assume we can't get rid of the init sequences it
feels like we have to find some way to make the tables work for at
least the large chunks of init code and encourage people to make the
tables readable...


[1] 
https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nyu_hae0_...@mail.gmail.com

Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang  wrote:
>
> > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > + .type = INIT_DCS_CMD, \
> > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > +
> > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > + .type = DELAY_CMD,\
> > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > + .data = (char[]){__VA_ARGS__} }
> > > >
> > > > This is the third panel driver using the same appoach. Can you use
> > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > the table, we should extract this framework to a common helper.
> > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > >
> > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > kernel size grows a lot since every sequence will be expanded.
> > >
> > > Similar discussion in here:
> > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_js7tf1tsaazbb...@mail.gmail.com/
> > >
> > > This patch would increase the module size from 157K to 572K.
> > > scripts/bloat-o-meter shows chg +235.95%.
> > >
> > > So maybe the common helper is better regarding the kernel module size?
> >
> > Yes, let's get a framework done in a useful way.
> > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > used instead (and it's up to the developer to select correct delay
> > function).
> >
> > >
> > > > > +
> > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = 
> > > > > {
> > > > > + _INIT_DELAY_CMD(50),
> > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> >
> > [skipped the body of the table]
> >
> > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > +
> > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> >
> > > > > + _INIT_DCS_CMD(0X11),
> >
> > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> >
> > > > > + /* T6: 120ms */
> > > > > + _INIT_DELAY_CMD(120),
> > > > > + _INIT_DCS_CMD(0X29),
> >
> > And this is mipi_dsi_dcs_set_display_on().
> >
> > Having a single table enourages people to put known commands into the
> > table, the practice that must be frowned upon and forbidden.
> >
> > We have functions for some of the standard DCS commands. So, maybe
> > instead of adding a single-table based approach we can improve
> > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > error handling to a common part of enable() / prepare() function.
> >
>
> For this panel, I think it can also refer to how
> panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> not what operation to use, and use mipi_dsi_generic_write_seq() when
> looping through the table.

Even more similar discussion:

https://lore.kernel.org/r/CAD=FV=ugdbnvamjzwsovxybgikqcvw9jsrtbxhvg8_97ype...@mail.gmail.com

Re: [PATCH] drm/panel-edp: Add panel CSOT MNB601LS1-1

[Doug Anderson]

Hi,

On Mon, Apr 22, 2024 at 6:28 PM Xuxin Xiong
 wrote:
>
> Yes, I read the edid from the panels, one is CSO and the other is CSW.
> The details are as follows, please help check. Thank you!
>
>
> 1. MNC207QS1-1
> edid-decode (hex):
> 00 ff ff ff ff ff ff 00 0e 6f 00 12 e7 00 00 00
> 1e 21 01 04 a5 1b 12 78 03 8a d5 9c 5e 59 90 25
> 1b 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 ae 3f 80 18 71 b0 23 40 30 20
> 36 00 07 a4 10 00 00 1a 00 00 00 fd 00 28 3c 4b
> 4b 11 01 0a 20 20 20 20 20 20 00 00 00 fe 00 43
> 53 4f 54 20 54 39 0a 20 20 20 20 20 00 00 00 fe
> 00 4d 4e 43 32 30 37 51 53 31 2d 31 0a 20 00 32
> 
> Block 0, Base EDID:
> EDID Structure Version & Revision: 1.4
> Vendor & Product Identification:
> Manufacturer: CSO
> Model: 4608
> Serial Number: 231
> Made in: week 30 of 2023
> Basic Display Parameters & Features:
> Digital display
> Bits per primary color channel: 8
> DisplayPort interface
> Maximum image size: 27 cm x 18 cm
> Gamma: 2.20
> Supported color formats: RGB 4:4:4
> First detailed timing includes the native pixel format and preferred refresh 
> rate
> Display is continuous frequency
> Color Characteristics:
> Red : 0.6113, 0.3671
> Green: 0.3496, 0.5644
> Blue : 0.1474, 0.1064
> White: 0.3134, 0.3291
> Established Timings I & II: none
> Standard Timings: none
> Detailed Timing Descriptors:
> DTD 1: 1920x1200 60.000 Hz 8:5 74.100 kHz 163.020 MHz (263 mm x 164 mm)
> Hfront 48 Hsync 32 Hback 200 Hpol P
> Vfront 3 Vsync 6 Vback 26 Vpol N
> Display Range Limits:
> Monitor ranges (Bare Limits): 40-60 Hz V, 75-75 kHz H, max dotclock 170 MHz
> Alphanumeric Data String: 'CSOT T9'
> Alphanumeric Data String: 'MNC207QS1-1'
> Checksum: 0x32
>
> 2. MNB601LS1-1
> edid-decode (hex):
>
> 00 ff ff ff ff ff ff 00 0e 77 00 11 00 00 00 00
> 00 22 01 04 95 1a 0e 78 03 a1 35 9b 5e 58 91 25
> 1c 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 09 1e 56 dc 50 00 28 30 30 20
> 36 00 00 90 10 00 00 1a 00 00 00 fd 00 28 3c 30
> 30 08 01 0a 20 20 20 20 20 20 00 00 00 fe 00 43
> 53 4f 54 20 54 39 0a 20 20 20 20 20 00 00 00 fe
> 00 4d 4e 42 36 30 31 4c 53 31 2d 31 0a 20 00 37
> 
> Block 0, Base EDID:
> EDID Structure Version & Revision: 1.4
> Vendor & Product Identification:
> Manufacturer: CSW
> Model: 4352
> Made in: 2024
> Basic Display Parameters & Features:
> Digital display
> Bits per primary color channel: 6
> DisplayPort interface
> Maximum image size: 26 cm x 14 cm
> Gamma: 2.20
> Supported color formats: RGB 4:4:4
> First detailed timing includes the native pixel format and preferred refresh 
> rate
> Display is continuous frequency
> Color Characteristics:
> Red : 0.6074, 0.3691
> Green: 0.3437, 0.5673
> Blue : 0.1445, 0.1123
> White: 0.3134, 0.3291
> Established Timings I & II: none
> Standard Timings: none
> Detailed Timing Descriptors:
> DTD 1: 1366x768 60.001 Hz 683:384 48.480 kHz 76.890 MHz (256 mm x 144 mm)
> Hfront 48 Hsync 32 Hback 140 Hpol P
> Vfront 3 Vsync 6 Vback 31 Vpol N
> Display Range Limits:
> Monitor ranges (Bare Limits): 40-60 Hz V, 48-48 kHz H, max dotclock 80 MHz
> Alphanumeric Data String: 'CSOT T9'
> Alphanumeric Data String: 'MNB601LS1-1'
> Checksum: 0x37

OK. This made me look a little deeper. I believe that the three-letter
code is managed by UEFI and I found:

https://uefi.org/PNP_ID_List

...as far as I can tell "CSW" is actually correct for CSOT but "CSO"
was _not_. Looks as if CSO was supposed to be for "California
Institute of Technology" (Caltech). :(

We're probably OK with the recent work that Hsin-Yi did in commit
bf201127c1b8 ("drm/panel-edp: Match edp_panels with panel identity")
to match against the panel name even if Caltech does ship a "CSO"
0x1200 in the future since it looks like the string you added matches
the panel.

That being said, is there any chance that it's not too late and you
can get CSOT to fix the EDID on "MNC207QS1-1"? Can you also make sure
that they're aware of this and don't make the same mistake in the
future?


In any case, given this investigation and the EDID I'm going to say
that the panel you've added here is fine.

Reviewed-by: Douglas Anderson 

I've pushed your change to drm-misc-next:

a6325ad47bc8 drm/panel-edp: Add panel CSOT MNB601LS1-1

-Doug

Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 2:37 AM cong yang
 wrote:
>
> > > +static int starry_init_cmd(struct hx83102 *ctx)
> > > +{
> > > +   struct mipi_dsi_device *dsi = ctx->dsi;
> > > +
> > > +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 
> > > 0x55, 0x00);
> > > +
> > > +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 
> > > 0x31, 0xF1, 0x31, 0xD7, 0x2F,
> > > + 0x36, 0x36, 0x36, 0x36, 
> > > 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> > > + 0xFF, 0x8F, 0xFF, 0x08, 
> > > 0x74, 0x33);
> >
> > I know this is a sticking point between Linus W. and me, but I'm
> > really not a fan of all the hardcoded function calls since it bloats
> > the code so much. I think we need to stick with something more table
> > based at least for the majority of the commands. If I understand
> > correctly, Linus was OK w/ something table based as long as it was in
> > common code [1]. I think he also wanted the "delay" out of the table,
> > but since those always seem to be at the beginning or the end it seems
> > like we could still have the majority of the code as table based. Do
> > you want to make an attempt at that? If not I can try to find some
> > time to write up a patch in the next week or so.
>
> Do you mean not add "delay" in the table?  However, the delay
> required by each panel may be different. How should this be handled?

In the case of the "himax-hx83102" driver, it looks as if all the
delays are at the beginning or end of the init sequence. That means
you could just make those extra parameters that are set per-panel and
you're back to having a simple sequence without delays.

If you had panels that needed delays in a more complicated way, you
could keep the per-panel functions but just make the bulk of the
function calls apply a sequence. For instance:

static int my_panel_init_cmd(...)
{
  ret = mipi_dsi_dcs_write_cmd_seq(dsi, my_panel_init_cmd_seq);
  if (ret)
return ret;
  mdelay(100);
  ret = mipi_dsi_dcs_write(dsi, ...);
  if (ret)
return ret;
  mdelay(50);
  ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
  if (ret)
return ret;
}

The vast majority of the work is still table driven so it doesn't
bloat the code, but you don't have the "delay" in the command sequence
since Linus didn't like it. I think something like the above would
make Linus happy and I'd be OK w/ it as well. Ideally you should still
make your command sequence as easy to understand as possible, kind of
like how we did with _INIT_SWITCH_PAGE_CMD() in
"panel-ilitek-ili9882t.c"

As part of this, you'd have to add a patch to create
mipi_dsi_dcs_write_cmd_seq(), but hopefully that shouldn't be too
complicated?


> It would be great if you could help provide a patch. Thank you so much.

Sure, I can, though maybe you want to give it a shot with the above description?

-Doug

Re: [PATCH v2 3/7] arm64: defconfig: Enable HIMAX_HX83102 panel

[Doug Anderson]

Hi,

On Mon, Apr 22, 2024 at 2:03 AM Cong Yang
 wrote:
>
> DRM_PANEL_HIMAX_HX83102 is being split out from DRM_PANEL_BOE_TV101WUM_NL6.
> Since the arm64 defconfig had the BOE panel driver enabled, let's also
> enable the himax driver.
>
> Signed-off-by: Cong Yang 
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson 

Re: [PATCH v2 2/7] drm/panel: himax-hx83102: Break out as separate driver

[Doug Anderson]

Hi,

On Mon, Apr 22, 2024 at 2:03 AM Cong Yang
 wrote:
>
> The Starry HX83102 based mipi panel should never have been part of the boe
> tv101wum driver. Discussion with Doug and Linus in V1 [1], we need a
> separate driver to enable the hx83102 controller.
>
> In hx83102 driver, add DSI commands as macros. So it can add some panels
> with same control model in the future.
>
> [1]: 
> https://lore.kernel.org/all/CACRpkdbzYZAS0=zbqjuc4cb2wj4s1h6n6asazqvdmv95r3z...@mail.gmail.com
>
> Signed-off-by: Cong Yang 
> ---
>  drivers/gpu/drm/panel/Kconfig |   9 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c|  99 ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c   | 567 ++
>  4 files changed, 577 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/gpu/drm/panel/panel-himax-hx83102.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d037b3b8b999..eb378c897353 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -145,6 +145,15 @@ config DRM_PANEL_LVDS
>   handling of power supplies or control signals. It implements 
> automatic
>   backlight handling if the panel is attached to a backlight 
> controller.
>
> +config DRM_PANEL_HIMAX_HX83102
> +   tristate "himax HX83102-based panels"

Capital "h" for "Himax".


> +#define DRV_NAME "panel-himax-hx83102"

I don't think DRV_NAME buys you very much. Get rid of this #define and
just use it below.


> +struct hx83102 {
> +   struct drm_panel base;
> +   struct mipi_dsi_device *dsi;
> +
> +   const struct hx83102_panel_desc *desc;
> +
> +   enum drm_panel_orientation orientation;
> +   struct regulator *pp1800;
> +   struct regulator *avee;
> +   struct regulator *avdd;
> +   struct gpio_desc *enable_gpio;
> +
> +   bool prepared;

We're trying to get rid of the tracking of "prepared" in panels. You
should be able to delete this and remove the code dealing with it. The
core DRM code should ensure that your prepare/unprepare functions are
called appropriately.



> +struct hx83102_panel_desc {
> +   const struct drm_display_mode *modes;
> +   unsigned int bpc;
> +
> +   /**
> +* @width_mm: width of the panel's active display area
> +* @height_mm: height of the panel's active display area
> +*/
> +   struct {
> +   unsigned int width_mm;
> +   unsigned int height_mm;
> +   } size;
> +
> +   unsigned long mode_flags;
> +   enum mipi_dsi_pixel_format format;
> +   unsigned int lanes;
> +   bool lp11_before_reset;

Seems like you can remove "lp11_before_reset" since it's always true
for this controller. If later you find someone using this controller
that needs this false then we can always add it back in.

I think you could also remove "bpc", "format", and "mode_flags". If
these are all the same controller then that will be common between all
the panels, right? So you shouldn't need to define those on a
per-panel basis... You could maybe even remove "lanes" unless some
people using this panel are expected to hook up fewer lanes...


> +static int starry_init_cmd(struct hx83102 *ctx)
> +{
> +   struct mipi_dsi_device *dsi = ctx->dsi;
> +
> +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 
> 0x00);
> +
> +   mipi_dsi_dcs_write_seq(dsi, HX83102_SETPOWER, 0x2C, 0xB5, 0xB5, 0x31, 
> 0xF1, 0x31, 0xD7, 0x2F,
> + 0x36, 0x36, 0x36, 0x36, 
> 0x1A, 0x8B, 0x11, 0x65, 0x00, 0x88, 0xFA, 0xFF,
> + 0xFF, 0x8F, 0xFF, 0x08, 
> 0x74, 0x33);

I know this is a sticking point between Linus W. and me, but I'm
really not a fan of all the hardcoded function calls since it bloats
the code so much. I think we need to stick with something more table
based at least for the majority of the commands. If I understand
correctly, Linus was OK w/ something table based as long as it was in
common code [1]. I think he also wanted the "delay" out of the table,
but since those always seem to be at the beginning or the end it seems
like we could still have the majority of the code as table based. Do
you want to make an attempt at that? If not I can try to find some
time to write up a patch in the next week or so.

[1] 
https://lore.kernel.org/r/CACRpkdYtM=5jdqddcqrfgbrxvcjejk1uljnkkfz7jhhkgxv...@mail.gmail.com

...also: nit that, in general, the Linux community prefers lowercase
hex, so 0xfa instead of 0xFA, for instance.


> +static int hx83102_get_modes(struct drm_panel *panel,
> +   struct drm_connector *connector)
> +{
> +   struct hx83102 *ctx = panel_to_hx83102(panel);
> +   const struct drm_display_mode *m = ctx->desc->modes;
> +   struct drm_display_mode *mode;
> +
> +   mode = drm_mode_duplicate(connector->dev, m);
> +   if 

Re: [PATCH v2 1/7] dt-bindings: display: panel: Add himax hx83102 panel bindings

[Doug Anderson]

Hi,

On Mon, Apr 22, 2024 at 8:14 AM Rob Herring  wrote:
>
> > +additionalProperties: false
>
> Perhaps 'unevaluatedProperties' instead. Don't you want to support
> standard properties such as width-mm/height-mm?

For at least those two properties, it looks like the answer is "no".
>From reading the Linux driver it appears that physical width/height is
implied by the compatible string. Unless there are other properties we
think we need, maybe it's fine to keep "additionalProperties" ?

-Doug

Re: [PATCH] drm/panel-edp: Add panel CSOT MNB601LS1-1

[Doug Anderson]

Hi,

On Sun, Apr 21, 2024 at 11:09 PM Xuxin Xiong
 wrote:
>
> Add support for the following panel:
> CSOT MNB601LS1-1
>
> Signed-off-by: Xuxin Xiong 
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c 
> b/drivers/gpu/drm/panel/panel-edp.c
> index d58f90bc48fb..5e0b1c94bc62 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -2036,6 +2036,8 @@ static const struct edp_panel_entry edp_panels[] = {
>
> EDP_PANEL_ENTRY('C', 'S', 'O', 0x1200, _200_500_e50, 
> "MNC207QS1-1"),
>
> +   EDP_PANEL_ENTRY('C', 'S', 'W', 0x1100, _200_500_e80_d50, 
> "MNB601LS1-1"),

Are you positive that both this panel and the one above it (which you
also added) are correct? You identified both of them as CSO panels but
one has "CSO" and one has "CSW". Please double-check and let me know.

-Doug

Re: [PATCH v1 2/2] HID: i2c-hid: elan: Add ili2900 timing

[Doug Anderson]

Hi,

On Thu, Apr 18, 2024 at 5:48 AM lvzhaoxiong
 wrote:
>
> ILI2900 requires reset to pull down time greater than 10ms,
> so the configuration post_power_delay_ms is 10, and the chipset
> initial time is required to be greater than 100ms,
> so the post_gpio_reset_on_delay_ms is set to 100.
>
> Signed-off-by: lvzhaoxiong 

Please use a proper name instead of "lvzhaoxiong".


> ---
>  drivers/hid/i2c-hid/i2c-hid-of-elan.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c 
> b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> index 5b91fb106cfc..3073620b2dec 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
> @@ -137,10 +137,18 @@ static const struct elan_i2c_hid_chip_data 
> ilitek_ili2901_chip_data = {
> .main_supply_name = "vcc33",
>  };
>
> +static const struct elan_i2c_hid_chip_data ilitek_ili2900_chip_data = {
> +   .post_power_delay_ms = 10,
> +   .post_gpio_reset_on_delay_ms = 100,
> +   .hid_descriptor_address = 0x0001,
> +   .main_supply_name = NULL,

There's really no main power supply for this device? It feels likely
that there is one.

Also: other than the main power supply, there is no difference between
this and the ili2901. If you actually do have a main power supply,
then you probably don't need a new table. You probably don't even need
your own compatible string and in the device tree you could just
specify:

compatible = "ilitek,ili2900, "ilitek,ili2901";

...which says "I actually have an ILI 2900, but if you don't have any
special driver for the ILI 2900 it's likely that the driver for the
ILI 2901 will work because the hardware is almost the same."


> +};
> +
>  static const struct of_device_id elan_i2c_hid_of_match[] = {
> { .compatible = "elan,ekth6915", .data = _ekth6915_chip_data },
> { .compatible = "ilitek,ili9882t", .data = _ili9882t_chip_data 
> },
> { .compatible = "ilitek,ili2901", .data = _ili2901_chip_data },
> +   { .compatible = "ilitek,ili2900", .data = _ili2900_chip_data },

If you do need this, these should be sorted. 2900 should come before 2901.


-Doug

Re: [PATCH v1 2/4] drm/panel: boe-tv101wum-nl6: Support for BOE nv110wum-l60 MIPI-DSI panel

[Doug Anderson]

Hi,

On Wed, Apr 10, 2024 at 12:15 AM Cong Yang
 wrote:
>
> The BOE nv110wum-l60 is a 11.0" WUXGA TFT LCD panel, which fits in nicely
> with the existing panel-boe-tv101wum-nl6 driver. Hence, we add a new
> compatible with panel specific config.

I guess we have the same question we've had with this driver in the
past: do we add more tables here, or do we break this out into a
separate driver like we ended up doing with "ili9882t". I guess the
question is: what is the display controller used with this panel and
is it the same (or nearly the same) display controller as other panels
in this driver or is it a completely different display controller.
Maybe you could provide this information in the commit message to help
reviewers understand.


> Signed-off-by: Cong Yang 
> ---
>  .../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 115 ++
>  1 file changed, 115 insertions(+)

Maybe add Linus W to your patches since he has had opinions on this
driver in the past. I've added him as CC here but you should make sure
to CC him on future versions unless he says not to. ;-)


> diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c 
> b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> index 0ffe8f8c01de..f91827e1548c 100644
> --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> @@ -1368,6 +1368,91 @@ static const struct panel_init_cmd 
> starry_himax83102_j02_init_cmd[] = {
> {},
>  };
>
> +static const struct panel_init_cmd boe_nv110wum_init_cmd[] = {
> +   _INIT_DELAY_CMD(60),
> +   _INIT_DCS_CMD(0xB9, 0x83, 0x10, 0x21, 0x55, 0x00),

Given that the first command of "(0xB9, 0x83, 0x10, 0x21, 0x55, 0x00)"
seems to be the same as "starry_himax83102_j02" maybe those two are
the same controller? I'm just guessing, but if those are the same
controller as the two new ones you're adding in this series, maybe all
3 of them should be in their own driver? Maybe we can do something to
make more sense of some of these commands too? There certainly seem to
be a lot of commonalities in the init sequences of all 3 and if we can
define the init sequence more logically then we can share more of the
code between the different panels and we don't have a giant duplicated
blob.


> +   _INIT_DCS_CMD(0xB9, 0x00, 0x00, 0x00),
> +   _INIT_DELAY_CMD(50),
> +   _INIT_DCS_CMD(0x11),
> +   _INIT_DELAY_CMD(110),
> +   _INIT_DCS_CMD(0x29),
> +   _INIT_DELAY_CMD(25),
> +   {},
> +};
>  static inline struct boe_panel *to_boe_panel(struct drm_panel *panel)

nit: should have a blank line between the end of your struct and the
next function.


> +static const struct panel_desc boe_nv110wum_desc = {
> +   .modes = _tv110wum_default_mode,
> +   .bpc = 8,
> +   .size = {
> +   .width_mm = 147,
> +   .height_mm = 235,
> +   },
> +   .lanes = 4,
> +   .format = MIPI_DSI_FMT_RGB888,
> +   .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_MODE_LPM,
> +   .init_cmds = boe_nv110wum_init_cmd,
> +   .lp11_before_reset = true,
> +};
>  static int boe_panel_get_modes(struct drm_panel *panel,
>struct drm_connector *connector)

nit: should have a blank line between the end of your struct and the
next function.


> @@ -1973,6 +2085,9 @@ static const struct of_device_id boe_of_match[] = {
> { .compatible = "starry,himax83102-j02",
>   .data = _himax83102_j02_desc
> },
> +   { .compatible = "boe,nv110wum-l60",
> + .data = _nv110wum_desc
> +   },

nit: the existing panels that are supported are sorted alphabetically.
Please sort things alphabetically throughout your patch series.

-Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix ti_sn_bridge_set_dsi_rate function

[Doug Anderson]

Hi,

On Wed, Apr 10, 2024 at 4:42 AM Jayesh Choudhary  wrote:
>
> Hello Doug,
>
> Thanks for the review.
>
> On 08/04/24 14:33, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 8, 2024 at 12:36 AM Jayesh Choudhary  wrote:
> >>
> >> Due to integer calculations, the rounding off can cause errors in the final
> >> value propagated in the registers.
> >> Considering the example of 1080p (very common resolution), the mode->clock
> >> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> >> clock frequency would come as 444 when we are expecting the value 445.5
> >> which would reflect in SN_DSIA_CLK_FREQ_REG.
> >> So move the division to be the last operation where rounding off will not
> >> impact the register value.
> >
> > Given that this driver is used on a whole pile of shipping Chromebooks
> > and those devices have been working just fine (with 1080p resolution)
> > for years, I'm curious how you noticed this. Was it actually causing
> > real problems for you, or did you notice it just from code inspection?
> > You should include this information in the commit message.
>
> I am trying to add display support for TI SoC which uses this particular
> bridge. While debugging, I was trying to get all the register value in
> sync with the datasheet and it was then that I observed this issue while
> inspecting the code.
> Maybe Chromebooks are using different set of parameters which does not
> expose this issue. Since parameters for my display (mentioned in commit
> message) yields the frequency at the border, I saw this issue. My debug
> is still ongoing but since the value is not in sync with the
> documentation, I sent out this patch.

OK, sounds good. It would be good to include some of this type of into
in the patch description for the next version.


> > I'm travelling for the next two weeks so I can't actually check on a
> > device to see if your patch makes any difference on hardware I have,
> > but I'd presume that things were working "well enough" with the old
> > value and they'll still work with the new value?
> >
> >
>
> Yes, ideally they should still work well with this change.

OK, I can validate it in a few weeks.


> >> Also according to the SN65DSI86 datasheet[0], the minimum value for that
> >> reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
> >> check for that.
> >
> > Maybe the range checking should be a separate patch?
>
> Check should be done before propagating the register value so I added it
> in the same function and hence in the same patch.

I was thinking you could have patch #1 add the checks. ...then patch
#2 could fix the math.


> >> -#define MIN_DSI_CLK_FREQ_MHZ   40
> >> +/*
> >> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
> >> + * DSI clock frequency range is in 5-MHz increments
> >> + * So minimum frequency 40MHz translates to 0x08
> >> + * And maximum frequency 755MHz translates to 0x97
> >> + */
> >> +#define MIN_DSI_CLK_RANGE  0x8
> >> +#define MAX_DSI_CLK_RANGE  0x97
> >
> > It's a little weird to call this min/max and have one be inclusive and
> > one be exclusive. Be consistent and say that this is the minimum legal
> > value and the maximum legal value. I think that means the MAX should
> > be 0x96.
>
> The comment above does specify the inclusive/exclusive behavior.
> Since a value corresponds to 5MHz range, associating the value with
> the range makes more sense if I keep it 0x97 (0x97 * 5 -> 755MHz)
> 0x96 corresponds to the range of [750Mz,755MHz).
>
> If this argument does not make sense, I can change it to 0x96 and handle
> it with the inequalities in the function call.

Right that the comment is correct so that's good, but I'd still like
to see the constants changing. For instance, if I had code like this:

/*
 * I know 2 * 2 is not really 5, but it makes my math work out
 * so we'll just define it that way.
 */
#define TWO_TIMES_TWO 5

...and then later you had code:

if (x * y >= TWO_TIMES_TWO)

When you read the code you probably wouldn't go back and read the
comment so you'd be confused. AKA the above would be better as:

#define TWO_TIMES_TWO 4

if (x * y > TWO_TIMES_TWO)

Better to make the name of the #define make sense on its own. In this
case "min" and "max" should be the minimum legal value and the maximum
legal value, not "one past".


> >> +*/
> >> +   bit_rate_khz = mode->clock *
> >> +  mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> >>

Re: [PATCH v2] dt-bindings: display: bridge: it6505: Add #sound-dai-cells

[Doug Anderson]

Hi,

On Wed, Mar 27, 2024 at 10:33 AM Rob Herring  wrote:
>
>
> On Wed, 27 Mar 2024 16:52:48 +0800, Chen-Yu Tsai wrote:
> > The ITE IT6505 display bridge can take one I2S input and transmit it
> > over the DisplayPort link.
> >
> > Add #sound-dai-cells (= 0) to the binding for it.
> >
> > Signed-off-by: Chen-Yu Tsai 
> > ---
> > Changes since v1 [1]:
> > - Reference /schemas/sound/dai-common.yaml
> > - Change "additionalProperties: false" to "unevaluatedProperties: false"
> >
> > The driver side changes [2] are still being worked on.
> >
> > [1] 
> > https://lore.kernel.org/dri-devel/20240126073511.2708574-1-we...@chromium.org/
> > [2] 
> > https://lore.kernel.org/linux-arm-kernel/20230730180803.22570-4-jiaxin...@mediatek.com/
> > ---
> >  .../devicetree/bindings/display/bridge/ite,it6505.yaml| 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
>
> Reviewed-by: Rob Herring 

Pushed to drm-misc-next:

325af1bef5b9 dt-bindings: display: bridge: it6505: Add #sound-dai-cells

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix ti_sn_bridge_set_dsi_rate function

[Doug Anderson]

Hi,

On Mon, Apr 8, 2024 at 12:36 AM Jayesh Choudhary  wrote:
>
> Due to integer calculations, the rounding off can cause errors in the final
> value propagated in the registers.
> Considering the example of 1080p (very common resolution), the mode->clock
> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> clock frequency would come as 444 when we are expecting the value 445.5
> which would reflect in SN_DSIA_CLK_FREQ_REG.
> So move the division to be the last operation where rounding off will not
> impact the register value.

Given that this driver is used on a whole pile of shipping Chromebooks
and those devices have been working just fine (with 1080p resolution)
for years, I'm curious how you noticed this. Was it actually causing
real problems for you, or did you notice it just from code inspection?
You should include this information in the commit message.

I'm travelling for the next two weeks so I can't actually check on a
device to see if your patch makes any difference on hardware I have,
but I'd presume that things were working "well enough" with the old
value and they'll still work with the new value?


> Also according to the SN65DSI86 datasheet[0], the minimum value for that
> reg is 0x08 (inclusive) and the maximum value is 0x97 (exclusive). So add
> check for that.

Maybe the range checking should be a separate patch?


> [0]: 
>
> Fixes: ca1b885cbe9e ("drm/bridge: ti-sn65dsi86: Split the setting of the dp 
> and dsi rates")

Are you sure that's the commit you're fixing? In the commit text of
that commit I wrote that it was supposed to "have zero functional
change". Looking back at the change I still believe it had zero
functional change. The rounding error looks like it predates the
patch.

As far as I can tell the rounding error has been there since commit
a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver").


> Signed-off-by: Jayesh Choudhary 

It's great to see a TI engineer contributing to this driver! Awesome!


> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 48 +--
>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 84698a0b27a8..f9cf6b14d85e 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -111,7 +111,14 @@
>  #define  AUX_IRQ_STATUS_AUX_SHORT  BIT(5)
>  #define  AUX_IRQ_STATUS_NAT_I2C_FAIL   BIT(6)
>
> -#define MIN_DSI_CLK_FREQ_MHZ   40
> +/*
> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
> + * DSI clock frequency range is in 5-MHz increments
> + * So minimum frequency 40MHz translates to 0x08
> + * And maximum frequency 755MHz translates to 0x97
> + */
> +#define MIN_DSI_CLK_RANGE  0x8
> +#define MAX_DSI_CLK_RANGE  0x97

It's a little weird to call this min/max and have one be inclusive and
one be exclusive. Be consistent and say that this is the minimum legal
value and the maximum legal value. I think that means the MAX should
be 0x96.


>  /* fudge factor required to account for 8b/10b encoding */
>  #define DP_CLK_FUDGE_NUM   10
> @@ -820,22 +827,37 @@ static void ti_sn_bridge_atomic_disable(struct 
> drm_bridge *bridge,
> regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 
> 0);
>  }
>
> -static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
>  {
> -   unsigned int bit_rate_mhz, clk_freq_mhz;
> +   unsigned int bit_rate_khz;
> unsigned int val;
> struct drm_display_mode *mode =
> >bridge.encoder->crtc->state->adjusted_mode;
>
> -   /* set DSIA clk frequency */
> -   bit_rate_mhz = (mode->clock / 1000) *
> -   mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> -   clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> +   /*
> +* Set DSIA clk frequency
> +* Maximum supported value of bit_rate_khz turns out to be
> +* 604 which can be put in 32-bit variable so no overflow
> +* possible in this calculation.

The way you've written this comment makes me worried. You're saying
that the only supported result of the math has to fit in 32-bits so
we're OK. ...and then _after_ you do the math you check to see if the
clock rate is within the supported range. It makes me feel like you
could still overflow.

I think your comment here should say something like:

The maximum value returned by mipi_dsi_pixel_format_to_bpp() is 24.
That means that as long as "mode->clock" is less than 178,956,971 kHz
then the calculation can't overflow and can fit in 32-bits.

If you wanted to be really good you could even put a check earlier in
the function to make sure that mode->clock wasn't something totally
crazy (could confirm it's < 100GHz maybe?) so you absolutely knew it
couldn't 

Re: [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 5:07 PM Hsin-Yi Wang  wrote:
>
> On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson  
> wrote:
> >
> > If we're using the AUX channel for eDP backlight and it fails to probe
> > for some reason, let's _not_ fail the panel probe.
> >
> > At least one case where we could fail to init the backlight is because
> > of a dead or physically missing panel. As talked about in detail in
> > the earlier patch in this series, ("drm/panel-edp: If we fail to
> > powerup/get EDID, use conservative timings"), this can cause the
> > entire system's display pipeline to fail to come up and that's
> > non-ideal.
> >
> > If we fail to init the backlight for some transitory reason, we should
> > dig in and see if there's a way to fix this (perhaps retries?). Even
> > in that case, though, having a panel whose backlight is stuck at 100%
> > (the default, at least in the panel Samsung ATNA33XC20 I tested) is
> > better than having no panel at all.
> >
> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Hsin-Yi Wang 

Pushed to drm-misc-next:

b48ccb18e642 drm-panel: If drm_panel_dp_aux_backlight() fails, don't
fail panel probe

Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 5:05 PM Hsin-Yi Wang  wrote:
>
> On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson  
> wrote:
> >
> > If at boot we fail to power up the eDP panel (most often happens if
> > the eDP panel never asserts HPD to us) or if we are unable to read the
> > EDID at bootup to figure out the panel's ID then let's use the
> > conservative eDP panel powerup/powerdown timings but _not_ fail the
> > probe.
> >
> > It might seem strange to _not_ fail the probe in this case since we
> > were unable to powerup the panel and confirm it's there. However,
> > there is a reason to do this. Specifically, if we fail to probe the
> > panel then it really throws the whole display pipeline for loop. Most
> > DRM subsystems are written so that they wait until all components
> > (including the panel) have probed before they set everything up. When
> > the panel doesn't come up then this never happens. As a side effect of
> > not setting everything up then other display adapters don't get
> > initialized. As a practical example, I can see that if I open up a
> > sc7180-trogdor based Chromebook that's using the generic "edp-panel"
> > and unplug the eDP panel that it causes the _external_ DP monitor not
> > to function. This is obviously bad because it means that a device with
> > a dead eDP panel becomes e-waste when it could instead still be given
> > useful life with an external display.
> >
> > NOTES:
> > - When we fail to probe like this, boot is a bit slow because we try
> >   several times to power the panel up. This doesn't feel horrible
> >   because it'll eventually work and the retries are known to help
> >   bring some panels up.
> > - In the case where we hit the condition of failing to power up, the
> >   display will likely _never_ have a chance to work again until
> >   reboot. Once the panel-edp pm_runtime resume function fails it
> >   doesn't ever seem to retry. This is probably for the best given that
> >   we don't have any real timing/modes. eDP isn't expected to be
> >   "hotplugged" so this makes some sense.
> > - It turns out that this makes panel-edp behave more similarly for
> >   users of the generic "edp-panel" compatible string and the old fixed
> >   panel compatible string. With the old fixed panel compatible string
> >   we don't talk to the panel during probe so we'd actually behave much
> >   the same way that we'll now behave for the generic "edp-panel".
> >
> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Hsin-Yi Wang 

Pushed to drm-misc-next:

ce0ff22388ab drm/panel-edp: If we fail to powerup/get EDID, use
conservative timings

Re: [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 4:52 PM Hsin-Yi Wang  wrote:
>
> On Mon, Mar 25, 2024 at 2:56 PM Douglas Anderson  
> wrote:
> >
> > If we're using the generic "edp-panel" compatible string and we fail
> > to detect an eDP panel then we fall back to conservative timings for
> > powering up and powering down the panel. Abstract out the function for
> > setting these timings so it can be used in future patches.
> >
> > No functional change expected--just code movement.
> >
> > Signed-off-by: Douglas Anderson 
>
> Reviewed-by: Hsin-Yi Wang 

Pushed to drm-misc-next:

2cbee8ae55f5 drm/panel-edp: Abstract out function to set conservative timings

Re: [PATCH] drm/msm/dp: allow voltage swing / pre emphasis of 3

[Doug Anderson]

Hi,

On Sat, Feb 3, 2024 at 5:47 AM Dmitry Baryshkov
 wrote:
>
> Both dp_link_adjust_levels() and dp_ctrl_update_vx_px() limit swing and
> pre-emphasis to 2, while the real maximum value for the sum of the
> voltage swing and pre-emphasis is 3. Fix the DP code to remove this
> limitation.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c |  6 +++---
>  drivers/gpu/drm/msm/dp/dp_link.c | 22 +++---
>  drivers/gpu/drm/msm/dp/dp_link.h | 14 +-
>  3 files changed, 15 insertions(+), 27 deletions(-)

What ever happened with this patch? It seemed important so I've been
trying to check back on it, but it seems to still be in limbo. I was
assuming that (maybe?) Abhinav would check things against the hardware
documentation and give it a Reviewed-by and then it would land...

-Doug

Re: [PATCH v2] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels

[Doug Anderson]

Hi,

On Tue, Mar 19, 2024 at 3:45 PM Dmitry Baryshkov
 wrote:
>
> On Tue, 19 Mar 2024 at 22:58, Douglas Anderson  wrote:
> >
> > In response to my patch removing the "wait for HPD" logic at the
> > beginning of the MSM DP transfer() callback [1], we had some debate
> > about what the "This is an optional function" meant in the
> > documentation of the wait_hpd_asserted() callback. Let's clarify.
> >
> > As talked about in the MSM DP patch [1], before wait_hpd_asserted()
> > was introduced there was no great way for panel drivers to wait for
> > HPD in the case that the "built-in" HPD signal was used. Panel drivers
> > could only wait for HPD if a GPIO was used. At the time, we ended up
> > just saying that if we were using the "built-in" HPD signal that DP
> > AUX controllers needed to wait for HPD themselves at the beginning of
> > their transfer() callback. The fact that the wait for HPD at the
> > beginning of transfer() was awkward/problematic was the whole reason
> > wait_hpd_asserted() was added.
> >
> > Let's make it obvious that if a DP AUX controller implements
> > wait_hpd_asserted() that they don't need a loop waiting for HPD at the
> > start of their transfer() function. We'll still allow DP controllers
> > to work the old way but mark it as deprecated.
> >
> > [1] 
> > https://lore.kernel.org/r/20240315143621.v2.3.I535606f6d4f7e3e5588bb75c55996f61980183cd@changeid
> >
> > Reviewed-by: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
> > ---
> > I would consider changing the docs to say that implementing
> > wait_hpd_asserted() is actually _required_ for any DP controllers that
> > want to support eDP panels parented on the DP AUX bus. The issue is
> > that one DP controller (tegra/dpaux.c, found by looking for those that
> > include display/drm_dp_aux_bus.h) does populate the DP AUX bus but
> > doesn't implement wait_hpd_asserted(). I'm actually not sure how/if
> > this work on tegra since I also don't see any delay loop for HPD in
> > tegra's transfer() callback. For now, I've left wait_hpd_asserted() as
> > optional and described the old/deprecated way things used to work
> > before wait_hpd_asserted().
> >
> > Changes in v2:
> > - Make it clear that panels don't need to call if HPD is a GPIO.
> >
> >  include/drm/display/drm_dp_helper.h | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
>
> Reviewed-by: Dmitry Baryshkov 

I don't think this is controversial and I've been involved / written
most of the code for eDP panel interactions, so I think I can land
this myself with Dmitry and Abhinav's review (thanks!). If someone
later comes back with additional thoughts I'm happy to update things
more.

Pushed to drm-misc-next:

6376eb8b9115 drm/dp: Clarify that wait_hpd_asserted() is not optional for panels

Re: [PATCH 1/2] drm/panel: Remove redundant checks in multiple panels

[Doug Anderson]

Hi,

On Sat, Mar 23, 2024 at 7:05 PM Emilio Mendoza Reyes
 wrote:
>
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA512
>
> From: Emilio Mendoza Reyes 
>
> The patch ("drm/panel: Check for already prepared/enabled in drm_panel")
> moved checking for (en/dis)abled and [un]prepared panels before specific
> function calls to drm_panel.c.Those checks that still exist within the
> panels are redundant. This patch removes those redundant checks.
>
> Removing those checks was/is also a todo in the kernel docs
> Link: 
> https://www.kernel.org/doc/html/v6.8/gpu/todo.html#clean-up-checks-for-already-prepared-enabled-in-panels
>
> Signed-off-by: Emilio Mendoza Reyes 
> - ---
>  drivers/gpu/drm/panel/panel-boe-himax8279d.c | 12 
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c   |  6 --
>  drivers/gpu/drm/panel/panel-edp.c| 14 --
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c| 12 
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c   | 12 
>  drivers/gpu/drm/panel/panel-khadas-ts050.c   |  9 -
>  .../gpu/drm/panel/panel-kingdisplay-kd097d04.c   | 12 
>  .../gpu/drm/panel/panel-leadtek-ltk050h3146w.c   |  6 --
>  .../gpu/drm/panel/panel-leadtek-ltk500hd1829.c   |  6 --
>  drivers/gpu/drm/panel/panel-novatek-nt36672a.c   |  6 --
>  .../gpu/drm/panel/panel-olimex-lcd-olinuxino.c   | 12 
>  .../gpu/drm/panel/panel-osd-osd101t2587-53ts.c   | 12 
>  .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 12 
>  drivers/gpu/drm/panel/panel-raydium-rm67191.c| 12 
>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c|  6 --
>  drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 16 
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c  | 12 
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c  | 12 
>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c  |  6 --
>  drivers/gpu/drm/panel/panel-simple.c | 14 --
>  drivers/gpu/drm/panel/panel-sitronix-st7703.c|  6 --
>  drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c |  6 --
>  drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c |  6 --
>  23 files changed, 227 deletions(-)

Aside from the formatting issues (several lines start with an extra
"-" and there is the PGP stuff), there are a few high-level issues
here:

1. In general, you need to be a little more careful here because not
all these panels are going through the code path that the new code
covers. For instance, look at the first panel here
("panel-boe-himax8279d.c"). The panel_shutdown() function in that
driver _directly_ calls boe_panel_disable() and boe_panel_unprepare()
rather than calling the drm_panel equivalent function. That means that
they _won't_ get the benefit of the checking I added to drm_panel.c.
What that means is that if someone using the "panel-boe-himax8279d.c"
panel shuts down / reboots while their panel is off you'll probably
get a bunch of errors. I _think_ this is as simple as just changing
all those panels to call the drm_panel equivalent function.

2. As much as possible, not only should you be removing the checks,
but you should also be removing all the code that tracks the state of
prepared/enabled in the panel drivers and then removing the "prepared"
/ "enabled" members from the structs. If the panel driver needs to
track prepared/enabled for something else, you might need to keep it
though. One example would be sony-acx565akm, as per my previous
attempt [1].

In general, maybe a good approach would be to actually start with
patches #5 - #9 in my previous series [2] but instead of calling
drm_panel_helper_shutdown() just do:
  drm_panel_disable(...);
  drm_panel_unprepare(...);

Feel free to take my patches, change them, and post them. If you want,
you could add a Co-Developed-by (see submitting-patches.rst).

For some panels the above would be just leaving what's already there.
For some panels that might convert them from their static function to
the drm_panel equivalent.

Leaving the drm_panel_disable() / drm_panel_unprepare() in the panel
driver shutdown/remove code is not an ideal long term solution, but it
at least moves us on the right path and at least lets us get rid of
most of the prepared / enabled tracking. IMO that should be landable,
though perhaps others would have different opinions.

After doing all that, then you could submit patches that simply get
rid of the drm_panel_disable() / drm_panel_unprepare() for any panel
drivers if you know that those panels are only used by DRM drivers
that properly call the DRM shutdown code. See my series that tried to
add that to a bunch of DRM drivers [3]. Not everything landed, but
quite a bit of it did. As Maxime and I talked about [4] that _should_
be as simple as tracking down the panel's compatible string, seeing
which device trees use it, and then seeing if the associated DRM
driver is properly shutting 

Re: [PATCH] drm/panel-edp: Add AUO B120XAN01.0

[Doug Anderson]

Hi,

On Mon, Mar 25, 2024 at 5:59 AM Pin-yen Lin  wrote:
>
> Add support for the AUO B120XAN01.0 panel.
>
> Signed-off-by: Pin-yen Lin 
> ---
>
>  drivers/gpu/drm/panel/panel-edp.c | 1 +
>  1 file changed, 1 insertion(+)

Looks fine.

Reviewed-by: Douglas Anderson 

Applied to drm-misc-next:

1864c45deb77 drm/panel-edp: Add AUO B120XAN01.0

Re: [drm-tip:drm-tip 4/11] drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:73: error: '.bin' directive output may be truncated writing 4 bytes into a region of size between 2 and 31

[Doug Anderson]

Hi,

On Sat, Mar 23, 2024 at 10:15 AM kernel test robot  wrote:
>
> tree:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
> head:   657dd8fcd2f1d1205c6f98fdb8b60915228991d1
> commit: 0885186926a13c697d78f5af03f32445414b6ad5 [4/11] Merge remote-tracking 
> branch 'drm-misc/drm-misc-next' into drm-tip
> config: microblaze-allmodconfig 
> (https://download.01.org/0day-ci/archive/20240324/202403240115.1lao588s-...@intel.com/config)
> compiler: microblaze-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): 
> (https://download.01.org/0day-ci/archive/20240324/202403240115.1lao588s-...@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version 
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot 
> | Closes: 
> https://lore.kernel.org/oe-kbuild-all/202403240115.1lao588s-...@intel.com/
>
> All errors (new ones prefixed by >>):
>
>drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c: In function 
> 'amdgpu_vcn_early_init':
>drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:66: error: 'snprintf' output 
> may be truncated before the last format character [-Werror=format-truncation=]
>  102 | snprintf(fw_name, sizeof(fw_name), 
> "amdgpu/%s.bin", ucode_prefix);
>  |  ^
>drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:17: note: 'snprintf' output 
> between 12 and 41 bytes into a destination of size 40
>  102 | snprintf(fw_name, sizeof(fw_name), 
> "amdgpu/%s.bin", ucode_prefix);
>  | 
> ^
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:73: error: '.bin' directive 
> >> output may be truncated writing 4 bytes into a region of size between 2 
> >> and 31 [-Werror=format-truncation=]
>  105 | snprintf(fw_name, sizeof(fw_name), 
> "amdgpu/%s_%d.bin", ucode_prefix, i);
>  |
>  ^~~~
>drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:25: note: 'snprintf' output 
> between 14 and 43 bytes into a destination of size 40
>  105 | snprintf(fw_name, sizeof(fw_name), 
> "amdgpu/%s_%d.bin", ucode_prefix, i);
>  | 
> ^~~
>cc1: all warnings being treated as errors
>
>
> vim +105 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>
> 95d0906f850655 Leo Liu2016-12-21   93
> 69939009bde70c Mario Limonciello  2022-12-28   94  int 
> amdgpu_vcn_early_init(struct amdgpu_device *adev)
> 69939009bde70c Mario Limonciello  2022-12-28   95  {
> 69939009bde70c Mario Limonciello  2022-12-28   96   char ucode_prefix[30];
> 69939009bde70c Mario Limonciello  2022-12-28   97   char fw_name[40];
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06   98   int r, i;
> 69939009bde70c Mario Limonciello  2022-12-28   99
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  100   for (i = 0; i < 
> adev->vcn.num_vcn_inst; i++) {
> 69939009bde70c Mario Limonciello  2022-12-28  101   
> amdgpu_ucode_ip_version_decode(adev, UVD_HWIP, ucode_prefix, 
> sizeof(ucode_prefix));
> 69939009bde70c Mario Limonciello  2022-12-28 @102   
> snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  103   if 
> (amdgpu_ip_version(adev, UVD_HWIP, 0) ==  IP_VERSION(4, 0, 6) &&
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  104   i == 
> 1) {
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06 @105   
> snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_%d.bin", ucode_prefix, i);
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  106   }
> 69939009bde70c Mario Limonciello  2022-12-28  107
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  108   r = 
> amdgpu_ucode_request(adev, >vcn.fw[i], fw_name);
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  109   if (r) {
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  110   
> amdgpu_ucode_release(>vcn.fw[i]);
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  111   
> return r;
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  112   }
> 6a7cbbc267c0ca Saleemkhan Jamadar 2024-03-06  113   }
> 69939009bde70c Mario Limonciello  2022-12-28  114   return r;
> 69939009bde70c Mario Limonciello  2022-12-28  115  }
> 69939009bde70c Mario Limonciello  2022-12-28  116
>
> :: The code at line 105 was first introduced by commit
> :: 6a7cbbc267c0cafa2b027983a40276deb673c066 drm/amdgpu/vcn: enable vcn1 
> fw load for VCN 4_0_6
>
> :: TO: Saleemkhan Jamadar 
> :: CC: Alex Deucher 

Not quite sure why this came to me and not the people involved with
that commit. Adding them here.

-Doug

Re: [PATCH] drm/panel: atna33xc20: Fix unbalanced regulator in the case HPD doesn't assert

[Doug Anderson]

Hi,

On Thu, Mar 14, 2024 at 3:32 PM Jessica Zhang  wrote:
>
> On 3/13/2024 2:12 PM, Douglas Anderson via B4 Relay wrote:
> > From: Douglas Anderson 
> >
> > When the atna33xc20 driver was first written the resume code never
> > returned an error. If there was a problem waiting for HPD it just
> > printed a warning and moved on. This changed in response to review
> > feedback [1] on a future patch but I accidentally didn't account for
> > rolling back the regulator enable in the error cases. Do so now.
> >
> > [1] 
> > https://lore.kernel.org/all/5f3cf3a6-1cc2-63e4-f76b-4ee686764...@linaro.org/
> >
> > Fixes: 3b5765df375c ("drm/panel: atna33xc20: Take advantage of 
> > wait_hpd_asserted() in struct drm_dp_aux")
> > Signed-off-by: Douglas Anderson 
> > ---
> >   drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 22 
> > +-
> >   1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c 
> > b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> > index 76c2a8f6718c..9c336c71562b 100644
> > --- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> > +++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> > @@ -109,19 +109,17 @@ static int atana33xc20_resume(struct device *dev)
> >   if (hpd_asserted < 0)
> >   ret = hpd_asserted;
> >
> > - if (ret)
> > + if (ret) {
> >   dev_warn(dev, "Error waiting for HPD GPIO: %d\n", 
> > ret);
> > -
> > - return ret;
> > - }
> > -
> > - if (p->aux->wait_hpd_asserted) {
> > + goto error;
> > + }
> > + } else if (p->aux->wait_hpd_asserted) {
>
> Hi Doug,
>
> Acked-by: Jessica Zhang 

Pushed with Jessica's Ack to drm-misc-next.

5e842d55bad7 drm/panel: atna33xc20: Fix unbalanced regulator in the
case HPD doesn't assert

I chose drm-misc-next instead of drm-misc-fixes because this isn't
super urgent and the patch would have to be modified on drm-misc-fixes
because we don't have commit 8df1ddb5bf11 ("drm/dp: Don't attempt AUX
transfers when eDP panels are not powered") there.

Re: [PATCH] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels

[Doug Anderson]

Hi,

On Tue, Mar 19, 2024 at 1:55 PM Dmitry Baryshkov
 wrote:
>
> >  -* panel to finish powering on. This is an optional function.
> >  +* panel to finish powering on. It is optional for DP AUX 
> >  controllers
> >  +* to implement this function but required for DP AUX 
> >  endpoints (panel
> >  +* drivers) to call it after powering up but before doing AUX 
> >  transfers.
> >  +* If a DP AUX controller does not implement this function 
> >  then it
> >  +* may still support eDP panels that use the AUX controller's 
> >  built-in
> >  +* HPD signal by implementing a long wait for HPD in the 
> >  transfer()
> >  +* callback, though this is deprecated.
> > >>>
> > >>> It doesn't cover a valid case when the panel driver handles HPD signal
> > >>> on its own.
> > >>>
> > >>
> > >> This doc is only for wait_for_hpd_asserted(). If panel driver handles
> > >> HPD signal on its own, this will not be called. Do we need a doc for 
> > >> that?
> > >
> > > This comment declares that this callback must be called by the panel
> > > driver: '...but required for DP AUX endpoints [...] to call it after
> > > powering up but before doing AUX transfers.'
> > >
> > > If we were to follow documentation changes from this patch, we'd have
> > > to patch panel-edp to always call wait_for_hpd_asserted, even if HPD
> > > GPIO is used. However this is not correct from my POV.
> > >
> >
> > hmmm I dont mind explicitly saying "unless the panel can independently
> > check the HPD state" but not required in my opinion because if panel was
> > capable of checking the HPD gpio (its self-capable) why would it even
> > call wait_for_hpd_asserted?
>
> I'm fine with the proposed change. Doug?
>
> >
> > I will let you and Doug discuss this but fwiw, I am fine without this
> > additional clarification. So the R-b stands with or without this
> > additional clause.

Adjusted wording in v2. Kept Abhniav's R-b. PTAL.

https://lore.kernel.org/r/20240319135836.v2.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid

Re: [PATCH v2 3/4] drm/msm/dp: Delete the old 500 ms wait for eDP HPD in aux transfer

[Doug Anderson]

Hi,

On Tue, Mar 19, 2024 at 10:27 AM Dmitry Baryshkov
 wrote:
>
> On Tue, 19 Mar 2024 at 19:13, Abhinav Kumar  wrote:
> >
> >
> >
> > On 3/18/2024 5:55 PM, Dmitry Baryshkov wrote:
> > > On Tue, 19 Mar 2024 at 02:19, Abhinav Kumar  
> > > wrote:
> > >>
> > >> +bjorn, johan as fyi for sc8280xp
> > >>
> > >> On 3/15/2024 2:36 PM, Douglas Anderson wrote:
> > >>> Before the introduction of the wait_hpd_asserted() callback in commit
> > >>> 841d742f094e ("drm/dp: Add wait_hpd_asserted() callback to struct
> > >>> drm_dp_aux") the API between panel drivers and DP AUX bus drivers was
> > >>> that it was up to the AUX bus driver to wait for HPD in the transfer()
> > >>> function.
> > >>>
> > >>> Now wait_hpd_asserted() has been added. The two panel drivers that are
> > >>> DP AUX endpoints use it. See commit 2327b13d6c47 ("drm/panel-edp: Take
> > >>> advantage of wait_hpd_asserted() in struct drm_dp_aux") and commit
> > >>> 3b5765df375c ("drm/panel: atna33xc20: Take advantage of
> > >>> wait_hpd_asserted() in struct drm_dp_aux"). We've implemented
> > >>> wait_hpd_asserted() in the MSM DP driver as of commit e2969ee30252
> > >>> ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe()"). There is
> > >>> no longer any reason for long wait in the AUX transfer() function.
> > >>> Remove it.
> > >>>
> > >>> NOTE: the wait_hpd_asserted() is listed as "optional". That means it's
> > >>> optional for the DP AUX bus to implement. In the case of the MSM DP
> > >>> driver we implement it so we can assume it will be called.
> > >>>
> > >>
> > >> How do we enforce that for any new edp panels to be used with MSM, the
> > >> panels should atleast invoke wait_hpd_asserted()?
> > >>
> > >> I agree that since MSM implements it, even though its listed as
> > >> optional, we can drop this additional wait. So nothing wrong with this
> > >> patch for current users including sc8280xp, sc7280 and sc7180.
> > >>
> > >> But, does there need to be some documentation that the edp panels not
> > >> using the panel-edp framework should still invoke wait_hpd_asserted()?
> > >>
> > >> Since its marked as optional, what happens if the edp panel driver,
> > >> skips calling wait_hpd_asserted()?
> > >
> > > It is optional for the DP AUX implementations, not for the panel to be 
> > > called.
> > >
> >
> > Yes, I understood that part, but is there anything from the panel side
> > which mandates calling wait_hpd_asserted()?
> >
> > Is this documented somewhere for all edp panels to do:
> >
> > if (aux->wait_hpd_asserted)
> > aux->wait_hpd_asserted(aux, wait_us);
>
> That's obviously not true, e.g. if panel-edp.c handled HPD signal via
> the GPIO pin.
>
> But the documentation explicitly says that the host will be powered up
> automatically, but the caller must ensure that the panel is powered
> on. `It's up to the caller of this code to make sure that the panel is
> powered on if getting an error back is not OK.'

It wouldn't hurt to send out a documentation patch that makes this
more explicit. OK, I sent:

https://lore.kernel.org/r/20240319111432.1.I521dad0693cc24fe4dd14cba0c7048d94f5b6b41@changeid

-Doug

Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)

[Doug Anderson]

Hi,

On Mon, Mar 18, 2024 at 12:26 PM Stephen Boyd  wrote:
>
> Quoting Douglas Anderson (2024-03-15 14:36:32)
> > This is a no-op change to just fix a typo in the name of a static function.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > Changes in v2:
> > - ("Fix typo in static function (ststus => status)") new for v2.
>
> This was sent at
> https://lore.kernel.org/r/20240306193515.455388-1-quic_abhin...@quicinc.com

Whoops! I guess we both noticed it at about the same time. My patch
should be dropped then. The rest of my series (patches #1 - #3) are
still relevant. I won't repost them since they can be applied just
fine even if this patch is dropped.

-Doug