| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6e4768d1-929e-4700-82bd-2e247b68de1f@rocketmail.com>
Date: Sat, 26 Oct 2024 12:47:17 +0200
From: Jakob Hauser <jahau@...ketmail.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
Jessica Zhang <quic_jesszhan@...cinc.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Thierry Reding <thierry.reding@...il.com>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v3 4/5] drm/panel: samsung-s6e88a0-ams427ap24: Add
brightness control
Hi Linus,
On 25.10.24 21:27, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@...ketmail.com> wrote:
>> +static const int s6e88a0_ams427ap24_br_to_cd[NUM_STEPS_CANDELA] = {
>
> (...)
>> + /* brightness till, candela */
>
> Brightness to candela conversion table? Edit comment?
In the downstream driver there is a table with four columns:
<idx> <from> <till> <candella>.
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L341-L397
The first column is a counter, the second and third is the
from-till-range of brightness steps that correspond to the forth column
of candela identifier.
In the patch here I only adopted the third and forth column, because the
others were not necessary. The comment "brightness till, candela" was
intended to label those two columns.
To make it more clear, I could add the keyword "columns" and the column
"brightness from".
/* columns: brightness from, brightness till, candela */
/* 0 */ 10, /* 10CD */
/* 11 */ 11, /* 11CD */
/* 12 */ 12, /* 12CD */
/* 13 */ 13, /* 13CD */
/* 14 */ 14, /* 14CD */
...
/* 30 */ 30, /* 39CD */
/* 31 */ 32, /* 41CD */
/* 33 */ 34, /* 44CD */
/* 35 */ 36, /* 47CD */
...
/* 92 */ 97, /* 126CD */
/* 98 */ 104, /* 134CD */
/* 105 */ 110, /* 143CD */
/* 111 */ 118, /* 152CD */
...
/* 182 */ 205, /* 249CD */
/* 206 */ 234, /* 265CD */
/* 235 */ 254, /* 282CD */
/* 255 */ 255, /* 300CD */
>> +static const u8 s6e88a0_ams427ap24_aid[NUM_STEPS_AID][SEQ_LENGTH_AID] = {
>
> If you know that the sequence 0xb2, 0x40, 0x08, 0x20 means "set AID"
> (or is it AOR??) you can #define
>
> #define S6E88A0_SET_AID 0xb2
Thanks to Alexey Min, who looked this up, I can say:
"The PWM mechanism used on Samsung displays is called AOR (AMOLED off
ratio), the related function on the kernel driver is called AID (AMOLED
impulsive driving)."
source: xdaforums
The downstream driver of ams427ap24 uses the "aid" labeling, therefore I
stick to that. (Interestingly the older downstream driver ams427ap01
uses the "aor" labeling.)
> Then make a small buffer:
>
> u8 set_aid[5] = { S6E88A0_SET_AID, 0x40, 0x08, 0x20, 0x00, 0x00 };
>
> then you can strip the first three bytes from the entire table,
> just copy in the two relevant bytes into set_aor[]
> and send that.
Ok, I'll try to implement that. The size of the second array dimension
of that table will then become [SEQ_LENGTH_AID - 3].
...
>> +static int s6e88a0_ams427ap24_set_brightness(struct backlight_device *bd)
>> +{
>> + struct s6e88a0_ams427ap24 *ctx = bl_get_data(bd);
>> + struct mipi_dsi_device *dsi = ctx->dsi;
>> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>> + struct device *dev = &dsi->dev;
>> + int brightness = bd->props.brightness;
>> + int candela_enum;
>> + u8 b2[SEQ_LENGTH_AID + 1];
>> + u8 b6[SEQ_LENGTH_ELVSS + 1];
>> + u8 ca[SEQ_LENGTH_GAMMA + 1];
>
> Rename them to something like my suggestions so we understand what it is
> all about. It seems the infrastructure for what I suggested is mostly already
> there.
These defines are intended to be the sequence length of the payload for
commands aid, elvss and gamma. The naming makes sense to me.
The "+ 1" became necessary because when changing the DCS commands to
multi type I ran into the issue that there is one for
"mipi_dsi_dcs_write_seq" and one for "mipi_dsi_dcs_write_buffer"... but
none for "mipi_dsi_dcs_write" :( So I had to convert those into
"mipi_dsi_dcs_write_buffer"+multi, thus including the command register
value into the payload string.
...
>> + if (candela_enum <= CANDELA_111CD) {
>> + memcpy(&b6[1], s6e88a0_ams427ap24_elvss[0], SEQ_LENGTH_ELVSS);
>> + } else {
>> + memcpy(&b6[1], s6e88a0_ams427ap24_elvss[candela_enum - CANDELA_111CD],
>> + SEQ_LENGTH_ELVSS);
>> + }
>> +
>> + /* get gamma */
>> + ca[0] = 0xca;
>
> #define S6E88A0_SET_GAMMA 0xca
As stated in my reply on patch 3, I would like to avoid those defines
because firstly the naming becomes arbitrary and secondly it spoils the
readability of the larger DCS command blocks due to necessary line breaks.
In this specific case here a define would make sense. But I can hardly
implement it here without doing it elsewhere. Therefore I would like to
keep that as it is.
...
>> + mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b2, ARRAY_SIZE(b2));
>> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
>
> 0x55 is MIPI_DCS_WRITE_POWER_SAVE in <video/mipi_display.h>
It's the only one that could be used from <video/mipi_display.h>.
Though "MIPI_DCS_WRITE_POWER_SAVE, 0x00" doesn't say much. In the
downstream driver there are four levels of ACL:
0x55, 0x00 -> ACL off
0x55, 0x01 -> default ACL 15 %
0x55, 0x02 -> ACL 30 %, also corresponds to the "ACL on" command
0x55, 0x03 -> doesn't seem to be used
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L275-L281
I would prefer to stay at 0x55 and add comment "acl off". Embedded in a
block of other DCS commands with plain command register values and
single line comments appended, as proposed in my reply on patch 3, it
looks more readable and descriptive in the context of the other commands.
Kind regards,
Jakob
Powered by blists - more mailing lists