Re: [PATCH] drm/panel: leadtek-ltk050h3146w: transition to mipi_dsi wrapped functions
Hi,
On Wed, Oct 30, 2024 at 12:24 AM Tejas Vipin wrote:
>
> On 10/29/24 12:24 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 25, 2024 at 9:00 PM Tejas Vipin wrote:
> >>
> >> @@ -418,79 +398,42 @@ static const struct ltk050h3146w_desc
> >> ltk050h3146w_data = {
> >> MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
> >> };
> >>
> >> -static int ltk050h3146w_a2_select_page(struct ltk050h3146w *ctx, int page)
> >> +static void ltk050h3146w_a2_select_page(struct mipi_dsi_multi_context
> >> *dsi_ctx, int page)
> >> {
> >> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> >> - u8 d[3] = { 0x98, 0x81, page };
> >> + u8 d[4] = { 0xff, 0x98, 0x81, page };
> >>
> >> - return mipi_dsi_dcs_write(dsi, 0xff, d, ARRAY_SIZE(d));
> >> + mipi_dsi_dcs_write_buffer_multi(dsi_ctx, d, ARRAY_SIZE(d));
> >
> > FWIW: the above might be slightly better as:
> >
> > mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0xff, 0x98, 0x81, page);
> >
> > That would make it more documenting that the 0xff is the "cmd", has
> > fewer lines of code, and also gets the array marked as "static const"
> > which might make the compiler slightly more efficient. ;-)
> >
> > Not really a huge deal, though.
> >
>
> I did try this initially, but got an error because of page not being a
> compile time constant. Not sure how I should handle this.
Ha, that makes sense! It can't be "static const" because that means
that there's one storage location that's never changing and that's
just not true. I tried to see if there was some way to make the
mipi_dsi_dcs_write_seq_multi() smarter and have it detect if
everything is constant but I couldn't find any way to do that. The
__builtin_constant_p() trick doesn't seem to work with more than one
number.
So I think what you have is fine then. If this becomes common I guess
we can make an alternative version of mipi_dsi_dcs_write_seq_multi()
that just uses "const" instead of "static const".
I'll plan to apply your patch next week unless someone beats me to it.
-Doug
Re: [PATCH] drm/panel: leadtek-ltk050h3146w: transition to mipi_dsi wrapped functions
Hi,
On Fri, Oct 25, 2024 at 9:00 PM Tejas Vipin wrote:
>
> @@ -418,79 +398,42 @@ static const struct ltk050h3146w_desc ltk050h3146w_data
> = {
> MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET,
> };
>
> -static int ltk050h3146w_a2_select_page(struct ltk050h3146w *ctx, int page)
> +static void ltk050h3146w_a2_select_page(struct mipi_dsi_multi_context
> *dsi_ctx, int page)
> {
> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> - u8 d[3] = { 0x98, 0x81, page };
> + u8 d[4] = { 0xff, 0x98, 0x81, page };
>
> - return mipi_dsi_dcs_write(dsi, 0xff, d, ARRAY_SIZE(d));
> + mipi_dsi_dcs_write_buffer_multi(dsi_ctx, d, ARRAY_SIZE(d));
FWIW: the above might be slightly better as:
mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0xff, 0x98, 0x81, page);
That would make it more documenting that the 0xff is the "cmd", has
fewer lines of code, and also gets the array marked as "static const"
which might make the compiler slightly more efficient. ;-)
Not really a huge deal, though.
Reviewed-by: Douglas Anderson
Re: [PATCH v3 08/14] drm/mediatek: Add DRM_MODE_ROTATE_0 to rotation property
Hi Shawn,
On Thu, Oct 24, 2024 at 6:32 PM Shawn Sung (宋孝謙)
wrote:
>
> Hi Doug,
>
> On Thu, 2024-10-24 at 13:47 -0700, Doug Anderson wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > Hi,
> >
> > On Wed, Jun 19, 2024 at 9:39 AM Hsiao Chien Sung via B4 Relay
> > wrote:
> > >
> > > From: Hsiao Chien Sung
> > >
> > > Always add DRM_MODE_ROTATE_0 to rotation property to meet
> > > IGT's (Intel GPU Tools) requirement.
> > >
> > > Reviewed-by: CK Hu
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delre...@collabora.com>
> > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > > Signed-off-by: Hsiao Chien Sung
> > > ---
> > > drivers/gpu/drm/mediatek/mtk_ddp_comp.h | 6 +-
> > > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 17 +
> > > drivers/gpu/drm/mediatek/mtk_plane.c| 2 +-
> > > 3 files changed, 11 insertions(+), 14 deletions(-)
> >
> > FWIW, this patch got into ChromeOS's 5.15 branch via stable merge and
> > apparently broke things. As a short term fix we've reverted it there:
> >
> > https://crrev.com/c/5960799
>
> Thank you for reporting this issue. We are currently investigating the
> bug.
>
> Since I am unable to access the Google issue tracker [1], could you
> please provide more details about this bug? The message in the revert
> commit mentions "hana/sycamore360" (MT8173) and it appears that there
> is a rotation issue in tablet mode.
Thanks for the followup. I've only been peripherally involved in the
problem, but I can at least copy the relevant bits over.
It looks as if the problem is somehow only showing up when running
Android apps on those devices, so whatever the problem is it's subtle.
The report says that the apps work OK when the device is in tablet
mode and in one rotation but the problem shows up when rotated 90
degrees. The report says that "Screen content appears inverted". To me
it also sounds _possible_ that the problem is somewhere in our
userspace.
I think Hsin-Yi and Ross are continuing to dig a bit more. Maybe once
they've dug they can add any details they find or can loop in others
as it makes sense?
> > ...apparently the patch is fine on newer kernels so maybe there is a
> > missing dependency? Hopefully someone on this list can dig into this
> > and either post the revert to stable 5.15 kernels or suggest
> > additional backports.
> >
>
> There are known issues [2] regarding forward compatibility. Could you
> confirm whether this patch is unable to resolve the mentioned problem?
> Thanks.
>
> [1] https://issuetracker.google.com/issues/369688659
> [2]
> https://patchwork.kernel.org/project/linux-mediatek/list/?series=896964
The patches in [2] look related to alpha blending but I think they are
seeing issues related to rotation. ...so I'm going to assume it's
different? I don't have this hardware in front of me, so I'm just
going by the report.
-Doug
Re: [PATCH v3 08/14] drm/mediatek: Add DRM_MODE_ROTATE_0 to rotation property
Hi,
On Wed, Jun 19, 2024 at 9:39 AM Hsiao Chien Sung via B4 Relay
wrote:
>
> From: Hsiao Chien Sung
>
> Always add DRM_MODE_ROTATE_0 to rotation property to meet
> IGT's (Intel GPU Tools) requirement.
>
> Reviewed-by: CK Hu
> Reviewed-by: AngeloGioacchino Del Regno
>
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Hsiao Chien Sung
> ---
> drivers/gpu/drm/mediatek/mtk_ddp_comp.h | 6 +-
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 17 +
> drivers/gpu/drm/mediatek/mtk_plane.c| 2 +-
> 3 files changed, 11 insertions(+), 14 deletions(-)
FWIW, this patch got into ChromeOS's 5.15 branch via stable merge and
apparently broke things. As a short term fix we've reverted it there:
https://crrev.com/c/5960799
...apparently the patch is fine on newer kernels so maybe there is a
missing dependency? Hopefully someone on this list can dig into this
and either post the revert to stable 5.15 kernels or suggest
additional backports.
-Doug
Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances
Hi, On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven wrote: > > > > > > > However, using i2c_client->adapter->nr instead of ida_alloc() > > > > > > in the TI driver does sound like a good idea to me... > > > > > > > > > > Great! > > > > > > > With the I2C adapter numbers, that becomes: > > > > > > > > /sys/bus/auxiliary/devices > > > > ├── ti_sn65dsi86.gpio.1 > > > > ├── ti_sn65dsi86.pwm.1 > > > > ├── ti_sn65dsi86.aux.1 > > > > ├── ti_sn65dsi86.bridge.1 > > > > ├── ti_sn65dsi86.gpio.4 > > > > ├── ti_sn65dsi86.pwm.4 > > > > ├── ti_sn65dsi86.aux.4 > > > > └── ti_sn65dsi86.bridge.4 > > > > > > > > > adapter->nr instead like other aux subsystems already do. > > > > > > Unfortunately the devil is in the details, as usual: there can be > > > multiple instances of the sn65dsi86 bridge on a single I2C bus, > > > so adapter->nr is not guaranteed to generate a unique name. > > > > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge > > chip is always at bus address 0x2d so you can't have more than one on > > the same bus. Unless you added something funky atop it (like a mux of > > some sort) you might be OK. > > It's 0x2c on mine ;-) > > 8.5.1 Local I2C Interface Overview > The 7-bit device address for SN65DSI86 is factory preset to 010110X > with the least significant bit being determined by the ADDR control > input. Doh! I missed that in my search of the doc. I guess because they decided to specify the address in binary in that part so my searching for both the 7-bit and 8-bit I2C address didn't trigger. Oh well. > > > Changing the auxiliary bus to use the parent's name instead of the > > > module name, as suggested by Laurent, would fix that. > > > > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If > > we had a second on i2c bus 4, we'd have: > > > > /sys/bus/auxiliary/devices > > ├── 2-002d.gpio.0 > > ├── 2-002d.pwm.0 > > ├── 2-002d.aux.0 > > ├── 2-002d.bridge.0 > > ├── 4-002d.gpio.0 > > ├── 4-002d.pwm.0 > > ├── 4-002d.aux.0 > > └── 4-002d.bridge.0 > > > > ...and I think that's guaranteed to be unique because all the i2c > > devices are flat in "/sys/bus/i2c/devices". > > Correct. So given everything, using the dev_name() of the "parent" sounds pretty good and seems like it addresses everyone's concerns. Was there a part of the conversation where someone pointed out problems with it that I missed? Is the next step to post a patch implementing that? It'll change sysfs paths and dev names for everyone using AUX bus, but presumably that's OK? -Doug
Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances
Hi, On Mon, Oct 21, 2024 at 1:48 AM Geert Uytterhoeven wrote: > > Hi Greg, > > On Mon, Oct 21, 2024 at 10:23 AM Geert Uytterhoeven > wrote: > > On Mon, Oct 21, 2024 at 9:27 AM Greg KH wrote: > > > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote: > > > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH > > > > wrote: > > > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote: > > > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote: > > > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote: > > > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote: > > > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven > > > > > > > > > wrote: > > > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote: > > > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert > > > > > > > > > > > Uytterhoeven wrote: > > > > > > > > > > > > Each bridge instance creates up to four auxiliary > > > > > > > > > > > > devices with different > > > > > > > > > > > > names. However, their IDs are always zero, causing > > > > > > > > > > > > duplicate filename > > > > > > > > > > > > errors when a system has multiple bridges: > > > > > > > > > > > > > > > > > > > > > > > > sysfs: cannot create duplicate filename > > > > > > > > > > > > '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0' > > > > > > > > > > > > > > > > > > > > > > > > Fix this by using a unique instance ID per bridge > > > > > > > > > > > > instance. > > > > > > > > > > > > > > > > > > > > > > Isn't this something that should be handled by the AUX > > > > > > > > > > > core ? The code > > > > > > > > > > > below would otherwise need to be duplicated by all > > > > > > > > > > > drivers, which seems > > > > > > > > > > > a burden we should avoid. > > > > > > > > > > > > > > > > > > > > According to the documentation, this is the responsibility > > > > > > > > > > of the caller > > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81 > > > > > > > > > > I believe this is the same for platform devices. > > > > > > > > > > See also the example at > > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116 > > > > > > > > > > > > > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but > > > > > > > > > > the auxiliary > > > > > > > > > > bus does not. > > > > > > > > > > > > > > > > > > Yes, it does not as it's up to the caller to create a unique > > > > > > > > > name, like > > > > > > > > > your patch here does. I'd argue that platform should also > > > > > > > > > not do > > > > > > > > > automatic device ids, but that's a different argument :) > > > > > > > > > > > > > > > > __auxiliary_device_add() creates the device name with > > > > > > > > > > > > > > > > dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, > > > > > > > > auxdev->id); > > > > > > > > > > > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but > > > > > > > > shouldn't the first component of the device name use the > > > > > > > > parent's name > > > > > > > > instead of the module name ? > > > > > > > > > > > > > > Why would the parent's name not be the module name? That name is > > > > > > > guaranteed unique in the system. If you want "uniqueness" within > > > > > > > the > > > > > > > driver/module, use the name and id field please. > > > > > > > > > > > > > > That's worked well so far, but to be fair, aux devices are pretty > > > > > > > new. > > > > > > > What problem is this naming scheme causing? > > > > > > > > > > > > Auxiliary devices are created as children of a parent device. When > > > > > > multiple instances of the same parent type exist, this will be > > > > > > reflected > > > > > > in the /sys/devices/ devices tree hierarchy without any issue. The > > > > > > problem comes from the fact the the auxiliary devices need a unique > > > > > > name > > > > > > for /sys/bus/auxialiary/devices/, where we somehow have to > > > > > > differenciate > > > > > > devices of identical types. > > > > > > > > > > > > Essentially, we're trying to summarize a whole hierarchy (path in > > > > > > /sys/devices/) into a single string. There are different ways to > > > > > > solve > > > > > > this. For platform devices, we use a device ID. For I2C devices, we > > > > > > use > > > > > > the parent's bus number. Other buses use different schemes. > > > > > > > > > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to > > > > > > handle this, and assign an id managed by the parent. In a sense we > > > > > > could > > > > > > consider this to be similar to what is done for I2C, where the bus > > > > > > number is also a property of the parent. However, the big > > > > > > difference is > > > > > > that the I2C bus number is managed by the I2C subsystem, while here > > > > > > the > > > > > > id is managed by the ti-sn65dsi86
Re: [PATCH v3] drm/display: Drop obsolete dependency on COMPILE_TEST
Hi,
On Tue, Oct 15, 2024 at 4:46 AM Jean Delvare wrote:
>
> Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it
> is possible to test-build any driver which depends on OF on any
> architecture by explicitly selecting OF. Therefore depending on
> COMPILE_TEST as an alternative is no longer needed.
>
> To avoid reintroducing the randconfig bug originally fixed by commit
> 876271118aa4 ("drm/display: Fix build error without CONFIG_OF"),
> DRM_MSM which selects DRM_DISPLAY_DP_HELPER must explicitly depend
> on OF. This is consistent with what all other DRM drivers are doing.
>
> Signed-off-by: Jean Delvare
> Reviewed-by: Javier Martinez Canillas
> Cc: David Airlie
> Cc: Daniel Vetter
> ---
> For regular builds, this is a no-op, as OF is always enabled on
> ARCH_QCOM and SOC_IMX5. So this change only affects test builds. As
> explained before, allowing test builds only when OF is enabled
> improves the quality of these test builds, as the result is then
> closer to how the code is built on its intended targets.
>
> Changes in v3:
> * Rebase on top of kernel v6.11.
> Changes in v2:
> * Let DRM_MSM depend on OF so that random test builds won't break.
>
> drivers/gpu/drm/display/Kconfig |2 +-
> drivers/gpu/drm/msm/Kconfig |1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-6.11.orig/drivers/gpu/drm/display/Kconfig
> +++ linux-6.11/drivers/gpu/drm/display/Kconfig
> @@ -3,7 +3,7 @@
> config DRM_DISPLAY_DP_AUX_BUS
> tristate
> depends on DRM
> - depends on OF || COMPILE_TEST
> + depends on OF
>
> config DRM_DISPLAY_HELPER
> tristate
> --- linux-6.11.orig/drivers/gpu/drm/msm/Kconfig
> +++ linux-6.11/drivers/gpu/drm/msm/Kconfig
> @@ -6,6 +6,7 @@ config DRM_MSM
> depends on ARCH_QCOM || SOC_IMX5 || COMPILE_TEST
> depends on COMMON_CLK
> depends on IOMMU_SUPPORT
> + depends on OF
Perhaps nobody landed this because you're missing the msm maintainers
as specified by `./scripts/get_maintainer.pl -f
drivers/gpu/drm/msm/Kconfig` ? I've added them here. It seems like
we'd at least need an Ack by those guys since this modified the
msm/Kconfig...
FWIW I haven't spent massive time studying this, but what you have
here looks reasonable. I'm happy at least with this from a DP AUX bus
perspective:
Acked-by: Douglas Anderson
Presumably landing this via drm-misc makes the most sense after MSM
guys give it an Ack.
-Doug
Re: [PATCH] drm/mediatek: Fix color format MACROs in OVL
Hi,
On Tue, Oct 15, 2024 at 3:30 AM Hsin-Te Yuan wrote:
>
> In commit 9f428b95ac89 ("drm/mediatek: Add new color format MACROs in
> OVL"), some new color formats are defined in the MACROs to make the
> switch statement more concise. However, there are typos in these
> formats MACROs, which cause the return value to be incorrect. Fix the
> typos to ensure the return value remains unchanged.
Right. I had a little bit of time figuring it out from the commit
message of the original CL, but I think the commit you're fixing was
_intended_ to be a noop and just a cleanup. ...but after resolving the
#defines before and after I can see that it wasn't.
> Fix: 9f428b95ac89 ("drm/mediatek: Add new color format MACROs in OVL")
The above syntax isn't quite right. It should be:
Fixes: 9f428b95ac89 ("drm/mediatek: Add new color format MACROs in OVL")
See `Documentation/process/submitting-patches.rst` which talks about
how to get `git` to format it for you.
> Signed-off-by: Hsin-Te Yuan
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index
> 89b439dcf3a6af9f5799487fdc0f128a9b5cbe4a..1632ac5c23d87e1cdc41013a9cf7864728dcb63b
> 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -65,8 +65,8 @@
> #define OVL_CON_CLRFMT_RGB (1 << 12)
> #define OVL_CON_CLRFMT_ARGB(2 << 12)
> #define OVL_CON_CLRFMT_RGBA(3 << 12)
> -#define OVL_CON_CLRFMT_ABGR(OVL_CON_CLRFMT_RGBA |
> OVL_CON_BYTE_SWAP)
> -#define OVL_CON_CLRFMT_BGRA(OVL_CON_CLRFMT_ARGB |
> OVL_CON_BYTE_SWAP)
> +#define OVL_CON_CLRFMT_ABGR(OVL_CON_CLRFMT_ARGB |
> OVL_CON_BYTE_SWAP)
> +#define OVL_CON_CLRFMT_BGRA(OVL_CON_CLRFMT_RGBA |
> OVL_CON_BYTE_SWAP)
Other than the slightly broken "Fixes" tag, the above looks right to me.
Reviewed-by: Douglas Anderson
Re: [PATCH] drm/panel: himax-hx83102: Adjust power and gamma to optimize brightness
Hi,
On Fri, Oct 11, 2024 at 7:20 AM Doug Anderson wrote:
>
> Hi,
>
> On Thu, Oct 10, 2024 at 7:08 PM Cong Yang
> wrote:
> >
> > The current panel brightness is only 360 nit. Adjust the power and gamma to
> > optimize the panel brightness. The brightness after adjustment is 390 nit.
> >
> > Fixes: 3179338750d8 ("drm/panel: Support for IVO t109nw41 MIPI-DSI panel")
When applying your patch, I got a yell about your "Fixes" line. It
turns out you didn't copy the subject of the patch you're fixing
exactly. The above should be:
Fixes: 3179338750d8 ("drm/panel: himax-hx83102: Support for IVO
t109nw41 MIPI-DSI panel")
I'll fix that when applying. Please make sure you get the commit
subject exactly in the future.
> > Signed-off-by: Cong Yang
> > ---
> > drivers/gpu/drm/panel/panel-himax-hx83102.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
>
> No objection on my part. This is just modification of some constants,
> is well described, and is to a panel that you added so I don't think
> it needs a long bake time on the list. I'll plan to apply this on
> Monday unless there are comments or someone beats me to applying.
>
> Reviewed-by: Douglas Anderson
I've applied and pushed to drm-misc-fixes:
[1/1] drm/panel: himax-hx83102: Adjust power and gamma to optimize brightness
commit: fcf38bc321fbc87dfcd829f42e64e541f17599f7
-Doug
Re: [PATCH] drm/panel: himax-hx83102: Adjust power and gamma to optimize brightness
Hi,
On Thu, Oct 10, 2024 at 7:08 PM Cong Yang
wrote:
>
> The current panel brightness is only 360 nit. Adjust the power and gamma to
> optimize the panel brightness. The brightness after adjustment is 390 nit.
>
> Fixes: 3179338750d8 ("drm/panel: Support for IVO t109nw41 MIPI-DSI panel")
> Signed-off-by: Cong Yang
> ---
> drivers/gpu/drm/panel/panel-himax-hx83102.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
No objection on my part. This is just modification of some constants,
is well described, and is to a panel that you added so I don't think
it needs a long bake time on the list. I'll plan to apply this on
Monday unless there are comments or someone beats me to applying.
Reviewed-by: Douglas Anderson
Thanks!
-Doug
Re: [PATCH v2 1/1] drm/edp-panel: Add panels used by Dell XPS 13 9345
Hi, On Tue, Oct 8, 2024 at 12:30 AM Aleksandrs Vinarskis wrote: > > Introduce low-res IPS and OLED panels for mentioned device. > > SHP panel's timings were picked experimentally, without this patch or with > `delay_200_500_e50` panel sometimes fails to boot/stays black on startup. > > LGD panel's timings were copied from other LGD panels and tested to be > working. > > Particular laptop also comes in high-res IPS variant, which unfortunately > I do not have access to verify. > > The raw edid for SHP panel is: > > 00 ff ff ff ff ff ff 00 4d 10 93 15 00 00 00 00 > 2c 21 01 04 a5 1d 12 78 07 ee 95 a3 54 4c 99 26 > 0f 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 f0 7b 80 90 70 b0 52 45 30 20 > 36 00 20 b4 10 00 00 18 00 00 00 fd 00 1e 78 9a > 9a 20 01 0a 20 20 20 20 20 20 00 00 00 fe 00 4b > 4a 46 47 52 80 4c 51 31 33 34 4e 31 00 00 00 00 > 00 02 41 0c 32 00 01 00 00 0b 41 0a 20 20 01 ef > > 70 20 79 02 00 20 00 13 8c 52 19 93 15 00 00 00 > 00 2c 17 07 4c 51 31 33 34 4e 31 21 00 1d 40 0b > 08 07 80 07 b0 04 88 3d 8a 54 cd a4 99 66 62 0f > 02 45 54 d0 5f d0 5f 00 34 12 78 26 00 09 02 00 > 00 00 00 00 01 00 00 22 00 14 5e d7 04 05 7f 07 > 8f 00 2f 00 1f 00 af 04 50 00 02 00 05 00 25 01 > 09 5e d7 04 5e d7 04 1e 78 80 81 00 0b e3 05 80 > 00 e6 06 01 01 6a 6a 39 00 00 00 00 00 00 ce 90 > > The raw edid for LGD panel is: > > 00 ff ff ff ff ff ff 00 30 e4 78 07 00 00 00 00 > 00 22 01 04 b5 1d 12 78 06 96 65 b0 4f 3c b9 23 > 0b 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 ef 83 40 a0 b0 08 34 70 30 20 > 36 00 20 b4 10 00 00 1a 00 00 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 fe 00 44 > 48 39 50 57 80 31 33 34 57 54 31 0a 00 00 00 00 > 00 04 04 03 28 00 01 00 00 2b 01 0a 20 20 01 d4 > > 70 20 79 02 00 20 00 13 3c e6 24 78 07 00 00 00 > 00 00 18 07 31 33 34 57 54 31 0a 21 00 1d 41 0b > 08 07 40 0b 08 07 88 06 6b 4f c3 a3 b9 35 82 0b > 02 45 54 40 5e 1a 60 18 10 23 78 26 00 09 04 00 > 00 00 00 00 41 00 00 22 00 14 55 27 05 85 3f 0b > 9f 00 2f 80 1f 00 07 07 33 00 02 00 05 00 25 01 > 09 55 27 05 55 27 05 3c 3c 00 81 00 0b e3 05 80 > 00 e6 06 05 01 6d 60 02 00 00 00 00 00 00 31 90 > > Signed-off-by: Aleksandrs Vinarskis > --- > drivers/gpu/drm/panel/panel-edp.c | 2 ++ > 1 file changed, 2 insertions(+) Pushed to drm-misc-next: [1/1] drm/edp-panel: Add panels used by Dell XPS 13 9345 commit: 6b3815c6815f07acc7eeffa8ae734d1a1c0ee817 -Doug
Re: [PATCH v2 1/1] drm/edp-panel: Add panels used by Dell XPS 13 9345
Hi,
On Tue, Oct 8, 2024 at 12:30 AM Aleksandrs Vinarskis
wrote:
>
> Introduce low-res IPS and OLED panels for mentioned device.
>
> SHP panel's timings were picked experimentally, without this patch or with
> `delay_200_500_e50` panel sometimes fails to boot/stays black on startup.
>
> LGD panel's timings were copied from other LGD panels and tested to be
> working.
>
> Particular laptop also comes in high-res IPS variant, which unfortunately
> I do not have access to verify.
>
> The raw edid for SHP panel is:
>
> 00 ff ff ff ff ff ff 00 4d 10 93 15 00 00 00 00
> 2c 21 01 04 a5 1d 12 78 07 ee 95 a3 54 4c 99 26
> 0f 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 f0 7b 80 90 70 b0 52 45 30 20
> 36 00 20 b4 10 00 00 18 00 00 00 fd 00 1e 78 9a
> 9a 20 01 0a 20 20 20 20 20 20 00 00 00 fe 00 4b
> 4a 46 47 52 80 4c 51 31 33 34 4e 31 00 00 00 00
> 00 02 41 0c 32 00 01 00 00 0b 41 0a 20 20 01 ef
>
> 70 20 79 02 00 20 00 13 8c 52 19 93 15 00 00 00
> 00 2c 17 07 4c 51 31 33 34 4e 31 21 00 1d 40 0b
> 08 07 80 07 b0 04 88 3d 8a 54 cd a4 99 66 62 0f
> 02 45 54 d0 5f d0 5f 00 34 12 78 26 00 09 02 00
> 00 00 00 00 01 00 00 22 00 14 5e d7 04 05 7f 07
> 8f 00 2f 00 1f 00 af 04 50 00 02 00 05 00 25 01
> 09 5e d7 04 5e d7 04 1e 78 80 81 00 0b e3 05 80
> 00 e6 06 01 01 6a 6a 39 00 00 00 00 00 00 ce 90
>
> The raw edid for LGD panel is:
>
> 00 ff ff ff ff ff ff 00 30 e4 78 07 00 00 00 00
> 00 22 01 04 b5 1d 12 78 06 96 65 b0 4f 3c b9 23
> 0b 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 ef 83 40 a0 b0 08 34 70 30 20
> 36 00 20 b4 10 00 00 1a 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 fe 00 44
> 48 39 50 57 80 31 33 34 57 54 31 0a 00 00 00 00
> 00 04 04 03 28 00 01 00 00 2b 01 0a 20 20 01 d4
>
> 70 20 79 02 00 20 00 13 3c e6 24 78 07 00 00 00
> 00 00 18 07 31 33 34 57 54 31 0a 21 00 1d 41 0b
> 08 07 40 0b 08 07 88 06 6b 4f c3 a3 b9 35 82 0b
> 02 45 54 40 5e 1a 60 18 10 23 78 26 00 09 04 00
> 00 00 00 00 41 00 00 22 00 14 55 27 05 85 3f 0b
> 9f 00 2f 80 1f 00 07 07 33 00 02 00 05 00 25 01
> 09 55 27 05 55 27 05 3c 3c 00 81 00 0b e3 05 80
> 00 e6 06 05 01 6d 60 02 00 00 00 00 00 00 31 90
>
> Signed-off-by: Aleksandrs Vinarskis
> ---
> 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 767e47a2b0c1..8566e9cf2f82 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1977,11 +1977,13 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('L', 'G', 'D', 0x0567, &delay_200_500_e200_d200,
> "Unknown"),
> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200,
> "Unknown"),
> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200,
> "Unknown"),
> + EDP_PANEL_ENTRY('L', 'G', 'D', 0x0778, &delay_200_500_e200_d200,
> "134WT1"),
>
> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50,
> "LQ140M1JW48"),
> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50,
> "LQ140M1JW46"),
> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50,
> "LQ140T1JH01"),
> EDP_PANEL_ENTRY('S', 'H', 'P', 0x154c, &delay_200_500_p2e100,
> "LQ116M1JW10"),
> + EDP_PANEL_ENTRY('S', 'H', 'P', 0x1593, &delay_200_500_p2e100,
> "LQ134N1"),
Reviewed-by: Douglas Anderson
I'll plan to give it a day or so and then land it. Thanks!
-Doug
Re: [PATCH v1 1/1] drm/edp-panel: Add panels used by Dell XPS 13 9345
Hi, On Mon, Oct 7, 2024 at 1:14 PM Aleksandrs Vinarskis wrote: > > Introduce low-res IPS and OLED panels for mentioned device. > > SHP panel's timings were picked experimentally, without this patch or with > `delay_200_500_e50` panel sometimes fails to boot/stays black on startup. > > LGD panel's timings were copied from other LGD panels and tested to be > working. > > Particular laptop also comes in high-res IPS variant, which unfortunately > I do not have access to verify. > > Signed-off-by: Aleksandrs Vinarskis > Tested-by: Peter de Kraker Your signed-off-by should be _below_ Peter's Tested-by. That means that you're the one that signed-off on the fact that Peter tested this. > --- > drivers/gpu/drm/panel/panel-edp.c | 2 ++ > 1 file changed, 2 insertions(+) This looks OK to me. I've been requesting people include the RAW EDID of panels that they add in the commit message, though. Could you do that and send a v2? Also: note that since I didn't see Peter provide his Tested-by himself, I'd probably wait a little longer before landing to give him a chance to object. -Doug
Re: [PATCH v2 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off
Hi,
On Thu, Sep 26, 2024 at 2:29 AM Pin-yen Lin wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Dmitry Baryshkov
> Reviewed-by: Douglas Anderson
>
> ---
>
> Changes in v2:
> - Collect review tags
>
> drivers/gpu/drm/bridge/ite-it6505.c | 2 ++
> 1 file changed, 2 insertions(+)
Like with patch #1, meant to push to drm-misc-fixes but ended up on
drm-misc-next. Yell if this is a problem, but I think it should be OK.
[2/2] drm/bridge: it6505: Drop EDID cache on bridge power off
commit: 574c558ddb68591c9a4b7a95e45e935ab22c0fc6
-Doug
Re: [PATCH v2 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off
Hi,
On Thu, Sep 26, 2024 at 10:15 AM Doug Anderson wrote:
>
> Hi,
>
> On Thu, Sep 26, 2024 at 2:29 AM Pin-yen Lin wrote:
> >
> > The bridge might miss the display change events when it's powered off.
> > This happens when a user changes the external monitor when the system
> > is suspended and the embedded controller doesn't not wake AP up.
> >
> > It's also observed that one DP-to-HDMI bridge doesn't work correctly
> > when there is no EDID read after it is powered on.
> >
> > Drop the cache to force an EDID read after system resume to fix this.
> >
> > Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> > Signed-off-by: Pin-yen Lin
> > Reviewed-by: Dmitry Baryshkov
>
> I don't think it needs a re-spin, but for future reference you're
> always supposed to move your own Signed-off-by to the bottom whenever
> you "touch" a patch. Thus yours should be below Dmitry's tag.
>
> In any case,
>
> Reviewed-by: Douglas Anderson
>
> If these haven't been applied and there's no other feedback at the end
> of next week I'll plan to apply both this and the next patch to
> drm-misc-fixes.
Dang. My brain wasn't working quite right and I pushed these to
drm-misc-next instead of drm-misc-fixes. I'll assume that this is OK
because the problem fixed isn't exactly new and the patch will still
make it to mainline before too long. If this causes anyone problems
let me know and I can also land it on drm-misc-fixes.
[1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off
commit: 00ae002116a14c2e6a342c4c9ae080cdbb9b4b21
-Doug
Re: [PATCH v3 2/2] drm/panel: boe-th101mb31ig002: Modify Starry panel timing
Hi,
On Fri, Sep 27, 2024 at 2:44 AM Zhaoxiong Lv
wrote:
>
> In MTK chips, if the DRM runtime resume has not yet completed and the
> system enters sleep mode at the same time, there is a possibility of
> a black screen after waking the machine. Reduce the disable delay
> resolves this issue,
Similar to patch #1, this sounds very suspicious and I think you need
to root cause the problem and submit a proper fix instead of just
playing with timings.
> The "backlight_off_to_display_off_delay_ms" was added between
> "backlight off" and "display off" to prevent "display off" from being
> executed when the backlight is not fully powered off, which may cause
> a white screen. However, we removed this
> "backlight_off_to_display_off_delay_ms" and found that this situation
> did not occur. Therefore, in order to solve the problem mentioned
> above, we removed this delay, and the delay between "display off" and
> "enter sleep" is not defined in the spec, so we reduce it from 120ms
> to 50ms.
>
> In addition, T14 >= 120ms, so we change
> "enter_sleep_to_reset_down_delay_ms" from 100ms to 120ms.
>
> The panel spec:
> 1. https://github.com/Vme5o/power-on-off-sequential
>
> Fixes: e4bd1db1c1f7 ("drm/panel: boe-th101mb31ig002: Support for
> starry-er88577 MIPI-DSI panel")
>
> Signed-off-by: Zhaoxiong Lv
Similar to patch #1, no blank space between the "Fixes:" tag and the
"Signed-off-by:" tag.
-Doug
Re: [PATCH v3 1/2] drm/panel: jd9365da: Modify Kingdisplay and Melfas panel timing
Hi,
On Fri, Sep 27, 2024 at 2:44 AM Zhaoxiong Lv
wrote:
>
> In MTK chips, if the DRM runtime resume has not yet completed and the
> system enters sleep mode at the same time, there is a possibility of
> a black screen after waking the machine. Reduce the disable delay
> resolves this issue,
This sounds _really_ suspicious to me and it feels like you're just
pushing around timing to get lucky and avoid the problem. Generally if
decreasing delays like this fixes a functional problem then the
problem is still there (just hidden) and has the potential to come
back if a little extra delay shows up.
I don't understand _why_ it's important for DRM runtime resume to
complete before the system enters sleep. Is this causing the Mediatek
DRM driver to get confused? I would have expected that if the system
went fully into suspend that it would have totally powered off the
panel and then when we resume the panel shouldn't maintain any state
from before the suspend.
...so this needs to be debugged more and a real fix needs to be made.
> The "backlight_off_to_display_off_delay_ms" was added between
> "backlight off" and "display off" to prevent "display off" from being
> executed when the backlight is not fully powered off, which may cause
> a white screen. However, we removed this
> "backlight_off_to_display_off_delay_ms" and found that this situation
> did not occur. Therefore, in order to solve the problem mentioned
> above, we reduced it from 100ms to 3ms (tCMD_OFF >= 1ms).
>
> This is the timing specification for the two panels:
> 1. Kingdisplay panel timing spec:
> https://github.com/KD54183/-JD9365DA_Power-On-Off-Sequence_V0120240923
> 2. LMFBX101117480 timing spec: https://github.com/chiohsin-lo/TDY-JD_LIB
>
> Fixes: 2b976ad760dc ("drm/panel: jd9365da: Support for kd101ne3-40ti MIPI-DSI
> panel")
> Fixes: c4ce398cf18a ("drm/panel: jd9365da: Support for Melfas lmfbx101117480
> MIPI-DSI panel")
>
> Signed-off-by: Zhaoxiong Lv
For future reference: please don't put a blank line between the
"Fixes" and the "Signed-off-by".
-Doug
Re: [PATCH v3] drm/panel: elida-kd35t133: transition to mipi_dsi wrapped functions
Hi, On Wed, Sep 25, 2024 at 1:00 AM Tejas Vipin wrote: > > Changes the elida-kd35t133 panel to use multi style functions for > improved error handling. > > Reviewed-by: Jessica Zhang > Reviewed-by: Dmitry Baryshkov > Signed-off-by: Tejas Vipin > --- > Changes in v3: > - Added back bytes that were removed > - Replaced mipi_dsi_dcs_write_buffer_multi with > mipi_dsi_dcs_write_seq_multi > > Changes in v2: > - Changed mipi_dsi_dcs_write to mipi_dsi_dcs_write_buffer_multi > - Cleaned up error handling > > Link to v2: > https://lore.kernel.org/all/20240923122558.728516-1-tejasvipi...@gmail.com/ > Link to v1: > https://lore.kernel.org/all/20240917071710.1254520-1-tejasvipi...@gmail.com/ > --- > drivers/gpu/drm/panel/panel-elida-kd35t133.c | 108 --- > 1 file changed, 45 insertions(+), 63 deletions(-) Pushed to drm-misc-next: [1/1] drm/panel: elida-kd35t133: transition to mipi_dsi wrapped functions commit: a7b3bcc8e8495ff45128caab7ceee2534d1b8e8d -Doug
Re: [PATCH v2 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off
Hi,
On Thu, Sep 26, 2024 at 2:29 AM Pin-yen Lin wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Dmitry Baryshkov
I don't think it needs a re-spin, but for future reference you're
always supposed to move your own Signed-off-by to the bottom whenever
you "touch" a patch. Thus yours should be below Dmitry's tag.
In any case,
Reviewed-by: Douglas Anderson
If these haven't been applied and there's no other feedback at the end
of next week I'll plan to apply both this and the next patch to
drm-misc-fixes.
-Doug
Re: [PATCH v3] drm/panel: elida-kd35t133: transition to mipi_dsi wrapped functions
Hi, On Wed, Sep 25, 2024 at 1:00 AM Tejas Vipin wrote: > > Changes the elida-kd35t133 panel to use multi style functions for > improved error handling. > > Reviewed-by: Jessica Zhang > Reviewed-by: Dmitry Baryshkov > Signed-off-by: Tejas Vipin > --- > Changes in v3: > - Added back bytes that were removed > - Replaced mipi_dsi_dcs_write_buffer_multi with > mipi_dsi_dcs_write_seq_multi > > Changes in v2: > - Changed mipi_dsi_dcs_write to mipi_dsi_dcs_write_buffer_multi > - Cleaned up error handling > > Link to v2: > https://lore.kernel.org/all/20240923122558.728516-1-tejasvipi...@gmail.com/ > Link to v1: > https://lore.kernel.org/all/20240917071710.1254520-1-tejasvipi...@gmail.com/ > --- > drivers/gpu/drm/panel/panel-elida-kd35t133.c | 108 --- > 1 file changed, 45 insertions(+), 63 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH v2] drm/panel: elida-kd35t133: transition to mipi_dsi wrapped functions
Hi,
On Mon, Sep 23, 2024 at 5:33 AM Tejas Vipin wrote:
>
> -static int kd35t133_init_sequence(struct kd35t133 *ctx)
> +static void kd35t133_init_sequence(struct mipi_dsi_multi_context *dsi_ctx)
> {
> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> - struct device *dev = ctx->dev;
> -
> /*
> * Init sequence was supplied by the panel vendor with minimal
> * documentation.
> */
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_POSITIVEGAMMA,
> - 0x00, 0x13, 0x18, 0x04, 0x0f, 0x06, 0x3a, 0x56,
> - 0x4d, 0x03, 0x0a, 0x06, 0x30, 0x3e, 0x0f);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_NEGATIVEGAMMA,
> - 0x00, 0x13, 0x18, 0x01, 0x11, 0x06, 0x38, 0x34,
> - 0x4d, 0x06, 0x0d, 0x0b, 0x31, 0x37, 0x0f);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_POWERCONTROL1, 0x18, 0x17);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_POWERCONTROL2, 0x41);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_VCOMCONTROL, 0x00, 0x1a,
> 0x80);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x48);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_INTERFACEMODECTRL, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_FRAMERATECTRL, 0xa0);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_DISPLAYINVERSIONCTRL, 0x02);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_DISPLAYFUNCTIONCTRL,
> - 0x20, 0x02);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_SETIMAGEFUNCTION, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, KD35T133_CMD_ADJUSTCONTROL3,
> - 0xa9, 0x51, 0x2c, 0x82);
> - mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_INVERT_MODE, NULL, 0);
> -
> - dev_dbg(dev, "Panel init sequence done\n");
> - return 0;
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_POSITIVEGAMMA,
> +0x00, 0x13, 0x18, 0x04, 0x0f, 0x06,
> 0x3a, 0x56,
> +0x4d, 0x03, 0x0a, 0x06, 0x30, 0x3e,
> 0x0f);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_NEGATIVEGAMMA,
> +0x13, 0x18, 0x01, 0x11, 0x06, 0x38, 0x34,
> +0x06, 0x0d, 0x0b, 0x31, 0x37, 0x0f);
It seems like you dropped a few bytes in the above. Was this
intentional? You seem to have dropped the first byte from both of the
continuation lines (0x00 from the first, 0x4d from the second).
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_POWERCONTROL1,
> 0x18, 0x17);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_POWERCONTROL2,
> 0x41);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_VCOMCONTROL, 0x00,
> 0x1a, 0x80);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, MIPI_DCS_SET_ADDRESS_MODE,
> 0x48);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, MIPI_DCS_SET_PIXEL_FORMAT,
> 0x55);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_INTERFACEMODECTRL,
> 0x00);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_FRAMERATECTRL,
> 0xa0);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx,
> KD35T133_CMD_DISPLAYINVERSIONCTRL, 0x02);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx,
> KD35T133_CMD_DISPLAYFUNCTIONCTRL,
> +0x02);
This used to be the bytes 0x20, 0x02. Now it's just 0x02?
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_SETIMAGEFUNCTION,
> 0x00);
> + mipi_dsi_dcs_write_seq_multi(dsi_ctx, KD35T133_CMD_ADJUSTCONTROL3,
> +0x51, 0x2c, 0x82);
This used to be the bytes 0xa9, 0x51, 0x2c, 0x82. Now it's just 0x51,
0x2c, 0x82?
> + mipi_dsi_dcs_write_buffer_multi(dsi_ctx, NULL, 0);
Are you certain that the above is equivalent to the old
"mipi_dsi_dcs_write(dsi, MIPI_DCS_ENTER_INVERT_MODE, NULL, 0);" ?
Where is the "MIPI_DCS_ENTER_INVERT_MODE" constant?
I think the above needs to be:
mipi_dsi_dcs_write_seq_multi(dsi_ctx, MIPI_DCS_ENTER_INVERT_MODE);
I've confirmed that this compiles OK and that there's no need to
actually have any bytes in the sequence.
-Doug
Re: [PATCH 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off
Hi,
On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
> Signed-off-by: Pin-yen Lin
Ah, I guess this answers my question in the previous patch about
whether caching was important even for external displays since this
driver only supports external DP and the commit you mention in "Fixes"
says that caching was important.
So this looks reasonable. One thing I wonder is if you're totally
guaranteed to get a PM Runtime suspend whenever you get an unplug /
replug of a display. I tried to dig a little bit but I'm not super
familiar with this bridge and it looks complicated enough that I guess
I'll have to trust that it's fine. So...
Reviewed-by: Douglas Anderson
Re: [PATCH 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off
Hi,
On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> Signed-off-by: Pin-yen Lin
> ---
>
> drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
> 1 file changed, 1 insertion(+)
I'm not totally sure if it matters, but I wonder if you should change
this to only delete the EDID cache if you're in DP mode and not eDP
mode? For eDP mode the panel is not allowed to change and re-reading
it needlessly seems like it would slow down things like
suspend/resume. I think this would only matter if someone were using
eDP panels in the "old" way (not under the aux-bus) because we don't
set the "DRM_BRIDGE_OP_EDID" when we see "aux-bus", so maybe we don't
care that much but still...
Other than that, I know that there have been discussions in the past
about EDID caches but I can't quite remember all the details. I know
that panel-edp.c still caches it, so we must have concluded that it's
at least fine/reasonable for panels. I don't remember whether caching
is encouraged / suggested for external displays, though. Do you happen
to know if it even makes a difference there (in other words, do you
actually see multiple calls to read the EDID when you plug in a DP
display)?
-Doug
Re: [PATCH v2] drm/panel: raydium-rm69380: transition to mipi_dsi wrapped functions
Hi, On Fri, Sep 13, 2024 at 8:44 PM Tejas Vipin wrote: > > Changes the raydium-rm69380 panel to use multi style functions for > improved error handling. > > Reviewed-by: Douglas Anderson > Signed-off-by: Tejas Vipin > --- > Changes in v2: > - Fix whitespace issues > > Link to v1: > https://lore.kernel.org/all/20240907140130.410349-1-tejasvipi...@gmail.com/ > --- > drivers/gpu/drm/panel/panel-raydium-rm69380.c | 93 ++- > 1 file changed, 29 insertions(+), 64 deletions(-) Pushed to drm-misc-next: [1/1] drm/panel: raydium-rm69380: transition to mipi_dsi wrapped functions commit: f70181b3bdec6b8a166c6295937c4a8a5322515c -Doug
Re: [PATCH 4/8] drm/bridge: ti-sn65dsi86: annotate ti_sn_pwm_pin_{request, release} with __maybe_unused
Hi,
On Wed, Sep 11, 2024 at 1:02 AM Jani Nikula wrote:
>
> On Tue, 10 Sep 2024, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 10, 2024 at 3:04 AM Jani Nikula wrote:
> >>
> >> Building with clang, W=1, CONFIG_PM=n and CONFIG_OF_GPIO=n leads to
> >> warning about unused ti_sn_pwm_pin_request() and
> >> ti_sn_pwm_pin_release(). Fix by annotating them with __maybe_unused.
> >>
> >> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> >> inline functions for W=1 build").
> >>
> >> Signed-off-by: Jani Nikula
> >>
> >> ---
> >>
> >> Cc: Douglas Anderson
> >> Cc: Andrzej Hajda
> >> Cc: Neil Armstrong
> >> Cc: Robert Foss
> >> Cc: Laurent Pinchart
> >> Cc: Jonas Karlman
> >> Cc: Jernej Skrabec
> >> Cc: Nathan Chancellor
> >> ---
> >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Douglas Anderson
> >
> > I'm happy to land this in drm-misc-next unless there are any extra
> > dependencies. Let me know. In some sense I guess this could even be
> > considered a "Fix", though I guess given the history of the compiler
> > options that might be a bit of a stretch.
>
> drm-misc-next is fine. Agree on not considering this a fix.
It's only been on the list a day but it's simple so I just landed it
to drm-misc-next:
[4/8] drm/bridge: ti-sn65dsi86: annotate ti_sn_pwm_pin_{request,
release} with __maybe_unused
commit: 868cd000c19f77e4c25ce87c47b6f951facf4394
Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions
Hi,
On Wed, Sep 11, 2024 at 12:41 AM wrote:
>
> On 10/09/2024 23:19, Doug Anderson wrote:
> > Hi,
> >
> > On Sat, Sep 7, 2024 at 1:32 AM Tejas Vipin wrote:
> >>
> >> On 9/7/24 3:53 AM, Jessica Zhang wrote:
> >>>
> >>>
> >>> On 9/6/2024 3:14 PM, Jessica Zhang wrote:
> >>>>
> >>>>
> >>>> On 9/4/2024 7:15 AM, Tejas Vipin wrote:
> >>>>> Changes the himax-hx83112a panel to use multi style functions for
> >>>>> improved error handling.
> >>>>>
> >>>>> Signed-off-by: Tejas Vipin
> >>>>
> >>>> Reviewed-by: Jessica Zhang
> >>>
> >>> Hi Tejas,
> >>>
> >>> Just a heads up, it seems that this might be a duplicate of this change
> >>> [1]?
> >>>
> >>> Thanks,
> >>>
> >>> Jessica Zhang
> >>>
> >>> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
> >>
> >> Ah, thanks for letting me know. I hadn't realized someone else had
> >> started working on this too.
> >>
> >> However, I would argue that my patch [2] is a better candidate for merging
> >> because of the following reasons:
> >>
> >> 1) Removes unnecessary error printing:
> >> The mipi_dsi_*_multi() functions all have inbuilt error printing which
> >> makes printing errors after hx83112a_on unnecessary as is addressed in
> >> [2] like so:
> >>
> >>> - ret = hx83112a_on(ctx);
> >>> + ret = hx83112a_on(ctx->dsi);
> >>>if (ret < 0) {
> >>> - dev_err(dev, "Failed to initialize panel: %d\n", ret);
> >>>gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> >>>regulator_bulk_disable(ARRAY_SIZE(ctx->supplies),
> >>> ctx->supplies);
> >>> - return ret;
> >>>}
> >>
> >> [2] also removes the unnecessary dev_err after regulator_bulk_enable as was
> >> addressed in [3] like so:
> >>
> >>>ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies),
> >>> ctx->supplies);
> >>> - if (ret < 0) {
> >>> - dev_err(dev, "Failed to enable regulators: %d\n", ret);
> >>> + if (ret < 0)
> >>>return ret;
> >>> - }
> >>
> >> 2) Better formatting
> >>
> >> The mipi_dsi_dcs_write_seq_multi statements in [1] aren't formatted
> >> quite right according to what has been done so far. They are written as
> >> such in [1]:
> >>
> >>> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
> >>> 0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4,
> >>> 0x0e);
> >>
> >> Where they should be written as such in [2]:
> >>
> >>> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
> >>> + 0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d,
> >>> 0xa4, 0x0e);
> >>
> >> All in all, the module generated using my patch ends up being a teensy
> >> bit smaller (1% smaller). But if chronology is what is important, then
> >> it would at least be nice to see the above changes as part of Abhishek's
> >> patch too. And I'll be sure to look at the mail in the drm inbox now
> >> onwards :p
> >>
> >> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
> >> [2]
> >> https://lore.kernel.org/all/20240904141521.554451-1-tejasvipi...@gmail.com/
> >> [3]
> >> https://lore.kernel.org/all/CAD=FV=xrzkl_ppjukdk61fqkwhhiqcjlfmvbs7wso4suux2...@mail.gmail.com/
> >
> > I would tend to agree that Tejas's patch looks slightly better, but
> > Abhishek's patch appears to have been posted first.
> >
> > Neil: any idea what to do here? Maybe a Co-Developed-by or something?
> > ...or we could land Abhishek and Tejas could post a followup for the
> > extra cleanup?
>
> Yeah usually I take the first one when they are equal, but indeed Tejas
> cleanup up a little further and better aligned the parameters so I think
> Tejas's one is a better looking version.
>
> In this case we should apply Teja's one, nothing personal Abhishek!
Pushed to drm-misc-next:
[1/1] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions
commit: 32e5666b8a4d0f2aee39a0b2f8386cf9f86a8225
Re: [PATCH 4/8] drm/bridge: ti-sn65dsi86: annotate ti_sn_pwm_pin_{request, release} with __maybe_unused
Hi,
On Tue, Sep 10, 2024 at 3:04 AM Jani Nikula wrote:
>
> Building with clang, W=1, CONFIG_PM=n and CONFIG_OF_GPIO=n leads to
> warning about unused ti_sn_pwm_pin_request() and
> ti_sn_pwm_pin_release(). Fix by annotating them with __maybe_unused.
>
> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> inline functions for W=1 build").
>
> Signed-off-by: Jani Nikula
>
> ---
>
> Cc: Douglas Anderson
> Cc: Andrzej Hajda
> Cc: Neil Armstrong
> Cc: Robert Foss
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: Nathan Chancellor
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Douglas Anderson
I'm happy to land this in drm-misc-next unless there are any extra
dependencies. Let me know. In some sense I guess this could even be
considered a "Fix", though I guess given the history of the compiler
options that might be a bit of a stretch.
-Doug
Re: [PATCH] drm/panel: raydium-rm69380: transition to mipi_dsi wrapped functions
Hi, On Sat, Sep 7, 2024 at 7:01 AM Tejas Vipin wrote: > > Changes the raydium-rm69380 panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-raydium-rm69380.c | 95 ++- > 1 file changed, 30 insertions(+), 65 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH] drm/panel: samsung-s6e3fa7: transition to mipi_dsi wrapped functions
Hi, On Mon, Sep 2, 2024 at 12:10 AM Tejas Vipin wrote: > > Changes the samsung-s6e3fa7 panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c | 71 ++- > 1 file changed, 21 insertions(+), 50 deletions(-) Pushed to drm-misc-next: [1/1] drm/panel: samsung-s6e3fa7: transition to mipi_dsi wrapped functions commit: f327bfdbf6c6d7d8e5402795c7c97fb97c2dcf79 -Doug
Re: [PATCH v2] drm/panel: hx83112a: Transition to wrapped mipi_dsi functions
Hi, On Tue, Sep 3, 2024 at 10:32 AM Abhishek Tamboli wrote: > > Transition to mipi_dsi_dcs_write_seq_multi() macros for initialization > sequences. The previous mipi_dsi_dcs_write_seq() macros were > non-intuitive and use other wrapped MIPI DSI functions in the > driver code to simplify the code pattern. > > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202409040049.2hf8jrzg-...@intel.com/ You'd only include the above two tags if the original problematic commit had actually landed. Since it didn't you leave them off and the Robot gets no credit (even though it is awesome). > Signed-off-by: Abhishek Tamboli > --- > Changes in v2: > - Update the commit message to explain the reason for the change. > - Correct the code by changing 'dsi->mode_flags' to 'dsi_ctx.dsi->mode_flags' > This change addresses a build error in v1 reported by kernel test robot > caused by using an undeclared variable 'dsi'. > [v1] : > https://lore.kernel.org/all/20240902170153.34512-1-abhishektambo...@gmail.com/ > > drivers/gpu/drm/panel/panel-himax-hx83112a.c | 140 --- > 1 file changed, 60 insertions(+), 80 deletions(-) Just adding a note that there's nearly the same patch in https://lore.kernel.org/r/20240904141521.554451-1-tejasvipi...@gmail.com and we're discussing what to do about it there. -Doug
Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions
Hi,
On Sat, Sep 7, 2024 at 1:32 AM Tejas Vipin wrote:
>
> On 9/7/24 3:53 AM, Jessica Zhang wrote:
> >
> >
> > On 9/6/2024 3:14 PM, Jessica Zhang wrote:
> >>
> >>
> >> On 9/4/2024 7:15 AM, Tejas Vipin wrote:
> >>> Changes the himax-hx83112a panel to use multi style functions for
> >>> improved error handling.
> >>>
> >>> Signed-off-by: Tejas Vipin
> >>
> >> Reviewed-by: Jessica Zhang
> >
> > Hi Tejas,
> >
> > Just a heads up, it seems that this might be a duplicate of this change [1]?
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> > [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
>
> Ah, thanks for letting me know. I hadn't realized someone else had
> started working on this too.
>
> However, I would argue that my patch [2] is a better candidate for merging
> because of the following reasons:
>
> 1) Removes unnecessary error printing:
> The mipi_dsi_*_multi() functions all have inbuilt error printing which
> makes printing errors after hx83112a_on unnecessary as is addressed in
> [2] like so:
>
> > - ret = hx83112a_on(ctx);
> > + ret = hx83112a_on(ctx->dsi);
> > if (ret < 0) {
> > - dev_err(dev, "Failed to initialize panel: %d\n", ret);
> > gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> > regulator_bulk_disable(ARRAY_SIZE(ctx->supplies),
> > ctx->supplies);
> > - return ret;
> > }
>
> [2] also removes the unnecessary dev_err after regulator_bulk_enable as was
> addressed in [3] like so:
>
> > ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > - if (ret < 0) {
> > - dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > + if (ret < 0)
> > return ret;
> > - }
>
> 2) Better formatting
>
> The mipi_dsi_dcs_write_seq_multi statements in [1] aren't formatted
> quite right according to what has been done so far. They are written as
> such in [1]:
>
> > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
> > 0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4,
> > 0x0e);
>
> Where they should be written as such in [2]:
>
> > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
> > + 0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d,
> > 0xa4, 0x0e);
>
> All in all, the module generated using my patch ends up being a teensy
> bit smaller (1% smaller). But if chronology is what is important, then
> it would at least be nice to see the above changes as part of Abhishek's
> patch too. And I'll be sure to look at the mail in the drm inbox now
> onwards :p
>
> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
> [2]
> https://lore.kernel.org/all/20240904141521.554451-1-tejasvipi...@gmail.com/
> [3]
> https://lore.kernel.org/all/CAD=FV=xrzkl_ppjukdk61fqkwhhiqcjlfmvbs7wso4suux2...@mail.gmail.com/
I would tend to agree that Tejas's patch looks slightly better, but
Abhishek's patch appears to have been posted first.
Neil: any idea what to do here? Maybe a Co-Developed-by or something?
...or we could land Abhishek and Tejas could post a followup for the
extra cleanup?
Abhishek: are you planning to post more _multi cleanups? If so, please
make sure to CC Tejas (who has been posting a bunch of them) and
perhaps me since I've been helping to review them a bit.
-Doug
Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions
Hi, On Wed, Sep 4, 2024 at 7:15 AM Tejas Vipin wrote: > > Changes the himax-hx83112a panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-himax-hx83112a.c | 297 +-- > 1 file changed, 136 insertions(+), 161 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH] drm/panel: himax-hx83102: Add NULL pointer check in hx83102_get_modes
Hi,
On Fri, Aug 23, 2024 at 9:49 AM Doug Anderson wrote:
>
> Hi,
>
> On Fri, Aug 23, 2024 at 9:35 AM Charles Han wrote:
> >
> > In hx83102_get_modes(), the return value of drm_mode_duplicate()
> > is assigned to mode, which will lead to a possible NULL pointer
> > dereference on failure of drm_mode_duplicate(). Even though a
> > small allocation failing is basically impossible, kernel policy
> > is still to check for NULL so add the check.
> >
> > Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate
> > driver")
> > Signed-off-by: Charles Han
> > ---
> > drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> FWIW, this looks to be v4 of your patch, right? The subject line
> should include a version number and you should be providing version
> history "after the cut" in your patch. Tools like "b4" and "patman"
> can help you get this correct [1]. If you plan to continue posting
> patches you'll need to start getting this right. The next version of
> your patch would be v5.
>
> [1] https://sched.co/1aBGS
>
> I see:
>
> v1: https://lore.kernel.org/r/20240821095039.15282-1-hanchunc...@inspur.com
> v2: https://lore.kernel.org/r/20240822093442.4262-1-hanchunc...@inspur.com
> v3: https://lore.kernel.org/r/20240823083657.7100-1-hanchunc...@inspur.com
>
>
> > diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > index 6e4b7e4644ce..e67555323d3b 100644
> > --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > @@ -565,6 +565,8 @@ static int hx83102_get_modes(struct drm_panel *panel,
> > struct drm_display_mode *mode;
> >
> > mode = drm_mode_duplicate(connector->dev, m);
> > + if (!mode)
> > + return -EINVAL;
>
> I would have returned -ENOMEM since drm_mode_duplicate() is defined to
> allocate memory copy the mode (like strdup does for strings) and it
> should be clear that the only failure case is failure to allocate
> memory. Other callers convert a NULL return as -ENOMEM.
FWIW: if you spin v5 of this patch and have it return -ENOMEM then I'm
happy to apply it.
-Doug
Re: [PATCH] drm/panel: samsung-s6e3fa7: transition to mipi_dsi wrapped functions
Hi, On Mon, Sep 2, 2024 at 12:10 AM Tejas Vipin wrote: > > Changes the samsung-s6e3fa7 panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c | 71 ++- > 1 file changed, 21 insertions(+), 50 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH V3 RFC 8/9] usb: dwc2: Implement recovery after PM domain off
Hi,
On Wed, Aug 21, 2024 at 2:41 PM Stefan Wahren wrote:
>
> According to the dt-bindings there are some platforms, which have a
> dedicated USB power domain for DWC2 IP core supply. If the power domain
> is switched off during system suspend then all USB register will lose
> their settings.
>
> Use GUSBCFG_TOUTCAL as a canary to detect that the power domain has
> been powered off during suspend. Since the GOTGCTL_CURMODE_HOST doesn't
> match on all platform with the current mode, additionally backup
> GINTSTS. This works reliable to decide which registers should be
> restored.
>
> Signed-off-by: Stefan Wahren
> ---
> drivers/usb/dwc2/core.c | 1 +
> drivers/usb/dwc2/core.h | 2 ++
> drivers/usb/dwc2/platform.c | 38 +
> 3 files changed, 41 insertions(+)
> +static int dwc2_restore_critical_registers(struct dwc2_hsotg *hsotg)
> +{
> + struct dwc2_gregs_backup *gr;
> +
> + gr = &hsotg->gr_backup;
> +
> + if (!gr->valid) {
> + dev_err(hsotg->dev, "%s: no registers to restore\n",
> + __func__);
nit: IMO "__func__" should not be in dev_err() messages. The message
plus the device should be enough. If __func__ should have been in
dev_err() messages then the Linux kernel would have automatically put
it there.
Aside from that, this looks reasonable to me and discussed previously.
Reviewed-by: Douglas Anderson
Re: [PATCH v2] drm/panel: novatek-nt35950: transition to mipi_dsi wrapped functions
Hi, On Wed, Aug 28, 2024 at 11:26 AM Tejas Vipin wrote: > > Changes the novatek-nt35950 panel to use multi style functions for > improved error handling. > > Reviewed-by: Neil Armstrong > Signed-off-by: Tejas Vipin > --- > Changes in v2: > - Style changes > - Fixed changes in logic > > v1: > https://lore.kernel.org/all/20240824084422.202946-1-tejasvipi...@gmail.com/ > --- > drivers/gpu/drm/panel/panel-novatek-nt35950.c | 211 ++ > 1 file changed, 66 insertions(+), 145 deletions(-) Reviewed-by: Douglas Anderson Neil: Let me know if you want to land this or you want me to land it. Thanks! -Doug
Re: [PATCH 2/2] drm/panel: visionox-vtdr6130: switch to devm_regulator_bulk_get_const
Hi, On Wed, Aug 28, 2024 at 9:03 AM Neil Armstrong wrote: > > Switch to devm_regulator_bulk_get_const() to stop setting the supplies > list in probe(), and move the regulator_bulk_data struct in static const. > > Signed-off-by: Neil Armstrong > --- > drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 26 > +++-- > 1 file changed, 16 insertions(+), 10 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH 1/2] drm/panel: visionox-vtdr6130: switch to mipi_dsi wrapped functions
Hi,
On Wed, Aug 28, 2024 at 9:03 AM Neil Armstrong
wrote:
>
> Make usage of the new _multi() mipi_dsi functions instead of the
> deprecated macros, improving error handling and printing.
>
> bloat-o-meter gives a 12% gain on arm64:
> Function old new delta
> visionox_vtdr6130_unprepare 208 204 -4
> visionox_vtdr6130_prepare 1192 896-296
> Total: Before=2348, After=2048, chg -12.78%
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 186
> +++-
> 1 file changed, 82 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> index 540099253e1b..ebe92871dbb6 100644
> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> @@ -40,120 +40,103 @@ static void visionox_vtdr6130_reset(struct
> visionox_vtdr6130 *ctx)
> static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx)
> {
> struct mipi_dsi_device *dsi = ctx->dsi;
> - struct device *dev = &dsi->dev;
> - int ret;
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
This isn't something you introduced in your patch, but I wonder if we
should avoid setting the "MIPI_DSI_MODE_LPM" bit when the function
returns an error?
In any case:
Reviewed-by: Douglas Anderson
Re: [PATCH v2 2/2] drm/panel: add BOE tv101wum-ll2 panel driver
Hi,
On Wed, Aug 28, 2024 at 2:22 AM Neil Armstrong
wrote:
>
> +static int boe_tv101wum_ll2_off(struct boe_tv101wum_ll2 *ctx)
> +{
> + struct mipi_dsi_device *dsi = ctx->dsi;
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +
> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +
> + mipi_dsi_msleep(&dsi_ctx, 70);
> +
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +
> + mipi_dsi_msleep(&dsi_ctx, 20);
> +
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x04, 0x5a);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x05, 0x5a);
> +
> + mipi_dsi_msleep(&dsi_ctx, 150);
> +
> + return dsi_ctx.accum_err;
> +}
optional nit: now that the single caller of this function isn't
looking at the error code, you could make boe_tv101wum_ll2_off()
return "void".
In any case, this looks good.
Reviewed-by: Douglas Anderson
Re: [PATCH v2 2/4] Revert "drm/panel-edp: Add SDC ATNA45AF01"
Hi,
On Tue, Aug 27, 2024 at 9:26 AM wrote:
>
> On 27/08/2024 17:36, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jul 22, 2024 at 8:49 AM Doug Anderson wrote:
> >>
> >> Hi,
> >>
> >> On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong
> >>> wrote:
> >>>>
> >>>> On 15/07/2024 14:54, Stephan Gerhold wrote:
> >>>>> On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote:
> >>>>>> On 15/07/2024 14:15, Stephan Gerhold wrote:
> >>>>>>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01.
> >>>>>>>
> >>>>>>> The panel should be handled through the samsung-atna33xc20 driver for
> >>>>>>> correct power up timings. Otherwise the backlight does not work
> >>>>>>> correctly.
> >>>>>>>
> >>>>>>> We have existing users of this panel through the generic "edp-panel"
> >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> >>>>>>> partially in that configuration: It works after boot but once the
> >>>>>>> screen
> >>>>>>> gets disabled it does not turn on again until after reboot. It
> >>>>>>> behaves the
> >>>>>>> same way with the default "conservative" timings, so we might as well
> >>>>>>> drop
> >>>>>>> the configuration from the panel-edp driver. That way, users with old
> >>>>>>> DTBs
> >>>>>>> will get a warning and can move to the new driver.
> >>>>>>>
> >>>>>>> Reviewed-by: Douglas Anderson
> >>>>>>> Signed-off-by: Stephan Gerhold
> >>>>>>> ---
> >>>>>>> drivers/gpu/drm/panel/panel-edp.c | 2 --
> >>>>>>> 1 file changed, 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/panel/panel-edp.c
> >>>>>>> b/drivers/gpu/drm/panel/panel-edp.c
> >>>>>>> index 3a574a9b46e7..d2d682385e89 100644
> >>>>>>> --- a/drivers/gpu/drm/panel/panel-edp.c
> >>>>>>> +++ b/drivers/gpu/drm/panel/panel-edp.c
> >>>>>>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry
> >>>>>>> edp_panels[] = {
> >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200,
> >>>>>>> "Unknown"),
> >>>>>>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200,
> >>>>>>> "Unknown"),
> >>>>>>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200,
> >>>>>>> "ATNA45AF01"),
> >>>>>>> -
> >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50,
> >>>>>>> "LQ140M1JW48"),
> >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50,
> >>>>>>> "LQ140M1JW46"),
> >>>>>>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50,
> >>>>>>> "LQ140T1JH01"),
> >>>>>>>
> >>>>>>
> >>>>>> How will we handle current/old crd DT with new kernels ?
> >>>>>>
> >>>>>
> >>>>> I think this is answered in the commit message:
> >>>>>
> >>>>>>> We have existing users of this panel through the generic "edp-panel"
> >>>>>>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> >>>>>>> partially in that configuration: It works after boot but once the
> >>>>>>> screen
> >>>>>>> gets disabled it does not turn on again until after reboot. It
> >>>>>>> behaves the
> >>>>>>> same way with the default "conser
Re: [PATCH] drm/panel: novatek-nt35950: transition to mipi_dsi wrapped functions
Hi,
On Sat, Aug 24, 2024 at 1:44 AM Tejas Vipin wrote:
>
> +static void nt35950_set_cmd2_page(struct mipi_dsi_multi_context *dsi_ctx,
> + struct nt35950 *nt, u8 page)
> {
> const u8 mauc_cmd2_page[] = { MCS_CMD_MAUCCTR, 0x55, 0xaa, 0x52,
> 0x08, page };
> - int ret;
>
> - ret = mipi_dsi_dcs_write_buffer(nt->dsi[0], mauc_cmd2_page,
> + mipi_dsi_dcs_write_buffer_multi(dsi_ctx, mauc_cmd2_page,
> ARRAY_SIZE(mauc_cmd2_page));
> - if (ret < 0)
> - return ret;
> + if (dsi_ctx->accum_err)
> + return;
>
> nt->last_page = page;
> - return 0;
nit: It's a style choice, but I personally would have changed the above to just:
if (!dsi_ctx->accum_err)
nt->last_page = page;
> @@ -284,109 +258,68 @@ static int nt35950_on(struct nt35950 *nt)
> {
> const struct nt35950_panel_mode *mode_data = nt->desc->mode_data;
> struct mipi_dsi_device *dsi = nt->dsi[0];
> - struct device *dev = &dsi->dev;
> - int ret;
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> nt->cur_mode = nt35950_get_current_mode(nt);
> nt->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
> nt->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
>
> - ret = nt35950_set_cmd2_page(nt, 0);
> - if (ret < 0)
> - return ret;
> -
> - ret = nt35950_set_data_compression(nt,
> mode_data[nt->cur_mode].compression);
> - if (ret < 0)
> - return ret;
> -
> - ret = nt35950_set_scale_mode(nt, mode_data[nt->cur_mode].scaler_mode);
> - if (ret < 0)
> - return ret;
> -
> - ret = nt35950_set_scaler(nt, mode_data[nt->cur_mode].scaler_on);
> - if (ret < 0)
> - return ret;
> + nt35950_set_cmd2_page(&dsi_ctx, nt, 0);
> + nt35950_set_data_compression(&dsi_ctx, nt,
> mode_data[nt->cur_mode].compression);
> + nt35950_set_scale_mode(&dsi_ctx, mode_data[nt->cur_mode].scaler_mode);
> + nt35950_set_scaler(&dsi_ctx, mode_data[nt->cur_mode].scaler_on);
> + nt35950_set_dispout(&dsi_ctx, nt);
>
> - ret = nt35950_set_dispout(nt);
> - if (ret < 0)
> - return ret;
> -
> - ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear on: %d\n", ret);
> - return ret;
> - }
> -
> - ret = mipi_dsi_dcs_set_tear_scanline(dsi, 0);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear scanline: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> + mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx, 0);
>
> /* CMD2 Page 1 */
> - ret = nt35950_set_cmd2_page(nt, 1);
> - if (ret < 0)
> - return ret;
> + nt35950_set_cmd2_page(&dsi_ctx, nt, 1);
>
> /* Unknown command */
> - mipi_dsi_dcs_write_seq(dsi, 0xd4, 0x88, 0x88);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd4, 0x88, 0x88);
>
> /* CMD2 Page 7 */
> - ret = nt35950_set_cmd2_page(nt, 7);
> - if (ret < 0)
> - return ret;
> + nt35950_set_cmd2_page(&dsi_ctx, nt, 7);
>
> /* Enable SubPixel Rendering */
> - mipi_dsi_dcs_write_seq(dsi, MCS_PARAM_SPR_EN, 0x01);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MCS_PARAM_SPR_EN, 0x01);
>
> /* SPR Mode: YYG Rainbow-RGB */
> - mipi_dsi_dcs_write_seq(dsi, MCS_PARAM_SPR_MODE,
> MCS_SPR_MODE_YYG_RAINBOW_RGB);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MCS_PARAM_SPR_MODE,
> +MCS_SPR_MODE_YYG_RAINBOW_RGB);
>
> /* CMD3 */
> - ret = nt35950_inject_black_image(nt);
> - if (ret < 0)
> - return ret;
> + nt35950_inject_black_image(&dsi_ctx);
> + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 120);
>
> - ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> - if (ret < 0)
> - return ret;
> - msleep(120);
> -
> - ret = mipi_dsi_dcs_set_display_on(dsi);
> - if (ret < 0)
> - return ret;
> - msleep(120);
> + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 120);
>
> nt->dsi[0]->mode_flags &= ~MIPI_DSI_MODE_LPM;
> nt->dsi[1]->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - return 0;
> + return dsi_ctx.accum_err;
I think you only want to clear "MIPI_DSI_MODE_LPM" if there was no
error, right? That matches the old code. So you'd want:
if (dsi_ctx.accum_err)
return dsi_ctx.accum_err;
nt->dsi[0]->mode_flags &= ~MIPI_DSI_MODE_LPM;
nt->dsi[1]->mode_flags &= ~MIPI_DSI_MODE_LPM;
return 0;
> static int nt35950_off(struct nt35950 *nt)
> {
> - st
Re: [PATCH v2 2/4] Revert "drm/panel-edp: Add SDC ATNA45AF01"
Hi,
On Mon, Jul 22, 2024 at 8:49 AM Doug Anderson wrote:
>
> Hi,
>
> On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson wrote:
> >
> > Hi,
> >
> > On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong
> > wrote:
> > >
> > > On 15/07/2024 14:54, Stephan Gerhold wrote:
> > > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote:
> > > >> On 15/07/2024 14:15, Stephan Gerhold wrote:
> > > >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01.
> > > >>>
> > > >>> The panel should be handled through the samsung-atna33xc20 driver for
> > > >>> correct power up timings. Otherwise the backlight does not work
> > > >>> correctly.
> > > >>>
> > > >>> We have existing users of this panel through the generic "edp-panel"
> > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> > > >>> partially in that configuration: It works after boot but once the
> > > >>> screen
> > > >>> gets disabled it does not turn on again until after reboot. It
> > > >>> behaves the
> > > >>> same way with the default "conservative" timings, so we might as well
> > > >>> drop
> > > >>> the configuration from the panel-edp driver. That way, users with old
> > > >>> DTBs
> > > >>> will get a warning and can move to the new driver.
> > > >>>
> > > >>> Reviewed-by: Douglas Anderson
> > > >>> Signed-off-by: Stephan Gerhold
> > > >>> ---
> > > >>>drivers/gpu/drm/panel/panel-edp.c | 2 --
> > > >>>1 file changed, 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c
> > > >>> b/drivers/gpu/drm/panel/panel-edp.c
> > > >>> index 3a574a9b46e7..d2d682385e89 100644
> > > >>> --- a/drivers/gpu/drm/panel/panel-edp.c
> > > >>> +++ b/drivers/gpu/drm/panel/panel-edp.c
> > > >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry
> > > >>> edp_panels[] = {
> > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200,
> > > >>> "Unknown"),
> > > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200,
> > > >>> "Unknown"),
> > > >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200,
> > > >>> "ATNA45AF01"),
> > > >>> -
> > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50,
> > > >>> "LQ140M1JW48"),
> > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50,
> > > >>> "LQ140M1JW46"),
> > > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50,
> > > >>> "LQ140T1JH01"),
> > > >>>
> > > >>
> > > >> How will we handle current/old crd DT with new kernels ?
> > > >>
> > > >
> > > > I think this is answered in the commit message:
> > > >
> > > >>> We have existing users of this panel through the generic "edp-panel"
> > > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> > > >>> partially in that configuration: It works after boot but once the
> > > >>> screen
> > > >>> gets disabled it does not turn on again until after reboot. It
> > > >>> behaves the
> > > >>> same way with the default "conservative" timings, so we might as well
> > > >>> drop
> > > >>> the configuration from the panel-edp driver. That way, users with old
> > > >>> DTBs
> > > >>> will get a warning and can move to the new driver.
> > > >
> > > > Basically with the entry removed, the panel-edp driver will fallback to
> > > > default "conservative" timings when using old DTBs. There will be a
> > > > warning in dmesg, but otherwise the panel will somewhat work just as
> > >
Re: [PATCH v2] drm/panel-edp: add BOE NE140WUM-N6G panel entry
Hi, On Mon, Aug 26, 2024 at 5:39 AM Abel Vesa wrote: > > Add an eDP panel entry for BOE NE140WUM-N6G. > > Due to lack of documentation, use the delay_200_500_e80 timings like > some other BOE entries for now. > > The raw edid of the panel is: > > 00 ff ff ff ff ff ff 00 09 e5 66 0b 00 00 00 00 > 1a 20 01 04 a5 1e 13 78 07 01 5f a7 54 4c 9b 24 > 11 51 56 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 9c 3e 80 c8 70 b0 3c 40 30 20 > 36 00 2e bc 10 00 00 1a 16 32 80 c8 70 b0 3c 40 > 30 20 36 00 2e bc 10 00 00 1a 00 00 00 fd 00 1e > 3c 4c 4c 10 01 0a 20 20 20 20 20 20 00 00 00 fe > 00 4e 45 31 34 30 57 55 4d 2d 4e 36 47 0a 00 dc > > Reviewed-by: Douglas Anderson > Signed-off-by: Abel Vesa > --- > Changes in v2: > - Added raw EDID to commit message, as per Doug's reguest > - Picked up Doug's R-b tag > - Link to v1: > https://lore.kernel.org/r/20240823-drm-panel-edp-add-boe-ne140wum-n6g-v1-1-7bdd3c003...@linaro.org > --- > drivers/gpu/drm/panel/panel-edp.c | 1 + > 1 file changed, 1 insertion(+) Pushed to drm-misc-next: [1/1] drm/panel-edp: add BOE NE140WUM-N6G panel entry commit: 51394119f640423858a2f04076d6f1c3e83fa715
Re: [PATCH] drm/panel: himax-hx83102: Add NULL pointer check in hx83102_get_modes
Hi,
On Fri, Aug 23, 2024 at 9:35 AM Charles Han wrote:
>
> In hx83102_get_modes(), the return value of drm_mode_duplicate()
> is assigned to mode, which will lead to a possible NULL pointer
> dereference on failure of drm_mode_duplicate(). Even though a
> small allocation failing is basically impossible, kernel policy
> is still to check for NULL so add the check.
>
> Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate driver")
> Signed-off-by: Charles Han
> ---
> drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 ++
> 1 file changed, 2 insertions(+)
FWIW, this looks to be v4 of your patch, right? The subject line
should include a version number and you should be providing version
history "after the cut" in your patch. Tools like "b4" and "patman"
can help you get this correct [1]. If you plan to continue posting
patches you'll need to start getting this right. The next version of
your patch would be v5.
[1] https://sched.co/1aBGS
I see:
v1: https://lore.kernel.org/r/20240821095039.15282-1-hanchunc...@inspur.com
v2: https://lore.kernel.org/r/20240822093442.4262-1-hanchunc...@inspur.com
v3: https://lore.kernel.org/r/20240823083657.7100-1-hanchunc...@inspur.com
> diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> index 6e4b7e4644ce..e67555323d3b 100644
> --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> @@ -565,6 +565,8 @@ static int hx83102_get_modes(struct drm_panel *panel,
> struct drm_display_mode *mode;
>
> mode = drm_mode_duplicate(connector->dev, m);
> + if (!mode)
> + return -EINVAL;
I would have returned -ENOMEM since drm_mode_duplicate() is defined to
allocate memory copy the mode (like strdup does for strings) and it
should be clear that the only failure case is failure to allocate
memory. Other callers convert a NULL return as -ENOMEM.
-Doug
Re: [PATCH] drm/panel-edp: add BOE NE140WUM-N6G panel entry
Hi Abel,
On Fri, Aug 23, 2024 at 5:16 AM Abel Vesa wrote:
>
> Add an eDP panel entry for BOE NE140WUM-N6G.
>
> Due to lack of documentation, use the delay_200_500_e80 timings like
> some other BOE entries for now.
>
> Signed-off-by: Abel Vesa
> ---
> drivers/gpu/drm/panel/panel-edp.c | 1 +
> 1 file changed, 1 insertion(+)
This looks fine to me:
Reviewed-by: Douglas Anderson
I started getting in the habit of requesting that people include the
raw EDID of panels in the commit message when adding them. Any way you
could post a v2 with that info? I just imagine this might be useful
someday if we ever have another instance of the type of issue Hsin-Yi
had to fix in commit ca3c7819499e ("drm/panel-edp: Fix AUO 0x405c
panel naming and add a variant").
-Doug
Re: [PATCH v4 0/2] Add Add elan-ekth6a12nay on the basis of elan-ekth6915
Jiri / Ben, On Mon, Jul 22, 2024 at 12:31 AM Zhaoxiong Lv wrote: > > Elan-ekth6a12nay 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 300ms, so the > post_gpio_reset_on_delay_ms is set to 300. > > The Elan-ekth6a12nay touch screen chip same as Elan-eKTH6915 controller > has a reset gpio. The difference is that they have different > post_power_delay_ms. > > Changes between V4 and V3: > - PATCH 1/2: Combine the 2 const into an enum. > - PATCH 2/2: No changes. > - Link to v3: > https://lore.kernel.org/all/20240716082851.18173-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Changes between V3 and V2: > - PATCH 1/2: "ekth6915" isn't a fallback, modify it. > - PATCH 2/2: No changes. > - Link to v2: > https://lore.kernel.org/all/20240715073159.25064-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Changes between V2 and V1: > - PATCH 1/2: Respin the series on top of v6.10. > - PATCH 2/2: No changes. > - Link to v1: > https://lore.kernel.org/all/2024070408.11204-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Zhaoxiong Lv (2): > dt-bindings: HID: i2c-hid: elan: Introduce Elan ekth6a12nay > HID: i2c-hid: elan: Add elan-ekth6a12nay timing > > .../devicetree/bindings/input/elan,ekth6915.yaml | 4 +++- > drivers/hid/i2c-hid/i2c-hid-of-elan.c | 8 > 2 files changed, 11 insertions(+), 1 deletion(-) I think this series is ready for you to merge at your leisure. If there's anything blocking it then please yell. Thanks! :-) -Doug
Re: [PATCH] drm/panel: fix null pointer dereference in hx83102_get_modes
Hi,
On Thu, Aug 22, 2024 at 3:02 AM cong yang
wrote:
>
> Hi,
>
> Charles Han 于2024年8月22日周四 17:34写道：
> >
> > In hx83102_get_modes(), the return value of drm_mode_duplicate()
> > is assigned to mode, which will lead to a possible NULL
> > pointer dereference on failure of drm_mode_duplicate(). Add a
> > check to avoid npd.
> >
> > Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate
> > driver")
> >
> > Signed-off-by: Charles Han
Note: please no blank line between "Fixes" and "Signed-off-by". All
tags should be together in the last "paragraph".
> > ---
> > drivers/gpu/drm/panel/panel-himax-hx83102.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > index 6e4b7e4644ce..7c2a5e9b7fb3 100644
> > --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> > @@ -565,6 +565,10 @@ static int hx83102_get_modes(struct drm_panel *panel,
> > struct drm_display_mode *mode;
> >
> > mode = drm_mode_duplicate(connector->dev, m);
> > + if (!mode) {
> > + dev_err(&ctx->dsi->dev, "bad mode or failed to add mode\n");
> > + return -EINVAL;
> > + }
>
> In my V2 series, Doug suggested:
> "nit: no need for an error message when drm_mode_duplicate() fails.
> It is incredibly unlikely that the allocation will fail and the Linux"
>
> https://lore.kernel.org/all/CAD=FV=v2o2afdvn5cjbxfgcolkmnp-g3chvqqkoub2mdb+n...@mail.gmail.com/
Sorry for missing that we still need the NULL check and we should
definitely add it in. Cong is right, though, that the error message
here is pointless. The only way the function can fail is if a small
memory allocation fails. Even though such a small allocation failing
is basically impossible, kernel policy is still to check for NULL so
we should add the check. ...but the kernel already adds a WARN_ON
splat for all failed allocations so the extra message here is just
wasteful.
-Doug
Re: [PATCH v3 2/2] drm/panel: jdi-fhd-r63452: transition to mipi_dsi wrapped functions
Hi, On Sat, Aug 17, 2024 at 11:08 PM Tejas Vipin wrote: > > Changes the jdi-fhd-r63452 panel to use multi style functions for > improved error handling. > > Reviewed-by: Douglas Anderson > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 149 ++- > 1 file changed, 48 insertions(+), 101 deletions(-) Pushed to drm-misc-next. [2/2] drm/panel: jdi-fhd-r63452: transition to mipi_dsi wrapped functions commit: 04b5b362bc2a36f1dfe5cad52c83b1ea9d25b87c
Re: [PATCH v3 1/2] drm/mipi-dsi: Add mipi_dsi_dcs_set_tear_scanline_multi
Hi, On Sat, Aug 17, 2024 at 11:08 PM Tejas Vipin wrote: > > mipi_dsi_dcs_set_tear_scanline_multi can heavily benefit from being > converted to a multi style function as it is often called in the context of > similar functions. > > Reviewed-by: Douglas Anderson > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/drm_mipi_dsi.c | 31 +++ > include/drm/drm_mipi_dsi.h | 2 ++ > 2 files changed, 33 insertions(+) Since I adjusted the whitespace on the previous patch when applying I had to adjust this one to fix the merge conflict. Then I also fixed the whitespace on this patch since the DRM tools run checkpatch in a more strict mode. :P In any case, pushed to drm-misc-next. [1/2] drm/mipi-dsi: Add mipi_dsi_dcs_set_tear_scanline_multi commit: 051c86afc342aed1f84d66ff5d09dc9e1c1685a1
Re: [PATCH] drm/panel: mantix-mlaf057we51: transition to mipi_dsi wrapped functions
Hi, On Sun, Aug 18, 2024 at 12:24 AM Tejas Vipin wrote: > > Changes the mantix-mlaf057we51 panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > .../gpu/drm/panel/panel-mantix-mlaf057we51.c | 79 +++ > 1 file changed, 27 insertions(+), 52 deletions(-) I'll leave it up to Neil if he wants you to respin with the upper/lowercase done in a separate patch. Given that Neil is tagged as "DRM PANEL DRIVERS" maintainer, the paint color he wants for the bikeshed [1] trumps the paint color that Dmitry wants. ;-) With, or without the upper/lowercase patch split out, feel free to add: Reviewed-by: Douglas Anderson [1] https://en.wiktionary.org/wiki/bikeshedding
Re: [PATCH v3 0/2] extend "multi" functions and use them in jdi-fhd-r63452
Hi, On Sat, Aug 17, 2024 at 11:08 PM Tejas Vipin wrote: > > This patch adds mipi_dsi_dcs_set_tear_scanline_multi to the list of multi > functions and uses it with other multi functions in the jdi-fhd-r63452 > panel. > > This patch uses functions introduced in [1] and must be applied after > it. > > [1] > https://lore.kernel.org/all/20240806135949.468636-1-tejasvipi...@gmail.com/ > --- > Changes in v3: > - use mipi_dsi_usleep_range Oh! Thanks for updating this. I had been debating whether we should add mipi_dsi_usleep_range() but hadn't noticed that someone already had. Nice! :-) I think this series is pretty much ready to apply, but I'll give it one more day (or Neil can apply them if he's good w/ them). -Doug
Re: [PATCH] drm/panel: mantix-mlaf057we51: transition to mipi_dsi wrapped functions
Hi, On Mon, Aug 19, 2024 at 8:36 AM wrote: > > Hi, > > On 18/08/2024 09:23, Tejas Vipin wrote: > > Changes the mantix-mlaf057we51 panel to use multi style functions for > > improved error handling. > > > > Signed-off-by: Tejas Vipin > > --- > > .../gpu/drm/panel/panel-mantix-mlaf057we51.c | 79 +++ > > 1 file changed, 27 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c > > b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c > > index ea4a6bf6d35b..4db852ffb0f6 100644 > > --- a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c > > +++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c > > @@ -23,7 +23,7 @@ > > > > /* Manufacturer specific Commands send via DSI */ > > #define MANTIX_CMD_OTP_STOP_RELOAD_MIPI 0x41 > > -#define MANTIX_CMD_INT_CANCEL 0x4C > > +#define MANTIX_CMD_INT_CANCEL 0x4c > > Please move cleanups to separate patches LOL, in a previous patch series I had the upper-to-lowercase in a separate patch and someone yelled at me to do the opposite and squash it together [1]. It doesn't really matter too much to me, but given the previous feedback I've just been suggesting that Tejas squash it together with his conversions. I'm OK either way, though. [1] https://lore.kernel.org/r/caa8ejpo4wzmpnjpnkht-_gje2taf_i_g+etajrgipmezppc...@mail.gmail.com -Doug
Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off
Hi, On Wed, Aug 14, 2024 at 2:48 PM Stefan Wahren wrote: > > >> You're saying that your > >> registers get saved _unless_ the power domain gets turned off, right? > On BCM2835 there is no need to store the registers because there is no > power management supported by USB core except of the power domain. So > DWC2 don't expect a register loss. > >> ...and the device core keeps power domains on for suspended devices if > >> they are wakeup sources, which makes sense. > >> > >> So with that, your patch sounds like a plausible way to do it. I guess > >> one other way to do it would be some sort of "canary" approach. You > >> could _always_ save registers and then, at resume time, you could > >> detect if some "canary" register had reset to its power-on default. If > >> you see this then you can assume power was lost and re-init all the > >> registers. This could be pretty much any register that you know won't > >> be its power on default. In some ways a "canary" approach is uglier > >> but it also might be more reliable across more configurations? > I don't have enough knowledge about DWC2 and i also don't have the > databook to figure out if there is a magic register which could be used > for the canary approach. But all these different platforms, host vs > gadget role, different low modes let me think the resulting solution > would be also fragile and ugly. I won't admit to having a DWC2 databook. ;-) ...but don't think it's too hard to find a good canary. What about "GAHBCFG_GLBL_INTR_EN" ? From a quick glance it looks like the driver seems to set that bit during driver startup and then it stays on until driver shutdown. The databook that I definitely won't admit to having almost certainly says that this register resets to 0 on all hardware and it's applicable to both host and device. I think you could say that if the register is 0 at resume time that registers must have been lost and you can restore them. I guess if that doesn't work then "GUSBCFG_TOUTCAL" could be used (I think that resets to 0 but must be initted to non-0 by the driver). Yet another register that could probably work as a canary would be "GINTMSK". I believe that inits to all 0 (everything is masked) and obviously to use the device we've got to unmask _some_ interrupts. I can look for more, if need be. -Doug
Re: [PATCH V2 14/16] WIP: usb: dwc2: Implement recovery after PM domain off
Hi, On Mon, Aug 12, 2024 at 4:48 PM Stefan Wahren wrote: > > Hi Doug, > > Am 28.07.24 um 15:00 schrieb Stefan Wahren: > > DO NOT MERGE > > > > According to the dt-bindings there are some platforms, which have a > > dedicated USB power domain for DWC2 IP core supply. If the power domain > > is switched off during system suspend then all USB register will lose > > their settings. > > > > So use the power on/off notifier in order to save & restore the USB > > registers during system suspend. > sorry for bothering you with this DWC2 stuff, but it would great if you > can gave some feedback about this patch. Boy, it's been _ages_ since I looked at anything to do with dwc2, but I still have some fondness in my heart for the crufty old driver :-P I know I was involved with some of the patches to get wakeup-from-suspend working on dwc2 host controllers in the past but, if I remember correctly, I mostly shepherded / fixed patches from Rockchip. Not sure I can spend the days trawling through the driver and testing things with printk that really answering properly would need, but let's see... > I was working a lot to get > suspend to idle working on Raspberry Pi. And this patch is the most > complex part of the series. > > Would you agree with this approach or did i miss something? > > The problem is that the power domain driver acts independent from dwc2, > so we cannot prevent the USB domain power down except declaring a USB > device as wakeup source. So i decided to use the notifier approach. This > has been successful tested on some older Raspberry Pi boards. My genpd knowledge is probably not as good as it should be. Don't tell anyone (aside from all the people and lists CCed here). ;-) ...so I guess you're relying on the fact that dev_pm_genpd_add_notifier() will return an error if a power-domain wasn't specified for dwc2 in the device tree, then you ignore that error and your callback will never happen. You assume that the power domain isn't specified then the dwc2 registers will be saved? I guess one thing is that I'd wonder if that's really reliable. Maybe some dwc2 controllers lose their registers over system suspend but _don't_ specify a power domain? Maybe the USB controller just gets its power yanked as part of system suspend. Maybe that's why the functions for saving / restoring registers are already there? It looks like there are ways for various platforms to specify that registers are lost in some cases... ...but I guess you can't use the existing ways to say that registers are lost because you're trying to be dynamic. You're saying that your registers get saved _unless_ the power domain gets turned off, right? ...and the device core keeps power domains on for suspended devices if they are wakeup sources, which makes sense. So with that, your patch sounds like a plausible way to do it. I guess one other way to do it would be some sort of "canary" approach. You could _always_ save registers and then, at resume time, you could detect if some "canary" register had reset to its power-on default. If you see this then you can assume power was lost and re-init all the registers. This could be pretty much any register that you know won't be its power on default. In some ways a "canary" approach is uglier but it also might be more reliable across more configurations? I guess those would be my main thoughts on the topic. Is that roughly the feedback you were looking for? -Doug
Re: [PATCH v2 2/2] drm/panel: jdi-fhd-r63452: transition to mipi_dsi wrapped functions
Hi, On Mon, Aug 12, 2024 at 11:31 PM Tejas Vipin wrote: > > Changes the jdi-fhd-r63452 panel to use multi style functions for > improved error handling. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 150 ++- > 1 file changed, 49 insertions(+), 101 deletions(-) Looks nice to me now. Reviewed-by: Douglas Anderson ...as per usual, I'll give this a snooze for a week or so and plan to apply it if nothing comes up.
Re: [PATCH v3 2/2] drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped functions
Hi, On Tue, Aug 6, 2024 at 7:00 AM Tejas Vipin wrote: > > Use multi style wrapped functions for mipi_dsi in the > startek-kd070fhfid015 panel. > > Signed-off-by: Tejas Vipin > --- > .../drm/panel/panel-startek-kd070fhfid015.c | 115 ++ > 1 file changed, 35 insertions(+), 80 deletions(-) Pushed to drm-misc-next: [2/2] drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped functions commit: b080a60731ad909eae4463684acc23d322e93579
Re: [PATCH v3 1/2] drm/mipi-dsi: add more multi functions for better error handling
Hi, On Tue, Aug 6, 2024 at 7:00 AM Tejas Vipin wrote: > > Add more functions that can benefit from being multi style and mark > older variants as deprecated to eventually convert all mipi_dsi functions > to multi style. > > Acked-by: Maxime Ripard > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/drm_mipi_dsi.c | 194 + > include/drm/drm_mipi_dsi.h | 10 ++ > 2 files changed, 204 insertions(+) Pushed to drm-misc-next: [1/2] drm/mipi-dsi: add more multi functions for better error handling commit: 5ddb0a8aa8e4754a8fb77e284e0d6f46c2350f88
Re: [PATCH 2/2] drm/panel: jdi-fhd-r63452: transition to mipi_dsi wrapped functions
Hi,
On Fri, Aug 9, 2024 at 9:55 PM Tejas Vipin wrote:
>
> @@ -41,79 +41,41 @@ static void jdi_fhd_r63452_reset(struct jdi_fhd_r63452
> *ctx)
> static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
> {
> struct mipi_dsi_device *dsi = ctx->dsi;
> - struct device *dev = &dsi->dev;
> - int ret;
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x00);
> - mipi_dsi_generic_write_seq(dsi, 0xd6, 0x01);
> - mipi_dsi_generic_write_seq(dsi, 0xec,
> - 0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00, 0x0b,
> 0x0b,
> - 0x13, 0x15, 0x68, 0x0b, 0xb5);
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x03);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x00);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xd6, 0x01);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xec,
> +0x64, 0xdc, 0xec, 0x3b, 0x52, 0x00,
> 0x0b, 0x0b,
> +0x13, 0x15, 0x68, 0x0b, 0xb5);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x03);
>
> - ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear on: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_ADDRESS_MODE,
> 0x00);
>
> - ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set pixel format: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx, 0x77);
> + mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0x, 0x0437);
> + mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0x, 0x077f);
> + mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx, 0x);
> + mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x00ff);
>
> - ret = mipi_dsi_dcs_set_column_address(dsi, 0x, 0x0437);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set column address: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx,
> MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x24);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_POWER_SAVE,
> 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx,
> MIPI_DCS_SET_CABC_MIN_BRIGHTNESS, 0x00);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x84, 0x00);
>
> - ret = mipi_dsi_dcs_set_page_address(dsi, 0x, 0x077f);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set page address: %d\n", ret);
> - return ret;
> - }
> -
> - ret = mipi_dsi_dcs_set_tear_scanline(dsi, 0x);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set tear scanline: %d\n", ret);
> - return ret;
> - }
> + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 20);
> + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> + mipi_dsi_msleep(&dsi_ctx, 80);
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> - if (ret < 0) {
> - dev_err(dev, "Failed to set display brightness: %d\n", ret);
> - return ret;
> - }
> -
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x24);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_CABC_MIN_BRIGHTNESS, 0x00);
> - mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
> -
> - 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(20);
> -
> - 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(80);
> -
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x04);
> - mipi_dsi_dcs_write_seq(dsi, 0x84, 0x00);
> - mipi_dsi_generic_write_seq(dsi, 0xc8, 0x11);
> - mipi_dsi_generic_write_seq(dsi, 0xb0, 0x03);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x04);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x84, 0x00);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xc8, 0x11);
> + mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0xb0, 0x03);
>
> return 0;
Whoops! Not "return 0". "return dsi_ctx.accum_err".
> @@ -121,31 +83,22 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
> static int jdi_fhd_r63452_off(struct
Re: [PATCH 1/2] drm/mipi-dsi: Add mipi_dsi_dcs_set_tear_scanline_multi
Hi, On Fri, Aug 9, 2024 at 9:55 PM Tejas Vipin wrote: > > mipi_dsi_dcs_set_tear_scanline_multi can heavily benefit from being > converted to a multi style function as it is often called in the context of > similar functions. > > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/drm_mipi_dsi.c | 31 +++ > include/drm/drm_mipi_dsi.h | 2 ++ > 2 files changed, 33 insertions(+) Reviewed-by: Douglas Anderson
Re: [PATCH 0/2] add more multi functions for streamlined error handling
Hi, On Fri, Aug 9, 2024 at 9:55 PM Tejas Vipin wrote: > > This patch adds mipi_dsi_dcs_set_tear_scanline_multi to the list of multi > functions and uses it with other multi functions in the jdi-fhd-r63452 > panel. > > Tejas Vipin (2): > drm/mipi-dsi: Add mipi_dsi_dcs_set_tear_scanline_multi > drm/panel: jdi-fhd-r63452: transition to mipi_dsi wrapped functions > > drivers/gpu/drm/drm_mipi_dsi.c | 31 + > drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 125 ++- > include/drm/drm_mipi_dsi.h | 2 + > 3 files changed, 72 insertions(+), 86 deletions(-) Not worth spinning just for this, but a few comments: 1. For the cover letter, it's better if you can make the subject more different than the subject of your previous patch series. Comparing this and the previous series you sent out side-by-side: [PATCH 0/2] add more multi functions for streamlined error handling [PATCH v3 0/2] add more multi functions to streamline error handling Maybe this patch's cover letter should have a subject more like: drm/mipi-dsi: convert jdi-fhd-r63452 to mipi_dsi "multi", adding more "multi" ...or something like that. 2. In your cover letter you should note that this series only applies cleanly if you apply it atop your previous series. You should point to it w/ lore links based on the Message-Id, like: https://lore.kernel.org/r/20240806135949.468636-1-tejasvipi...@gmail.com
Re: [PATCH v3 1/2] drm/panel: jd9365da: Move "exit sleep mode" and "set display on" cmds
Jessica, On Thu, Aug 8, 2024 at 3:56 PM Jessica Zhang wrote: > > > > On 8/7/2024 3:04 AM, Zhaoxiong Lv wrote: > > Move the "exit sleep mode" and "set display on" command from > > enable() to init() function. > > > > As mentioned in the patch: > > https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxi...@huaqin.corp-partner.google.com/ > > > > The Mediatek Soc DSI host has different modes in prepare() and > > enable() functions, prepare() is in LP mode and enable() is in > > HS mode. Since the "exit sleep mode" and "set display on" > > command must also be sent in LP mode, so we also move "exit > > sleep mode" and "set display on" command to the init() function. > > > > We have no other actions in the enable() function after moves > > "exit sleep mode" and "set display on", and we checked the call > > of the enable() function during the "startup" process. It seems > > that only one judgment was made in drm_panel_enabel(). If the > > panel does not define enable(), the judgment will skip the > > enable() and continue execution. This does not seem to have > > any other effect, and we found that some drivers also seem > > to have no enable() function added, for example: > > panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d... > > In addition, we briefly tested the kingdisplay_kd101ne3 panel and > > melfas_lmfbx101117480 panel, and it seems that there is no garbage > > on the panel, so we delete enable() function. > > > > After moving the "exit sleep mode" and "set display on" command > > to the init() function, we no longer need additional delay > > judgment, so we delete variables "exit_sleep_to_display_on_delay_ms" > > and "display_on_delay_ms". > > > > Reviewed-by: Douglas Anderson > > Signed-off-by: Zhaoxiong Lv > > Acked-by: Jessica Zhang Does this Ack mean you're confident enough about this patch that we should go ahead and merge it, or do you think we should wait on anything else (like Neil getting a chance to look at it)? As I mentioned in my reply to the cover letter [1] the patches look OK to me but I still don't consider myself to have a wonderful understanding of the intricacies of MIPI DSI. If you think this is OK from a MIPI DSI point of view then we can land it... [1] https://lore.kernel.org/r/CAD=FV=WCw6pAump-PUFCW0cgbRY+5_2tPNLe=hN3-dnXD=b...@mail.gmail.com Thanks! -Doug
Re: [PATCH] dt-bindings: display: panel: samsung, atna45dc02: Fix indentation
Hi,
On Thu, Aug 8, 2024 at 11:47 AM Rob Clark wrote:
>
> On Thu, Aug 8, 2024 at 11:44 AM Douglas Anderson
> wrote:
> >
> > The yaml had indentation errors:
> >
> > ./Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml:21:9:
> > [warning] wrong indentation: expected 10 but found 8 (indentation)
> >
> > ./Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml:23:11:
> > [warning] wrong indentation: expected 12 but found 10 (indentation)
> >
> > Fix them.
> >
> > Reported-by: Rob Herring
> > Closes:
> > https://lore.kernel.org/r/cal_jsqlrtgqrpcfxy4g9hlohmd-uax4_c90bv_ozn4mk+-8...@mail.gmail.com
> > Fixes: 1c4a057d01f4 ("dt-bindings: display: panel: samsung,atna45dc02:
> > Document ATNA45DC02")
> > Signed-off-by: Douglas Anderson
>
> Reviewed-by: Rob Clark
> Thanked-by: Rob Clark
Pushed to drm-misc-fixes.
cd9aae921ab6 dt-bindings: display: panel: samsung,atna45dc02: Fix indentation
Re: [PATCH v3 1/2] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02
Hi, On Thu, Aug 8, 2024 at 11:14 AM Rob Herring wrote: > > > > > From: Rob Clark > > > > > > > > The Samsung ATNA45DC02 panel is an AMOLED eDP panel, similar to the > > > > existing ATNA45AF01 and ATNA33XC20 panel but with a higher resolution. > > > > > > > > Signed-off-by: Rob Clark > > > > Acked-by: Conor Dooley > > > > --- > > > > .../bindings/display/panel/samsung,atna33xc20.yaml | 9 ++--- > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > Reviewed-by: Douglas Anderson > > > > > > I'll plan to land this in drm-misc-fixes next week unless someone > > > objects. "fixes" instead of "next" for the same reasons discussed > > > previously [1] that the dts patch should probably be considered a fix > > > and there's a chance that the dts patch could land in an earlier > > > version of mainline than the bindings unless we consider the bindings > > > a fix. > > > > > > [1] > > > https://patchwork.freedesktop.org/patch/msgid/20240715-x1e80100-crd-backlight-v2-1-31b7f2f65...@linaro.org > > > > Landed in drm-misc-fixes. > > > > [1/2] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02 > > commit: 1c4a057d01f4432704c4dc8842b6e888a91d95df > > And now warning in linux-next: > > ./Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml:21:9: > [warning] wrong indentation: expected 10 but found 8 (indentation) > ./Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml:23:11: > [warning] wrong indentation: expected 12 but found 10 (indentation) > > Please send a fix. Doh! I'm just about to hop out on vacation, but here's a fix. If someone reviews in the next 30 minutes or so I'll land it. Otherwise hopefully someone else can land... https://lore.kernel.org/r/20240808114407.1.I099e8e9e36407a0785d846b953031d40ea71e559@changeid
Re: [PATCH v3 2/2] drm/panel: startek-kd070fhfid015: transition to mipi_dsi wrapped functions
Hi, On Tue, Aug 6, 2024 at 7:00 AM Tejas Vipin wrote: > > Use multi style wrapped functions for mipi_dsi in the > startek-kd070fhfid015 panel. > > Signed-off-by: Tejas Vipin > --- > .../drm/panel/panel-startek-kd070fhfid015.c | 115 ++ > 1 file changed, 35 insertions(+), 80 deletions(-) Reviewed-by: Douglas Anderson If nobody else has any comments, I'll plan to apply this midway through next week.
Re: [PATCH v3 1/2] drm/mipi-dsi: add more multi functions for better error handling
Hi, On Tue, Aug 6, 2024 at 7:00 AM Tejas Vipin wrote: > > Add more functions that can benefit from being multi style and mark > older variants as deprecated to eventually convert all mipi_dsi functions > to multi style. > > Acked-by: Maxime Ripard > Signed-off-by: Tejas Vipin > --- > drivers/gpu/drm/drm_mipi_dsi.c | 194 + > include/drm/drm_mipi_dsi.h | 10 ++ > 2 files changed, 204 insertions(+) Reviewed-by: Douglas Anderson If nobody else has any comments, I'll plan to apply this midway through next week.
Re: [PATCH v3 0/2] Modify the method of sending "exit sleep
Hi, On Wed, Aug 7, 2024 at 3:04 AM Zhaoxiong Lv wrote: > > This "exit sleep mode" and "set display on" command needs to > be sent in LP mode, so move "exit sleep mode" and "set display > on" command to the init() function. > > Modify the Melfas panel init code to satisfy the gamma value of 2.2. > > Changes between V3 and V2: > - PATCH 1/2: Modify the commit message and subject. > - PATCH 2/2: No changes. > - Link to v2: > https://lore.kernel.org/all/20240806034015.11884-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Changes between V2 and V1: > - PATCH 1/2: Modify the commit message and subject. > - PATCH 2/2: No changes. > - Link to v1: > https://lore.kernel.org/all/20240725083245.12253-1-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Zhaoxiong Lv (2): > drm/panel: jd9365da: Move "exit sleep mode" and "set display on" cmds > drm/panel: jd9365da: Modify the init code of Melfas > > .../gpu/drm/panel/panel-jadard-jd9365da-h3.c | 137 +- > 1 file changed, 71 insertions(+), 66 deletions(-) As per my response in v2 [1], I feel like patch #1 would be best reviewed by someone with more MIPI DSI experience. My current plan for this series. * Snooze for 2 weeks. If someone else has reviewed / landed in the meantime then great! * After two weeks, send a warning that I'll apply the series soon unless someone yells. * After a few more days, apply the series. Please yell if you disagree with any of the above. [2] https://lore.kernel.org/r/CAD=FV=WrMxyxkuCYEbd=aYFaTJKNqGqXr6Re+V=b_h9jnjh...@mail.gmail.com
Re: [RFC PATCH] drm/panel: synaptics-r63353: Fix regulator unbalance
Hi, On Wed, Aug 7, 2024 at 5:39 AM Michael Nazzareno Trimarchi wrote: > > Hi Doug > > +cc Doug > > I have seen that you have done some re-working and investigation on > drm stack, do you have some > suggestion on this case? > > On Mon, Jun 24, 2024 at 8:53 PM Michael Trimarchi > wrote: > > > > The shutdown function can be called when the display is already > > unprepared. For example during reboot this trigger a kernel > > backlog. Calling the drm_panel_unprepare, allow us to avoid > > to trigger the kernel warning > > > > Signed-off-by: Michael Trimarchi > > --- > > > > It's not obviovus if shutdown can be dropped or this problem depends > > on the display stack as it is implmented. More feedback is required > > here In general the shutdown should be dropped and it should be up to the display driver to do the shutdown. If your panel needs to be used with a DRM Modeset driver that doesn't properly call shutdown then the ideal solution would be to fix the DRM Modeset driver. If this is somehow impossible, I suspect folks would (begrudgingly) accept some other solution. >From a super quick look, I see: * This panel seems to be used upstream by "imx8mn-bsh-smm-s2-display.dtsi" * In "imx8mn.dtsi" I see "lcdif" is "fsl,imx6sx-lcdif". * "fsl,imx6sx-lcdif" seems to be handled by "drivers/gpu/drm/mxsfb/mxsfb_drv.c" * Previously I determined that "mxsfb-drm" was indeed calling drm_atomic_helper_shutdown() properly [1] ...so it seems like just dropping the shutdown handler in this panel is correct. [1] https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid -Doug
Re: [PATCH v3 1/2] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02
Hi, On Wed, Jul 31, 2024 at 4:39 PM Doug Anderson wrote: > > Hi, > > On Mon, Jul 29, 2024 at 1:57 PM Rob Clark wrote: > > > > From: Rob Clark > > > > The Samsung ATNA45DC02 panel is an AMOLED eDP panel, similar to the > > existing ATNA45AF01 and ATNA33XC20 panel but with a higher resolution. > > > > Signed-off-by: Rob Clark > > Acked-by: Conor Dooley > > --- > > .../bindings/display/panel/samsung,atna33xc20.yaml | 9 ++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > Reviewed-by: Douglas Anderson > > I'll plan to land this in drm-misc-fixes next week unless someone > objects. "fixes" instead of "next" for the same reasons discussed > previously [1] that the dts patch should probably be considered a fix > and there's a chance that the dts patch could land in an earlier > version of mainline than the bindings unless we consider the bindings > a fix. > > [1] > https://patchwork.freedesktop.org/patch/msgid/20240715-x1e80100-crd-backlight-v2-1-31b7f2f65...@linaro.org Landed in drm-misc-fixes. [1/2] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02 commit: 1c4a057d01f4432704c4dc8842b6e888a91d95df -Doug
Re: [PATCH v2 1/2] drm/panel: jd9365da: Move the location of "exit sleep mode" and "set display on" commands
Hi,
On Mon, Aug 5, 2024 at 8:40 PM Zhaoxiong Lv
wrote:
>
> Move the "exit sleep mode" and "set display on" command from
> enable() to init() function.
>
> As mentioned in the patch:
> https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxi...@huaqin.corp-partner.google.com/
>
> Our DSI host has different modes in prepare() and enable()
nit: it's not obvious to the reader of this patch which DSI host is
"our"s. Maybe spell out which SoC you're using? I assume this is a
Mediatek SoC?
> functions. prepare() is in LP mode and enable() is in HS mode.
> Since the "exit sleep mode" and "set display on" command must
> also be sent in LP mode, so we also move "exit sleep mode" and
> "set display on" command to the init() function.
>
> We have no other actions in the enable() function after moves
> "exit sleep mode" and "set display on", and we checked the call
> of the enable() function during the "startup" process. It seems
> that only one judgment was made in drm_panel_enabel(). If the
> panel does not define enable(), the judgment will skip the
> enable() and continue execution. This does not seem to have
> any other effects,and we found that some drivers also seem
s/effects,and/effect, and/
> to have no enable() function added, for example:
> panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d ...
> In addition, we briefly tested the kingdisplay_kd101ne3 panel and
> melfas_lmfbx101117480 panel, and it seems that there is no garbage
> on the panel, so we delete enable() function.
>
> After moving the "exit sleep mode" and "set display on" command
> to the init() function, we no longer need additional delay
> judgment, so we deletevariables "exit_sleep_to_display_on_delay_ms"
nit: s/deletevariables/delete variables/
> and "display_on_delay_ms".
>
> Signed-off-by: Zhaoxiong Lv
> ---
> Changes between V2 and V1:
> - 1. The code has not changed, Modify the commit information.
> v1:
> https://lore.kernel.org/all/20240725083245.12253-2-lvzhaoxi...@huaqin.corp-partner.google.com/
> ---
> .../gpu/drm/panel/panel-jadard-jd9365da-h3.c | 59 ++-
> 1 file changed, 32 insertions(+), 27 deletions(-)
nit: ${SUBJECT} is a bit long. In general it's worth abbreviating a
bit more so that the subject doesn't go to crazy.
drm/panel: jd9365da: Move "exit sleep mode" and "set display on" cmds
Aside from the above nits, this looks OK to me. I wouldn't object to
fixing some of my own nits when applying or you could send a v3 if
there is no other feedback. In any case:
Reviewed-by: Douglas Anderson
I'd prefer someone with more MIPI panel experience give a review,
though, so I'll expect that Jessica or Neil or someone else gives a
review.
-Doug
Re: [PATCH v1 1/2] drm/panel: jd9365da: Move the sending location of the 11/29 command
Hi, On Sun, Aug 4, 2024 at 7:38 PM zhaoxiong lv wrote: > > Hi all > > Do you have any other suggestions for this patch? > Looking forward to your reply, thank you Please make sure not to "top post". Folks on the mailing lists generally frown on this and it's a good way to get your email ignored by some people. At this point I think folks are waiting for you to post the next version addressing comments. Specifically, things you'd want to change for the next version: * In the commit message (and subject), "refer to the commands with their names" (Jani) * In the commit message, address Dmitry's concern. In other words, say something about the fact that this doesn't cause garbage being displayed on the panel during startup and why not. When sending v2, don't forget to include Jessica's "Ack" on patch #2.
Re: [PATCH v1] drm/panel-edp: Fix HKC MB116AN01 name
Hi,
On Fri, Aug 2, 2024 at 12:06 AM Terry Hsiao
wrote:
>
> Rename HKC MB116AN01 from Unknown to MB116AN01
>
> Signed-off-by: Terry Hsiao
> ---
> drivers/gpu/drm/panel/panel-edp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c
> b/drivers/gpu/drm/panel/panel-edp.c
> index 2733366b02b0..7183df26 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1948,7 +1948,7 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('C', 'S', 'W', 0x1104, &delay_200_500_e50,
> "MNB601LS1-4"),
>
> EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d51, &delay_200_500_e200,
> "Unknown"),
> - EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d5b, &delay_200_500_e200,
> "Unknown"),
> + EDP_PANEL_ENTRY('H', 'K', 'C', 0x2d5b, &delay_200_500_e200,
> "MB116AN01"),
Nice!
Reviewed-by: Douglas Anderson
...and pushed to drm-misc-next:
[1/1] drm/panel-edp: Fix HKC MB116AN01 name
commit: 21e97d3ca814ea59d5ddb6a734125bd006b66a60
Re: [PATCH v3 1/2] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02
Hi, On Mon, Jul 29, 2024 at 1:57 PM Rob Clark wrote: > > From: Rob Clark > > The Samsung ATNA45DC02 panel is an AMOLED eDP panel, similar to the > existing ATNA45AF01 and ATNA33XC20 panel but with a higher resolution. > > Signed-off-by: Rob Clark > Acked-by: Conor Dooley > --- > .../bindings/display/panel/samsung,atna33xc20.yaml | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Douglas Anderson I'll plan to land this in drm-misc-fixes next week unless someone objects. "fixes" instead of "next" for the same reasons discussed previously [1] that the dts patch should probably be considered a fix and there's a chance that the dts patch could land in an earlier version of mainline than the bindings unless we consider the bindings a fix. [1] https://patchwork.freedesktop.org/patch/msgid/20240715-x1e80100-crd-backlight-v2-1-31b7f2f65...@linaro.org
Re: [PATCH v2 1/2] drm/mipi-dsi: add more multi functions for better error handling
Hi,
On Mon, Jul 29, 2024 at 11:07 PM Tejas Vipin wrote:
> +/**
> + * mipi_dsi_dcs_get_display_brightness_multi() - gets the current brightness
> value
> + *of the display
> + * @ctx: Context for multiple DSI transactions
> + * @brightness: brightness value
> + *
> + * Like mipi_dsi_dcs_get_display_brightness() but deals with errors in a way
> that
> + * makes it convenient to make several calls in a row.
> + */
> +void mipi_dsi_dcs_get_display_brightness_multi(struct mipi_dsi_multi_context
> *ctx,
> + u16 *brightness)
> +{
> + struct mipi_dsi_device *dsi = ctx->dsi;
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + if (ctx->accum_err)
> + return;
> +
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, brightness);
> + if (ret < 0) {
> + ctx->accum_err = ret;
> + dev_err(dev, "Failed to get display brightness: %d\n",
> + ctx->accum_err);
> + }
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness_multi);
I'd be interested in others' opinions, but this function strikes me as
one that *shouldn't* be converted to _multi.
Specifically the whole point of the _multi abstraction is that you can
fire off a whole pile of initialization commands without needing to
check for errors constantly. You can check for errors once at the end
of a sequence of commands and you can be sure that an error message
was printed for the command that failed and that all of the future
commands didn't do anything.
I have a hard time believing that _get_ brightness would be part of
this pile of initialization commands. ...and looking at how you use it
in the next patch I can see that, indeed, it's a bit awkward using the
_multi variant in the case you're using it.
The one advantage of the _multi functions is that they are also
"chatty" and we don't need to print the error everywhere. However, it
seems like we could just make the existing function print an error
message but still return the error directly. If this automatic
printing an error message is a problem for someone then I guess maybe
we've already reached the "tomorrow" [1] and need to figure out if we
need to keep two variants of the function around instead of marking
one as deprecated.
NOTE: If we don't convert this then the "set" function will still be
_multi but the "get" one won't be. I think that's fine since the "set"
function could plausibly be in a big sequence of commands but the
"get" function not so much...
[1]
https://lore.kernel.org/r/CAD=FV=wbxdnm4or3ae+nyoqw1sce0jp6fwtchshsaluefnh...@mail.gmail.com
Re: [PATCH] drm/panel-edp: Add CSW MNB601LS1-4
Hi, On Thu, Jul 25, 2024 at 4:53 AM Haikun Zhou wrote: > > Add support for the CSW MNB601LS1-4, pleace the EDID here for > subsequent reference. > 00 ff ff ff ff ff ff 00 0e 77 04 11 00 00 00 00 > 00 22 01 04 a5 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 34 0a 20 00 20 > > Signed-off-by: Haikun Zhou > --- > drivers/gpu/drm/panel/panel-edp.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Douglas Anderson Applied, thanks! [1/1] drm/panel-edp: Add CSW MNB601LS1-4 commit: 9d8e91439fc3890de55eef2bcfde97470b7dc04d
Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context
Hi, On Fri, Jul 26, 2024 at 2:15 AM Maxime Ripard wrote: > > > c) Declare that, since there are no known cases where we want to > > suppress the error printouts, that suppressing the error printouts is > > a "tomorrow" problem. We transition everyone to _multi but don't > > provide a way to suppress the printouts. > > That's my preferred solution. OK, fair enough. So I guess the transition plan would be: 1. Add a wrapper like we're doing today. 2. Transition everyone to the _multi variant. 3. Delete the non-multi variant which will cause us to delete the wrapper. -Doug
Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context
Hi,
On Thu, Jul 25, 2024 at 1:28 AM Maxime Ripard wrote:
>
> On Wed, Jul 24, 2024 at 06:32:14PM GMT, Jani Nikula wrote:
> > On Wed, 24 Jul 2024, Tejas Vipin wrote:
> > > Changes all the multi functions to check if the current context requires
> > > errors to be printed or not using the quiet member.
> > >
> > > Signed-off-by: Tejas Vipin
> > > ---
> > > drivers/gpu/drm/drm_mipi_dsi.c | 20
> > > 1 file changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
> > > b/drivers/gpu/drm/drm_mipi_dsi.c
> > > index a471c46f5ca6..cbb77342d201 100644
> > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > @@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct
> > > mipi_dsi_multi_context *ctx,
> > > ret = mipi_dsi_generic_write(dsi, payload, size);
> > > if (ret < 0) {
> > > ctx->accum_err = ret;
> > > + if (ctx->quiet)
> > > + return;
> > > dev_err(dev, "sending generic data %*ph failed: %d\n",
> > > (int)size, payload, ctx->accum_err);
> >
> > A maintainability nitpick. Imagine someone wishing to add some more
> > error handling here. Or something else after the block.
> >
> > IMO the dev_err() should be wrapped in if (!ctx->quiet) instead of
> > adding an early return.
> >
> > Ditto everywhere.
>
> I'm not even sure why we need that parameter in the first place.
>
> Like, what's the expected use of that parameter? Is it supposed to be
> set in users of that function?
>
> If so, wouldn't it prevent any sort of meaningful debugging if some
> drivers set it and some don't?
>
> It looks to me like you're reimplementing drm.debug.
I can explain how we got here and maybe you can explain how it should
be designed differently.
1. The majority of MIPI panels drivers just have a pile of commands
that need to be sent during panel init time. Each time you send a
command it technically can fail but it's very unlikely. In order to
make things debuggable in the unlikely case of failure, you want a
printout about which command failed. In order to avoid massive numbers
of printouts in each driver you want the printout in the core. This is
the impetus behind the "_multi" functions that were introduced
recently and I think most people who have looked at converted drivers
are pretty pleased. The functions are readable/non-bloated but still
check for errors and print messages in the right place.
2. As Tejas was adding more "_multi" variants things were starting to
feel a bit awkward. Dmitry proposed [1] that maybe the answer was that
we should work to get rid of the non-multi variants and then we don't
need these awkward wrappers.
3. The issue with telling everyone to use the "_multi" variants is
that they print the error message for you. While this is the correct
default behavior and the correct behavior for the vast majority of
users, I can imagine there being a legitimate case where a driver
might not want error messages printed. I don't personally know of a
case, but in my experience there's always some case where you're
dealing with weird hardware and the driver knows that a command has a
high chance of failure. Maybe the driver will retry or maybe it'll
detect /handle the error, but the idea is that the driver wouldn't
want a printout.
Said another way: I think of the errors of these MIPI initialization
functions a lot like errors allocating memory. By default kmalloc()
reports an error so all drivers calling kmalloc() don't need to print,
but if your driver specifically knows that an allocation failure is
likely and it knows how to handle it then it can pass a flag to tell
the core kernel not to print.
So I guess options are:
a) Accept what Tejas is doing here as reasonable.
b) Don't accept what Tejas is doing here and always keep the "_multi"
functions as wrappers.
c) Declare that, since there are no known cases where we want to
suppress the error printouts, that suppressing the error printouts is
a "tomorrow" problem. We transition everyone to _multi but don't
provide a way to suppress the printouts.
d) Declare that the _multi functions are terrible and undo all the
recent changes. Hopefully we don't do this. :-P
[1]
https://lore.kernel.org/r/p7fahem6egnooi5org4lv3gtz2elafylvlnyily7buvzcpv2qh@vssvpfci3lwn
Re: [PATCH 2/2] drm/panel: add BOE tv101wum-ll2 panel driver
Hi, On Wed, Jul 24, 2024 at 12:51 AM Neil Armstrong wrote: > > >> @@ -0,0 +1,240 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +// Generated with linux-mdss-dsi-panel-driver-generator from vendor > >> device tree: > >> +// Copyright (c) 2013, The Linux Foundation. All rights reserved. > >> +// Copyright (c) 2024, Neil Armstrong > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > > > > nit: sort header files? > > Will do while I'm it, but I don't personally care of the include order.. FWIW: the main reason I push for sorting in cases like this is to avoid merge conflicts as the driver changes. If everyone adds new #includes at the end then every change will cause a merge conflict. If things are kept sorted it's still possible to get a merge conflict but the number goes down. Probably not super relevant in such a simple driver but just the policy I push for in general. The criteria for sorting doesn't matter to me (some people put "nested" includes in separate sections and some just do a normal sort) as long as it's obvious / consistent for a given file. -Doug
Re: [PATCH v4 2/2] drm/panel: boe-th101mb31ig002 : using drm_connector_helper_get_modes_fixed()
Hi, On Mon, Jul 22, 2024 at 11:26 PM Zhaoxiong Lv wrote: > > Use public functions( drm_connector_helper_get_modes_fixed()) to > get porch parameters. > > Signed-off-by: Zhaoxiong Lv > --- > Changes between V4 and V3: > - 1.Modify the return value, return > drm_connector_helper_get_modes_fixed(connector, desc_mode). > v3: > https://lore.kernel.org/all/20240722092428.24499-3-lvzhaoxi...@huaqin.corp-partner.google.com/ > > Changes between V3 and V2: > - 1. Keep bpc settings and drm_connector_set_panel_orientation() function.. > v2: > https://lore.kernel.org/all/20240716121112.14435-3-lvzhaoxi...@huaqin.corp-partner.google.com/ > --- > .../drm/panel/panel-boe-th101mb31ig002-28a.c | 19 ++- > 1 file changed, 2 insertions(+), 17 deletions(-) Reviewed-by: Douglas Anderson I'd assume that Neil or Jessica will apply these two patches assuming they agree it looks OK. If this is stagnant for a while then I'll apply it. -Doug
Re: [PATCH 2/2] drm/panel: add BOE tv101wum-ll2 panel driver
Hi,
On Tue, Jul 9, 2024 at 6:06 AM Neil Armstrong wrote:
>
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5581387707c6..79c90894b6a4 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_PANEL_BOE_BF060Y8M_AJ0) +=
> panel-boe-bf060y8m-aj0.o
> obj-$(CONFIG_DRM_PANEL_BOE_HIMAX8279D) += panel-boe-himax8279d.o
> obj-$(CONFIG_DRM_PANEL_BOE_TH101MB31UIG002_28A) +=
> panel-boe-th101mb31ig002-28a.o
> obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_NL6) += panel-boe-tv101wum-nl6.o
> +obj-$(CONFIG_DRM_PANEL_BOE_TV101WUM_LL2) += panel-boe-tv101wum-ll2.o
nit: please sort. L comes before N.
> obj-$(CONFIG_DRM_PANEL_DSI_CM) += panel-dsi-cm.o
> obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
> obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
> diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-ll2.c
> b/drivers/gpu/drm/panel/panel-boe-tv101wum-ll2.c
> new file mode 100644
> index ..5513cb48d949
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-ll2.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Generated with linux-mdss-dsi-panel-driver-generator from vendor device
> tree:
> +// Copyright (c) 2013, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2024, Neil Armstrong
> +
> +#include
> +#include
> +#include
> +#include
> +#include
nit: sort header files?
> +static int boe_tv101wum_ll2_prepare(struct drm_panel *panel)
> +{
> + struct boe_tv101wum_ll2 *ctx = to_boe_tv101wum_ll2(panel);
> + struct device *dev = &ctx->dsi->dev;
> + int ret;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies),
> + ctx->supplies);
> + if (ret < 0)
> + return ret;
> +
> + boe_tv101wum_ll2_reset(ctx);
> +
> + ret = boe_tv101wum_ll2_on(ctx);
> + if (ret < 0) {
> + dev_err(dev, "Failed to initialize panel: %d\n", ret);
nit: Do you really need this error message? The "_multi" variants are
all chatty and print the error message, so we don't really need this
here...
> + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> + return ret;
Shouldn't you turn off the regulators?
> +static int boe_tv101wum_ll2_unprepare(struct drm_panel *panel)
> +{
> + struct boe_tv101wum_ll2 *ctx = to_boe_tv101wum_ll2(panel);
> + struct device *dev = &ctx->dsi->dev;
> + int ret;
> +
> + ret = boe_tv101wum_ll2_off(ctx);
> + if (ret < 0)
> + dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
nit: Do you really need this error message? The "_multi" variants are
all chatty and print the error message, so we don't really need this
here...
> +
> + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +
> + regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +
> + return 0;
Maybe add a comment justifying why you don't return the error code
that boe_tv101wum_ll2_off() returned?
> +static int boe_tv101wum_ll2_get_modes(struct drm_panel *panel,
> + struct drm_connector *connector)
> +{
> + return drm_connector_helper_get_modes_fixed(connector,
> &boe_tv101wum_ll2_mode);
Random question for you: on panels that don't use the
drm_connector_helper the "bpc" gets set here. Is there a reason why
some panel drivers (like this one) don't set bpc?
> +static int boe_tv101wum_ll2_probe(struct mipi_dsi_device *dsi)
> +{
> + struct device *dev = &dsi->dev;
> + struct boe_tv101wum_ll2 *ctx;
> + int ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->supplies[0].supply = "vsp";
> + ctx->supplies[1].supply = "vsn";
> +
> + ret = devm_regulator_bulk_get(&dsi->dev, ARRAY_SIZE(ctx->supplies),
> + ctx->supplies);
Any chance I can convince you to use devm_regulator_bulk_get_const()?
Then you can list your supply structures as "static const" instead of
having to initialize them via code.
> + if (ret < 0)
> + return ret;
> +
> + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ctx->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
> +"Failed to get reset-gpios\n");
> +
> + ctx->dsi = dsi;
> + mipi_dsi_set_drvdata(dsi, ctx);
> +
> + dsi->lanes = 4;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> + MIPI_DSI_MODE_VIDEO_HSE;
> +
> + drm_panel_init(&ctx->panel, dev, &boe_tv101wum_ll2_panel_funcs,
> + DRM_MODE_CONNECTOR_DSI);
> + ctx->panel.prepare_prev_first = true;
> +
> + ret = drm_panel_of_backlight(&ctx->pane
Re: [PATCH v3 2/2] drm/panel: boe-th101mb31ig002 : using drm_connector_helper_get_modes_fixed()
Hi,
On Mon, Jul 22, 2024 at 2:24 AM Zhaoxiong Lv
wrote:
>
> @@ -313,29 +314,15 @@ static int boe_th101mb31ig002_get_modes(struct
> drm_panel *panel,
> struct
> boe_th101mb31ig002,
> panel);
> const struct drm_display_mode *desc_mode = ctx->desc->modes;
> - struct drm_display_mode *mode;
> -
> - mode = drm_mode_duplicate(connector->dev, desc_mode);
> - if (!mode) {
> - dev_err(panel->dev, "Failed to add mode %ux%u@%u\n",
> - desc_mode->hdisplay, desc_mode->vdisplay,
> - drm_mode_vrefresh(desc_mode));
> - return -ENOMEM;
> - }
> -
> - drm_mode_set_name(mode);
>
> connector->display_info.bpc = 8;
> - connector->display_info.width_mm = mode->width_mm;
> - connector->display_info.height_mm = mode->height_mm;
> -
> /*
> * TODO: Remove once all drm drivers call
> * drm_connector_set_orientation_from_panel()
> */
> drm_connector_set_panel_orientation(connector, ctx->orientation);
>
> - drm_mode_probed_add(connector, mode);
> + drm_connector_helper_get_modes_fixed(connector, desc_mode);
>
> return 1;
Don't always return 1. This should be:
return drm_connector_helper_get_modes_fixed(connector, desc_mode);
...so if it fails to add a mode then you'll return 0.
-Doug
Re: [PATCH v2] drm/panel-edp: Add 6 panels used by MT8186 Chromebooks
Hi, On Sun, Jul 21, 2024 at 3:04 AM Terry Hsiao wrote: > > The raw EDIDs for each panel: > > AUO > - B116XTN02.3 > 00 ff ff ff ff ff ff 00 06 af aa 73 00 00 00 00 > 00 21 01 04 95 1a 0e 78 02 6b f5 91 55 54 91 27 > 22 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 ce 1d 56 e2 50 00 1e 30 26 16 > 36 00 00 90 10 00 00 18 df 13 56 e2 50 00 1e 30 > 26 16 36 00 00 90 10 00 00 18 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 10 48 ff 0f 3c 7d 50 05 18 7d 20 20 20 00 67 > - B116XAN06.1 > 00 ff ff ff ff ff ff 00 06 af 99 a1 00 00 00 00 > 00 1f 01 04 95 1a 0e 78 02 9e a5 96 59 58 96 28 > 1b 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 ce 1d 56 ea 50 00 1a 30 30 20 > 46 00 00 90 10 00 00 18 df 13 56 ea 50 00 1a 30 > 30 20 46 00 00 90 10 00 00 18 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 10 48 ff 0f 3c 7d 0c 0a 2a 7d 20 20 20 00 3a > - B116XAT04.1 > 00 ff ff ff ff ff ff 00 06 af b4 c4 00 00 00 00 > 12 22 01 04 95 1a 0e 78 02 9e a5 96 59 58 96 28 > 1b 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 ce 1d 56 ea 50 00 1a 30 30 20 > 46 00 00 90 10 00 00 18 df 13 56 ea 50 00 1a 30 > 30 20 46 00 00 90 10 00 00 18 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 10 48 ff 0f 3c 7d 0c 0a 2a 7d 20 20 20 00 e7 > > BOE > - NV116WHM-A4D > 00 ff ff ff ff ff ff 00 09 e5 fa 0c 00 00 00 00 > 12 22 01 04 95 1a 0e 78 03 0b 55 9a 5f 58 95 28 > 1e 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 96 1d 56 c8 50 00 26 30 30 20 > 36 00 00 90 10 00 00 1a b9 13 56 c8 50 00 26 30 > 30 20 36 00 00 90 10 00 00 1a 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 0d 40 ff 0a 3c 7d 0f 0c 17 7d 00 00 00 00 1a > > CMN > - N116BCA-EA2 > 00 ff ff ff ff ff ff 00 0d ae 5d 11 00 00 00 00 > 0f 21 01 04 95 1a 0e 78 03 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 1a e7 13 56 e2 50 00 20 30 > 30 20 a6 00 00 90 10 00 00 1a 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 0c 3d ff 0d 3c 7d 0d 0a 15 7d 00 00 00 00 0f > - N116BCP-EA2 > 00 ff ff ff ff ff ff 00 0d ae 61 11 00 00 00 00 > 0f 21 01 04 95 1a 0e 78 03 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 1a e7 13 56 e2 50 00 20 30 > 30 20 a6 00 00 90 10 00 00 1a 00 00 00 00 00 00 > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 > 00 0c 3d ff 0d 3c 7d 0d 0a 15 7d 00 00 00 00 0b > > Signed-off-by: Terry Hsiao > --- > Change from v1 to v2 > * Modify the description of subject > * Add the raw EDIDs > * Sorted according to the order > drivers/gpu/drm/panel/panel-edp.c | 6 ++ > 1 file changed, 6 insertions(+) Reviewed-by: Douglas Anderson ...and pushed to drm-misc-next. [1/1] drm/panel-edp: Add 6 panels used by MT8186 Chromebooks commit: d4b9b6da5777bb03f36f01bb6b05c6cc303ededb -Doug
Re: [PATCH v1] drm/panel-edp: Add panels with conservative timings
Hi, On Sun, Jul 21, 2024 at 4:00 AM Terry Hsiao wrote: > > Hi Doug, > > Thank you for your reply. > The patch has been modified and will be sent to you shortly. For future reference, the Linux community frowns upon "top posting". Search for "top-posting" on [1] [1] https://www.arm.linux.org.uk/mailinglists/etiquette.php > The timings are set based on the panel datasheets in IssueTracker > (https://partnerissuetracker.corp.google.com/issues/348109270) FWIW, if you want to privately provide links to datasheets to me to double-check your work then that's fine, but the above link is useless to others on the Linux kernel mailing list and people usually don't appreciate such links. In this case you could have replied publicly and told others that you'd gotten your work double-checked and that would have been sufficient for the public lists. > B116XTN02.3: B116XTN02.3(HW 9A)_HP_ Functional Spec_0617Y24.pdf > B116XAN06.1: B116XAN06.1_7A_HP_ Final Functional Spec 0617Y24.pdf > B116XAT04.1: B116XAT04.1 HW 0 A(HH)_ Pre Functional Spec_HP_ 0425Y24.pdf > NV116WHM-A4D: NV116WHM-A4D V8.0 Teacake Product Specification-20240416.pdf > N116BCA-EA2: Approval Specification N116BCA-EA2_C3_20231212.pdf > N116BCP-EA2: TFT-LCD Tentative N116BCP-EA2 C2 for HP Ver 0.2-240502.pdf > > On page 24 of the N116BCP-EA2 > datasheet(https://partnerissuetracker.corp.google.com/action/issues/348109270/attachments/57530666?download=false), > the value for t9 as disable is "null". > > If I have misunderstood what you mean, please correct me. I've double-checked and this looks fine to me. -Doug
Re: [PATCH] drm/mipi-dsi: Introduce macros to create mipi_dsi_*_multi functions
Hi, On Fri, Jul 19, 2024 at 10:19 PM Tejas Vipin wrote: > > On 7/19/24 10:29 PM, Doug Anderson wrote: > > Hi, > > > > On Wed, Jul 17, 2024 at 3:07 AM Dmitry Baryshkov > > wrote: > >> > >>>> However it might be better to go other way arround. > >>>> Do we want for all the drivers to migrate to _multi()-kind of API? If > >>>> so, what about renaming the multi and non-multi functions accordingly > >>>> and making the old API a wrapper around the multi() function? > >>>> > >>> > >>> I think this would be good. For the wrapper to make a multi() function > >>> non-multi, what do you think about a macro that would just pass a > >>> default dsi_ctx to the multi() func and return on error? In this case > >>> it would also be good to let the code fill inline instead of generating > >>> a whole new function imo. > >>> > >>> So in this scenario all the mipi dsi functions would be multi functions, > >>> and a function could be called non-multi like MACRO_NAME(func) at the > >>> call site. > >> > >> Sounds good to me. I'd suggest to wait for a day or two for further > >> feedback / comments from other developers. > > > > I don't totally hate the idea of going full-multi and just having > > macros to wrap anyone who hasn't converted, but I'd be curious how > > much driver bloat this will cause for drivers that aren't converted. > > Would the compiler do a good job optimizing here? Maybe we don't care > > if we just want everyone to switch over? If nothing else, it might > > make sense to at least keep both versions for the very generic > > functions (mipi_dsi_generic_write_multi and > > mipi_dsi_dcs_write_buffer_multi) > > > > ...oh, but wait. We probably have the non-multi versions wrap the > > _multi ones. One of the things about the _multi functions is that they > > are also "chatty" and print errors. They're designed for the use case > > of a pile of init code where any error is fatal and needs to be > > printed. I suspect that somewhere along the way someone will want to > > be able to call these functions and not have them print errors... > > > > I think what would be interesting is if we had "chatty" member as a > part of mipi_dsi_multi_context as a check for if dev_err should run. > That way, we could make any function not chatty. If we did this, is > there any reason for a driver to not switch over to using the multi > functions? I'd be OK with that. My preference would be that "chatty" would be the zero-initialized value just to make that slightly more efficient and preserve existing behavior. So I guess the member would be something like "quiet" and when 0 (false) it wouldn't be quiet. > > Maybe going with Dmitry's suggested syntax is the best option here? > > Depending on how others feel, we could potentially even get rid of the > > special error message and just stringify the function name for the > > error message? > > > The problem with any macro solution that defines new multi functions is > that it creates a lot of bloat. Defining the function through macros > might be smaller than defining them manually, but there are still twice > as many function declarations as there would be if we went all multi. > The generated code is still just as big as if we manually defined > everything. I think a macro that defines functions should be more of a > last resort if we don't have any other options. Ah, somehow I was thinking that Dmitry's solution was conflated with switching back to a macro. I haven't prototyped it, but I thought Dmitry's primary complaint was that the syntax for declaring the "_multi" function was a bit dodgy and I'd expect that using a macro would solve that. In any case, I'm good with keeping away from macros / bloating things. -Doug
Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
Hi, On Mon, Jul 15, 2024 at 9:40 AM Doug Anderson wrote: > > Hi, > > On Fri, Jun 21, 2024 at 1:46 PM Doug Anderson wrote: > > > > Hi, > > > > On Fri, Jun 21, 2024 at 1:45 PM Douglas Anderson > > wrote: > > > > > > At shutdown if you've got a _properly_ coded DRM modeset driver then > > > you'll get these two warnings at shutdown time: > > > > > > Skipping disable of already disabled panel > > > Skipping unprepare of already unprepared panel > > > > > > These warnings are ugly and sound concerning, but they're actually a > > > sign of a properly working system. That's not great. > > > > > > We're not ready to get rid of the calls to drm_panel_disable() and > > > drm_panel_unprepare() because we're not 100% convinced that all DRM > > > modeset drivers are properly calling drm_atomic_helper_shutdown() or > > > drm_helper_force_disable_all() at the right times. However, having the > > > warning show up for correctly working systems is bad. > > > > > > As a bit of a workaround, add some "if" tests to try to avoid the > > > warning on correctly working systems. Also add some comments and > > > update the TODO items in the hopes that future developers won't be too > > > confused by what's going on here. > > > > > > Suggested-by: Daniel Vetter > > > Signed-off-by: Douglas Anderson > > > --- > > > This patch came out of discussion on dri-devel on 2024-06-21 > > > [1]. NOTE: I have put all changes into one patch since it didn't seem > > > to add anything to break up the updating of the TODO or the comments > > > in the core into separate patches since the patch is all about one > > > topic and all code is expected to land in the same tree. > > > > > > Previous versions: > > > v0: > > > https://lore.kernel.org/r/20240604172305.v3.24.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid/ > > > v1: > > > https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid > > > > > > [1] > > > https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2024-06-21 > > > > > > Documentation/gpu/todo.rst | 35 +--- > > > drivers/gpu/drm/drm_panel.c | 18 ++ > > > drivers/gpu/drm/panel/panel-edp.c| 26 ++--- > > > drivers/gpu/drm/panel/panel-simple.c | 26 ++--- > > > 4 files changed, 68 insertions(+), 37 deletions(-) > > > > Ugh! I realized right after I hit "send" that I forgot to mark this as > > V2 and give it version history. Sorry! :( Please consider this to be > > v2. It's basically totally different than v1 based on today's IRC > > discussion, which should be linked above. > > > > If I need to send a new version I will send it as v3. > > Is anyone willing to give me a Reviewed-by and/or Acked by for this > patch? ...or does anything want me to make any changes? Given all the > discussion we had, it would be nice to get this landed before we > forget what we agreed upon. :-P Landed in drm-misc-next with Neil and Linus W's tags. [1/1] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown commit: f00bfaca704ca1a2c4e31501a0a7d4ee434e73a7 -Doug
Re: [PATCH v2 2/4] Revert "drm/panel-edp: Add SDC ATNA45AF01"
Hi,
On Mon, Jul 15, 2024 at 6:51 AM Doug Anderson wrote:
>
> Hi,
>
> On Mon, Jul 15, 2024 at 6:02 AM Neil Armstrong
> wrote:
> >
> > On 15/07/2024 14:54, Stephan Gerhold wrote:
> > > On Mon, Jul 15, 2024 at 02:42:12PM +0200, Neil Armstrong wrote:
> > >> On 15/07/2024 14:15, Stephan Gerhold wrote:
> > >>> This reverts commit 8ebb1fc2e69ab8b89a425e402c7bd85e053b7b01.
> > >>>
> > >>> The panel should be handled through the samsung-atna33xc20 driver for
> > >>> correct power up timings. Otherwise the backlight does not work
> > >>> correctly.
> > >>>
> > >>> We have existing users of this panel through the generic "edp-panel"
> > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> > >>> partially in that configuration: It works after boot but once the screen
> > >>> gets disabled it does not turn on again until after reboot. It behaves
> > >>> the
> > >>> same way with the default "conservative" timings, so we might as well
> > >>> drop
> > >>> the configuration from the panel-edp driver. That way, users with old
> > >>> DTBs
> > >>> will get a warning and can move to the new driver.
> > >>>
> > >>> Reviewed-by: Douglas Anderson
> > >>> Signed-off-by: Stephan Gerhold
> > >>> ---
> > >>>drivers/gpu/drm/panel/panel-edp.c | 2 --
> > >>>1 file changed, 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/panel/panel-edp.c
> > >>> b/drivers/gpu/drm/panel/panel-edp.c
> > >>> index 3a574a9b46e7..d2d682385e89 100644
> > >>> --- a/drivers/gpu/drm/panel/panel-edp.c
> > >>> +++ b/drivers/gpu/drm/panel/panel-edp.c
> > >>> @@ -1960,8 +1960,6 @@ static const struct edp_panel_entry edp_panels[]
> > >>> = {
> > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05af, &delay_200_500_e200_d200,
> > >>> "Unknown"),
> > >>> EDP_PANEL_ENTRY('L', 'G', 'D', 0x05f1, &delay_200_500_e200_d200,
> > >>> "Unknown"),
> > >>> - EDP_PANEL_ENTRY('S', 'D', 'C', 0x416d, &delay_100_500_e200,
> > >>> "ATNA45AF01"),
> > >>> -
> > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1511, &delay_200_500_e50,
> > >>> "LQ140M1JW48"),
> > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x1523, &delay_80_500_e50,
> > >>> "LQ140M1JW46"),
> > >>> EDP_PANEL_ENTRY('S', 'H', 'P', 0x153a, &delay_200_500_e50,
> > >>> "LQ140T1JH01"),
> > >>>
> > >>
> > >> How will we handle current/old crd DT with new kernels ?
> > >>
> > >
> > > I think this is answered in the commit message:
> > >
> > >>> We have existing users of this panel through the generic "edp-panel"
> > >>> compatible (e.g. the Qualcomm X1E80100 CRD), but the screen works only
> > >>> partially in that configuration: It works after boot but once the screen
> > >>> gets disabled it does not turn on again until after reboot. It behaves
> > >>> the
> > >>> same way with the default "conservative" timings, so we might as well
> > >>> drop
> > >>> the configuration from the panel-edp driver. That way, users with old
> > >>> DTBs
> > >>> will get a warning and can move to the new driver.
> > >
> > > Basically with the entry removed, the panel-edp driver will fallback to
> > > default "conservative" timings when using old DTBs. There will be a
> > > warning in dmesg, but otherwise the panel will somewhat work just as
> > > before. I think this is a good way to remind users to upgrade.
> >
> > I consider this as a regression
> >
> > >
> > >> Same question for patch 3, thie serie introduces a bindings that won't
> > >> be valid
> > >> if we backport patch 3. I don't think patch should be backported, and
> > >> this patch
> > >> should be dropped.
> > >
> > > There would be a dtbs_check warning, yea
Re: [PATCH v2 1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01
Hi, On Fri, Jul 19, 2024 at 10:07 AM Doug Anderson wrote: > > Hi, > > On Thu, Jul 18, 2024 at 7:59 AM Doug Anderson wrote: > > > > Hi, > > > > On Thu, Jul 18, 2024 at 7:56 AM Conor Dooley wrote: > > > > > > On Thu, Jul 18, 2024 at 07:45:57AM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Wed, Jul 17, 2024 at 11:19 PM Krzysztof Kozlowski > > > > wrote: > > > > > > > > > > On 18/07/2024 02:21, Doug Anderson wrote: > > > > > > Conor (and/or) Krzysztof and Rob, > > > > > > > > > > > > On Mon, Jul 15, 2024 at 8:31 AM Conor Dooley > > > > > > wrote: > > > > > >> > > > > > >> On Mon, Jul 15, 2024 at 02:15:37PM +0200, Stephan Gerhold wrote: > > > > > >>> The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has > > > > > >>> backlight > > > > > >>> control over the DP AUX channel. While it works almost correctly > > > > > >>> with the > > > > > >>> generic "edp-panel" compatible, the backlight needs special > > > > > >>> handling to > > > > > >>> work correctly. It is similar to the existing ATNA33XC20 panel, > > > > > >>> just with > > > > > >>> a larger resolution and size. > > > > > >>> > > > > > >>> Add a new "samsung,atna45af01" compatible to describe this panel > > > > > >>> in the DT. > > > > > >>> Use the existing "samsung,atna33xc20" as fallback compatible > > > > > >>> since existing > > > > > >>> drivers should work as-is, given that resolution and size are > > > > > >>> discoverable > > > > > >>> through the eDP link. > > > > > >>> > > > > > >>> Signed-off-by: Stephan Gerhold > > > > > >> > > > > > >> Acked-by: Conor Dooley > > > > > > > > > > > > Can you comment on whether you would consider this bindings a "Fix" > > > > > > since it's a dependency for later patches in this series (which are > > > > > > "Fix"es) to pass dtbs_check? See: > > > > > > > > > > > > https://lore.kernel.org/r/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org > > > > > > > > > > The patch itself is not a fix, for sure, but it might be a dependency > > > > > of > > > > > a fix (which you wrote above), thus could be pulled to stable as a > > > > > dependency. > > > > > > > > > > I do not care about dtbs_check warnings in stable kernels, mostly > > > > > because dtbs_check warnings depend heavily on dtschema and dtschema > > > > > follows mainline kernel. Basically if you had warnings-free v6.8 but > > > > > try > > > > > to run dtbs_check now with latest dtschema, your results will differ. > > > > > > > > > > At some point in the future, I could imagine "no new dtbs_check > > > > > warnings > > > > > in stable kernels" requirement or at least preference, but so far I > > > > > don't think there is any benefit. > > > > > > > > In this case it's not about whether it makes it to the stable kernel > > > > but about which main kernel it goes through. > > > > > > > > If we land the bindings in drm-misc-next right now then it'll be a > > > > long time before it makes it into Linus's tree because of the way that > > > > drm-misc-next merges. It will make it to Linus's tree at 6.12. You can > > > > see the drm-misc merging strategy at: > > > > > > > > https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html > > > > > > > > If we land the dts change (a fix) through the Qualcomm tree as a fix > > > > then it should target 6.11. > > > > > > > > This means that the 6.11 tree will have a dtbs_check error because it > > > > has the dts change (a fix) but not the bindings change (not a fix). > > > > > > > > One way to resolve this would be to treat this bindings as a "fix" and > > > > land it through "drm-misc-fixes". That would make the bindings and > > > > device tree change meet up in Linux 6.11. > > > > > > > > Did I get that all correct? > > > > > > Is not not fairly established that a dependency for a fix can go onto a > > > fixes branch even if it is not a fix in and of itself? > > > > That would certainly be my take on it, but DT folks confirmation was > > requested by Neil in: > > > > https://lore.kernel.org/all/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org/ > > FWIW, I'd rather not let this stagnate too long. I'm fairly confident > in my assertion that this should go into drm-misc-fixes. I'll give it > until Monday and then I'm just going to land this bindings change in > drm-misc-fixes. Shout soon if you feel strongly that I shouldn't do > this. If someone wants to flame me after the fact then so be it. Landed in drm-misc-next fixes as per the flow chart [1] since the "samsung,atna33xc20.yaml" split from "panel-simple.yaml" is in mainline linux but not in any "rc" candidates yet. [1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01 commit: b6f7d984ebf826069d3dc6fa187b4d1cfb90f965 [1] https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html#where-do-i-apply-my-patch -Doug
Re: [PATCH v2 1/2] drm/panel: boe-th101mb31ig002 : Fix the way to get porch parameters
Hi,
On Tue, Jul 16, 2024 at 5:11 AM Zhaoxiong Lv
wrote:
>
> The current driver can only obtain the porch parameters
> of boe-th101mb31ig002. Modify it to obtain the porch
> parameters of the panel currently being used.
>
> Fixes: 24179ff9a2e4524 ("drm/panel: boe-th101mb31ig002 : Make it compatible
> with other panel.")
> Signed-off-by: Zhaoxiong Lv
> ---
> Changes between V2 and V1:
> - 1. No changes, Modify the commit information format.
> v1:
> https://lore.kernel.org/all/20240715031845.6687-2-lvzhaoxi...@huaqin.corp-partner.google.com/
> ---
> drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Douglas Anderson
I assume we'd want to see a re-post of this series with patch #2
comments addressed before landing? What do others think?
Re: [PATCH v2 1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01
Hi, On Thu, Jul 18, 2024 at 7:59 AM Doug Anderson wrote: > > Hi, > > On Thu, Jul 18, 2024 at 7:56 AM Conor Dooley wrote: > > > > On Thu, Jul 18, 2024 at 07:45:57AM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Jul 17, 2024 at 11:19 PM Krzysztof Kozlowski > > > wrote: > > > > > > > > On 18/07/2024 02:21, Doug Anderson wrote: > > > > > Conor (and/or) Krzysztof and Rob, > > > > > > > > > > On Mon, Jul 15, 2024 at 8:31 AM Conor Dooley wrote: > > > > >> > > > > >> On Mon, Jul 15, 2024 at 02:15:37PM +0200, Stephan Gerhold wrote: > > > > >>> The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has > > > > >>> backlight > > > > >>> control over the DP AUX channel. While it works almost correctly > > > > >>> with the > > > > >>> generic "edp-panel" compatible, the backlight needs special > > > > >>> handling to > > > > >>> work correctly. It is similar to the existing ATNA33XC20 panel, > > > > >>> just with > > > > >>> a larger resolution and size. > > > > >>> > > > > >>> Add a new "samsung,atna45af01" compatible to describe this panel in > > > > >>> the DT. > > > > >>> Use the existing "samsung,atna33xc20" as fallback compatible since > > > > >>> existing > > > > >>> drivers should work as-is, given that resolution and size are > > > > >>> discoverable > > > > >>> through the eDP link. > > > > >>> > > > > >>> Signed-off-by: Stephan Gerhold > > > > >> > > > > >> Acked-by: Conor Dooley > > > > > > > > > > Can you comment on whether you would consider this bindings a "Fix" > > > > > since it's a dependency for later patches in this series (which are > > > > > "Fix"es) to pass dtbs_check? See: > > > > > > > > > > https://lore.kernel.org/r/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org > > > > > > > > The patch itself is not a fix, for sure, but it might be a dependency of > > > > a fix (which you wrote above), thus could be pulled to stable as a > > > > dependency. > > > > > > > > I do not care about dtbs_check warnings in stable kernels, mostly > > > > because dtbs_check warnings depend heavily on dtschema and dtschema > > > > follows mainline kernel. Basically if you had warnings-free v6.8 but try > > > > to run dtbs_check now with latest dtschema, your results will differ. > > > > > > > > At some point in the future, I could imagine "no new dtbs_check warnings > > > > in stable kernels" requirement or at least preference, but so far I > > > > don't think there is any benefit. > > > > > > In this case it's not about whether it makes it to the stable kernel > > > but about which main kernel it goes through. > > > > > > If we land the bindings in drm-misc-next right now then it'll be a > > > long time before it makes it into Linus's tree because of the way that > > > drm-misc-next merges. It will make it to Linus's tree at 6.12. You can > > > see the drm-misc merging strategy at: > > > > > > https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html > > > > > > If we land the dts change (a fix) through the Qualcomm tree as a fix > > > then it should target 6.11. > > > > > > This means that the 6.11 tree will have a dtbs_check error because it > > > has the dts change (a fix) but not the bindings change (not a fix). > > > > > > One way to resolve this would be to treat this bindings as a "fix" and > > > land it through "drm-misc-fixes". That would make the bindings and > > > device tree change meet up in Linux 6.11. > > > > > > Did I get that all correct? > > > > Is not not fairly established that a dependency for a fix can go onto a > > fixes branch even if it is not a fix in and of itself? > > That would certainly be my take on it, but DT folks confirmation was > requested by Neil in: > > https://lore.kernel.org/all/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org/ FWIW, I'd rather not let this stagnate too long. I'm fairly confident in my assertion that this should go into drm-misc-fixes. I'll give it until Monday and then I'm just going to land this bindings change in drm-misc-fixes. Shout soon if you feel strongly that I shouldn't do this. If someone wants to flame me after the fact then so be it. -Doug
Re: [PATCH] drm/mipi-dsi: Introduce macros to create mipi_dsi_*_multi functions
Hi, On Wed, Jul 17, 2024 at 3:07 AM Dmitry Baryshkov wrote: > > > > However it might be better to go other way arround. > > > Do we want for all the drivers to migrate to _multi()-kind of API? If > > > so, what about renaming the multi and non-multi functions accordingly > > > and making the old API a wrapper around the multi() function? > > > > > > > I think this would be good. For the wrapper to make a multi() function > > non-multi, what do you think about a macro that would just pass a > > default dsi_ctx to the multi() func and return on error? In this case > > it would also be good to let the code fill inline instead of generating > > a whole new function imo. > > > > So in this scenario all the mipi dsi functions would be multi functions, > > and a function could be called non-multi like MACRO_NAME(func) at the > > call site. > > Sounds good to me. I'd suggest to wait for a day or two for further > feedback / comments from other developers. I don't totally hate the idea of going full-multi and just having macros to wrap anyone who hasn't converted, but I'd be curious how much driver bloat this will cause for drivers that aren't converted. Would the compiler do a good job optimizing here? Maybe we don't care if we just want everyone to switch over? If nothing else, it might make sense to at least keep both versions for the very generic functions (mipi_dsi_generic_write_multi and mipi_dsi_dcs_write_buffer_multi) ...oh, but wait. We probably have the non-multi versions wrap the _multi ones. One of the things about the _multi functions is that they are also "chatty" and print errors. They're designed for the use case of a pile of init code where any error is fatal and needs to be printed. I suspect that somewhere along the way someone will want to be able to call these functions and not have them print errors... Maybe going with Dmitry's suggested syntax is the best option here? Depending on how others feel, we could potentially even get rid of the special error message and just stringify the function name for the error message? -Doug
Re: [PATCH 2/3] drm/panel: samsung, atna33xc20: Add compatible for ATNA45DC02
Hi,
On Thu, Jul 18, 2024 at 11:44 AM Rob Clark wrote:
>
> From: Rob Clark
>
> The Samsung ATNA45DC02 panel needs the same power sequencing as the
> ATNA45AF01 and ATNA33XC20.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 1 +
> 1 file changed, 1 insertion(+)
I believe this patch should be dropped and, until there is special
logic needed for "samsung,atna45dc02" we can just rely on the fallback
compatible ("samsung,atna33xc20") matching.
-Doug
Re: [PATCH 1/3] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02
Hi, On Thu, Jul 18, 2024 at 2:36 PM Dmitry Baryshkov wrote: > > On Thu, Jul 18, 2024 at 11:44:32AM GMT, Rob Clark wrote: > > From: Rob Clark > > > > The Samsung ATNA45DC02 panel is an AMOLED eDP panel, similar to the > > existing ATNA45AF01 and ATNA33XC20 panel but with a higher resolution. > > > > Signed-off-by: Rob Clark > > --- > > .../devicetree/bindings/display/panel/samsung,atna33xc20.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > index d668e8d0d296..284a4ee79bbf 100644 > > --- > > a/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > +++ > > b/Documentation/devicetree/bindings/display/panel/samsung,atna33xc20.yaml > > @@ -19,6 +19,8 @@ properties: > >- samsung,atna33xc20 > ># Samsung 14.5" WQXGA+ (2880x1800 pixels) eDP AMOLED panel > >- samsung,atna45af01 > > + # Samsung 14.5" 3K (2944x1840 pixels) eDP AMOLED panel > > + - samsung,atna45dc02 Please follow what we decided in Stephan Gerhold's case and use a fallback compatible. In other words: your patch seems to be based on his v1 and not his v2. > This makes me wonder if we should define a cover compatible like > samsung,amoled-edp-panel. I will bow to the wisdom of the DT folks, but my understanding is that we shouldn't do that. The fallback compatible can just be the compatible of the first panel that we supported that used the same interface. -Doug
Re: [RFC] drm/panel/simple-edp: Add Samsung ATNA45DC02
Hi, On Thu, Jul 18, 2024 at 9:25 AM Rob Clark wrote: > > On Thu, Jul 18, 2024 at 9:00 AM Doug Anderson wrote: > > > > Hi, > > > > On Wed, Jul 17, 2024 at 6:09 PM Rob Clark wrote: > > > > > > On Wed, Jul 17, 2024 at 5:19 PM Doug Anderson > > > wrote: > > > > > > > > Hi, > > > > > > > > On Wed, Jul 17, 2024 at 2:58 PM Rob Clark wrote: > > > > > > > > > > From: Rob Clark > > > > > > > > > > Just a guess on the panel timings, but they appear to work. > > > > > > > > > > Signed-off-by: Rob Clark > > > > > --- > > > > > This adds the panel I have on my lenovo yoga slim 7x laptop. I > > > > > couldn't > > > > > find any datasheet so timings is just a guess. But AFAICT everything > > > > > works fine. > > > > > > > > > > drivers/gpu/drm/panel/panel-edp.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > Given that this is a Samsung ATNA, is there any chance it's an > > > > OLED panel? Should it be supported with the Samsung OLED panel driver > > > > like this: > > > > > > > > https://lore.kernel.org/r/20240715-x1e80100-crd-backlight-v2-0-31b7f2f65...@linaro.org > > > > > > it is an OLED panel, and I did see that patchset and thought that > > > samsung panel driver would work. But changing the compat string on > > > the yoga dts only gave me a black screen (and I didn't see any of the > > > other mentioned problems with bl control or dpms with panel-edp). So > > > :shrug:? It could be I overlooked something else, but it _seems_ like > > > panel-edp is fine for this panel. Plus, it would avoid awkwardness if > > > it turned out there were other non-samsung 2nd sources, but so far > > > with a sample size of two 100% of these laptops have the same panel > > > > Hmm, OK. One question for you: are you using the "enable" GPIO in > > panel-edp? IMO the code handling that GPIO in panel-edp is somewhat > > dodgy, but it predates my deeper involvement with panels. I've never > > seen an eDP panel that could use panel-edp where it was actually > > required--every instance where someone thought it was required was > > better modeled by using that GPIO as the backlight enable. On the > > other hand, the "enable" GPIO in the Samsung OLED panel driver came > > straight from the panel datasheet and it makes sense for it to be in > > the panel driver since the backlight is handled straight by the > > panel... > > hmm, at least current dts doesn't have an enable gpio. Which could be > why panel-samsung-atna33xc20 wasn't working. > > It is entirely possible we are relying on something left on by the bootloader. That would be my best guess. Is there any way for you to find out if there's an enable GPIO? > > In any case, I guess if things are working it doesn't really hurt to > > take it in panel-edp for now... > > > > I wonder if using compatible="edp-panel" everywhere isn't a great idea > if we want to switch drivers later. But I guess that is already water > under the bridge (so to speak) For panels that aren't OLED it's all very standard and we're kinda forced to use something generic since manufacturers want lots of 2nd (and 3rd and 4th and ...) sourcing. As far as I've been able to tell you can't do 2nd sourcing between OLED panels and other panels since the wires hooked up to the panels are a little different for the OLED panels and the power sequencing is a bit different. It would also be pretty obvious to an end user if some of their devices had an OLED panel and some didn't. I'm not aware of OLED panels other than the Samsung ones, but I haven't done any real research here... -Doug
Re: [RFC] drm/panel/simple-edp: Add Samsung ATNA45DC02
Hi, On Wed, Jul 17, 2024 at 6:09 PM Rob Clark wrote: > > On Wed, Jul 17, 2024 at 5:19 PM Doug Anderson wrote: > > > > Hi, > > > > On Wed, Jul 17, 2024 at 2:58 PM Rob Clark wrote: > > > > > > From: Rob Clark > > > > > > Just a guess on the panel timings, but they appear to work. > > > > > > Signed-off-by: Rob Clark > > > --- > > > This adds the panel I have on my lenovo yoga slim 7x laptop. I couldn't > > > find any datasheet so timings is just a guess. But AFAICT everything > > > works fine. > > > > > > drivers/gpu/drm/panel/panel-edp.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > Given that this is a Samsung ATNA, is there any chance it's an > > OLED panel? Should it be supported with the Samsung OLED panel driver > > like this: > > > > https://lore.kernel.org/r/20240715-x1e80100-crd-backlight-v2-0-31b7f2f65...@linaro.org > > it is an OLED panel, and I did see that patchset and thought that > samsung panel driver would work. But changing the compat string on > the yoga dts only gave me a black screen (and I didn't see any of the > other mentioned problems with bl control or dpms with panel-edp). So > :shrug:? It could be I overlooked something else, but it _seems_ like > panel-edp is fine for this panel. Plus, it would avoid awkwardness if > it turned out there were other non-samsung 2nd sources, but so far > with a sample size of two 100% of these laptops have the same panel Hmm, OK. One question for you: are you using the "enable" GPIO in panel-edp? IMO the code handling that GPIO in panel-edp is somewhat dodgy, but it predates my deeper involvement with panels. I've never seen an eDP panel that could use panel-edp where it was actually required--every instance where someone thought it was required was better modeled by using that GPIO as the backlight enable. On the other hand, the "enable" GPIO in the Samsung OLED panel driver came straight from the panel datasheet and it makes sense for it to be in the panel driver since the backlight is handled straight by the panel... In any case, I guess if things are working it doesn't really hurt to take it in panel-edp for now... > But that was the reason for posting this as an RFC. I was hoping > someone had some hint about where to find datasheets (my google'ing > was not successful), etc. I don't personally have any hints. -Doug
Re: [PATCH v2 1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01
Hi, On Thu, Jul 18, 2024 at 7:56 AM Conor Dooley wrote: > > On Thu, Jul 18, 2024 at 07:45:57AM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, Jul 17, 2024 at 11:19 PM Krzysztof Kozlowski > > wrote: > > > > > > On 18/07/2024 02:21, Doug Anderson wrote: > > > > Conor (and/or) Krzysztof and Rob, > > > > > > > > On Mon, Jul 15, 2024 at 8:31 AM Conor Dooley wrote: > > > >> > > > >> On Mon, Jul 15, 2024 at 02:15:37PM +0200, Stephan Gerhold wrote: > > > >>> The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > > > >>> control over the DP AUX channel. While it works almost correctly with > > > >>> the > > > >>> generic "edp-panel" compatible, the backlight needs special handling > > > >>> to > > > >>> work correctly. It is similar to the existing ATNA33XC20 panel, just > > > >>> with > > > >>> a larger resolution and size. > > > >>> > > > >>> Add a new "samsung,atna45af01" compatible to describe this panel in > > > >>> the DT. > > > >>> Use the existing "samsung,atna33xc20" as fallback compatible since > > > >>> existing > > > >>> drivers should work as-is, given that resolution and size are > > > >>> discoverable > > > >>> through the eDP link. > > > >>> > > > >>> Signed-off-by: Stephan Gerhold > > > >> > > > >> Acked-by: Conor Dooley > > > > > > > > Can you comment on whether you would consider this bindings a "Fix" > > > > since it's a dependency for later patches in this series (which are > > > > "Fix"es) to pass dtbs_check? See: > > > > > > > > https://lore.kernel.org/r/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org > > > > > > The patch itself is not a fix, for sure, but it might be a dependency of > > > a fix (which you wrote above), thus could be pulled to stable as a > > > dependency. > > > > > > I do not care about dtbs_check warnings in stable kernels, mostly > > > because dtbs_check warnings depend heavily on dtschema and dtschema > > > follows mainline kernel. Basically if you had warnings-free v6.8 but try > > > to run dtbs_check now with latest dtschema, your results will differ. > > > > > > At some point in the future, I could imagine "no new dtbs_check warnings > > > in stable kernels" requirement or at least preference, but so far I > > > don't think there is any benefit. > > > > In this case it's not about whether it makes it to the stable kernel > > but about which main kernel it goes through. > > > > If we land the bindings in drm-misc-next right now then it'll be a > > long time before it makes it into Linus's tree because of the way that > > drm-misc-next merges. It will make it to Linus's tree at 6.12. You can > > see the drm-misc merging strategy at: > > > > https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html > > > > If we land the dts change (a fix) through the Qualcomm tree as a fix > > then it should target 6.11. > > > > This means that the 6.11 tree will have a dtbs_check error because it > > has the dts change (a fix) but not the bindings change (not a fix). > > > > One way to resolve this would be to treat this bindings as a "fix" and > > land it through "drm-misc-fixes". That would make the bindings and > > device tree change meet up in Linux 6.11. > > > > Did I get that all correct? > > Is not not fairly established that a dependency for a fix can go onto a > fixes branch even if it is not a fix in and of itself? That would certainly be my take on it, but DT folks confirmation was requested by Neil in: https://lore.kernel.org/all/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org/ -Doug
Re: [PATCH v2 1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01
Hi, On Wed, Jul 17, 2024 at 11:19 PM Krzysztof Kozlowski wrote: > > On 18/07/2024 02:21, Doug Anderson wrote: > > Conor (and/or) Krzysztof and Rob, > > > > On Mon, Jul 15, 2024 at 8:31 AM Conor Dooley wrote: > >> > >> On Mon, Jul 15, 2024 at 02:15:37PM +0200, Stephan Gerhold wrote: > >>> The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > >>> control over the DP AUX channel. While it works almost correctly with the > >>> generic "edp-panel" compatible, the backlight needs special handling to > >>> work correctly. It is similar to the existing ATNA33XC20 panel, just with > >>> a larger resolution and size. > >>> > >>> Add a new "samsung,atna45af01" compatible to describe this panel in the > >>> DT. > >>> Use the existing "samsung,atna33xc20" as fallback compatible since > >>> existing > >>> drivers should work as-is, given that resolution and size are discoverable > >>> through the eDP link. > >>> > >>> Signed-off-by: Stephan Gerhold > >> > >> Acked-by: Conor Dooley > > > > Can you comment on whether you would consider this bindings a "Fix" > > since it's a dependency for later patches in this series (which are > > "Fix"es) to pass dtbs_check? See: > > > > https://lore.kernel.org/r/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org > > The patch itself is not a fix, for sure, but it might be a dependency of > a fix (which you wrote above), thus could be pulled to stable as a > dependency. > > I do not care about dtbs_check warnings in stable kernels, mostly > because dtbs_check warnings depend heavily on dtschema and dtschema > follows mainline kernel. Basically if you had warnings-free v6.8 but try > to run dtbs_check now with latest dtschema, your results will differ. > > At some point in the future, I could imagine "no new dtbs_check warnings > in stable kernels" requirement or at least preference, but so far I > don't think there is any benefit. In this case it's not about whether it makes it to the stable kernel but about which main kernel it goes through. If we land the bindings in drm-misc-next right now then it'll be a long time before it makes it into Linus's tree because of the way that drm-misc-next merges. It will make it to Linus's tree at 6.12. You can see the drm-misc merging strategy at: https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html If we land the dts change (a fix) through the Qualcomm tree as a fix then it should target 6.11. This means that the 6.11 tree will have a dtbs_check error because it has the dts change (a fix) but not the bindings change (not a fix). One way to resolve this would be to treat this bindings as a "fix" and land it through "drm-misc-fixes". That would make the bindings and device tree change meet up in Linux 6.11. Did I get that all correct? -Doug
Re: [PATCH v2 1/4] dt-bindings: display: panel: samsung,atna33xc20: Document ATNA45AF01
Conor (and/or) Krzysztof and Rob, On Mon, Jul 15, 2024 at 8:31 AM Conor Dooley wrote: > > On Mon, Jul 15, 2024 at 02:15:37PM +0200, Stephan Gerhold wrote: > > The Samsung ATNA45AF01 panel is an AMOLED eDP panel that has backlight > > control over the DP AUX channel. While it works almost correctly with the > > generic "edp-panel" compatible, the backlight needs special handling to > > work correctly. It is similar to the existing ATNA33XC20 panel, just with > > a larger resolution and size. > > > > Add a new "samsung,atna45af01" compatible to describe this panel in the DT. > > Use the existing "samsung,atna33xc20" as fallback compatible since existing > > drivers should work as-is, given that resolution and size are discoverable > > through the eDP link. > > > > Signed-off-by: Stephan Gerhold > > Acked-by: Conor Dooley Can you comment on whether you would consider this bindings a "Fix" since it's a dependency for later patches in this series (which are "Fix"es) to pass dtbs_check? See: https://lore.kernel.org/r/4bca316a-2334-425b-87a6-e1bb241d2...@linaro.org -Doug
Re: [RFC] drm/panel/simple-edp: Add Samsung ATNA45DC02
Hi, On Wed, Jul 17, 2024 at 2:58 PM Rob Clark wrote: > > From: Rob Clark > > Just a guess on the panel timings, but they appear to work. > > Signed-off-by: Rob Clark > --- > This adds the panel I have on my lenovo yoga slim 7x laptop. I couldn't > find any datasheet so timings is just a guess. But AFAICT everything > works fine. > > drivers/gpu/drm/panel/panel-edp.c | 2 ++ > 1 file changed, 2 insertions(+) Given that this is a Samsung ATNA, is there any chance it's an OLED panel? Should it be supported with the Samsung OLED panel driver like this: https://lore.kernel.org/r/20240715-x1e80100-crd-backlight-v2-0-31b7f2f65...@linaro.org -Doug
Re: [PATCH v1] drm/panel-edp: Add panels with conservative timings
Hi,
On Wed, Jul 17, 2024 at 2:39 AM Terry Hsiao
wrote:
>
> The 6 panels are used on Mediatek MT8186 Chromebooks
> - B116XAT04.1 (06AF/B4C4)
> - NV116WHM-A4D (09E5/FA0C)
> - N116BCP-EA2 (0DAE/6111)
> - B116XTN02.3 (06AF/AA73)
> - B116XAN06.1 (06AF/99A1)
> - N116BCA-EA2 (0DAE/5D11)
>
> Signed-off-by: Terry Hsiao
> ---
> drivers/gpu/drm/panel/panel-edp.c | 6 ++
> 1 file changed, 6 insertions(+)
Please resend with a better patch subject, like "drm/panel-edp: Add 6
panels used by MT8186 Chromebooks".
Also: are you adding timings based on the datasheets, or are you just
guessing here? The previous patches that added "conservative" timings
were because the Chromebooks involved were really old and couldn't be
tracked down and folks couldn't find the relevant datasheets. In the
case of MT8188 I'd expect you to be adding timings based on the
datasheets. Please confirm that you are.
If possible, it's really nice to have the raw EDIDs for the panels in
the commit message in case someone needs it later.
> diff --git a/drivers/gpu/drm/panel/panel-edp.c
> b/drivers/gpu/drm/panel/panel-edp.c
> index f85a6404ba58..ac280607998f 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1845,8 +1845,11 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x635c, &delay_200_500_e50,
> "B116XAN06.3"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x639c, &delay_200_500_e50,
> "B140HAK02.7"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x723c, &delay_200_500_e50,
> "B140XTN07.2"),
> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x73aa, &delay_200_500_e50,
> "B116XTN02.3"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50,
> "B133UAN01.0"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0xd497, &delay_200_500_e50,
> "B120XAN01.0"),
> + EDP_PANEL_ENTRY('A', 'U', 'O', 0xa199, &delay_200_500_e50,
> "B116XAN06.1"),
Please keep this sorted. For instance, 0xa199 should come _before_
0xd497, right?
> + EDP_PANEL_ENTRY('A', 'U', 'O', 0xc4b4, &delay_200_500_e50,
> "B116XAT04.1"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0xf390, &delay_200_500_e50,
> "B140XTN07.7"),
>
> EDP_PANEL_ENTRY('B', 'O', 'E', 0x0607, &delay_200_500_e200,
> "Unknown"),
> @@ -1901,6 +1904,7 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('B', 'O', 'E', 0x0b56, &delay_200_500_e80,
> "NT140FHM-N47"),
> EDP_PANEL_ENTRY('B', 'O', 'E', 0x0c20, &delay_200_500_e80,
> "NT140FHM-N47"),
> EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cb6, &delay_200_500_e200,
> "NT116WHM-N44"),
> + EDP_PANEL_ENTRY('B', 'O', 'E', 0x0cfa, &delay_200_500_e50,
> "NV116WHM-A4D"),
>
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x1130, &delay_200_500_e50,
> "N116BGE-EB2"),
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x1132, &delay_200_500_e80_d50,
> "N116BGE-EA2"),
> @@ -1916,8 +1920,10 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x1156, &delay_200_500_e80_d50,
> "Unknown"),
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x1157, &delay_200_500_e80_d50,
> "N116BGE-EA2"),
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x115b, &delay_200_500_e80_d50,
> "N116BCN-EB1"),
> + EDP_PANEL_ENTRY('C', 'M', 'N', 0x115d, &delay_200_500_e80_d50,
> "N116BCA-EA2"),
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x115e, &delay_200_500_e80_d50,
> "N116BCA-EA1"),
> EDP_PANEL_ENTRY('C', 'M', 'N', 0x1160, &delay_200_500_e80_d50,
> "N116BCJ-EAK"),
> + EDP_PANEL_ENTRY('C', 'M', 'N', 0x1161, &delay_200_500_e80,
> "N116BCP-EA2"),
It looks suspicious that all the panels around this one need 50 ms for
disable but yours doesn't. While it's certainly possible that things
changed for this panel, it's worth double-checking.
-Doug
Re: [PATCH v2 2/2] drm/panel: boe-th101mb31ig002 : using drm_connector_helper_get_modes_fixed()
Hi,
On Tue, Jul 16, 2024 at 5:11 AM Zhaoxiong Lv
wrote:
>
> Use public functions(drm_connector_helper_get_modes_fixed()) to
> get porch parameters.
>
> Signed-off-by: Zhaoxiong Lv
> ---
> .../drm/panel/panel-boe-th101mb31ig002-28a.c | 26 ++-
> 1 file changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> b/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> index d4e4abd103bb..4a61a11099b6 100644
> --- a/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> +++ b/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
>
> struct boe_th101mb31ig002;
>
> @@ -313,31 +314,8 @@ static int boe_th101mb31ig002_get_modes(struct drm_panel
> *panel,
> struct
> boe_th101mb31ig002,
> panel);
> const struct drm_display_mode *desc_mode = ctx->desc->modes;
> - struct drm_display_mode *mode;
>
> - mode = drm_mode_duplicate(connector->dev, desc_mode);
> - if (!mode) {
> - dev_err(panel->dev, "Failed to add mode %ux%u@%u\n",
> - desc_mode->hdisplay, desc_mode->vdisplay,
> - drm_mode_vrefresh(desc_mode));
> - return -ENOMEM;
> - }
> -
> - drm_mode_set_name(mode);
> -
> - connector->display_info.bpc = 8;
I notice that drm_connector_helper_get_modes_fixed() doesn't seem to
set bpc. Unless I'm mistaken and that gets set automatically somewhere
else then you should keep that, right?
> - connector->display_info.width_mm = mode->width_mm;
> - connector->display_info.height_mm = mode->height_mm;
> -
> - /*
> -* TODO: Remove once all drm drivers call
> -* drm_connector_set_orientation_from_panel()
> -*/
> - drm_connector_set_panel_orientation(connector, ctx->orientation);
Are we confident that all the other users of this panel are properly
getting the orientation and we can remove the above bit of code? It
looks like one other user is 'rk3566-pinetab2'.
>From what I recall, the relevant commits are commit 15b9ca1641f0
("drm: Config orientation property if panel provides it") and commit
e3ea1806e4ad ("drm/bridge: panel: Set orientation on panel_bridge
connector"). I think in all cases the assumption was that, to get the
right functionality we need to switch to "panel_bridge". That happens
when we use drmm_of_get_bridge() or devm_drm_of_get_bridge(). ...but
it looks like Rockchip DRM is directly using
drm_of_find_panel_or_bridge() and thus hasn't switched to panel
bridge.
...so, unless I'm mistaken, the other users of this panel driver still
need the drm_connector_set_panel_orientation() call here and you
shouldn't remove it. Perhaps Alexander Warnecke could comment about
whether this is still needed. ...or perhaps someone who maintains
Rockchip DRM can say whether they have any plans around this area?
If, for some reason, you do remove it then it should at least be
called out in the description since this is a functionality change.
-Doug
Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
Hi, On Fri, Jun 21, 2024 at 1:46 PM Doug Anderson wrote: > > Hi, > > On Fri, Jun 21, 2024 at 1:45 PM Douglas Anderson > wrote: > > > > At shutdown if you've got a _properly_ coded DRM modeset driver then > > you'll get these two warnings at shutdown time: > > > > Skipping disable of already disabled panel > > Skipping unprepare of already unprepared panel > > > > These warnings are ugly and sound concerning, but they're actually a > > sign of a properly working system. That's not great. > > > > We're not ready to get rid of the calls to drm_panel_disable() and > > drm_panel_unprepare() because we're not 100% convinced that all DRM > > modeset drivers are properly calling drm_atomic_helper_shutdown() or > > drm_helper_force_disable_all() at the right times. However, having the > > warning show up for correctly working systems is bad. > > > > As a bit of a workaround, add some "if" tests to try to avoid the > > warning on correctly working systems. Also add some comments and > > update the TODO items in the hopes that future developers won't be too > > confused by what's going on here. > > > > Suggested-by: Daniel Vetter > > Signed-off-by: Douglas Anderson > > --- > > This patch came out of discussion on dri-devel on 2024-06-21 > > [1]. NOTE: I have put all changes into one patch since it didn't seem > > to add anything to break up the updating of the TODO or the comments > > in the core into separate patches since the patch is all about one > > topic and all code is expected to land in the same tree. > > > > Previous versions: > > v0: > > https://lore.kernel.org/r/20240604172305.v3.24.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid/ > > v1: > > https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid > > > > [1] > > https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2024-06-21 > > > > Documentation/gpu/todo.rst | 35 +--- > > drivers/gpu/drm/drm_panel.c | 18 ++ > > drivers/gpu/drm/panel/panel-edp.c| 26 ++--- > > drivers/gpu/drm/panel/panel-simple.c | 26 ++--- > > 4 files changed, 68 insertions(+), 37 deletions(-) > > Ugh! I realized right after I hit "send" that I forgot to mark this as > V2 and give it version history. Sorry! :( Please consider this to be > v2. It's basically totally different than v1 based on today's IRC > discussion, which should be linked above. > > If I need to send a new version I will send it as v3. Is anyone willing to give me a Reviewed-by and/or Acked by for this patch? ...or does anything want me to make any changes? Given all the discussion we had, it would be nice to get this landed before we forget what we agreed upon. :-P -Doug
