[PATCH] drm/radeon: fix null pointer dereference in radeon_add_common_modes
In radeon_add_common_modes(), the return value of drm_cvt_mode() is assigned to mode, which will lead to a possible NULL pointer dereference on failure of drm_cvt_mode(). Add a check to avoid npd. Signed-off-by: Ma Ke --- drivers/gpu/drm/radeon/radeon_connectors.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b84b58926106..71ddc4672850 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -520,6 +520,8 @@ static void radeon_add_common_modes(struct drm_encoder *encoder, struct drm_conn continue; mode = drm_cvt_mode(dev, common_modes[i].w, common_modes[i].h, 60, false, false, false); + if (!mode) + continue; drm_mode_probed_add(connector, mode); } } -- 2.25.1
[PATCH] drm/client: fix null pointer dereference in drm_client_modeset_probe
In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is assigned to modeset->mode, which will lead to a possible NULL pointer dereference on failure of drm_mode_duplicate(). Add a check to avoid npd. Signed-off-by: Ma Ke --- drivers/gpu/drm/drm_client_modeset.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 31af5cf37a09..bbe21522dc6a 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -880,6 +880,8 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, kfree(modeset->mode); modeset->mode = drm_mode_duplicate(dev, mode); + if (!modeset->mode) + continue; drm_connector_get(connector); modeset->connectors[modeset->num_connectors++] = connector; modeset->x = offset->x; -- 2.25.1
[PATCH] drm/nouveau/dispnv04: fix null pointer dereference in nv17_tv_get_ld_modes
In nv17_tv_get_ld_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. Cc: sta...@vger.kernel.org Signed-off-by: Ma Ke --- drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c index 670c9739e5e1..4a08e61f3336 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c @@ -209,6 +209,8 @@ static int nv17_tv_get_ld_modes(struct drm_encoder *encoder, struct drm_display_mode *mode; mode = drm_mode_duplicate(encoder->dev, tv_mode); + if (!mode) + continue; mode->clock = tv_norm->tv_enc_mode.vrefresh * mode->htotal / 1000 * -- 2.25.1
[PATCH] drm/nouveau: fix null pointer dereference in nouveau_connector_get_modes
In nouveau_connector_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. Signed-off-by: Ma Ke --- drivers/gpu/drm/nouveau/nouveau_connector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 856b3ef5edb8..010eed56b14d 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -1001,6 +1001,8 @@ nouveau_connector_get_modes(struct drm_connector *connector) struct drm_display_mode *mode; mode = drm_mode_duplicate(dev, nv_connector->native_mode); + if (!mode) + return -ENOMEM; drm_mode_probed_add(connector, mode); ret = 1; } -- 2.25.1
[PATCH 1/2] dt-bindings: display: simple: Add AUO G104STN01 panel
Add AUO G104STN01 10.4" LCD-TFT LVDS panel compatible string. Signed-off-by: Paul Gerber --- Tested on TQ MBa8MPxL with TQMa8MPxL. .../devicetree/bindings/display/panel/panel-simple.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index 5067f5c0a272..8d75284845db 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -64,6 +64,8 @@ properties: # AU Optronics Corporation 10.4" (800x600) color TFT LCD panel - auo,g104sn02 # AU Optronics Corporation 12.1" (1280x800) TFT LCD panel + - auo,g104stn01 +# AU Optronics Corporation 10.4" (800x600) color TFT LCD panel - auo,g121ean01 # AU Optronics Corporation 15.6" (1366x768) TFT LCD panel - auo,g156xtn01 -- 2.44.1
[PATCH] drm/gma500: fix null pointer dereference in psb_intel_lvds_get_modes
In psb_intel_lvds_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.
Signed-off-by: Ma Ke
---
drivers/gpu/drm/gma500/psb_intel_lvds.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c
b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index 8486de230ec9..aa5bf2a8a319 100644
--- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
@@ -504,6 +504,8 @@ static int psb_intel_lvds_get_modes(struct drm_connector
*connector)
if (mode_dev->panel_fixed_mode != NULL) {
struct drm_display_mode *mode =
drm_mode_duplicate(dev, mode_dev->panel_fixed_mode);
+ if (!mode)
+ return -ENOMEM;
drm_mode_probed_add(connector, mode);
return 1;
}
--
2.25.1
[PATCH 2/2] drm/panel: simple: Add AUO G104STN01 panel entry
Add support for the AUO G104STN01 10.4" (800x600) LCD-TFT panel.
Signed-off-by: Paul Gerber
---
Tested on TQ MBa8MPxL with TQMa8MPxL.
drivers/gpu/drm/panel/panel-simple.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index dcb6d0b6ced0..5eacd2085a53 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1081,6 +1081,30 @@ static const struct panel_desc auo_g104sn02 = {
.connector_type = DRM_MODE_CONNECTOR_LVDS,
};
+static const struct drm_display_mode auo_g104stn01_mode = {
+ .clock = 4,
+ .hdisplay = 800,
+ .hsync_start = 800 + 40,
+ .hsync_end = 800 + 40 + 88,
+ .htotal = 800 + 40 + 88 + 128,
+ .vdisplay = 600,
+ .vsync_start = 600 + 1,
+ .vsync_end = 600 + 1 + 23,
+ .vtotal = 600 + 1 + 23 + 4,
+};
+
+static const struct panel_desc auo_g104stn01 = {
+ .modes = &auo_g104stn01_mode,
+ .num_modes = 1,
+ .bpc = 8,
+ .size = {
+ .width = 211,
+ .height = 158,
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
+ .connector_type = DRM_MODE_CONNECTOR_LVDS,
+};
+
static const struct display_timing auo_g121ean01_timing = {
.pixelclock = { 6000, 7440, 9000 },
.hactive = { 1280, 1280, 1280 },
@@ -4434,6 +4458,9 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "auo,g104sn02",
.data = &auo_g104sn02,
+ }, {
+ .compatible = "auo,g104stn01",
+ .data = &auo_g104stn01,
}, {
.compatible = "auo,g121ean01",
.data = &auo_g121ean01,
--
2.44.1
[PATCH] drm/nouveau/dispnv04: fix null pointer dereference in nv17_tv_get_hd_modes
In nv17_tv_get_hd_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(). The same applies to drm_cvt_mode().
Add a check to avoid null pointer dereference.
Cc: sta...@vger.kernel.org
Signed-off-by: Ma Ke
---
drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
index 670c9739e5e1..9c3dc9a5bb46 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
@@ -258,6 +258,8 @@ static int nv17_tv_get_hd_modes(struct drm_encoder *encoder,
if (modes[i].hdisplay == output_mode->hdisplay &&
modes[i].vdisplay == output_mode->vdisplay) {
mode = drm_mode_duplicate(encoder->dev, output_mode);
+ if (!mode)
+ continue;
mode->type |= DRM_MODE_TYPE_PREFERRED;
} else {
@@ -265,6 +267,8 @@ static int nv17_tv_get_hd_modes(struct drm_encoder *encoder,
modes[i].vdisplay, 60, false,
(output_mode->flags &
DRM_MODE_FLAG_INTERLACE), false);
+ if (!mode)
+ continue;
}
/* CVT modes are sometimes unsuitable... */
--
2.25.1
Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
On Thu, May 09, 2024 at 05:41:18PM +0300, Oded Gabbay wrote: > On Thu, May 09, 2024 at 03:53:01PM +0200, Tomeu Vizoso wrote: > > Oded, Dave, > > > > Do you have an opinion on this? > > > > Thanks, > > > > Tomeu > Hi Tomeu, > > Sorry for not replying earlier, I was down with Covid (again...). > > To your question, I don't have an objection to what you are > suggesting. My personal view of accel is that it is an integral part of > DRM and therefore, if there is an *existing* drm driver that wants to > create an accel node, I'm not against it. Yeah, there's a continum from "clearly 3d gpu" to "compute AI accelerator", with everything possible in-between shipping somewhere. Collaboration is the important part, hair-splitting on where exactly the driver should be is kinda secondary. I mean beyond "don't put a pure 3d driver into accel or vice versa" of course :-) > There is the question of why you want to expose an accel node, and > here I would like to hear Dave's and Sima's opinion on your suggested > solution as it may affect the direction of other drm drivers. So existing userspace that blindly assumes that any render node will give it useful 3d acceleration, then that's broken already. - kernel with new driver support but old mesa without that driver already gives you that, even for a pure 3d chip. - intel (and I think also amd) have pure compute chips without 3d, so this issue already exists Same for the other directions, 3d gpus have variable amounts of compute chips nowadays. That leaves imo just the pragmatic choice, and if we need to complicate the init flow of the kernel driver just for a different charnode major, then I don't really see the point. And if we do see the point in this, I think the right approach would be if we split the init flow further into allocating the drm_device, and then in a 2nd step either allocate the accel or render uapi stuff as needed. The DRIVER_FOO flags just aren't super flexible for this kinda of stuff and have a bit a midlayer taste to them. Cheers, Sima > > Thanks, > Oded. > > p.s. > Please only use bottom-posting when replying, thanks :) > > > > > On Fri, Apr 26, 2024 at 8:10 AM Tomeu Vizoso wrote: > > > > > > On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo > > > wrote: > > > > > > > > On 4/24/2024 12:37 AM, Tomeu Vizoso wrote: > > > > > If we expose a render node for NPUs without rendering capabilities, > > > > > the > > > > > userspace stack will offer it to compositors and applications for > > > > > rendering, which of course won't work. > > > > > > > > > > Userspace is probably right in not questioning whether a render node > > > > > might not be capable of supporting rendering, so change it in the > > > > > kernel > > > > > instead by exposing a /dev/accel node. > > > > > > > > > > Before we bring the device up we don't know whether it is capable of > > > > > rendering or not (depends on the features of its blocks), so first try > > > > > to probe a rendering node, and if we find out that there is no > > > > > rendering > > > > > hardware, abort and retry with an accel node. > > > > > > > > > > Signed-off-by: Tomeu Vizoso > > > > > Cc: Oded Gabbay > > > > > > > > I hope Oded chimes in as Accel maintainer. I think Airlie/Vetter had > > > > also previously mentioned they'd have opinions on what is Accel vs DRM. > > > > > > > > This gets a nack from me in its current state. This is not a strong > > > > nack, and I don't want to discourage you. I think there is a path > > > > forward. > > > > > > > > The Accel subsystem documentation says that accel drivers will reside in > > > > drivers/accel/ but this does not. > > > > > > Indeed, there is that code organization aspect. > > > > > > > Also, the commit text for "accel: add dedicated minor for accelerator > > > > devices" mentions - > > > > > > > > "for drivers that > > > > declare they handle compute accelerator, using a new driver feature > > > > flag called DRIVER_COMPUTE_ACCEL. It is important to note that this > > > > driver feature is mutually exclusive with DRIVER_RENDER. Devices that > > > > want to expose both graphics and compute device char files should be > > > > handled by two drivers that are connected using the auxiliary bus > > > > framework." > > > > > > > > I don't see any of that happening here (two drivers connected by aux > > > > bus, one in drivers/accel). > > > > > > Well, the text refers to devices, not drivers. The case we are talking > > > about is a driver that wants to sometimes expose an accel node, and > > > sometimes a render node, depending on the hardware it is dealing with. > > > So there would either be a device exposing a single render node, or a > > > device exposing a single accel node. > > > > > > Though by using the auxiliary bus we could in theory solve the code > > > organization problem mentioned above, I'm not quite seeing how to do > > > this in a clean way. The driver in /drivers/gpu/drm would have to be a > > > DRM driver that doesn't
Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
On Mon, Jun 17, 2024 at 07:01:05PM +0200, Tomeu Vizoso wrote: > Hi Lucas, > > Do you have any idea on how not to break userspace if we expose a render node? So if you get a new chip with an incompatible 3d block, you already have that issue. And I hope etnaviv userspace can cope. Worst case you need to publish a fake extremely_fancy_3d_block to make sure old mesa never binds against an NPU-only instance. Or mesa just doesn't cope, in which case we need a etnaviv-v2-we_are_sorry drm driver name, or something like that. -Sima > > Cheers, > > Tomeu > > On Wed, Jun 12, 2024 at 4:26 PM Tomeu Vizoso wrote: > > > > On Mon, May 20, 2024 at 1:19 PM Daniel Stone wrote: > > > > > > Hi, > > > > > > On Mon, 20 May 2024 at 08:39, Tomeu Vizoso wrote: > > > > On Fri, May 10, 2024 at 10:34 AM Lucas Stach > > > > wrote: > > > > > Am Mittwoch, dem 24.04.2024 um 08:37 +0200 schrieb Tomeu Vizoso: > > > > > > If we expose a render node for NPUs without rendering capabilities, > > > > > > the > > > > > > userspace stack will offer it to compositors and applications for > > > > > > rendering, which of course won't work. > > > > > > > > > > > > Userspace is probably right in not questioning whether a render node > > > > > > might not be capable of supporting rendering, so change it in the > > > > > > kernel > > > > > > instead by exposing a /dev/accel node. > > > > > > > > > > > > Before we bring the device up we don't know whether it is capable of > > > > > > rendering or not (depends on the features of its blocks), so first > > > > > > try > > > > > > to probe a rendering node, and if we find out that there is no > > > > > > rendering > > > > > > hardware, abort and retry with an accel node. > > > > > > > > > > On the other hand we already have precedence of compute only DRM > > > > > devices exposing a render node: there are AMD GPUs that don't expose a > > > > > graphics queue and are thus not able to actually render graphics. Mesa > > > > > already handles this in part via the PIPE_CAP_GRAPHICS and I think we > > > > > should simply extend this to not offer a EGL display on screens > > > > > without > > > > > that capability. > > > > > > > > The problem with this is that the compositors I know don't loop over > > > > /dev/dri files, trying to create EGL screens and moving to the next > > > > one until they find one that works. > > > > > > > > They take the first render node (unless a specific one has been > > > > configured), and assumes it will be able to render with it. > > > > > > > > To me it seems as if userspace expects that /dev/dri/renderD* devices > > > > can be used for rendering and by breaking this assumption we would be > > > > breaking existing software. > > > > > > Mm, it's sort of backwards from that. Compositors just take a > > > non-render DRM node for KMS, then ask GBM+EGL to instantiate a GPU > > > which can work with that. When run in headless mode, we don't take > > > render nodes directly, but instead just create an EGLDisplay or > > > VkPhysicalDevice and work backwards to a render node, rather than > > > selecting a render node and going from there. > > > > > > So from that PoV I don't think it's really that harmful. The only > > > complication is in Mesa, where it would see an etnaviv/amdgpu/... > > > render node and potentially try to use it as a device. As long as Mesa > > > can correctly skip, there should be no userspace API implications. > > > > > > That being said, I'm not entirely sure what the _benefit_ would be of > > > exposing a render node for a device which can't be used by any > > > 'traditional' DRM consumers, i.e. GL/Vulkan/winsys. > > > > What I don't understand yet from Lucas proposal is how this isn't > > going to break existing userspace. > > > > I mean, even if we find a good way of having userspace skip > > non-rendering render nodes, what about existing userspace that isn't > > able to do that? Any updates to newer kernels are going to break them. > > > > Regards, > > > > Tomeu -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v3 1/2] dt-bindings: display: bridge: tc358867: Document default DP preemphasis
Hi Marek,
Am Dienstag, 25. Juni 2024, 14:05:14 CEST schrieb Marek Vasut:
> Document default DP port preemphasis configurable via new DT property
> "toshiba,pre-emphasis". This is useful in case the DP link properties
> are known and starting link training from preemphasis setting of 0 dB
> is not useful. The preemphasis can be set separately for both DP lanes
> in range 0=0dB, 1=3.5dB, 2=6dB .
>
> This is an endpoint property, not a port property, because the TC9595
> datasheet does mention that the DP might operate in some sort of split
> mode, where each DP lane is used to feed one display, so in that case
> there might be two endpoints.
>
> Signed-off-by: Marek Vasut
> ---
> Cc: Andrzej Hajda
> Cc: Conor Dooley
> Cc: Daniel Vetter
> Cc: David Airlie
> Cc: Jernej Skrabec
> Cc: Jonas Karlman
> Cc: Krzysztof Kozlowski
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Neil Armstrong
> Cc: Rob Herring
> Cc: Robert Foss
> Cc: Thomas Zimmermann
> Cc: devicet...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: ker...@dh-electronics.com
> ---
> V2: - Fix the type to u8 array
> - Fix the enum items to match what they represent
> V3: - Update commit message, expand on this being an endpoint property
> ---
> .../display/bridge/toshiba,tc358767.yaml | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
> b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
> index 2ad0cd6dd49e0..9490854c22f3b 100644
> --- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
> @@ -98,6 +98,24 @@ properties:
> reference to a valid eDP panel input endpoint node. This port is
> optional, treated as DP panel if not defined
>
> +properties:
> + endpoint:
> +$ref: /schemas/media/video-interfaces.yaml#
> +unevaluatedProperties: false
> +
> +properties:
> + toshiba,pre-emphasis:
> +description:
> + Display port output Pre-Emphasis settings for both DP
> lanes.
> +$ref: /schemas/types.yaml#/definitions/uint8-array
> +minItems: 2
> +maxItems: 2
> +items:
> + enum:
> +- 0 # No pre-emphasis
> +- 1 # 3.5dB pre-emphasis
> +- 2 # 6dB pre-emphasis
> +
> oneOf:
>- required:
>- port@0
>
I get this warning:
> mx8mp-tqma8mpql-mba8mpxl.dtb: bridge@f: ports:port@2:endpoint: Unevaluated
> properties are not allowed ('toshiba,pre-emphasis' was unexpected)
DT node looks like this:
port@2 {
reg = <2>;
endpoint {
toshiba,pre-emphasis = /bits/ 8 <1 1>;
};
};
I think you are missing this change as well:
-- 8< --
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -92,7 +92,8 @@ properties:
reference to a valid DPI output or input endpoint node.
port@2:
-$ref: /schemas/graph.yaml#/properties/port
+$ref: /schemas/graph.yaml#/$defs/port-base
+unevaluatedProperties: false
description: |
eDP/DP output port. The remote endpoint phandle should be a
reference to a valid eDP panel input endpoint node. This port is
-- 8< --
How would you determine the values to be set here? I suspect it's the value
from register DP0_SnkLTChReq (0x6d4) after link training. Are they dependent
on the actual display to be attached?
Best regards,
Alexander
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: (subset) [PATCH 0/2] arm64: dts: amlogic: add power domain to hdmitx
Hi, On Tue, 25 Jun 2024 16:50:13 +0200, Jerome Brunet wrote: > This patchset add the bindings for the power domain of the HDMI Tx > on Amlogic SoC. > > This is a 1st step in cleaning HDMI Tx and its direct usage of HHI > register space. Eventually, this will help remove component usage from > the Amlogic display drivers. > > [...] Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-next) [1/2] dt-bindings: display: meson-dw-hdmi: add missing power-domain https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/c19f15b1e056a1ab896d54909f75febf70d98be6 -- Neil
Re: (subset) [PATCH 0/2] arm64: dts: amlogic: add power domain to hdmitx
Hi, On Tue, 25 Jun 2024 16:50:13 +0200, Jerome Brunet wrote: > This patchset add the bindings for the power domain of the HDMI Tx > on Amlogic SoC. > > This is a 1st step in cleaning HDMI Tx and its direct usage of HHI > register space. Eventually, this will help remove component usage from > the Amlogic display drivers. > > [...] Thanks, Applied to https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.11/arm64-dt) [2/2] arm64: dts: amlogic: add power domain to hdmitx https://git.kernel.org/amlogic/c/f1ab099d6591a353899a2ee09c89de0fc908e2d2 These changes has been applied on the intermediate git tree [1]. The v6.11/arm64-dt branch will then be sent via a formal Pull Request to the Linux SoC maintainers for inclusion in their intermediate git branches in order to be sent to Linus during the next merge window, or sooner if it's a set of fixes. In the cases of fixes, those will be merged in the current release candidate kernel and as soon they appear on the Linux master branch they will be backported to the previous Stable and Long-Stable kernels [2]. The intermediate git branches are merged daily in the linux-next tree [3], people are encouraged testing these pre-release kernels and report issues on the relevant mailing-lists. If problems are discovered on those changes, please submit a signed-off-by revert patch followed by a corrective changeset. [1] https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git -- Neil
Re: Time for drm-ci-next?
On Mon, Jun 24, 2024 at 10:25:25AM -0300, Helen Koike wrote: > > > On 24/06/2024 02:34, Vignesh Raman wrote: > > Hi, > > > > On 15/03/24 22:50, Rob Clark wrote: > > > On Fri, Mar 15, 2024 at 2:28 AM Jani Nikula > > > wrote: > > > > > > > > On Thu, 14 Mar 2024, Rob Clark wrote: > > > > > When we first merged drm/ci I was unsure if it would need it's own > > > > > -next branch. But after using it for a couple releases, a few times > > > > > I've found myself wanting to backmerge drm/ci changes without > > > > > necessarily backmerging all of drm-misc-next. > > > > > > > > > > So, maybe it makes some sense to have a drm-ci-next branch that > > > > > driver-maintainers could back-merge as-needed? > > > > > > > > That's a crossmerge instead of a backmerge, and I feel that could get > > > > messy. What if folks crossmerge drm-ci-next but it gets rejected for > > > > drm-next? Or the baselines are different, and the crossmerge pulls in > > > > way more stuff than it should? > > > > > > Yeah, it would defeat the point a bit of drm-ci-next was on too new of > > > a baseline, the whole point is to be able to merge CI changes without > > > pulling in unrelated changes. So drm-ci-next would need to base on > > > something older, like the previous kernel release tag. > > > > > > > IMO the route should be drm-ci-next -> pull request to drm-next -> > > > > backmerge drm-next to drivers and drm-misc-next. > > > > > > > > I'm not opposed to having drm-ci-next at all, mainly indifferent, but I > > > > question the merge flows. And then the question becomes, does my > > > > suggested merge flow complicate your original goal? > > > > > > > > > > I guess we could avoid merging drm-ci-next until it had been merged > > > into drm-next? Yes, either dedicated topic branch or only backmerging drm-next please, that's how we're handling the flow for all other subtrees too. > > > Basically, I often find myself needing to merge CI patches on top of > > > msm-next in order to run CI, and then after a clean CI run, reset HEAD > > > back before the merge and force-push. Which isn't really how things > > > should work. This sounds more like you want an integration tree like drm-tip. Get msm branches integrated there, done. Backmerges just for integration testing are not a good idea indeed. > > There are many CI patches merged recently to drm-misc-next. > > With the GitLab 18.0 release, CI/CD pipeline configurations must > > transition from using the deprecated CI_JOB_JWT to the new id_tokens > > method, as the former will be removed. > > > > Without the below commit kernel-build job pipelines fail in drm-ci, > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/cc806b74466672a9bbd4e9a04265d44eb506b686 > > > > We need to cherry pick only this commit to fix this issue. > > So it would be beneficial to have a drm-ci-next branch. > > > > Regards, > > Vignesh > > > I don't mind using a drm-ci-next branch if it is created. What exactly is the issue in backmerging drm-misc-next (well through drm-next really)? Also if there is an issue, generally we do ad-hoc topic branches. I'm very very skeptical of boutique trees with tiny focus, we've had that before drm-misc, it's a mess. Definitely no enthusiasm for getting back to that kind of world. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v3 2/2] drm/bridge: tc358767: Add configurable default preemphasis
Hi Marek,
thanks for patch.
Am Dienstag, 25. Juni 2024, 14:05:15 CEST schrieb Marek Vasut:
> Make the default DP port preemphasis configurable via new DT property
> "toshiba,pre-emphasis". This is useful in case the DP link properties
> are known and starting link training from preemphasis setting of 0 dB
> is not useful. The preemphasis can be set separately for both DP lanes
> in range 0=0dB, 1=3.5dB, 2=6dB .
>
> Signed-off-by: Marek Vasut
> ---
> Cc: Andrzej Hajda
> Cc: Conor Dooley
> Cc: Daniel Vetter
> Cc: David Airlie
> Cc: Jernej Skrabec
> Cc: Jonas Karlman
> Cc: Krzysztof Kozlowski
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Neil Armstrong
> Cc: Rob Herring
> Cc: Robert Foss
> Cc: Thomas Zimmermann
> Cc: devicet...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: ker...@dh-electronics.com
> ---
> V2: - Parse toshiba,pre-emphasis property out of an endpoint of port 2 (the
> DP port)
> V3: - No change
> ---
> drivers/gpu/drm/bridge/tc358767.c | 45 ++-
> 1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index dde1b2734c98a..257fe15080099 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -241,6 +241,10 @@
>
> /* Link Training */
> #define DP0_SRCCTRL 0x06a0
> +#define DP0_SRCCTRL_PRE1 GENMASK(29, 28)
> +#define DP0_SRCCTRL_SWG1 GENMASK(25, 24)
> +#define DP0_SRCCTRL_PRE0 GENMASK(21, 20)
> +#define DP0_SRCCTRL_SWG0 GENMASK(17, 16)
> #define DP0_SRCCTRL_SCRMBLDISBIT(13)
> #define DP0_SRCCTRL_EN810B BIT(12)
> #define DP0_SRCCTRL_NOTP (0 << 8)
> @@ -278,6 +282,8 @@
> #define AUDIFDATA6 0x0720 /* DP0 Audio Info Frame Bytes 27 to 24
> */
>
> #define DP1_SRCCTRL 0x07a0 /* DP1 Control Register */
> +#define DP1_SRCCTRL_PRE GENMASK(21, 20)
> +#define DP1_SRCCTRL_SWG GENMASK(17, 16)
>
> /* PHY */
> #define DP_PHY_CTRL 0x0800
> @@ -369,6 +375,7 @@ struct tc_data {
>
> u32 rev;
> u8 assr;
> + u8 pre_emphasis[2];
>
> struct gpio_desc*sd_gpio;
> struct gpio_desc*reset_gpio;
> @@ -1090,13 +1097,17 @@ static int tc_main_link_enable(struct tc_data *tc)
> return ret;
> }
>
> - ret = regmap_write(tc->regmap, DP0_SRCCTRL, tc_srcctrl(tc));
> + ret = regmap_write(tc->regmap, DP0_SRCCTRL,
> +tc_srcctrl(tc) |
> +FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
> +FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
> if (ret)
> return ret;
> /* SSCG and BW27 on DP1 must be set to the same as on DP0 */
> ret = regmap_write(tc->regmap, DP1_SRCCTRL,
>(tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
> - ((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
> + ((tc->link.rate != 162000) ? DP0_SRCCTRL_BW27 : 0) |
> + FIELD_PREP(DP1_SRCCTRL_PRE, tc->pre_emphasis[1]));
> if (ret)
> return ret;
>
> @@ -1188,8 +1199,10 @@ static int tc_main_link_enable(struct tc_data *tc)
> goto err_dpcd_write;
>
> /* Reset voltage-swing & pre-emphasis */
> - tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
> - DP_TRAIN_PRE_EMPH_LEVEL_0;
> + tmp[0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
> + FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[0]);
> + tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 |
> + FIELD_PREP(DP_TRAIN_PRE_EMPHASIS_MASK, tc->pre_emphasis[1]);
> ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
> if (ret < 0)
> goto err_dpcd_write;
> @@ -1213,7 +1226,9 @@ static int tc_main_link_enable(struct tc_data *tc)
> ret = regmap_write(tc->regmap, DP0_SRCCTRL,
> tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> DP0_SRCCTRL_AUTOCORRECT |
> -DP0_SRCCTRL_TP1);
> +DP0_SRCCTRL_TP1 |
> +FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
> +FIELD_PREP(DP0_SRCCTRL_PRE1, tc->pre_emphasis[1]));
> if (ret)
> return ret;
>
> @@ -1248,7 +1263,9 @@ static int tc_main_link_enable(struct tc_data *tc)
> ret = regmap_write(tc->regmap, DP0_SRCCTRL,
> tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> DP0_SRCCTRL_AUTOCORRECT |
> -DP0_SRCCTRL_TP2);
> +DP0_SRCCTRL_TP2 |
> +FIELD_PREP(DP0_SRCCTRL_PRE0, tc->pre_emphasis[0]) |
> +
Re: [PATCH 1/2] dt-bindings: display: simple: Add AUO G104STN01 panel
On 26/06/2024 06:36, Paul Gerber wrote: > Add AUO G104STN01 10.4" LCD-TFT LVDS panel compatible string. > > Signed-off-by: Paul Gerber Acked-by: Krzysztof Kozlowski --- This is an automated instruction, just in case, because many review tags are being ignored. If you know the process, you can skip it (please do not feel offended by me posting it here - no bad intentions intended). If you do not know the process, here is a short explanation: Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions, under or above your Signed-off-by tag. Tag is "received", when provided in a message replied to you on the mailing list. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for tags received on the version they apply. https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577 Best regards, Krzysztof
Re: [PATCH 1/8] drm: bridge: dw_hdmi: Call poweron/poweroff from atomic enable/disable
On Mon, Jun 24, 2024 at 02:19:36PM +0200, Jonas Karlman wrote:
> Hi Neil,
>
> On 2024-06-24 11:56, neil.armstr...@linaro.org wrote:
> > On 24/06/2024 11:41, Jonas Karlman wrote:
> >> Hi Neil,
> >>
> >> On 2024-06-24 11:23, Neil Armstrong wrote:
> >>> On 11/06/2024 17:50, Jonas Karlman wrote:
> Change to only call poweron/poweroff from atomic_enable/atomic_disable
> ops instead of trying to keep a bridge_is_on state and poweron/off in
> the hotplug irq handler.
>
> A benefit of this is that drm mode_config mutex is always held at
> poweron/off, something that may reduce the need for our own mutex.
>
> Signed-off-by: Jonas Karlman
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 33 ++-
> 1 file changed, 2 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 9f2bc932c371..34bc6f4754b8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -172,7 +172,6 @@ struct dw_hdmi {
> enum drm_connector_force force; /* mutex-protected force state
> */
> struct drm_connector *curr_conn;/* current connector (only
> valid when !disabled) */
> bool disabled; /* DRM has disabled our bridge
> */
> -bool bridge_is_on; /* indicates the bridge is on */
> bool rxsense; /* rxsense state */
> u8 phy_mask;/* desired phy int mask
> settings */
> u8 mc_clkdis; /* clock disable register */
> @@ -2383,8 +2382,6 @@ static void initialize_hdmi_ih_mutes(struct
> dw_hdmi *hdmi)
>
> static void dw_hdmi_poweron(struct dw_hdmi *hdmi)
> {
> -hdmi->bridge_is_on = true;
> -
> /*
> * The curr_conn field is guaranteed to be valid here, as this
> function
> * is only be called when !hdmi->disabled.
> @@ -2398,30 +2395,6 @@ static void dw_hdmi_poweroff(struct dw_hdmi *hdmi)
> hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
> hdmi->phy.enabled = false;
> }
> -
> -hdmi->bridge_is_on = false;
> -}
> -
> -static void dw_hdmi_update_power(struct dw_hdmi *hdmi)
> -{
> -int force = hdmi->force;
> -
> -if (hdmi->disabled) {
> -force = DRM_FORCE_OFF;
> -} else if (force == DRM_FORCE_UNSPECIFIED) {
> -if (hdmi->rxsense)
> -force = DRM_FORCE_ON;
> -else
> -force = DRM_FORCE_OFF;
> -}
> >>>
> >>> This means we always poweron the bridge even if rxsense is false ?
> >>
> >> If I follow the logic there should not be any difference, beside that
> >> power on now only happen in atomic_enable instead of sometime in irq
> >> handler.
> >>
> >> hdmi->rxsense is set to true based on hpd in dw_hdmi_setup_rx_sense().
> >> For both meson and dw-hdmi this means HPD=1 and not rxsense=1.
> >
> > Yeah I know, I was worried for other platforms using rxsense
>
> From what I can find only dw-hdmi.c and dw_hdmi_meson.c are caller of
> dw_hdmi_setup_rx_sense(). For meson the same value is passed for both
> hpd and rx_sense, everyone else hpd = HPD and rx_sense = RX_SENSE status.
>
> My understanding and testing based on Rockchip has shown that rx_sense
> has no significant impact before and after this change.
>
> >
> >>
> >> drm core will call the force/detect ops and enable/disable based on
> >> forced/HPD state, so I am not expecting any difference in how this
> >> currently works.
> >>
> >> This change to only poweron/setup in atomic_enable should also fix
> >> issues/situations where dw-hdmi was poweron too early during bootup in
> >> irq handler, before the drm driver was fully probed.
>
> I may have been wrong here, there is a check for disabled here so it
> should not have setup() before atomic_enable().
>
> Still we should ensure configure happen in atomic_enable(). The
> hpd_irq_event/hpd_notify calls will trigger a detect() and signal core
> if the bridge should be enabled/disabled when connector status changes.
>
> >
> > Hmm, but I thought the code wouldn't poweron the bridge is rxsense was 0
> > even if a mode was set, this won't work like this anymore right ?
>
> This is what I thought was happening, and the comment in code seem to
> indicate that. However, the code only seem to care about HPD=1 status to
> poweron() and possible both HPD=0 + RXSENSE=0 to poweroff().
More fundamental question: Why do we even care what happens in this case?
Userspace lit up an output that's not connected either
- it's broken, it gets
Re: [PATCH v1 1/3] dt-bindings: display/msm/gmu: Add Adreno X185 GMU
On Sun, Jun 23, 2024 at 02:40:14PM +0200, Krzysztof Kozlowski wrote: > On 23/06/2024 13:06, Akhil P Oommen wrote: > > Document Adreno X185 GMU in the dt-binding specification. > > > > Signed-off-by: Akhil P Oommen > > --- > > > > Documentation/devicetree/bindings/display/msm/gmu.yaml | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/msm/gmu.yaml > > b/Documentation/devicetree/bindings/display/msm/gmu.yaml > > index b3837368a260..9aa7151fd66f 100644 > > --- a/Documentation/devicetree/bindings/display/msm/gmu.yaml > > +++ b/Documentation/devicetree/bindings/display/msm/gmu.yaml > > @@ -23,6 +23,9 @@ properties: > >- items: > >- pattern: '^qcom,adreno-gmu-[67][0-9][0-9]\.[0-9]$' > >- const: qcom,adreno-gmu > > + - items: > > + - pattern: '^qcom,adreno-gmu-[x][1-9][0-9][0-9]\.[0-9]$' > > '[x]' is odd. Should be just 'x'. Ack -Akhil > > > Best regards, > Krzysztof >
Re: [PATCH v1 2/3] drm/msm/adreno: Add support for X185 GPU
On Mon, Jun 24, 2024 at 12:21:30AM +0300, Dmitry Baryshkov wrote:
> On Sun, Jun 23, 2024 at 04:36:29PM GMT, Akhil P Oommen wrote:
> > Add support in drm/msm driver for the Adreno X185 gpu found in
> > Snapdragon X1 Elite chipset.
> >
> > Signed-off-by: Akhil P Oommen
> > ---
> >
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 19 +++
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 ++
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 5 +
> > 4 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 0e3dfd4c2bc8..168a4bddfaf2 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -830,8 +830,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
> > unsigned int state)
> > */
> > gmu_write(gmu, REG_A6XX_GMU_CM3_CFG, 0x4052);
> >
> > + if (adreno_is_x185(adreno_gpu)) {
> > + chipid = 0x7050001;
> > /* NOTE: A730 may also fall in this if-condition with a future GMU fw
> > update. */
> > - if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > + } else if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > /* A7xx GPUs have obfuscated chip IDs. Use constant maj = 7 */
> > chipid = FIELD_PREP(GENMASK(31, 24), 0x7);
> >
> > @@ -1329,9 +1331,18 @@ static int a6xx_gmu_rpmh_arc_votes_init(struct
> > device *dev, u32 *votes,
> > if (!pri_count)
> > return -EINVAL;
> >
> > - sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > - if (IS_ERR(sec))
> > - return PTR_ERR(sec);
> > + /*
> > +* Some targets have a separate gfx mxc rail. So try to read that first
> > and then fall back
> > +* to regular mx rail if it is missing
>
> Can we use compatibles / flags to detect this?
I prefer the current approach so that we don't need to keep adding
checks here for future targets. If gmxc is prefer, we have to use that
in all targets.
>
> > +*/
> > + sec = cmd_db_read_aux_data("gmxc.lvl", &sec_count);
> > + if (PTR_ERR_OR_ZERO(sec) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(sec)) {
> > + sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > + if (IS_ERR(sec))
> > + return PTR_ERR(sec);
> > + }
>
> The following code might be slightly more idiomatic:
>
> sec = cmd_db_read_aux_data("gmxc.lvl", &sec_count);
> if (IS_ERR(sec) && sec != ERR_PTR(-EPROBE_DEFER))
> sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> if (IS_ERR(sec))
> return PTR_ERR(sec);
>
Ack. This is neater!
>
> >
> > sec_count >>= 1;
> > if (!sec_count)
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index 973872ad0474..97837f7f2a40 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -1319,9 +1319,7 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
> > count = ARRAY_SIZE(a660_protect);
> > count_max = 48;
> > BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48);
> > - } else if (adreno_is_a730(adreno_gpu) ||
> > - adreno_is_a740(adreno_gpu) ||
> > - adreno_is_a750(adreno_gpu)) {
> > + } else if (adreno_is_a7xx(adreno_gpu)) {
> > regs = a730_protect;
> > count = ARRAY_SIZE(a730_protect);
> > count_max = 48;
> > @@ -1891,7 +1889,7 @@ static int hw_init(struct msm_gpu *gpu)
> > gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, BIT(7) | 0x1);
> >
> > /* Set weights for bicubic filtering */
> > - if (adreno_is_a650_family(adreno_gpu)) {
> > + if (adreno_is_a650_family(adreno_gpu) || adreno_is_x185(adreno_gpu)) {
> > gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_0, 0);
> > gpu_write(gpu, REG_A6XX_TPL1_BICUBIC_WEIGHTS_TABLE_1,
> > 0x3fe05ff4);
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index c3703a51287b..139c7d828749 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -568,6 +568,20 @@ static const struct adreno_info gpulist[] = {
> > .zapfw = "a740_zap.mdt",
> > .hwcg = a740_hwcg,
> > .address_space_size = SZ_16G,
> > + }, {
> > + .chip_ids = ADRENO_CHIP_IDS(0x43050c01), /* "C512v2" */
> > + .family = ADRENO_7XX_GEN2,
> > + .fw = {
> > + [ADRENO_FW_SQE] = "gen70500_sqe.fw",
> > + [ADRENO_FW_GMU] = "gen70500_gmu.bin",
> > + },
> > + .gmem = 3 * SZ_1M,
> > + .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >
[PATCH] drm/msm/adreno: fix a7xx gpu init
The gpulist has twice the a6xx gpulist, replace the second one
with the a7xx gpulist.
Solves:
msm_dpu ae01000.display-controller: Unknown GPU revision: 7.3.0.1
msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.10.1
msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.20.1
on SM8450, SM8550 & SM8560.
Fixes: 8ed322f632a9 ("drm/msm/adreno: Split up giant device table")
Signed-off-by: Neil Armstrong
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 1e789ff6945e..cfc74a9e2646 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -33,7 +33,7 @@ static const struct adreno_gpulist *gpulists[] = {
&a4xx_gpulist,
&a5xx_gpulist,
&a6xx_gpulist,
- &a6xx_gpulist,
+ &a7xx_gpulist,
};
static const struct adreno_info *adreno_info(uint32_t chip_id)
---
base-commit: 62c97045b8f720c2eac807a5f38e26c9ed512371
change-id: 20240626-topic-sm8x50-upstream-fix-a7xx-gpu-init-9fca9746ba73
Best regards,
--
Neil Armstrong
Re: [PATCH 1/6] drm/amdgpu: allow ioctls to opt-out of runtime pm
On Tue, Jun 25, 2024 at 09:07:12AM +0200, Pierre-Eric Pelloux-Prayer wrote:
>
> Le 20/06/2024 à 15:36, Christian König a écrit :
> > Am 20.06.24 um 15:06 schrieb Pierre-Eric Pelloux-Prayer:
> > > Le 19/06/2024 à 11:26, Christian König a écrit :
> > > > Am 18.06.24 um 17:23 schrieb Pierre-Eric Pelloux-Prayer:
> > > > > Waking up a device can take multiple seconds, so if it's not
> > > > > going to be used we might as well not resume it.
> > > > >
> > > > > The safest default behavior for all ioctls is to resume the GPU,
> > > > > so this change allows specific ioctls to opt-out of generic
> > > > > runtime pm.
> > > >
> > > > I'm really wondering if we shouldn't put that into the IOCTL
> > > > description.
> > > >
> > > > See amdgpu_ioctls_kms and DRM_IOCTL_DEF_DRV() for what I mean.
> > >
> > > Are you suggesting to add a new entry in enum drm_ioctl_flags to
> > > indicate ioctls which need the device to be awake?
> > >
> > > Something like: "DRM_NO_DEVICE = BIT(6)" and then use it for both
> > > core and amdgpu ioctls?
> >
> > Yeah something like that. Maybe name that DRM_SW_ONLY or something like
> > that.
>
> + dri-devel to gauge interest in adding such a flag in shared code.
Eh please no. That's just fundamentally the wrong way round to do runtime
pm, but amdgpu goes way, way back in desing in that record to the vga
switcheroo, where the simple hack was to just add runtime pm handling at
the entry points and call it done.
If you look at any other drm driver, the runtime pm handling is done way
down when touching the actual relevant hardware blocks. Because especially
on arm it's actually a pile of runtime pm domains.
So essentially this is like pushing a big driver lock down the callchain
until it's actually at the right level. First step would be to push it
into each of the amdgpu ioctl callbacks, so that you can drop
amdgpu_drm_ioctl and use drm_ioctl again directly. And then push it
further until you can remove it from all the places where it's not needed.
DRM_SW_ONLY otoh is pure midlayer mistake design.
Cheers, Sima
>
> Pierre-Eric
>
>
>
> >
> > Christian.
> >
> > >
> > > Pierre-Eric
> > >
> > >
> > >
> > >
> > > >
> > > > Regards,
> > > > Christian.
> > > >
> > > > >
> > > > > Signed-off-by: Pierre-Eric Pelloux-Prayer
> > > > >
> > > > > ---
> > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 25
> > > > > -
> > > > > 1 file changed, 20 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > index 60d5758939ae..a9831b243bfc 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > @@ -2855,18 +2855,33 @@ long amdgpu_drm_ioctl(struct file *filp,
> > > > > {
> > > > > struct drm_file *file_priv = filp->private_data;
> > > > > struct drm_device *dev;
> > > > > + bool needs_device;
> > > > > long ret;
> > > > > dev = file_priv->minor->dev;
> > > > > - ret = pm_runtime_get_sync(dev->dev);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > +
> > > > > + /* Some ioctl can opt-out of powermanagement handling
> > > > > + * if they don't require the device to be resumed.
> > > > > + */
> > > > > + switch (cmd) {
> > > > > + default:
> > > > > + needs_device = true;
> > > > > + }
> > > > > +
> > > > > + if (needs_device) {
> > > > > + ret = pm_runtime_get_sync(dev->dev);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > + }
> > > > > ret = drm_ioctl(filp, cmd, arg);
> > > > > - pm_runtime_mark_last_busy(dev->dev);
> > > > > out:
> > > > > - pm_runtime_put_autosuspend(dev->dev);
> > > > > + if (needs_device) {
> > > > > + pm_runtime_mark_last_busy(dev->dev);
> > > > > + pm_runtime_put_autosuspend(dev->dev);
> > > > > + }
> > > > > +
> > > > > return ret;
> > > > > }
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Tue, Jun 25, 2024 at 04:03:21PM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 25.06.24 um 15:55 schrieb Jocelyn Falempe:
> >
> >
> > On 25/06/2024 15:18, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> >
> > Do you think it's possible to add a default to drm_encoder_init() to
> > avoid having to declare the same struct for each encoder ?
> >
> > something like:
> >
> > drm_encoder_init(...)
> > {
> >
> > if (!funcs)
> > funcs = &drm_encoder_default_funcs;
> >
> > So you can call it like this to get the default funcs:
> >
> > drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
> >
> >
> > I don't see this pattern in other drm functions, so it might not fit the
> > current coding style.
>
> Yeah, we don't do this in other places. And it's not guaranteed that
> drm_encoder_cleanup() is the correct helper in all cases; even the common
> ones. I would prefer to not add such tweaks. As for
> drm_simple_encoder_init(), it was an attempt to solve exactly this problem,
> but the function is so trivial that it's not actually worth the dependency.
For drmm_encoder_init/alloc this is doable, because it makes sure that
there really is only one correct encoder cleanup hook for simple encoders.
Without drmm_ there's the entire "who calls kfree() and how buggy is it"
mess :-/
So I'd very much welcome this kind of handling in drmm_encoder_alloc/init,
but in the unmanaged drm_encoder_init I agree it sounds like a mistake.
Cheers,
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm/msm/adreno: De-spaghettify the use of memory barriers
On Tue, Jun 25, 2024 at 08:54:41PM +0200, Konrad Dybcio wrote:
> Memory barriers help ensure instruction ordering, NOT time and order
> of actual write arrival at other observers (e.g. memory-mapped IP).
> On architectures employing weak memory ordering, the latter can be a
> giant pain point, and it has been as part of this driver.
>
> Moreover, the gpu_/gmu_ accessors already use non-relaxed versions of
> readl/writel, which include r/w (respectively) barriers.
>
> Replace the barriers with a readback (or drop altogether where possible)
> that ensures the previous writes have exited the write buffer (as the CPU
> must flush the write to the register it's trying to read back).
>
> Signed-off-by: Konrad Dybcio
Some in pci these readbacks are actually part of the spec and called
posting reads. I'd very much recommend drivers create a small wrapper
function for these cases with a void return value, because it makes the
code so much more legible and easier to understand.
-Sima
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 +---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 ++
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 0e3dfd4c2bc8..09d640165b18 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -466,9 +466,7 @@ static int a6xx_rpmh_start(struct a6xx_gmu *gmu)
> int ret;
> u32 val;
>
> - gmu_write(gmu, REG_A6XX_GMU_RSCC_CONTROL_REQ, 1 << 1);
> - /* Wait for the register to finish posting */
> - wmb();
> + gmu_write(gmu, REG_A6XX_GMU_RSCC_CONTROL_REQ, BIT(1));
>
> ret = gmu_poll_timeout(gmu, REG_A6XX_GMU_RSCC_CONTROL_ACK, val,
> val & (1 << 1), 100, 1);
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index c98cdb1e9326..4083d0cad782 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -855,14 +855,16 @@ static int hw_init(struct msm_gpu *gpu)
> /* Clear GBIF halt in case GX domain was not collapsed */
> if (adreno_is_a619_holi(adreno_gpu)) {
> gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
> + gpu_read(gpu, REG_A6XX_GBIF_HALT);
> +
> gpu_write(gpu, REG_A6XX_RBBM_GPR0_CNTL, 0);
> - /* Let's make extra sure that the GPU can access the memory.. */
> - mb();
> + gpu_read(gpu, REG_A6XX_RBBM_GPR0_CNTL);
> } else if (a6xx_has_gbif(adreno_gpu)) {
> gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
> + gpu_read(gpu, REG_A6XX_GBIF_HALT);
> +
> gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
> - /* Let's make extra sure that the GPU can access the memory.. */
> - mb();
> + gpu_read(gpu, REG_A6XX_RBBM_GBIF_HALT);
> }
>
> /* Some GPUs are stubborn and take their sweet time to unhalt GBIF! */
>
> --
> 2.45.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] printk: Add a short description string to kmsg_dump()
On Tue 2024-06-25 14:39:29, Jocelyn Falempe wrote: > kmsg_dump doesn't forward the panic reason string to the kmsg_dumper > callback. > This patch adds a new parameter "const char *desc" to the kmsg_dumper > dump() callback, and update all drivers that are using it. > > To avoid updating all kmsg_dump() call, it adds a kmsg_dump_desc() > function and a macro for backward compatibility. > > I've written this for drm_panic, but it can be useful for other > kmsg_dumper. > It allows to see the panic reason, like "sysrq triggered crash" > or "VFS: Unable to mount root fs on " on the drm panic screen. > > Signed-off-by: Jocelyn Falempe > --- > arch/powerpc/kernel/nvram_64.c | 3 ++- > arch/powerpc/platforms/powernv/opal-kmsg.c | 3 ++- > drivers/gpu/drm/drm_panic.c| 3 ++- > drivers/hv/hv_common.c | 3 ++- > drivers/mtd/mtdoops.c | 3 ++- > fs/pstore/platform.c | 3 ++- > include/linux/kmsg_dump.h | 13 ++--- > kernel/panic.c | 2 +- > kernel/printk/printk.c | 8 +--- > 9 files changed, 28 insertions(+), 13 deletions(-) The parameter is added into all dumpers. I guess that it would be used only drm_panic() because it is graphics and might be "fancy". The others simply dump the log buffer and the reason is in the dumped log as well. Anyway, the passed buffer is static. Alternative solution would be to make it global and export it like, for example, panic_cpu. Best Regards, Petr
Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach
On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote: > Thank you for testing and keeping up with this. I will wait for more > feedback if there is any (Frieder? Lucas? Michael?). If there are no > objections, then I can merge it in a week or two ? I'll try to use your approach on the tc358775. Hopefully, I'll find some time this week. -michael signature.asc Description: PGP signature
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
Re: [PATCH net-next v14 03/13] netdev: support binding dma-buf to netdevice
On Tue, Jun 25, 2024 at 07:53:51PM +, Mina Almasry wrote: > Add a netdev_dmabuf_binding struct which represents the > dma-buf-to-netdevice binding. The netlink API will bind the dma-buf to > rx queues on the netdevice. On the binding, the dma_buf_attach > & dma_buf_map_attachment will occur. The entries in the sg_table from > mapping will be inserted into a genpool to make it ready > for allocation. > > The chunks in the genpool are owned by a dmabuf_chunk_owner struct which > holds the dma-buf offset of the base of the chunk and the dma_addr of > the chunk. Both are needed to use allocations that come from this chunk. > > We create a new type that represents an allocation from the genpool: > net_iov. We setup the net_iov allocation size in the > genpool to PAGE_SIZE for simplicity: to match the PAGE_SIZE normally > allocated by the page pool and given to the drivers. > > The user can unbind the dmabuf from the netdevice by closing the netlink > socket that established the binding. We do this so that the binding is > automatically unbound even if the userspace process crashes. > > The binding and unbinding leaves an indicator in struct netdev_rx_queue > that the given queue is bound, but the binding doesn't take effect until > the driver actually reconfigures its queues, and re-initializes its page > pool. > > The netdev_dmabuf_binding struct is refcounted, and releases its > resources only when all the refs are released. > > Signed-off-by: Willem de Bruijn > Signed-off-by: Kaiyuan Zhang > Signed-off-by: Mina Almasry > Reviewed-by: Pavel Begunkov # excluding netlink Absolutely no idea on the netdev side of things, and I'll leave the entire "how much userspace do you want" up to netdev folks too. But the dma_buf side looks fine, so for that: Acked-by: Daniel Vetter Cheers, Sima > > --- > > v13: > - Fixed a couple of places that still listed DMA_BIDIRECTIONAL (Pavel). > - Added reviewed-by from Pavel. > > v11: > - Fix build error with CONFIG_DMA_SHARED_BUFFER && > !CONFIG_GENERIC_ALLOCATOR > - Rebased on top of no memory provider ops. > > v10: > - Moved net_iov_dma_addr() to devmem.h and made it devmem specific > helper (David). > > v9: > https://lore.kernel.org/all/20240403002053.2376017-5-almasrym...@google.com/ > - Removed net_devmem_restart_rx_queues and put it in its own patch > (David). > > v8: > - move dmabuf_devmem_ops usage to later patch to avoid patch-by-patch > build error. > > v7: > - Use IS_ERR() instead of IS_ERR_OR_NULL() for the dma_buf_get() return > value. > - Changes netdev_* naming in devmem.c to net_devmem_* (Yunsheng). > - DMA_BIDIRECTIONAL -> DMA_FROM_DEVICE (Yunsheng). > - Added a comment around recovering of the old rx queue in > net_devmem_restart_rx_queue(), and added freeing of old_mem if the > restart of the old queue fails. (Yunsheng). > - Use kernel-family sock-priv (Jakub). > - Put pp_memory_provider_params in netdev_rx_queue instead of the > dma-buf specific binding (Pavel & David). > - Move queue management ops to queue_mgmt_ops instead of netdev_ops > (Jakub). > - Remove excess whitespaces (Jakub). > - Use genlmsg_iput (Jakub). > > v6: > - Validate rx queue index > - Refactor new functions into devmem.c (Pavel) > > v5: > - Renamed page_pool_iov to net_iov, and moved that support to devmem.h > or netmem.h. > > v1: > - Introduce devmem.h instead of bloating netdevice.h (Jakub) > - ENOTSUPP -> EOPNOTSUPP (checkpatch.pl I think) > - Remove unneeded rcu protection for binding->list (rtnl protected) > - Removed extraneous err_binding_put: label. > - Removed dma_addr += len (Paolo). > - Don't override err on netdev_bind_dmabuf_to_queue failure. > - Rename devmem -> dmabuf (David). > - Add id to dmabuf binding (David/Stan). > - Fix missing xa_destroy bound_rq_list. > - Use queue api to reset bound RX queues (Jakub). > - Update netlink API for rx-queue type (tx/re) (Jakub). > > RFC v3: > - Support multi rx-queue binding > > --- > Documentation/netlink/specs/netdev.yaml | 4 + > include/net/devmem.h| 111 +++ > include/net/netdev_rx_queue.h | 2 + > include/net/netmem.h| 10 + > include/net/page_pool/types.h | 6 + > net/core/Makefile | 2 +- > net/core/dev.c | 3 + > net/core/devmem.c | 252 > net/core/netdev-genl-gen.c | 4 + > net/core/netdev-genl-gen.h | 4 + > net/core/netdev-genl.c | 101 +- > 11 files changed, 496 insertions(+), 3 deletions(-) > create mode 100644 include/net/devmem.h > create mode 100644 net/core/devmem.c > > diff --git a/Documentation/netlink/specs/netdev.yaml > b/Documentation/netlink/specs/netdev.yaml > index 899ac0882a098..d6d7cb01c145c 100644 > --- a/Documentation/netlink/specs/netdev.yaml > +++ b/Documentation/netlink/specs/netdev.yaml > @@ -673,6 +673,10 @@ operations: >
[ANNOUNCE] libdrm 2.4.122
Enrico Weigelt, metux IT consult (6): OpenBSD: fix FTBS on misspelled and missing variables fix FTBS on FreeBSD (or non-Linux in general) freedreno: fix FTBS on non-Linux platforms (unused header) etnaviv: fix FTBS on undefined linux/* headers on non-Linux platforms. ci: upgrade debian container to bookworm ci: upgrade FreeBSD VM to 14.1 Nicolas Caramelli (1): Remove libm in libdrm dependencies Simon Ser (2): Sync headers with drm-next build: bump version to 2.4.122 git tag: libdrm-2.4.122 https://dri.freedesktop.org/libdrm/libdrm-2.4.122.tar.xz SHA256: d9f5079b777dffca9300ccc56b10a93588cdfbc9dde2fae111940dfb6292f251 libdrm-2.4.122.tar.xz SHA512: ea6bac94416d4ba0e9805e142ae62904236bc49f803d4fc10c92968a4df64c818dd42524ad7a4e988062836783a148e27094050bb2754f751a368627f794ad13 libdrm-2.4.122.tar.xz PGP: https://dri.freedesktop.org/libdrm/libdrm-2.4.122.tar.xz.sig
Re: [PATCH v1 2/3] drm/msm/adreno: Add support for X185 GPU
On Mon, Jun 24, 2024 at 03:53:48PM +0200, Konrad Dybcio wrote:
>
>
> On 6/23/24 13:06, Akhil P Oommen wrote:
> > Add support in drm/msm driver for the Adreno X185 gpu found in
> > Snapdragon X1 Elite chipset.
> >
> > Signed-off-by: Akhil P Oommen
> > ---
> >
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 19 +++
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 ++
> > drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h| 5 +
> > 4 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 0e3dfd4c2bc8..168a4bddfaf2 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -830,8 +830,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
> > unsigned int state)
> > */
> > gmu_write(gmu, REG_A6XX_GMU_CM3_CFG, 0x4052);
> > + if (adreno_is_x185(adreno_gpu)) {
> > + chipid = 0x7050001;
>
> What's wrong with using the logic below?
patchid is BITS(7, 0), not (15, 8) in the case of x185. Due to the
changes in the chipid scheme within the a7x family, this is a bit
confusing. I will try to improve here in another series.
>
> > /* NOTE: A730 may also fall in this if-condition with a future GMU fw
> > update. */
> > - if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > + } else if (adreno_is_a7xx(adreno_gpu) && !adreno_is_a730(adreno_gpu)) {
> > /* A7xx GPUs have obfuscated chip IDs. Use constant maj = 7 */
> > chipid = FIELD_PREP(GENMASK(31, 24), 0x7);
> > @@ -1329,9 +1331,18 @@ static int a6xx_gmu_rpmh_arc_votes_init(struct
> > device *dev, u32 *votes,
> > if (!pri_count)
> > return -EINVAL;
> > - sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > - if (IS_ERR(sec))
> > - return PTR_ERR(sec);
> > + /*
> > +* Some targets have a separate gfx mxc rail. So try to read that first
> > and then fall back
> > +* to regular mx rail if it is missing
> > +*/
> > + sec = cmd_db_read_aux_data("gmxc.lvl", &sec_count);
> > + if (PTR_ERR_OR_ZERO(sec) == -EPROBE_DEFER) {
> > + return -EPROBE_DEFER;
> > + } else if (IS_ERR(sec)) {
> > + sec = cmd_db_read_aux_data("mx.lvl", &sec_count);
> > + if (IS_ERR(sec))
> > + return PTR_ERR(sec);
> > + }
>
> I assume GMXC would always be used if present, although please use the
> approach Dmitry suggested
Correct.
-Akhil
>
>
> The rest looks good!
>
> Konrad
Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
Am Mittwoch, dem 26.06.2024 um 09:28 +0200 schrieb Daniel Vetter: > On Mon, Jun 17, 2024 at 07:01:05PM +0200, Tomeu Vizoso wrote: > > Hi Lucas, > > > > Do you have any idea on how not to break userspace if we expose a render > > node? > > So if you get a new chip with an incompatible 3d block, you already have > that issue. And I hope etnaviv userspace can cope. > > Worst case you need to publish a fake extremely_fancy_3d_block to make > sure old mesa never binds against an NPU-only instance. > > Or mesa just doesn't cope, in which case we need a etnaviv-v2-we_are_sorry > drm driver name, or something like that. Mesa doesn't cope right now. Mostly because of the renderonly thing where we magically need to match render devices to otherwise render incapable KMS devices. The way this matching works is that the renderonly code tries to open a screen on a rendernode and if that succeeds we treat it as the matching render device. The core of the issue is that we have no way of specifying which kind of screen we need at that point, i.e. if the screen should have 3D render capabilities or if compute-only or even NN-accel-only would be okay. So we can't fail screen creation if there is no 3D engine, as this would break the teflon case, which needs a screen for the NN accel, but once we successfully create a screen reanderonly might treat the thing as a rendering device. So we are kind of stuck here between breaking one or the other use- case. I'm leaning heavily into the direction of just fixing Mesa, so we can specify the type of screen we need at creation time to avoid the renderonly issue, porting this change as far back as reasonably possible and file old userspace into shit-happens. Regards, Lucas > > > > > Cheers, > > > > Tomeu > > > > On Wed, Jun 12, 2024 at 4:26 PM Tomeu Vizoso wrote: > > > > > > On Mon, May 20, 2024 at 1:19 PM Daniel Stone wrote: > > > > > > > > Hi, > > > > > > > > On Mon, 20 May 2024 at 08:39, Tomeu Vizoso > > > > wrote: > > > > > On Fri, May 10, 2024 at 10:34 AM Lucas Stach > > > > > wrote: > > > > > > Am Mittwoch, dem 24.04.2024 um 08:37 +0200 schrieb Tomeu Vizoso: > > > > > > > If we expose a render node for NPUs without rendering > > > > > > > capabilities, the > > > > > > > userspace stack will offer it to compositors and applications for > > > > > > > rendering, which of course won't work. > > > > > > > > > > > > > > Userspace is probably right in not questioning whether a render > > > > > > > node > > > > > > > might not be capable of supporting rendering, so change it in the > > > > > > > kernel > > > > > > > instead by exposing a /dev/accel node. > > > > > > > > > > > > > > Before we bring the device up we don't know whether it is capable > > > > > > > of > > > > > > > rendering or not (depends on the features of its blocks), so > > > > > > > first try > > > > > > > to probe a rendering node, and if we find out that there is no > > > > > > > rendering > > > > > > > hardware, abort and retry with an accel node. > > > > > > > > > > > > On the other hand we already have precedence of compute only DRM > > > > > > devices exposing a render node: there are AMD GPUs that don't > > > > > > expose a > > > > > > graphics queue and are thus not able to actually render graphics. > > > > > > Mesa > > > > > > already handles this in part via the PIPE_CAP_GRAPHICS and I think > > > > > > we > > > > > > should simply extend this to not offer a EGL display on screens > > > > > > without > > > > > > that capability. > > > > > > > > > > The problem with this is that the compositors I know don't loop over > > > > > /dev/dri files, trying to create EGL screens and moving to the next > > > > > one until they find one that works. > > > > > > > > > > They take the first render node (unless a specific one has been > > > > > configured), and assumes it will be able to render with it. > > > > > > > > > > To me it seems as if userspace expects that /dev/dri/renderD* devices > > > > > can be used for rendering and by breaking this assumption we would be > > > > > breaking existing software. > > > > > > > > Mm, it's sort of backwards from that. Compositors just take a > > > > non-render DRM node for KMS, then ask GBM+EGL to instantiate a GPU > > > > which can work with that. When run in headless mode, we don't take > > > > render nodes directly, but instead just create an EGLDisplay or > > > > VkPhysicalDevice and work backwards to a render node, rather than > > > > selecting a render node and going from there. > > > > > > > > So from that PoV I don't think it's really that harmful. The only > > > > complication is in Mesa, where it would see an etnaviv/amdgpu/... > > > > render node and potentially try to use it as a device. As long as Mesa > > > > can correctly skip, there should be no userspace API implications. > > > > > > > > That being said, I'm not entirely sure what the _benefit_ would be of > > > > exposing a render node for a device which can't be used by any
Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC
On 25/06/2024 15:44, Krzysztof Kozlowski wrote:
On 25/06/2024 11:23, Alexandre Mergnat wrote:
On 21/06/2024 17:00, Krzysztof Kozlowski wrote:
On 19/06/2024 16:46, Alexandre Mergnat wrote:
Add the audio codec sub-device. This sub-device is used to set the
optional voltage values according to the hardware.
The properties are:
- Setup of microphone bias voltage.
- Setup of the speaker pin pull-down.
Also, add the audio power supply property which is dedicated for
the audio codec sub-device.
Signed-off-by: Alexandre Mergnat
---
.../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33
++
1 file changed, 33 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
index 37423c2e0fdf..d95307393e75 100644
--- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
+++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
@@ -37,6 +37,32 @@ properties:
"#interrupt-cells":
const: 2
+ vaud28-supply:
+description: 2.8 volt supply phandle for the audio codec
+
+ audio-codec:
+type: object
Still not much improved. You do not have any resources there, so these
should go to the parent node.
Hi Krzysztof,
vaud28-supply seems to be a mistake that I forward port.
In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the
"audio-codec"
sub-node to the parent node, which for me is the "pmic" (mfd), because the
property is considered as
power-supply.
pwrap {
pmic {
...
audio-codec {
...
Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and
AVDD28 is the input of
PMIC-audio-codec subsystem. Then:
- The property name is wrong and must be change to AVDD28, which is a consumer
(power input), not a
power-supply. => description: 2.8 volt power input for microphones (AU_VIN0,
AU_VIN1, AU_VIN2)
- IMHO, move this property to the next parent (pwrap) isn't consistent. It
should be moved back to
Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4)
into audio-codec
substystem, beside mediatek,micbias0-microvolt
I don't understand why do we talk again about supply. My comment was not
under the supply.
Because your word are:
"
And now you should see how odd it looks. Supplies are part of entire
chip, not subblock, even if they supply dedicated domain within that chip.
That's why I asked to put it in the parent node.
"
My bad, I forgot to link you the old message in my previous answer [1]
[1]
https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b02...@kernel.org/#t
--
Regards,
Alexandre
Re: Time for drm-ci-next?
On Wed, Jun 26, 2024 at 09:32:44AM GMT, Daniel Vetter wrote:
> On Mon, Jun 24, 2024 at 10:25:25AM -0300, Helen Koike wrote:
> >
> >
> > On 24/06/2024 02:34, Vignesh Raman wrote:
> > > Hi,
> > >
> > > On 15/03/24 22:50, Rob Clark wrote:
> > > > Basically, I often find myself needing to merge CI patches on top of
> > > > msm-next in order to run CI, and then after a clean CI run, reset HEAD
> > > > back before the merge and force-push. Which isn't really how things
> > > > should work.
>
> This sounds more like you want an integration tree like drm-tip. Get msm
> branches integrated there, done. Backmerges just for integration testing
> are not a good idea indeed.
Is it fine to get drm/msm{-fixes,-next} into drm-tip?
> What exactly is the issue in backmerging drm-misc-next (well through
> drm-next really)?
drm-misc-next is its own tree with separate cadence, its own bugs and
misfeatures. But probably just picking up drm-next for the tests should
be fine.
> Also if there is an issue, generally we do ad-hoc topic branches.
>
> I'm very very skeptical of boutique trees with tiny focus, we've had that
> before drm-misc, it's a mess. Definitely no enthusiasm for getting back
> to that kind of world.
> -Sima
--
With best wishes
Dmitry
[PATCH 2/2] drm/panic: Restrict graphical logo handling to built-in
When CONFIG_DRM_PANIC=y, but CONFIG_DRM=m:
ld: drivers/gpu/drm/drm_panic.o: in function `drm_panic_setup_logo':
drivers/gpu/drm/drm_panic.c:99: multiple definition of `init_module';
drivers/gpu/drm/drm_drv.o:drivers/gpu/drm/drm_drv.c:1079: first defined here
Fix this by restricting the graphical logo handling and its
device_initcall() to the built-in case. Logos are freed during late
kernel initialization, so they are no longer available at module load
time anyway.
Fixes: 294bbd1f2697ff28 ("drm/panic: Add support for drawing a monochrome
graphical logo")
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202406261341.gysblpn1-...@intel.com/
Signed-off-by: Geert Uytterhoeven
---
drivers/gpu/drm/drm_panic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 67f78b5a76b61e3d..948aed00595eb6dd 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -91,7 +91,7 @@ static const struct drm_panic_line logo_ascii[] = {
PANIC_LINE(" \\___)=(___/"),
};
-#ifdef CONFIG_LOGO
+#if defined(CONFIG_LOGO) && !defined(MODULE)
static const struct linux_logo *logo_mono;
static int drm_panic_setup_logo(void)
--
2.34.1
[PATCH 0/2] drm/panic: Miscellaneous fixes
Hi all,
Here are two more fixes for the DRM panic code.
Thanks for your comments!
Geert Uytterhoeven (2):
drm/panic: Do not select DRM_KMS_HELPER
drm/panic: Restrict graphical logo handling to built-in
drivers/gpu/drm/Kconfig | 1 -
drivers/gpu/drm/drm_panic.c | 3 +--
2 files changed, 1 insertion(+), 3 deletions(-)
--
2.34.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 1/2] drm/panic: Do not select DRM_KMS_HELPER
DRM core code cannot call into DRM helper code, as this would lead to
circular references in the modular case. Hence drop the selection of
DRM_KMS_HELPER. It was unused anyway, as v10 switched from using
the DRM format helpers to its own color format conversion, cfr. commit
9544309775c334c9 ("drm/panic: Add support for color format
conversion")).
Remove the unneeded include of .
Fixes: bf9fb17c6672868d ("drm/panic: Add a drm panic handler")
Signed-off-by: Geert Uytterhoeven
---
drivers/gpu/drm/Kconfig | 1 -
drivers/gpu/drm/drm_panic.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b903a2c0b5e8f95c..ce9bf2b6e9d332d4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -108,7 +108,6 @@ config DRM_KMS_HELPER
config DRM_PANIC
bool "Display a user-friendly message when a kernel panic occurs"
depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE)
- select DRM_KMS_HELPER
select FONT_SUPPORT
help
Enable a drm panic handler, which will display a user-friendly message
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 8d2eded1fd19ff6c..67f78b5a76b61e3d 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -20,7 +20,6 @@
#include
#include
-#include
#include
#include
#include
--
2.34.1
Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
On Wed, Jun 26, 2024 at 09:26:40AM GMT, Daniel Vetter wrote: > On Thu, May 09, 2024 at 05:41:18PM +0300, Oded Gabbay wrote: > > On Thu, May 09, 2024 at 03:53:01PM +0200, Tomeu Vizoso wrote: > > > Oded, Dave, > > > > > > Do you have an opinion on this? > > > > > > Thanks, > > > > > > Tomeu > > Hi Tomeu, > > > > Sorry for not replying earlier, I was down with Covid (again...). > > > > To your question, I don't have an objection to what you are > > suggesting. My personal view of accel is that it is an integral part of > > DRM and therefore, if there is an *existing* drm driver that wants to > > create an accel node, I'm not against it. > > Yeah, there's a continum from "clearly 3d gpu" to "compute AI > accelerator", with everything possible in-between shipping somewhere. > Collaboration is the important part, hair-splitting on where exactly the > driver should be is kinda secondary. I mean beyond "don't put a pure 3d > driver into accel or vice versa" of course :-) > > > There is the question of why you want to expose an accel node, and > > here I would like to hear Dave's and Sima's opinion on your suggested > > solution as it may affect the direction of other drm drivers. > > So existing userspace that blindly assumes that any render node will give > it useful 3d acceleration, then that's broken already. > > - kernel with new driver support but old mesa without that driver already > gives you that, even for a pure 3d chip. > > - intel (and I think also amd) have pure compute chips without 3d, so this > issue already exists > > Same for the other directions, 3d gpus have variable amounts of compute > chips nowadays. > > That leaves imo just the pragmatic choice, and if we need to complicate > the init flow of the kernel driver just for a different charnode major, > then I don't really see the point. > > And if we do see the point in this, I think the right approach would be if > we split the init flow further into allocating the drm_device, and then in > a 2nd step either allocate the accel or render uapi stuff as needed. The > DRIVER_FOO flags just aren't super flexible for this kinda of stuff and > have a bit a midlayer taste to them. Being able to defer render allocation would be extremely useful for MSM too as it's not currently possible to mask the driver_features during drm_dev_init() -- With best wishes Dmitry
Re: [PATCH] drm/msm/adreno: fix a7xx gpu init
On Wed, Jun 26, 2024 at 09:53:16AM GMT, Neil Armstrong wrote:
> The gpulist has twice the a6xx gpulist, replace the second one
> with the a7xx gpulist.
>
> Solves:
> msm_dpu ae01000.display-controller: Unknown GPU revision: 7.3.0.1
> msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.10.1
> msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.20.1
>
> on SM8450, SM8550 & SM8560.
>
> Fixes: 8ed322f632a9 ("drm/msm/adreno: Split up giant device table")
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
[PATCH RESEND] drm: bridge: dw-mipi-dsi: Allow sync-pulses to override the burst vid-mode
From: Heiko Stuebner
The state right now is that if the panel has the burst-mode flag it
will take precedence over the sync-pulses mode.
While sync-pulses are only relevant for the video-mode, the burst-mode
flag affects both the video-mode as well as the calculated lane_mbps.
Looking at drivers like the nwl-dsi [0] it only enables burst mode when
the panel's flags do not contain the sync_pulse flag.
So handle things similar for dw-dsi in that it selects the video-mode
with sync-pulses if that flag is set and only after that, checks for
the burst-mode. So panels selecting a combination of both burst and
sync-pulses get the sync-pulse mode.
The case this fixes can be found on the ltk050h3148w . It does need the
lane-rate to be calculated according to burst formulas [1], but without
sync-pulses we see the output shifted around 20 pixels to the right,
meaning that the last 20 pixels from each line appear at the start of
the next display line.
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/bridge/nwl-dsi.c#n301
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6c9dbee84cd005bed5f9d07b3a2797ae6414b435
Fixes: 93e82bb4de01 ("drm/bridge: synopsys: dw-mipi-dsi: Fix hcomponent lbcc
for burst mode")
Signed-off-by: Heiko Stuebner
---
resend, because I messed up and somehow forgot to include _all_
mailing lists.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 824fb3c65742e..28dd858a751bd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -605,10 +605,10 @@ static void dw_mipi_dsi_video_mode_config(struct
dw_mipi_dsi *dsi)
*/
val = ENABLE_LOW_POWER;
- if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
- val |= VID_MODE_TYPE_BURST;
- else if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
+ if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
val |= VID_MODE_TYPE_NON_BURST_SYNC_PULSES;
+ else if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
+ val |= VID_MODE_TYPE_BURST;
else
val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
--
2.39.2
[PATCH 0/3] Fix mst daisy chain light up issue after resume
Under DP mst daisy chain configuration, unplug one chained monitor during suspend and then resume, observe left connected monitor not light up. After analyzing, seems it's due to changing dpcd DP_MSTM_CTRL value after LT during reume. We used to defer handling UP request by disabling DP_UP_REQ_EN at the begining of resume process to avoid some problems. However, it turns out that it will cause problem on the hub if we change DP_UP_REQ_EN after LT. This series is trying to solve the problem by another way that we don't explicitly disable DP_UP_REQ_EN at source side. Instead, source should ignore the CSN event before source completeting topology probing during resume. Wayne Lin (3): drm/dp_mst: Fix all mstb marked as not probed after suspend/resume drm/dp_mst: Skip CSN if topology probing is not done yet drm/amd/display: Solve mst monitors blank out problem after resume drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++- drivers/gpu/drm/display/drm_dp_mst_topology.c | 15 +-- 2 files changed, 15 insertions(+), 3 deletions(-) -- 2.37.3
[PATCH 1/3] drm/dp_mst: Fix all mstb marked as not probed after suspend/resume
[Why]
After supend/resume, with topology unchanged, observe that
link_address_sent of all mstb are marked as false even the topology probing
is done without any error.
It is caused by wrongly also include "ret == 0" case as a probing failure
case.
[How]
Remove inappropriate checking conditions.
Cc: Lyude Paul
Cc: Harry Wentland
Cc: Jani Nikula
Cc: Imre Deak
Cc: Daniel Vetter
Cc: sta...@vger.kernel.org
Fixes: 37dfdc55ffeb ("drm/dp_mst: Cleanup drm_dp_send_link_address() a bit")
Signed-off-by: Wayne Lin
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7f8e1cfbe19d..68831f4e502a 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2929,7 +2929,7 @@ static int drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
/* FIXME: Actually do some real error handling here */
ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
- if (ret <= 0) {
+ if (ret < 0) {
drm_err(mgr->dev, "Sending link address failed with %d\n", ret);
goto out;
}
@@ -2981,7 +2981,7 @@ static int drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
mutex_unlock(&mgr->lock);
out:
- if (ret <= 0)
+ if (ret < 0)
mstb->link_address_sent = false;
kfree(txmsg);
return ret < 0 ? ret : changed;
--
2.37.3
[PATCH 2/3] drm/dp_mst: Skip CSN if topology probing is not done yet
[Why]
During resume, observe that we receive CSN event before we start topology
probing. Handling CSN at this moment based on uncertain topology is
unnecessary.
[How]
Add checking condition in drm_dp_mst_handle_up_req() to skip handling CSN
if the topology is yet to be probed.
Cc: Lyude Paul
Cc: Harry Wentland
Cc: Jani Nikula
Cc: Imre Deak
Cc: Daniel Vetter
Cc: sta...@vger.kernel.org
Signed-off-by: Wayne Lin
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 68831f4e502a..fc2ceae61db2 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4069,6 +4069,7 @@ static int drm_dp_mst_handle_up_req(struct
drm_dp_mst_topology_mgr *mgr)
if (up_req->msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
const struct drm_dp_connection_status_notify *conn_stat =
&up_req->msg.u.conn_stat;
+ bool handle_csn;
drm_dbg_kms(mgr->dev, "Got CSN: pn: %d ldps:%d ddps: %d mcs: %d
ip: %d pdt: %d\n",
conn_stat->port_number,
@@ -4077,6 +4078,16 @@ static int drm_dp_mst_handle_up_req(struct
drm_dp_mst_topology_mgr *mgr)
conn_stat->message_capability_status,
conn_stat->input_port,
conn_stat->peer_device_type);
+
+ mutex_lock(&mgr->probe_lock);
+ handle_csn = mgr->mst_primary->link_address_sent;
+ mutex_unlock(&mgr->probe_lock);
+
+ if (!handle_csn) {
+ drm_dbg_kms(mgr->dev, "Got CSN before finish topology
probing. Skip it.");
+ kfree(up_req);
+ goto out;
+ }
} else if (up_req->msg.req_type == DP_RESOURCE_STATUS_NOTIFY) {
const struct drm_dp_resource_status_notify *res_stat =
&up_req->msg.u.resource_stat;
--
2.37.3
[PATCH 3/3] drm/amd/display: Solve mst monitors blank out problem after resume
[Why]
In dm resume, we firstly restore dc state and do the mst resume for
topology probing thereafter. If we change dpcd DP_MSTM_CTRL value
after LT in mst reume, it will cause light up problem on the hub.
[How]
Revert the commit 202dc359adda ("drm/amd/display: Defer handling mst
up request in resume"). And adjust the reason to trigger
dc_link_detect by DETECT_REASON_RESUMEFROMS3S4.
Fixes: 202dc359adda ("drm/amd/display: Defer handling mst up request in resume")
Signed-off-by: Wayne Lin
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c64cc2378a94..b01452eb0981 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2569,6 +2569,7 @@ static void resume_mst_branch_status(struct
drm_dp_mst_topology_mgr *mgr)
ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
DP_MST_EN |
+DP_UP_REQ_EN |
DP_UPSTREAM_IS_SRC);
if (ret < 0) {
drm_dbg_kms(mgr->dev, "mst write failed - undocked during
suspend?\n");
@@ -3171,7 +3172,7 @@ static int dm_resume(void *handle)
} else {
mutex_lock(&dm->dc_lock);
dc_exit_ips_for_hw_access(dm->dc);
- dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD);
+ dc_link_detect(aconnector->dc_link,
DETECT_REASON_RESUMEFROMS3S4);
mutex_unlock(&dm->dc_lock);
}
--
2.37.3
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
Hi, On Wed, Jun 19, 2024 at 12:07:48PM GMT, Tomi Valkeinen wrote: > From: Tomi Valkeinen > > When a bridge driver uses devm_mipi_dsi_device_register_full() or > devm_mipi_dsi_attach(), the resource management is moved to devres, > which releases the resource automatically when the bridge driver is > unbound. > > However, if the DSI host goes away first, the host unregistration code > will automatically detach and unregister any DSI peripherals, without > notifying the devres about it. So when the bridge driver later is > unbound, the resources are released a second time, leading to crash. That's super surprising. mipi_dsi_device_unregister calls device_unregister, which calls device_del, which in turn calls devres_release_all. If that doesn't work like that, then it's what needs to be fixed, and not worked around in the MIPI-DSI bus. Maxime signature.asc Description: PGP signature
Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel
On Tue, Jun 18, 2024 at 02:05:50PM GMT, Neil Armstrong wrote: > On 18/06/2024 13:13, Conor Dooley wrote: > > On Tue, Jun 18, 2024 at 11:04:09AM +0200, Maxime Ripard wrote: > > > Hi Conor, > > > > > > Sorry, I missed the news of you becoming a DT maintainer, so most of my > > > previous points are obviously bogus. And congrats :) > > > > I've been doing it for over a year, so news travels to some corners slowly > > I guess. I'm not just being a pest in dozens of subsystems for fun! > > > > > On Thu, Jun 06, 2024 at 12:51:33PM GMT, Conor Dooley wrote: > > > > On Thu, Jun 06, 2024 at 01:23:03PM +0200, Maxime Ripard wrote: > > > > > On Thu, Jun 06, 2024 at 11:37:31AM GMT, Neil Armstrong wrote: > > > > > > On 06/06/2024 11:32, Maxime Ripard wrote: > > > > > > > On Fri, May 31, 2024 at 09:12:14AM GMT, Ryan Walklin wrote: > > > > > > > > The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display used in > > > > > > > > a > > > > > > > > number of handheld gaming devices made by Anbernic. By > > > > > > > > consensus a > > > > > > > > vendor prefix is not provided as the panel OEM is unknown. > > > > > > > > > > > > > > Where has this consensus been found? > > > > > > > > > > > > > > I had a look at the previous discussions, and I can't find any > > > > > > > consensus > > > > > > > being reached there. And for that kind of thing, having the ack or > > > > > > > review of any of the DT maintainers would have been great. > > > > > > > > > > > > There was a consensus with Conor, this is why he acked v2, see > > > > > > https://lore.kernel.org/all/20240525-velvet-citable-a45dd06847a7@spud/ > > > > > > > > > > It's probably a matter of semantics here, but if it's with only one > > > > > person, it's not a consensus but an agreement. > > > > > > > > > > > ``` > > > > > > I think if we genuinely do not know what the vendor is then we just > > > > > > don't have a prefix. > > > > > > ``` > > > > > > > > > > And even then, I don't interpret Conor's statement as a formal > > > > > agreement > > > > > but rather an acknowledgment of the issue. > > > > > > > > I mean, I specifically left an r-b below that line in v2: > > > > https://lore.kernel.org/all/20240530-satchel-playgroup-e8aa6937b8b9@spud/ > > > > > > > > I'm not a displays guy, so my sources were limited to what I could find > > > > from search engines, but I spent some time looking for an actual vendor > > > > of the panel and could not. All I found was various listings on places > > > > like AliExpress that did not mention an manufacturer. I'd rather not > > > > invent a vendor because we could not find the actual vendor of the > > > > panel & it seemed rather unreasonable to block support for the device > > > > on the basis of not being able to figure out the vendor. If you, as > > > > someone knowledgeable on displays, can figure the vendor out, then > > > > yeah we should definitely add it. > > > > > > It's still a bit surprising to me. We've merged[1][2][3][4], and are still > > > merging[5], panels from this particular vendor that have no clearly > > > identified OEMs. Just like any other panel, really. We almost *never* > > > have the actual OEM, we just go with whatever is the easiest to identify > > > it. > > > > It wasn't (isn't?) clear to me that Abernic is even the vendor of the > > panel, just that it works for their devices. If there's an established > > policy here of making up vendors for these panels, then sure, override > > me and use them as the prefix. > > > > > Plus, if there ever is another WL-355608-A8 part from a completely > > > unrelated vendor, then you'll have a naming clash with no clear > > > indication about which is which. > > Not sure we can say there's an established policy ongoing here, we try to > use the marking we find on the panel when possible and when not possible > we use the vendor + name of the device in last ressort. So pretty much what I was asking for? We're getting fairly late into the release cycle and I'd like to get it fixed before the release. Can you send a patch to address it please? Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically.
The hardware for ast is entirely static and ast uses
drmm_mode_config_init() for auto-cleaning up the modesetting pipeline.
Using drmm_encoder_init() seems like a bit of wasted resources for no gain.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support
On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov
wrote:
> On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
>> Add support for the Lincoln LCD197 1080x1920 DSI panel.
>>
>> Signed-off-by: Jerome Brunet
>> ---
>> drivers/gpu/drm/panel/Kconfig| 11 +
>> drivers/gpu/drm/panel/Makefile | 1 +
>> drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++
>> 3 files changed, 345 insertions(+)
>> create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
>>
>
> [...]
>
>> +
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
>
> - Please use lowercase hex instead
> - Please consider switching to _multi() functions.
Could you be a bit more specific about these '_multi' function ?
I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
Maybe I'm not looking in the right place.
>
>
>> +usleep_range(200, 300);
>
> This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
I don't really understand why I would need something else to just sleep
? Could you add some context please ?
Isn't 'msm_' usually something Qcom specific ?
>
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
>> +mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
>> +mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
>> +
>> +err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
>> +if (err < 0) {
>> +dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
>> +goto poweroff;
>> +}
>> +msleep(120);
>> +
>> +err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id,
>> 3);
>
> This probably needs new _multi helper too.
>
>> +if (err < 0) {
>> +dev_err(panel->dev, "Failed to read display id: %d\n", err);
>> +} else {
>> +dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
>> +display_id[0], display_id[1], display_id[2]);
>> +}
>> +
>> +lcd->prepared = true;
>
> Should not be required anymore.
The whole driver is heavily inspired by what is already in
drivers/gpu/drm/panel/ and a lot are doing something similar.
Maybe there has been a change since then and the existing have been
reworked yet. Would you mind pointing me that change if that is
the case ?
>
>> +
>> +return 0;
>> +
>> +poweroff:
>> +gpiod_set_value_cansleep(lcd->enable_gpio, 0);
>> +gpiod_set_value_cansleep(lcd->reset_gpio, 1);
>> +regulator_disable(lcd->supply);
>> +
>> +return err;
>> +}
>> +
>
>> +
>> +static const struct drm_display_mode default_mode = {
>> +.clock = 154002,
>> +.hdisplay = 1080,
>> +.hsync_start = 1080 + 20,
>> +.hsync_end = 1080 + 20 + 6,
>> +.htotal = 1080 + 204,
>> +.vdisplay = 1920,
>> +.vsync_start = 1920 + 4,
>> +.vsync_end = 1920 + 4 + 4,
>> +.vtotal = 1920 + 79,
>> +.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>> +};
>> +
>> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
>> + struct drm_connector *connector)
>> +{
>> +struct drm_display_mode *mode;
>> +
>> +mode = drm_mode_duplicate(connector->dev, &default_mode);
>> +if (!mode) {
>> +dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
>> +default_mode.hdisplay, default_mode.vdisplay,
>> +drm_mode_vrefresh(&default_mode));
>> +return -ENOMEM;
>> +}
>> +
>> +drm_mode_set_name(mode);
>> +drm_mode_probed_add(connector, mode);
>> +connector->display_info.width_mm = 79;
>> +connector->display_info.height_mm = 125;
>
> drm_connector_helper_get_modes_fixed()
Thanks for the hint
>
>> +
>> +return 1;
>> +}
>> +
>
>
>> +
>> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
>> +{
>> +struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
>> +
>> +drm_panel_disable(&lcd->panel);
>> +drm_panel_unprepare(&lcd->panel);
>> +}
>
> I think the agreement was that there should be no need for the panel's
> shutdown, the DRM driver should shutdown the panel.
I'm happy to drop that if there is such agreement. Again, most panel
drivers do implement that callback so I just did the same.
Could you point me to this 'agreement' please, so I can get a better
understanding of it ?
--
Jerome
Re: [PATCH] drm/bridge: display-connector: Fix atomic_get_input_bus_fmt hook
On 25/06/24 19:46, Neil Armstrong wrote:
> On 25/06/2024 11:50, Aradhya Bhatia wrote:
>> The display-connector acts as a pass-through bridge. To truly reflect
>> that, this bridge should accept the same input format, as it expects to
>> output. That in turn should be the same as what the preceding bridge has
>> to output.
>>
>> While the get_output_fmt hook does exactly that by calling the same hook
>> of the previous bridge, the get_input_fmt hook should simply propagate
>> the expected output format as its required input format.
>>
>> Let's say bridge(n) converts YUV bus format to RGB before transmitting
>> the video signals. B is supposed to be RGB and A is YUV. The
>> get_input_fmt hook of bridge(n) should receive RGB as its expected
>> output format for it to select YUV as its required input format.
>>
>> Moreover, since the display-connector is a pass-through bridge, X and Y
>> should both be RGB as well.
>>
>> +-+ +-+
>> A | | B X | | Y
>> --->| Bridge(n) +---> --->| Display +--->
>> | | | Connector |
>> | | | |
>> +-+ +-+
>>
>> But that's not what's happening at the moment.
>>
>> The core will call get_output_fmt hook of display-connector, which will
>> call the same hook of bridge(n). Y will get set to RGB because B is RGB.
>>
>> Now the core will call get_input_fmt hook of display-connector with Y =
>> RGB as its expected output format. This hook will in turn call the
>> get_input_fmt hook of bridge(n), with expected output as RGB. This hook
>> will then return YUV as its required input format, which will set X =
>> YUV.
>>
>> This is where things get off the track. The core will then call
>> bridge(n)'s get_input_fmt hook but this time the expected output will
>> have changed to X = YUV, instead of what ideally should have been X =
>> RGB. We don't know how bridge(n)'s input format requirement will change
>> now that its expected output format isn't RGB but YUV.
>>
>> Ideally, formats Y, X, B need to be the same and the get_input_fmt hook
>> for bridge(n) should be called with these as its expected output format.
>> Calling that hook twice can potentially change the expected output
>> format - which can then change the required input format again, or it
>> might just throw an -ENOTSUPP error.
>>
>> While many bridges don't utilize these APIs, or in a lot of cases A and
>> B are same anyway, it is not the biggest problem, but one that should be
>> fixed anyway.
>>
>> Fix this.
>>
>> Fixes: 7cd70656d128 ("drm/bridge: display-connector: implement bus
>> fmts callbacks")
>> Signed-off-by: Aradhya Bhatia
>> ---
>> drivers/gpu/drm/bridge/display-connector.c | 40 +-
>> 1 file changed, 1 insertion(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/display-connector.c
>> b/drivers/gpu/drm/bridge/display-connector.c
>> index ab8e00baf3f1..eebf1fbcdd23 100644
>> --- a/drivers/gpu/drm/bridge/display-connector.c
>> +++ b/drivers/gpu/drm/bridge/display-connector.c
>> @@ -131,50 +131,12 @@ static u32
>> *display_connector_get_output_bus_fmts(struct drm_bridge *bridge,
>> num_output_fmts);
>> }
>> -/*
>> - * Since this bridge is tied to the connector, it acts like a
>> passthrough,
>> - * so concerning the input bus formats, either pass the bus formats
>> from the
>> - * previous bridge or MEDIA_BUS_FMT_FIXED (like
>> select_bus_fmt_recursive())
>> - * when atomic_get_input_bus_fmts is not supported.
>> - * This supports negotiation if the bridge chain has all bits in place.
>> - */
>> -static u32 *display_connector_get_input_bus_fmts(struct drm_bridge
>> *bridge,
>> - struct drm_bridge_state *bridge_state,
>> - struct drm_crtc_state *crtc_state,
>> - struct drm_connector_state *conn_state,
>> - u32 output_fmt,
>> - unsigned int *num_input_fmts)
>> -{
>> - struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge);
>> - struct drm_bridge_state *prev_bridge_state;
>> -
>> - if (!prev_bridge ||
>> !prev_bridge->funcs->atomic_get_input_bus_fmts) {
>> - u32 *in_bus_fmts;
>> -
>> - *num_input_fmts = 1;
>> - in_bus_fmts = kmalloc(sizeof(*in_bus_fmts), GFP_KERNEL);
>> - if (!in_bus_fmts)
>> - return NULL;
>> -
>> - in_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
>> -
>> - return in_bus_fmts;
>> - }
>> -
>> - prev_bridge_state =
>> drm_atomic_get_new_bridge_state(crtc_state->state,
>> - prev_bridge);
>> -
>> - return prev_bridge->funcs->atomic_get_input_bus_fmts(prev_bridge,
>> prev_bridge_state,
>> - crtc_state, conn_state, output_fmt,
>> - num_input_fmts);
>> -}
>> -
>> static const struct dr
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
> Hi
>
> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> > On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> > >
> > > Signed-off-by: Thomas Zimmermann
> > > ---
> > > drivers/gpu/drm/ast/ast_mode.c | 45 ++
> > > 1 file changed, 40 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > b/drivers/gpu/drm/ast/ast_mode.c
> > > index 6695af70768f..2fd9c78eab73 100644
> > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > @@ -45,7 +45,6 @@
> > > #include
> > > #include
> > > #include
> > > -#include
> > > #include "ast_ddc.h"
> > > #include "ast_drv.h"
> > > @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> > > return 0;
> > > }
> > > +/*
> > > + * VGA Encoder
> > > + */
> > > +
> > > +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> > > + .destroy = drm_encoder_cleanup,
> > > +};
> > > +
> > > /*
> > >* VGA Connector
> > >*/
> > > @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> > > *ast)
> > > struct drm_connector *connector = &ast->output.vga.connector;
> > > int ret;
> > > - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> > > + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> > > +DRM_MODE_ENCODER_DAC, NULL);
> > What about using drmm_encoder_init() instead? It will call
> > drm_encoder_cleanup automatically.
>
> IIRC the original use case for the drmm_encoder_*() funcs was to solve
> problems with the clean-up order if the encoder was added dynamically. The
> hardware for ast is entirely static and ast uses drmm_mode_config_init() for
> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
--
With best wishes
Dmitry
Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel
Hi Maxime, On Wed, 26 Jun 2024, at 8:56 PM, Maxime Ripard wrote: > We're getting fairly late into the release cycle and I'd like to get it > fixed before the release. Can you send a patch to address it please? Sure, happy to. So to confirm add 'anbernic' to the vendor binding list and 'anbernic,wl-355608-a8' as the panel compatible? Regards, Ryan
Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support
On 26/06/2024 11:02, Jerome Brunet wrote:
On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov
wrote:
On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
Add support for the Lincoln LCD197 1080x1920 DSI panel.
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/panel/Kconfig| 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++
3 files changed, 345 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
[...]
+
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
- Please use lowercase hex instead
- Please consider switching to _multi() functions.
Could you be a bit more specific about these '_multi' function ?
I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
Maybe I'm not looking in the right place.
+ usleep_range(200, 300);
This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
I don't really understand why I would need something else to just sleep
? Could you add some context please ?
Isn't 'msm_' usually something Qcom specific ?
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
+ mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
+
+ err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
+ if (err < 0) {
+ dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+ goto poweroff;
+ }
+ msleep(120);
+
+ err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id,
3);
This probably needs new _multi helper too.
+ if (err < 0) {
+ dev_err(panel->dev, "Failed to read display id: %d\n", err);
+ } else {
+ dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
+ display_id[0], display_id[1], display_id[2]);
+ }
+
+ lcd->prepared = true;
Should not be required anymore.
The whole driver is heavily inspired by what is already in
drivers/gpu/drm/panel/ and a lot are doing something similar.
Maybe there has been a change since then and the existing have been
reworked yet. Would you mind pointing me that change if that is
the case ?
+
+ return 0;
+
+poweroff:
+ gpiod_set_value_cansleep(lcd->enable_gpio, 0);
+ gpiod_set_value_cansleep(lcd->reset_gpio, 1);
+ regulator_disable(lcd->supply);
+
+ return err;
+}
+
+
+static const struct drm_display_mode default_mode = {
+ .clock = 154002,
+ .hdisplay = 1080,
+ .hsync_start = 1080 + 20,
+ .hsync_end = 1080 + 20 + 6,
+ .htotal = 1080 + 204,
+ .vdisplay = 1920,
+ .vsync_start = 1920 + 4,
+ .vsync_end = 1920 + 4 + 4,
+ .vtotal = 1920 + 79,
+ .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(connector->dev, &default_mode);
+ if (!mode) {
+ dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+ default_mode.hdisplay, default_mode.vdisplay,
+ drm_mode_vrefresh(&default_mode));
+ return -ENOMEM;
+ }
+
+ drm_mode_set_name(mode);
+ drm_mode_probed_add(connector, mode);
+ connector->display_info.width_mm = 79;
+ connector->display_info.height_mm = 125;
drm_connector_helper_get_modes_fixed()
Thanks for the hint
+
+ return 1;
+}
+
+
+static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+ struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
+
+ drm_panel_disable(&lcd->panel);
+ drm_panel_unprepare(&lcd->panel);
+}
I think the agreement was that there should be no need for the panel's
shutdown, the DRM driver should shutdown the panel.
I'm happy to drop that if there is such agreement. Again, most panel
drivers do implement that callback so I just did the same.
Could you point me to this 'agreement' please, so I can get a better
understanding of it ?
please rebase on linux-next or drm-misc-next and use the new introduced macros
dmitry pointed out.
Neil
Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC
On 26/06/2024 10:30, Alexandre Mergnat wrote:
>
>
> On 25/06/2024 15:44, Krzysztof Kozlowski wrote:
>> On 25/06/2024 11:23, Alexandre Mergnat wrote:
>>>
>>>
>>> On 21/06/2024 17:00, Krzysztof Kozlowski wrote:
On 19/06/2024 16:46, Alexandre Mergnat wrote:
> Add the audio codec sub-device. This sub-device is used to set the
> optional voltage values according to the hardware.
> The properties are:
> - Setup of microphone bias voltage.
> - Setup of the speaker pin pull-down.
>
> Also, add the audio power supply property which is dedicated for
> the audio codec sub-device.
>
> Signed-off-by: Alexandre Mergnat
> ---
>.../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33
> ++
>1 file changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> index 37423c2e0fdf..d95307393e75 100644
> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> @@ -37,6 +37,32 @@ properties:
> "#interrupt-cells":
>const: 2
>
> + vaud28-supply:
> +description: 2.8 volt supply phandle for the audio codec
> +
> + audio-codec:
> +type: object
Still not much improved. You do not have any resources there, so these
should go to the parent node.
>>>
>>> Hi Krzysztof,
>>>
>>> vaud28-supply seems to be a mistake that I forward port.
>>> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply
>>> from the "audio-codec"
>>> sub-node to the parent node, which for me is the "pmic" (mfd), because the
>>> property is considered as
>>> power-supply.
>>>
>>> pwrap {
>>> pmic {
>>> ...
>>> audio-codec {
>>> ...
>>>
>>> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and
>>> AVDD28 is the input of
>>> PMIC-audio-codec subsystem. Then:
>>> - The property name is wrong and must be change to AVDD28, which is a
>>> consumer (power input), not a
>>> power-supply. => description: 2.8 volt power input for microphones
>>> (AU_VIN0, AU_VIN1, AU_VIN2)
>>> - IMHO, move this property to the next parent (pwrap) isn't consistent. It
>>> should be moved back to
>>> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4)
>>> into audio-codec
>>> substystem, beside mediatek,micbias0-microvolt
>>
>> I don't understand why do we talk again about supply. My comment was not
>> under the supply.
>
> Because your word are:
> "
> And now you should see how odd it looks. Supplies are part of entire
> chip, not subblock, even if they supply dedicated domain within that chip.
>
> That's why I asked to put it in the parent node.
> "
>
> My bad, I forgot to link you the old message in my previous answer [1]
>
> [1]
> https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b02...@kernel.org/#t
And you implemented this, so why do we talk again about it? It is
already solved, isn't it? Since previous version?
Best regards,
Krzysztof
Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC
On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> + >> + audio-codec: >> +type: object > > Still not much improved. You do not have any resources there, so these > should go to the parent node. Just to clarify: comment is about audio-codec. I meant what's inside audio-codec. "Parent node" -> parent of audio-codec, so the device node. I guess this was inaccurate. Best regards, Krzysztof
Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support
On Wed, Jun 26, 2024 at 11:02:25AM GMT, Jerome Brunet wrote:
> On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov
> wrote:
>
> > On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
> >> Add support for the Lincoln LCD197 1080x1920 DSI panel.
> >>
> >> Signed-off-by: Jerome Brunet
> >> ---
> >> drivers/gpu/drm/panel/Kconfig| 11 +
> >> drivers/gpu/drm/panel/Makefile | 1 +
> >> drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++
> >> 3 files changed, 345 insertions(+)
> >> create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
> >>
> >
> > [...]
> >
> >> +
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
> >
> > - Please use lowercase hex instead
> > - Please consider switching to _multi() functions.
>
> Could you be a bit more specific about these '_multi' function ?
> I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
>
> Maybe I'm not looking in the right place.
What is your baseline? Please see commits 966e397e4f60 ("drm/mipi-dsi:
Introduce mipi_dsi_*_write_seq_multi()") and f79d6d28d8fe
("drm/mipi-dsi: wrap more functions for streamline handling") (and
66055636a146 ("drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep") as
it fixes a mistake in those two).
>
> >
> >
> >> + usleep_range(200, 300);
> >
> > This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
>
> I don't really understand why I would need something else to just sleep
> ? Could you add some context please ?
>
> Isn't 'msm_' usually something Qcom specific ?
Yes, mipi_dsi_usleep_range(). Mea culpa.
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
> >> + mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
> >> +
> >> + err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
> >> + if (err < 0) {
> >> + dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> >> + goto poweroff;
> >> + }
> >> + msleep(120);
> >> +
> >> + err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id,
> >> 3);
> >
> > This probably needs new _multi helper too.
> >
> >> + if (err < 0) {
> >> + dev_err(panel->dev, "Failed to read display id: %d\n", err);
> >> + } else {
> >> + dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
> >> + display_id[0], display_id[1], display_id[2]);
> >> + }
> >> +
> >> + lcd->prepared = true;
> >
> > Should not be required anymore.
>
> The whole driver is heavily inspired by what is already in
> drivers/gpu/drm/panel/ and a lot are doing something similar.
>
> Maybe there has been a change since then and the existing have been
> reworked yet. Would you mind pointing me that change if that is
> the case ?
See d2aacaf07395 ("drm/panel: Check for already prepared/enabled in
drm_panel")
>
> >
> >> +
> >> + return 0;
> >> +
> >> +poweroff:
> >> + gpiod_set_value_cansleep(lcd->enable_gpio, 0);
> >> + gpiod_set_value_cansleep(lcd->reset_gpio, 1);
> >> + regulator_disable(lcd->supply);
> >> +
> >> + return err;
> >> +}
> >> +
> >
> >> +
> >> +static const struct drm_display_mode default_mode = {
> >> + .clock = 154002,
> >> + .hdisplay = 1080,
> >> + .hsync_start = 1080 + 20,
> >> + .hsync_end = 1080 + 20 + 6,
> >> + .htotal = 1080 + 204,
> >> + .vdisplay = 1920,
> >> + .vsync_start = 1920 + 4,
> >> + .vsync_end = 1920 + 4 + 4,
> >> + .vtotal = 1920 + 79,
> >> + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> >> +};
> >> +
> >> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
> >> +struct drm_connector *connector)
> >> +{
> >> + struct drm_display_mode *mode;
> >> +
> >> + mode = drm_mode_duplicate(connector->dev, &default_mode);
> >> + if (!mode) {
> >> + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> >> + default_mode.hdisplay, default_mode.vdisplay,
> >> + drm_mode_vrefresh(&default_mode));
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + drm_mode_set_name(mode);
> >> + drm_mode_probed_add(connector, mode);
> >> + connector->display_info.width_mm = 79;
> >> + connector->display_info.height_mm = 125;
> >
> > drm_connector_helper_get_modes_fixed()
>
> Thanks for the hint
>
> >
> >> +
> >> + return 1;
> >> +}
> >> +
> >
> >
> >> +
> >> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
> >> +{
> >> + struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
> >> +
> >> + drm_panel_disable(&lcd->panel);
> >> + drm_panel_unprepare(&lcd->panel);
> >> +}
> >
> > I think the agreement was that there should be no need for the panel's
> > shutdown, the DRM driver should shutdown the panel.
>
> I'm ha
Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel
On 26/06/2024 11:10, Ryan Walklin wrote: Hi Maxime, On Wed, 26 Jun 2024, at 8:56 PM, Maxime Ripard wrote: We're getting fairly late into the release cycle and I'd like to get it fixed before the release. Can you send a patch to address it please? Sure, happy to. So to confirm add 'anbernic' to the vendor binding list and 'anbernic,wl-355608-a8' as the panel compatible? Well anbernic is not the wl-355608-a8 panel manufaturer, so as Maxime is suggesting to use the name of the device where the panel is found like anbernic,rg353v-panel-v2 as submitted in https://lore.kernel.org/all/20230426143213.4178586-2-macroalph...@gmail.com/ Personally I don't care, if DT maintainers agrees having "wl-355608-a8" as compatible, it's perfectly fine to use it from the driver since it's solely a bindings decision and not a driver design issue. Neil Regards, Ryan
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
With drm_encoder_init() there's a static-const declared struct in RO
memory. With drmm_encoder_init(), there's a kalloc for the managed
callback data. It's RW memory and the alloc can fail. Therefore I prefer
drm_encoder_init() in this case.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, 26 Jun 2024 at 12:19, Thomas Zimmermann wrote:
>
> Hi
>
> Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
> > On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> >>> On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> The function drm_simple_encoder_init() is a trivial helper and
> deprecated. Replace it with the regular call to drm_encoder_init().
> Resolves the dependency on drm_simple_kms_helper.h. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/ast/ast_mode.c | 45 ++
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c
> b/drivers/gpu/drm/ast/ast_mode.c
> index 6695af70768f..2fd9c78eab73 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -45,7 +45,6 @@
> #include
> #include
> #include
> -#include
> #include "ast_ddc.h"
> #include "ast_drv.h"
> @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> return 0;
> }
> +/*
> + * VGA Encoder
> + */
> +
> +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> /*
> * VGA Connector
> */
> @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> *ast)
> struct drm_connector *connector = &ast->output.vga.connector;
> int ret;
> - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> + DRM_MODE_ENCODER_DAC, NULL);
> >>> What about using drmm_encoder_init() instead? It will call
> >>> drm_encoder_cleanup automatically.
> >> IIRC the original use case for the drmm_encoder_*() funcs was to solve
> >> problems with the clean-up order if the encoder was added dynamically. The
> >> hardware for ast is entirely static and ast uses drmm_mode_config_init()
> >> for
> >> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> >> like a bit of wasted resources for no gain.
> > I'd say it's qui pro quo. We are wasting resources on drmm handling, but
> > then keep it by dropping all drm_encoder_funcs instances.
>
> With drm_encoder_init() there's a static-const declared struct in RO
> memory. With drmm_encoder_init(), there's a kalloc for the managed
> callback data. It's RW memory and the alloc can fail. Therefore I prefer
> drm_encoder_init() in this case.
Ack.
--
With best wishes
Dmitry
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 11:19 schrieb Dmitry Baryshkov:
On Wed, 26 Jun 2024 at 12:19, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
With drm_encoder_init() there's a static-const declared struct in RO
memory. With drmm_encoder_init(), there's a kalloc for the managed
callback data. It's RW memory and the alloc can fail. Therefore I prefer
drm_encoder_init() in this case.
Ack.
One more though on this: we could discuss if we want some default
cleanup. Such as if no cleanup pointer has been set, we always call
drm_encoder_cleanup(). But IMHO that needs to be consistent among all
elements of the pipeline (planes, CRTCs, etc), and we need to document
clearly which and why the default has been chosen.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH v1 2/3] drm/msm/adreno: Add support for X185 GPU
On 26.06.2024 10:24 AM, Akhil P Oommen wrote:
> On Mon, Jun 24, 2024 at 03:53:48PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 6/23/24 13:06, Akhil P Oommen wrote:
>>> Add support in drm/msm driver for the Adreno X185 gpu found in
>>> Snapdragon X1 Elite chipset.
>>>
>>> Signed-off-by: Akhil P Oommen
>>> ---
>>>
>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 19 +++
>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 ++
>>> drivers/gpu/drm/msm/adreno/adreno_device.c | 14 ++
>>> drivers/gpu/drm/msm/adreno/adreno_gpu.h| 5 +
>>> 4 files changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> index 0e3dfd4c2bc8..168a4bddfaf2 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>>> @@ -830,8 +830,10 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu,
>>> unsigned int state)
>>> */
>>> gmu_write(gmu, REG_A6XX_GMU_CM3_CFG, 0x4052);
>>> + if (adreno_is_x185(adreno_gpu)) {
>>> + chipid = 0x7050001;
>>
>> What's wrong with using the logic below?
>
> patchid is BITS(7, 0), not (15, 8) in the case of x185. Due to the
> changes in the chipid scheme within the a7x family, this is a bit
> confusing. I will try to improve here in another series.
Ohh I overlooked this.. sounds a bit unfortunate..
Seems like it doesn't really fit the "else" branch anyway, let's
keep it for now then
Konrad
Re: [PATCH] drm/msm/adreno: fix a7xx gpu init
On 26.06.2024 9:53 AM, Neil Armstrong wrote:
> The gpulist has twice the a6xx gpulist, replace the second one
> with the a7xx gpulist.
>
> Solves:
> msm_dpu ae01000.display-controller: Unknown GPU revision: 7.3.0.1
> msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.10.1
> msm_dpu ae01000.display-controller: Unknown GPU revision: 67.5.20.1
>
> on SM8450, SM8550 & SM8560.
>
> Fixes: 8ed322f632a9 ("drm/msm/adreno: Split up giant device table")
> Signed-off-by: Neil Armstrong
> ---
Reviewed-by: Konrad Dybcio
Konrad
Re: [PATCH 7/7] arm64: dts: qcom: add OnePlus 8T (kebab)
On 24.06.2024 3:30 AM, Caleb Connolly wrote:
> Initial support for USB, UFS, touchscreen, panel, wifi, and bluetooth.
>
> Co-developed-by: Frieder Hannenheim
> Signed-off-by: Frieder Hannenheim
> Signed-off-by: Caleb Connolly
> ---
[...]
> +&adsp {
> + status = "okay";
> + firmware-name = "qcom/sm8250/OnePlus/adsp.mbn";
> +};
Status last pls
[...]
> +&gmu {
> + status = "okay";
> +};
already enabled (or should be)
[...]
> +/* LS-I2C1 */
> +&i2c15 {
> + status = "okay";
> +
> + // fcs,fsa4480 @ 42
Just describe it ;)
[...]
> + /* FIXME: There is a bug somewhere in the display stack and it
> isn't
/*
* FIXME:
> + * possible to get the panel to a working state after toggling
> reset.
> + * At best it just shows one or more vertical red lines. So for
> now
> + * let's skip the reset GPIO.
> + */
> + // reset-gpios = <&tlmm 75 GPIO_ACTIVE_LOW>;
/* */
[...]
> +&ufs_mem_hc {
> + status = "okay";
> +
> + vcc-supply = <&vreg_l17a_3p0>;
> + vcc-max-microamp = <80>;
> + vccq-supply = <&vreg_l6a_1p2>;
> + vccq-max-microamp = <80>;
> + vccq2-supply = <&vreg_s4a_1p8>;
> + vccq2-max-microamp = <80>;
allow set mode + allowed modes
Konrad
Re: [PATCH] drm/nouveau: fix null pointer dereference in nouveau_connector_get_modes
On Wed, 26 Jun 2024, Ma Ke wrote: > In nouveau_connector_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. > > Signed-off-by: Ma Ke > --- > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c > b/drivers/gpu/drm/nouveau/nouveau_connector.c > index 856b3ef5edb8..010eed56b14d 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c > @@ -1001,6 +1001,8 @@ nouveau_connector_get_modes(struct drm_connector > *connector) > struct drm_display_mode *mode; > > mode = drm_mode_duplicate(dev, nv_connector->native_mode); > + if (!mode) > + return -ENOMEM; Do not return negative values from .get_modes(). BR, Jani. > drm_mode_probed_add(connector, mode); > ret = 1; > } -- Jani Nikula, Intel
Re: [PATCH] drm/gma500: fix null pointer dereference in psb_intel_lvds_get_modes
On Wed, 26 Jun 2024, Ma Ke wrote:
> In psb_intel_lvds_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.
>
> Signed-off-by: Ma Ke
> ---
> drivers/gpu/drm/gma500/psb_intel_lvds.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c
> b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> index 8486de230ec9..aa5bf2a8a319 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
> @@ -504,6 +504,8 @@ static int psb_intel_lvds_get_modes(struct drm_connector
> *connector)
> if (mode_dev->panel_fixed_mode != NULL) {
> struct drm_display_mode *mode =
> drm_mode_duplicate(dev, mode_dev->panel_fixed_mode);
> + if (!mode)
> + return -ENOMEM;
Do not return negative values from .get_modes().
BR,
Jani.
> drm_mode_probed_add(connector, mode);
> return 1;
> }
--
Jani Nikula, Intel
Re: [PATCH] drm/mipi-dsi: Fix devm unregister & detach
On 26/06/2024 11:49, Maxime Ripard wrote: Hi, On Wed, Jun 19, 2024 at 12:07:48PM GMT, Tomi Valkeinen wrote: From: Tomi Valkeinen When a bridge driver uses devm_mipi_dsi_device_register_full() or devm_mipi_dsi_attach(), the resource management is moved to devres, which releases the resource automatically when the bridge driver is unbound. However, if the DSI host goes away first, the host unregistration code will automatically detach and unregister any DSI peripherals, without notifying the devres about it. So when the bridge driver later is unbound, the resources are released a second time, leading to crash. That's super surprising. mipi_dsi_device_unregister calls device_unregister, which calls device_del, which in turn calls devres_release_all. Hmm, right. If that doesn't work like that, then it's what needs to be fixed, and not worked around in the MIPI-DSI bus. Well, something causes a crash for both the device register/unregister case and the attach/detach case, and the call stacks and debug prints showed a double unregister/detach... I need to dig up the board and check again why the devres_release_all() in device_del() doesn't solve this. But I can probably only get back to this in August, so it's perhaps best to ignore this patch for now. However, the attach/detach case is still valid? I see no devres calls in the detach paths. Tomi
Re: [PATCH v4 01/11] drm/bridge: cdns-dsi: Fix OF node pointer
Hi,
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Fix the OF node pointer passed to the of_drm_find_bridge() call to find
the next bridge in the display chain.
To find the next bridge in the pipeline, we need to pass "np" - the OF
node pointer of the next entity in the devicetree chain. Passing
"of_node" to of_drm_find_bridge will make the function try to fetch the
bridge for the cdns-dsi which is not what's required.
Fix that.
The code looks fine, but I'd write the subject and desc from a different
perspective. The subject could be something like "Fix connecting to a
sink bridge", and the desc could first say that connecting the sink to a
DSI panel works, but connecting to a bridge fails, as wrong OF node is
passed to of_drm_find_bridge().
Reviewed-by: Tomi Valkeinen
Tomi
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 7457d38622b0..b016f2ba06bb 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -952,7 +952,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
bridge = drm_panel_bridge_add_typed(panel,
DRM_MODE_CONNECTOR_DSI);
} else {
- bridge = of_drm_find_bridge(dev->dev.of_node);
+ bridge = of_drm_find_bridge(np);
if (!bridge)
bridge = ERR_PTR(-EINVAL);
}
Re: [PATCH v4 02/11] drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Instead of manually finding the next bridge/panel, and maintaining the
panel-bridge (in-case the next entity is a panel), switch to using the
automatically managing devm_drm_of_get_bridge() API.
Drop the drm_panel support completely from the driver while at it.
Signed-off-by: Aradhya Bhatia
Reviewed-by: Tomi Valkeinen
Tomi
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 28 ++-
.../gpu/drm/bridge/cadence/cdns-dsi-core.h| 2 --
2 files changed, 3 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b016f2ba06bb..5159c3f0853e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -920,8 +920,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
struct cdns_dsi_output *output = &dsi->output;
struct cdns_dsi_input *input = &dsi->input;
struct drm_bridge *bridge;
- struct drm_panel *panel;
- struct device_node *np;
int ret;
/*
@@ -939,26 +937,10 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
/*
* The host <-> device link might be described using an OF-graph
* representation, in this case we extract the device of_node from
-* this representation, otherwise we use dsidev->dev.of_node which
-* should have been filled by the core.
+* this representation.
*/
- np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
- dev->channel);
- if (!np)
- np = of_node_get(dev->dev.of_node);
-
- panel = of_drm_find_panel(np);
- if (!IS_ERR(panel)) {
- bridge = drm_panel_bridge_add_typed(panel,
- DRM_MODE_CONNECTOR_DSI);
- } else {
- bridge = of_drm_find_bridge(np);
- if (!bridge)
- bridge = ERR_PTR(-EINVAL);
- }
-
- of_node_put(np);
-
+ bridge = devm_drm_of_get_bridge(dsi->base.dev, dsi->base.dev->of_node,
+ DSI_OUTPUT_PORT, dev->channel);
if (IS_ERR(bridge)) {
ret = PTR_ERR(bridge);
dev_err(host->dev, "failed to add DSI device %s (err = %d)",
@@ -968,7 +950,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
output->dev = dev;
output->bridge = bridge;
- output->panel = panel;
/*
* The DSI output has been properly configured, we can now safely
@@ -984,12 +965,9 @@ static int cdns_dsi_detach(struct mipi_dsi_host *host,
struct mipi_dsi_device *dev)
{
struct cdns_dsi *dsi = to_cdns_dsi(host);
- struct cdns_dsi_output *output = &dsi->output;
struct cdns_dsi_input *input = &dsi->input;
drm_bridge_remove(&input->bridge);
- if (output->panel)
- drm_panel_bridge_remove(output->bridge);
return 0;
}
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
index ca7ea2da635c..5db5dbbbcaad 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
@@ -10,7 +10,6 @@
#include
#include
-#include
#include
#include
@@ -21,7 +20,6 @@ struct reset_control;
struct cdns_dsi_output {
struct mipi_dsi_device *dev;
- struct drm_panel *panel;
struct drm_bridge *bridge;
union phy_configure_opts phy_opts;
};
Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver
On 6/23/24 17:46, Andrew Lunn wrote: >>> But what about when the system is under memory pressure? You say it >>> allocates memory. What happens if those allocations fail. Does >>> changing the MTU take me from a working system to a dead system? It is >>> good practice to not kill a working system under situations like >>> memory pressure. You try to first allocate the memory you need to >>> handle the new MTU, and only if successful do you free existing memory >>> you no longer need. That means if you cannot allocate the needed >>> memory, you still have the old memory, you can keep the old MTU and >>> return -ENOMEM, and the system keeps running. >>> >> >> That's a good optimization for these kind of on-the-fly configurations but >> as you wrote before, changing an MTU value is not a hot path so out of >> cost-benefit considerations we didn't find it mandatory to optimize this >> flow. > > I would not call this an optimization. And it is not just about > changing the MTU. ethtool set_ringparam() is also likely to run into > this problem, and any other configuration which requires reallocating > the rings. > > This is something else which comes up every few months on the list, > and drivers writers who monitor the list will write their drivers that > why, not 'optimise' it later. > Actually I was wrong, we don't allocate memory in this port reset flow, we only reset the rings. But I get your point, it makes sense. >> I get your point but still it will be good if it would be documented >> somewhere IMHO. > > Kernel documentation is poor, agreed. But kernel policy is also > somewhat fluid, best practices change, and any developers can > influence that policy, different subsystems can and do have > contradictory policy, etc. The mailing list is the best place to learn > and to take part in this community. You need to be on the list for > other reasons as well. > Ok, got it. >> I'm familiar with this logic but I don't understand your point. The point >> you are making is that setting this Autoneg bit in lp_advertising is >> pointless? I see other vendors setting it too in case that autoneg was >> completed. >> Is that redundant also in their case? because it looks to me that in this >> case we followed the same logic and conventions other vendors followed. > > Please show us the output from ethtool. Does it look like the example > i showed? I must admit, i'm more from the embedded world and don't > have access to high speed interfaces. But the basic concept of > auto-neg should not change that much. > >Andrew Here is the output: $ ethtool eth0 Settings for eth0: Supported ports: [ FIBRE Backplane ] Supported link modes: 10baseKR4/Full 10baseSR4/Full 10baseCR4/Full 10baseLR4_ER4/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseKR4/Full 10baseSR4/Full 10baseCR4/Full 10baseLR4_ER4/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 10Mb/s Duplex: Full Auto-negotiation: on There are few points to mention: 1. We don't allow to modify the advertised link modes so by definition the advertised ones are a copy of the supported ones. 2. Reading the peer advertised link modes is not supported so we don't report them (similarly to some other vendors). 3. Our speed is fixed and also cannot be changed so we don't mask lp_advertising with advertising to pick the highest speed. We aim for a specific speed and hence it's binary - or we'll have a link with that specific speed or we won't have a link at all. 4. If we support autoneg and it was completed, we can conclude that also our peer supports autoneg and hence we report that.
Re: [PATCH] drm/gma500: fix null pointer dereference in psb_intel_lvds_get_modes
> In psb_intel_lvds_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. 1. Can a wording approach (like the following) be a better change description? A null pointer is stored in the local variable “mode” after a call of the function “drm_mode_duplicate” failed. This pointer was passed to a subsequent call of the function “drm_mode_probed_add” where an undesirable dereference will be performed then. Thus add a corresponding return value check. 2. Would you like to add any tags (like “Fixes” and “Cc”) accordingly? 3. How do you think about to append parentheses to the function name in the summary phrase? 4. How do you think about to put similar results from static source code analyses into corresponding patch series? Regards, Markus
Re: [PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()
Hi,
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
during the _suspend().
Also power-off the Phy from bridge_disable.
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 5159c3f0853e..d89c32bae2b9 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge
*bridge)
if (dsi->platform_ops && dsi->platform_ops->disable)
dsi->platform_ops->disable(dsi);
+ phy_power_off(dsi->dphy);
+ dsi->link_initialized = false;
+ dsi->phy_initialized = false;
+
pm_runtime_put(dsi->base.dev);
}
@@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
DPHY_CMN_PDN | DPHY_PLL_PDN,
dsi->regs + MCTL_DPHY_CFG0);
- phy_init(dsi->dphy);
phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
phy_configure(dsi->dphy, &output->phy_opts);
phy_power_on(dsi->dphy);
@@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device
*dev)
clk_prepare_enable(dsi->dsi_p_clk);
clk_prepare_enable(dsi->dsi_sys_clk);
+ phy_init(dsi->dphy);
+
return 0;
}
@@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
{
struct cdns_dsi *dsi = dev_get_drvdata(dev);
+ phy_exit(dsi->dphy);
+
clk_disable_unprepare(dsi->dsi_sys_clk);
clk_disable_unprepare(dsi->dsi_p_clk);
reset_control_assert(dsi->dsi_p_rst);
- dsi->link_initialized = false;
return 0;
}
So with this patch, phy_init/exit will be called in the resume/suspend
functions. That looks fine.
But the phy_power_on/phy_power_off looks odd to me. Here you add
phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But
phy_power_on() is called in cdns_dsi_hs_init(), and that is called in
cdns_dsi_bridge_enable() (which sounds fine), but also in
cdns_dsi_bridge_pre_enable().
So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable()
is extra, as it effectively does nothing (it exists right away if
dsi->phy_initialized == true)?
Tomi
Re: [PATCH v4 04/11] drm/bridge: cdns-dsi: Fix the link and phy init order
On 22/06/2024 14:09, Aradhya Bhatia wrote:
The order of init of DSI link and DSI phy is wrong. The DSI link needs
to be configured before the DSI phy is getting configured. Otherwise,
the D-Phy is unable to lock in on the incoming PLL Reference clock[0].
Fix the order of inits.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index d89c32bae2b9..03a5af52ec0b 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -778,8 +778,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
- cdns_dsi_hs_init(dsi);
cdns_dsi_init_link(dsi);
+ cdns_dsi_hs_init(dsi);
writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
dsi->regs + VID_HSIZE1);
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
Hi, On Wed, 26 Jun 2024 at 09:28, Lucas Stach wrote: > Mesa doesn't cope right now. Mostly because of the renderonly thing > where we magically need to match render devices to otherwise render > incapable KMS devices. The way this matching works is that the > renderonly code tries to open a screen on a rendernode and if that > succeeds we treat it as the matching render device. > > The core of the issue is that we have no way of specifying which kind > of screen we need at that point, i.e. if the screen should have 3D > render capabilities or if compute-only or even NN-accel-only would be > okay. So we can't fail screen creation if there is no 3D engine, as > this would break the teflon case, which needs a screen for the NN > accel, but once we successfully create a screen reanderonly might treat > the thing as a rendering device. > So we are kind of stuck here between breaking one or the other use- > case. I'm leaning heavily into the direction of just fixing Mesa, so we > can specify the type of screen we need at creation time to avoid the > renderonly issue, porting this change as far back as reasonably > possible and file old userspace into shit-happens. Yeah, honestly this sounds like the best solution to me too. Cheers, Daniel
Re: [PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Allow the D-Phy config checks to use mode->clock instead of
mode->crtc_clock during mode_valid checks, like everywhere else in the
driver.
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 03a5af52ec0b..426f77092341 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
if (ret)
return ret;
- phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+ phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock :
mode->crtc_clock) * 1000,
mipi_dsi_pixel_format_to_bpp(output->dev->format),
nlanes, phy_cfg);
I think this is fine as a fix.
Reviewed-by: Tomi Valkeinen
However... The code looks a bit messy. Maybe the first one is something
that could be addressed in this series.
- Return value of phy_mipi_dphy_get_default_config() is not checked
- Using the non-crtc and crtc versions of the timings this way looks
bad, but that's not a problem of the driver. It would be better to have
a struct that contains the timings, and struct drm_display_mode would
contain two instances of that struct. The driver code could then just
pick the correct instance, instead of making the choice for each and
every field. This would be an interesting coccinelle project ;)
- Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd.
Everything should already have been checked. In fact, at the check phase
the resulting config values could have been stored somewhere, so that
they're ready for use by cdns_dsi_bridge_enable(). But this rises the
question if the non-crtc and crtc timings can actually be different, and
if they are... doesn't it break everything if at the check phase we use
the non-crtc ones, but at enable phase we use crtc ones?
Ah, I see, this is with non-atomic. Maybe after you switch to atomic
callbacks, atomic_check could be used so that there's no need for the
WARN_ON_ONCE() in enable callback.
Tomi
Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory
Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥):
>
> > In the step 3), we need to verify the dma-buf is allocated from
> > "restricted_mtk_cma", but there is no way to pass the secure flag
> > or
> > private data from userspace to the import interface in DRM driver.
>
> Why do you need to verify that?
I need to know the imported buffer is allocated from restricted cma and
mark it as a secure buffer in mediatek-drm driver. Then, I will add
some configuration to the hardware if the buffer is secure buffer, so
that it can get the permission to access the secure buffer.
Yeah so far that makes sense. This is basically what other drivers do
with secure buffers as well.
But why do you want the kernel to transport that information? Usually
drivers get the information from userspace what to do with a buffer.
In other words the format, stride, tilling and also if it's a secure
buffer or not comes from userspace.
What the hardware usually handles internally is things like encryption
keys, but you eventually get the information where to look for the key
from userspace as well.
Handling inside the kernel would only be necessary if userspace could
for example crash the system with invalid parameters. But for encryption
that is usually not the case.
>
> > So I can only verify it like this now:
> > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct
> > drm_device
> > *dev, struct dma_buf_attachment *attach, struct sg_table *sg)
> > {
> > struct mtk_gem_obj *mtk_gem;
> >
> > /* check if the entries in the sg_table are contiguous */
> > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
> > DRM_ERROR("sg_table is not contiguous");
> > return ERR_PTR(-EINVAL);
> > }
> > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size);
> > if (IS_ERR(mtk_gem))
> > return ERR_CAST(mtk_gem);
> >
> > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name,
> > "restricted",
> > 10));
> > mtk_gem->dma_addr = sg_dma_address(sg->sgl);
> > mtk_gem->size = attach->dmabuf->size;
> > mtk_gem->sg = sg;
> >
> > return &mtk_gem->base;
> > }
>
> Complete NAK from my side to that approach. Importing of a DMA-buf
> should be independent of the exporter.
>
> What you could do is to provide the secure buffer from a device and
> not a device heap.
>
You mean I should allocate buffer in mediate-drm driver not userspace?
Well that depends. The question is if you have multiple drivers which
needs to work with this secure buffer?
If yes then you should have a general allocation heap for it. If no then
the buffers could as well be allocated from the driver interface directly.
I just have modified this to userspace by the comment here:
https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.s...@mediatek.com/#25806766
> > I think I have the same problem as the ECC_FLAG mention in:
> >
> >
https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049...@kernel.org/
> >
> > I think it would be better to have the user configurable private
> > information in dma-buf, so all the drivers who have the same
> > requirement can get their private information from dma-buf directly
> > and
> > no need to change or add the interface.
> >
> > What's your opinion in this point?
>
> Well of hand I don't see the need for that.
>
> What happens if you get a non-secure buffer imported in your secure
> device?
We use the same mediatek-drm driver for secure and non-secure buffer.
If non-secure buffer imported to mediatek-drm driver, it's go to the
normal flow with normal hardware settings.
We use different configurations to make hardware have different
permission to access the buffer it should access.
So if we can't get the information of "the buffer is allocated from
restricted_mtk_cma" when importing the buffer into the driver, we won't
be able to configure the hardware correctly.
Why can't you get this information from userspace?
Regards,
Christian.
Regards,
Jason-JH.Lin
>
> Regards,
> Christian.
>
> > Regards,
> > Jason-JH.Lin
> >
> > > Regards,
> > > Christian.
> >
> > * MEDIATEK Confidentiality Notice
> >
> > The information contained in this e-mail message (including any
> > attachments) may be confidential, proprietary, privileged, or
> > otherwise
> > exempt from disclosure under applicable laws. It is intended to be
> > conveyed only to the designated recipient(s). Any use,
> > dissemination,
> > distribution, printing, retaining or copying of this e-mail
> > (including its
> > attachments) by unintended recipient(s) is strictly prohibited and
> > may
> > be unlawful. If you are not an intended recipient of this e-mail,
> > or believe
> >
> > that you have received this e-mail in error, please notify the
> > sender
> > immediately (by replying to this e-mail), delete any and all copies
> > of
> > this e-mail (including a
Re: [PATCH v4 06/11] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Once the DSI Link and DSI Phy are initialized, the code needs to wait
for Clk and Data Lanes to be ready, before continuing configuration.
This is in accordance with the DSI Start-up procedure, found in the
Technical Reference Manual of Texas Instrument's J721E SoC[0] which
houses this DSI TX controller.
If the previous bridge (or crtc/encoder) are configured pre-maturely,
the input signal FIFO gets corrupt. This introduces a color-shift on the
display.
Allow the driver to wait for the clk and data lanes to get ready during
DSI enable.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Tested-by: Dominik Haller
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 426f77092341..126e4bccd868 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -764,7 +764,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
struct phy_configure_opts_mipi_dphy *phy_cfg =
&output->phy_opts.mipi_dphy;
unsigned long tx_byte_period;
struct cdns_dsi_cfg dsi_cfg;
- u32 tmp, reg_wakeup, div;
+ u32 tmp, reg_wakeup, div, status;
int nlanes;
if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -781,6 +781,17 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
cdns_dsi_init_link(dsi);
cdns_dsi_hs_init(dsi);
+ /*
+* Now that the DSI Link and DSI Phy are initialized,
+* wait for the CLK and Data Lanes to be ready.
+*/
+ tmp = CLK_LANE_RDY;
+ for (int i = 0; i < nlanes; i++)
+ tmp |= DATA_LANE_RDY(i);
+
+ WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+ status & tmp, 100, 0));
I think an error print is more suitable than WARN_ON_ONCE(). Other than
that:
Reviewed-by: Tomi Valkeinen
Tomi
+
writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
dsi->regs + VID_HSIZE1);
writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
Re: [PATCH v5 1/5] drm/panel: jd9365da: Modify the method of sending commands
On Wed, Jun 26, 2024 at 1:49 AM Jessica Zhang wrote:
>
>
>
> On 6/25/2024 5:13 AM, zhaoxiong lv wrote:
> > On Tue, Jun 25, 2024 at 7:41 AM Jessica Zhang
> > wrote:
> >>
> >>
> >>
> >> On 6/24/2024 7:19 AM, Zhaoxiong Lv wrote:
> >>> Currently, the init_code of the jd9365da driver is placed
> >>> in the enable() function and sent, but this seems to take
> >>> a long time. It takes 17ms to send each instruction (an init
> >>> code consists of about 200 instructions), so it takes
> >>> about 3.5s to send the init_code. So we moved the sending
> >>> of the inti_code to the prepare() function, and each
> >>> instruction seemed to take only 25μs.
> >>>
> >>> We checked the DSI host and found that the difference in
> >>> command sending time is caused by the different modes of
> >>> the DSI host in prepare() and enable() functions.
> >>> Our DSI Host only supports sending cmd in LP mode, The
> >>> prepare() function can directly send init_code (LP->cmd)
> >>> in LP mode, but the enable() function is in HS mode and
> >>> needs to switch to LP mode before sending init code
> >>> (HS->LP->cmd->HS). Therefore, it takes longer to send
> >>> the command.
> >>>
> >>> Signed-off-by: Zhaoxiong Lv
> >>
> >> Hi Zhaoxiong,
> >>
> >> Just curious, if the host expects that commands are sent in LP mode, why
> >> isn't the MIPI_DSI_MODE_LPM flag set before sending the DCS commands?
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >
> > hi jessica
> >
> > We have tried to set dsi->mode_flags to MIPI_DSI_MODE_LPM in the
> > probe() function,
> > but this seems to still happen. MTK colleagues believe that the host
> > dsi configuration is
> > still in LP mode during the prepare() function, and when in the
> > enable() function, the host
> > dsi is already in HS mode. However, since the command must be sent in
> > LP mode, it will
> > switch back and forth between HS->LP->HS.
> >
> > Add Mediatek colleagues（shuijing...@mediatek.corp-partner.google.com）
>
> Got it. Even drivers that call their init commands in prepare() set the
> LPM flag [1][2] when applicable so I was just wondering why this driver
> doesn't seem to set LPM at all even though it is going into LP mode.
>
> [1]
> https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c#L46
>
> [2]
> https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/panel/panel-visionox-r66451.c#L46
hi jessica
The initial default setting of our host DSI is the LP mode.
>
> >
> >
> >>
> >>> ---
> >>> Changes between V5 and V4:
> >>> - 1. No changes.
> >>>
> >>> V4:https://lore.kernel.org/all/20240620080509.18504-2-lvzhaoxi...@huaqin.corp-partner.google.com/
> >>>
> >>> Changes between V4 and V3:
> >>> - 1. Only move mipi_dsi_dcs_write_buffer from enable() function to
> >>> prepare() function,
> >>> -and no longer use mipi_dsi_dcs_write_seq_multi.
> >>>
> >>> V3:https://lore.kernel.org/all/20240614145510.22965-2-lvzhaoxi...@huaqin.corp-partner.google.com/
> >>>
> >>> ---
> >>>.../gpu/drm/panel/panel-jadard-jd9365da-h3.c | 24 +--
> >>>1 file changed, 11 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> >>> b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> >>> index 4879835fe101..a9c483a7b3fa 100644
> >>> --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> >>> +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> >>> @@ -52,21 +52,9 @@ static int jadard_enable(struct drm_panel *panel)
> >>>{
> >>>struct device *dev = panel->dev;
> >>>struct jadard *jadard = panel_to_jadard(panel);
> >>> - const struct jadard_panel_desc *desc = jadard->desc;
> >>>struct mipi_dsi_device *dsi = jadard->dsi;
> >>> - unsigned int i;
> >>>int err;
> >>>
> >>> - msleep(10);
> >>> -
> >>> - for (i = 0; i < desc->num_init_cmds; i++) {
> >>> - const struct jadard_init_cmd *cmd = &desc->init_cmds[i];
> >>> -
> >>> - err = mipi_dsi_dcs_write_buffer(dsi, cmd->data,
> >>> JD9365DA_INIT_CMD_LEN);
> >>> - if (err < 0)
> >>> - return err;
> >>> - }
> >>> -
> >>>msleep(120);
> >>>
> >>>err = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >>> @@ -100,6 +88,8 @@ static int jadard_disable(struct drm_panel *panel)
> >>>static int jadard_prepare(struct drm_panel *panel)
> >>>{
> >>>struct jadard *jadard = panel_to_jadard(panel);
> >>> + const struct jadard_panel_desc *desc = jadard->desc;
> >>> + unsigned int i;
> >>>int ret;
> >>>
> >>>ret = regulator_enable(jadard->vccio);
> >>> @@ -117,7 +107,15 @@ static int jadard_prepare(struct drm_panel *panel)
> >>>msleep(10);
> >>>
> >>>gpiod_set_value(jadard->reset, 1);
> >>> - msleep(120);
> >>> + msleep(130);
> >>> +
> >>> + for (i = 0; i < desc->num_init_cmds; i++) {
> >>> + const struct jadard_init_cmd *cmd = &desc->init_cmds[i];
> >>> +
> >>>
Re: [PATCH v3 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel
On Wed, 26 Jun 2024, at 9:16 PM, Neil Armstrong wrote: > Well anbernic is not the wl-355608-a8 panel manufaturer, so as Maxime > is suggesting to use the > name of the device where the panel is found like > anbernic,rg353v-panel-v2 as submitted > in > https://lore.kernel.org/all/20230426143213.4178586-2-macroalph...@gmail.com/ Show quoted text Understood thanks. I have no strong feelings either, using the device name is sensible. Will prepare a patch. Regards, Ryan (apologies, replying-all this time)
Re: [PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO
On 22/06/2024 14:09, Aradhya Bhatia wrote: If any normal DCS write command has already been transmitted prior to transmitting any Zero-Parameter DCS command, then it is necessary to clear the TX FIFO by resetting it. Otherwise, the FIFO points to another location, and the DCS command transmits unnecessary data causing the panel to not work[0]. Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule, before any DCS packet is transmitted to the DSI peripheral. [0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical Reference Manual: https://www.ti.com/lit/zip/spruil1 Hmm so if I read the doc right, it says: if sending zero-parameter dcs command, clear the FIFO and write zero to direct_cmd_wrdat. Your patch seems to always clear the FIFO, not only for zero-parameter commands. Is that a problem (I don't think so, but...)? Also, is the direct_cmd_wrdat written at all when sending zero-parameter dcs command? Tomi Signed-off-by: Aradhya Bhatia --- drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 126e4bccd868..cad0c1478ef0 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host, cdns_dsi_init_link(dsi); + /* Reset the DCS Write FIFO */ + writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST); + ret = mipi_dsi_create_packet(&packet, msg); if (ret) goto out;
Re: [PATCH v4 08/11] drm/mipi-dsi: Add helper to find input format
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Add a helper API that can be used by the DSI hosts to find the required
input bus format for the given output dsi pixel format.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/drm_mipi_dsi.c | 37 ++
include/drm/drm_mipi_dsi.h | 1 +
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a471c46f5ca6..937aa16dfcf6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -36,6 +36,8 @@
#include
#include
+#include
+
#include
/**
@@ -866,6 +868,41 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi,
const void *params,
}
EXPORT_SYMBOL(mipi_dsi_generic_read);
+/**
+ * drm_mipi_dsi_get_input_bus_fmt() - Get the required MEDIA_BUS_FMT_* based
+ * input pixel format for a given DSI output
+ * pixel format
+ * @dsi_format: pixel format that a DSI host needs to output
+ *
+ * Various DSI hosts can use this function during their
+ * &drm_bridge_funcs.atomic_get_input_bus_fmts operation to ascertain
+ * the MEDIA_BUS_FMT_* pixel format required as input.
+ *
+ * RETURNS:
+ * a 32-bit MEDIA_BUS_FMT_* value on success or 0 in case of failure.
+ */
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format)
+{
+ switch (dsi_format) {
+ case MIPI_DSI_FMT_RGB888:
+ return MEDIA_BUS_FMT_RGB888_1X24;
+
+ case MIPI_DSI_FMT_RGB666:
+ return MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+
+ case MIPI_DSI_FMT_RGB666_PACKED:
+ return MEDIA_BUS_FMT_RGB666_1X18;
+
+ case MIPI_DSI_FMT_RGB565:
+ return MEDIA_BUS_FMT_RGB565_1X16;
+
+ default:
+ /* Unsupported DSI Format */
+ return 0;
+ }
+}
+EXPORT_SYMBOL(drm_mipi_dsi_get_input_bus_fmt);
+
/**
* mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
* @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 71d121aeef24..78a2c7d9eefb 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -290,6 +290,7 @@ void mipi_dsi_generic_write_multi(struct
mipi_dsi_multi_context *ctx,
const void *payload, size_t size);
ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
size_t num_params, void *data, size_t size);
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format);
#define mipi_dsi_msleep(ctx, delay) \
do {\
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v4 09/11] drm/bridge: cdns-dsi: Support atomic bridge APIs
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Change the existing (and deprecated) bridge hooks, to the bridge
atomic APIs.
Add drm helpers for duplicate_state, destroy_state, and bridge_reset
bridge hooks.
Further add support for the input format negotiation hook.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Aradhya Bhatia
Reviewed-by: Tomi Valkeinen
Tomi
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 51 ---
1 file changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index cad0c1478ef0..c9697818308e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,7 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -679,7 +680,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge
*bridge)
pm_runtime_put(dsi->base.dev);
}
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -755,7 +757,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
dsi->link_initialized = true;
}
-static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -906,7 +909,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge
*bridge)
writel(tmp, dsi->regs + MCTL_MAIN_EN);
}
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -918,13 +922,44 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge
*bridge)
cdns_dsi_hs_init(dsi);
}
+static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state
*bridge_state,
+ struct drm_crtc_state
*crtc_state,
+ struct drm_connector_state
*conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts)
+{
+ struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+ struct cdns_dsi *dsi = input_to_dsi(input);
+ struct cdns_dsi_output *output = &dsi->output;
+ u32 *input_fmts;
+
+ *num_input_fmts = 0;
+
+ input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ input_fmts[0] = drm_mipi_dsi_get_input_bus_fmt(output->dev->format);
+ if (!input_fmts[0])
+ return NULL;
+
+ *num_input_fmts = 1;
+
+ return input_fmts;
+}
+
static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
.attach = cdns_dsi_bridge_attach,
.mode_valid = cdns_dsi_bridge_mode_valid,
- .disable = cdns_dsi_bridge_disable,
- .pre_enable = cdns_dsi_bridge_pre_enable,
- .enable = cdns_dsi_bridge_enable,
- .post_disable = cdns_dsi_bridge_post_disable,
+ .atomic_disable = cdns_dsi_bridge_atomic_disable,
+ .atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
+ .atomic_enable = cdns_dsi_bridge_atomic_enable,
+ .atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
};
static int cdns_dsi_attach(struct mipi_dsi_host *host,
Re: [PATCH net-next v14 11/13] net: add SO_DEVMEM_DONTNEED setsockopt to release RX frags
On 25/06/2024 22:53, Mina Almasry wrote: Add an interface for the user to notify the kernel that it is done reading the devmem dmabuf frags returned as cmsg. The kernel will drop the reference on the frags to make them available for reuse. Signed-off-by: Willem de Bruijn Signed-off-by: Kaiyuan Zhang Signed-off-by: Mina Almasry --- v10: - Fix leak of tokens (Nikolay). v7: - Updated SO_DEVMEM_* uapi to use the next available entry (Arnd). v6: - Squash in locking optimizations from eduma...@google.com. With his changes we lock the xarray once per sock_devmem_dontneed operation rather than once per frag. Changes in v1: - devmemtoken -> dmabuf_token (David). - Use napi_pp_put_page() for refcounting (Yunsheng). - Fix build error with missing socket options on other asms. --- arch/alpha/include/uapi/asm/socket.h | 1 + arch/mips/include/uapi/asm/socket.h | 1 + arch/parisc/include/uapi/asm/socket.h | 1 + arch/sparc/include/uapi/asm/socket.h | 1 + include/uapi/asm-generic/socket.h | 1 + include/uapi/linux/uio.h | 4 ++ net/core/sock.c | 61 +++ 7 files changed, 70 insertions(+) FWIW, Reviewed-by: Nikolay Aleksandrov
[PATCH 0/3] Correct WL-355608-A8 panel compatible
The previous patch adding support for this panel [1] referred to previously by
its serial number only. As discussed after the patch was committed, the
preference is to use the integrating device vendor and name in this
circumstance.
This series corrects the panel compatible to reflect the vendor (Anbernic,
already in the vendor prefix table), updates the NV3052C panel driver with the
new compatible, and lastly adds num-chipselects and sck-gpios to the DT binding
example, identified by make dt_bindings_check as required for bit-banged SPI
over GPIO lines.
Regards,
Ryan
[1] https://lore.kernel.org/dri-devel/20240530211415.44201-1-r...@testtoast.com/
Ryan Walklin (3):
dt-bindings: display: panel: Rename WL-355608-A8 panel
drm: panel: nv3052c: Correct WL-355608-A8 panel compatible
dt-bindings: display: panel: correct Anbernic RG35XX panel example
.../{wl-355608-a8.yaml => anbernic,rg35xx-panel.yaml} | 11 +++
drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
rename Documentation/devicetree/bindings/display/panel/{wl-355608-a8.yaml =>
anbernic,rg35xx-panel.yaml} (76%)
--
2.45.2
[PATCH 1/3] dt-bindings: display: panel: Rename WL-355608-A8 panel
The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display from an unknown
OEM used in a number of handheld gaming devices made by Anbernic.
Previously committed using the OEM serial without a vendor prefix,
however the preference is to use the integrating device vendor and name
where the OEM is unknown.
Alter the filename and compatible string to reflect the convention.
Fixes: f08aac40639c ("drm: panel: nv3052c: Add WL-355608-A8 panel")
Signed-off-by: Ryan Walklin
---
.../{wl-355608-a8.yaml => anbernic,rg35xx-panel.yaml} | 8
1 file changed, 4 insertions(+), 4 deletions(-)
rename Documentation/devicetree/bindings/display/panel/{wl-355608-a8.yaml =>
anbernic,rg35xx-panel.yaml} (81%)
diff --git a/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
similarity index 81%
rename from Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
rename to
Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
index 397c26be9bda5..a7d5ad0f29389 100644
--- a/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
+++ b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
@@ -1,10 +1,10 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/display/panel/wl-355608-a8.yaml#
+$id: http://devicetree.org/schemas/display/panel/anbernic,rg35xx-panel.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: WL-355608-A8 3.5" (640x480 pixels) 24-bit IPS LCD panel
+title: Anbernic RG35XX (WL-355608-A8) 3.5" 640x480 24-bit IPS LCD panel
maintainers:
- Ryan Walklin
@@ -15,7 +15,7 @@ allOf:
properties:
compatible:
-const: wl-355608-a8
+const: anbernic,rg35xx-panel
reg:
maxItems: 1
@@ -41,7 +41,7 @@ examples:
#size-cells = <0>;
panel@0 {
-compatible = "wl-355608-a8";
+compatible = "anbernic,rg35xx-panel";
reg = <0>;
spi-3wire;
--
2.45.2
[PATCH 2/3] drm: panel: nv3052c: Correct WL-355608-A8 panel compatible
As per the previous dt-binding commit, update the WL-355608-A8 panel
compatible to reflect the the integrating device vendor and name.
Signed-off-by: Ryan Walklin
---
drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
index ee0ce271205e3..cee40269d9bb8 100644
--- a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
+++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
@@ -925,7 +925,7 @@ MODULE_DEVICE_TABLE(spi, nv3052c_ids);
static const struct of_device_id nv3052c_of_match[] = {
{ .compatible = "leadtek,ltk035c5444t", .data =
[PATCH 3/3] dt-bindings: display: panel: correct Anbernic RG35XX panel example
make dt_bindings_check reports that sck-gpios and num-chipselects are
required for spi nodes, therefore add these to the example.
Signed-off-by: Ryan Walklin
---
.../bindings/display/panel/anbernic,rg35xx-panel.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
index a7d5ad0f29389..610601c1594f3 100644
--- a/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
+++ b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
@@ -40,6 +40,9 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+sck-gpios = <&pio 8 9 GPIO_ACTIVE_HIGH>; // PI9
+num-chipselects = <1>;
+
panel@0 {
compatible = "anbernic,rg35xx-panel";
reg = <0>;
--
2.45.2
Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
On 22/06/2024 14:09, Aradhya Bhatia wrote:
Move the bridge pre_enable call before crtc enable, and the bridge
post_disable call after the crtc disable.
The sequence of enable after this patch will look like:
bridge[n]_pre_enable
...
bridge[1]_pre_enable
crtc_enable
encoder_enable
bridge[1]_enable
...
bridge[n]__enable
and vice-versa for the bridge chain disable sequence.
The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".
Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled. Fix that by
re-ordering the sequence of bridge pre_enable and bridge post_disable.
Signed-off-by: Aradhya Bhatia
---
drivers/gpu/drm/drm_atomic_helper.c | 165 ++--
include/drm/drm_atomic_helper.h | 7 ++
2 files changed, 114 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index fb97b51b38f1..e8ad08634f58 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -74,6 +74,7 @@
* also shares the &struct drm_plane_helper_funcs function table with the
plane
* helpers.
*/
+
Extra change.
static void
drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
struct drm_plane_state *old_plane_state,
@@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
}
static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state
*old_state,
+ enum bridge_chain_operation_type op_type)
{
struct drm_connector *connector;
struct drm_connector_state *old_conn_state, *new_conn_state;
- struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
int i;
@@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
if (WARN_ON(!encoder))
continue;
- funcs = encoder->helper_private;
-
- drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
- encoder->base.id, encoder->name);
-
/*
* Each encoder has at most one connector (since we always steal
* it away), so we won't call disable hooks twice.
*/
bridge = drm_bridge_chain_get_first_bridge(encoder);
- drm_atomic_bridge_chain_disable(bridge, old_state);
- /* Right function depends upon target state. */
- if (funcs) {
- if (funcs->atomic_disable)
- funcs->atomic_disable(encoder, old_state);
- else if (new_conn_state->crtc && funcs->prepare)
- funcs->prepare(encoder);
- else if (funcs->disable)
- funcs->disable(encoder);
- else if (funcs->dpms)
- funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
- }
+ switch (op_type) {
+ case DRM_ENCODER_BRIDGE_DISABLE:
+ funcs = encoder->helper_private;
+
+ drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+ encoder->base.id, encoder->name);
+
+ drm_atomic_bridge_chain_disable(bridge, old_state);
+
+ /* Right function depends upon target state. */
+ if (funcs) {
+ if (funcs->atomic_disable)
+ funcs->atomic_disable(encoder,
old_state);
+ else if (new_conn_state->crtc && funcs->prepare)
+ funcs->prepare(encoder);
+ else if (funcs->disable)
+ funcs->disable(encoder);
+ else if (funcs->dpms)
+ funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+ }
+
+ break;
+
+ case DRM_BRIDGE_POST_DISABLE:
+ drm_atomic_bridge_chain_post_disable(bridge, old_state);
- drm_atomic_bridge_chain_post_disable(bridge, old_state);
+ break;
+
+ default:
+ drm_err(dev, "Unrecognized Encoder/Bridge Operation
(%d).\n", op_type);
+ break;
+ }
}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+ struct drm_crtc *crtc;
+
Re: [PATCH] drm/nouveau: fix null pointer dereference in nouveau_connector_get_modes
> In nouveau_connector_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. 1. Can a wording approach (like the following) be a better change description? A null pointer is stored in the local variable “mode” after a call of the function “drm_mode_duplicate” failed. This pointer was passed to a subsequent call of the function “drm_mode_probed_add” where an undesirable dereference will be performed then. Thus add a corresponding return value check. 2. Would you like to add any tags (like “Fixes” and “Cc”) accordingly? 3. How do you think about to append parentheses to the function name in the summary phrase? 4. How do you think about to put similar results from static source code analyses into corresponding patch series? Regards, Markus
Re: [PATCH v4 11/11] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
On 22/06/2024 14:09, Aradhya Bhatia wrote:
The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).
Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
I think it makes sense to explain a bit about this in a comment in the
driver code. Otherwise doing all of this in pre_enable and post_disable
looks a bit odd.
Reviewed-by: Tomi Valkeinen
Tomi
Signed-off-by: Aradhya Bhatia
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c| 32 +++
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index c9697818308e..c352ea7db4ed 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,8 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -680,15 +680,6 @@ static void cdns_dsi_bridge_atomic_disable(struct
drm_bridge *bridge,
pm_runtime_put(dsi->base.dev);
}
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
-{
- struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
- struct cdns_dsi *dsi = input_to_dsi(input);
-
- pm_runtime_put(dsi->base.dev);
-}
-
static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
{
struct cdns_dsi_output *output = &dsi->output;
@@ -757,8 +748,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
dsi->link_initialized = true;
}
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
struct cdns_dsi *dsi = input_to_dsi(input);
@@ -909,19 +900,6 @@ static void cdns_dsi_bridge_atomic_enable(struct
drm_bridge *bridge,
writel(tmp, dsi->regs + MCTL_MAIN_EN);
}
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
- struct drm_bridge_state
*old_bridge_state)
-{
- struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
- struct cdns_dsi *dsi = input_to_dsi(input);
-
- if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
- return;
-
- cdns_dsi_init_link(dsi);
- cdns_dsi_hs_init(dsi);
-}
-
static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
struct drm_bridge_state
*bridge_state,
struct drm_crtc_state
*crtc_state,
@@ -952,9 +930,7 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct
drm_bridge *bridge,
static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
.attach = cdns_dsi_bridge_attach,
.mode_valid = cdns_dsi_bridge_mode_valid,
- .atomic_disable = cdns_dsi_bridge_atomic_disable,
.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
- .atomic_enable = cdns_dsi_bridge_atomic_enable,
.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
Re: [PATCH v2 06/17] backlight: ipaq-micro-backlight: Use backlight power constants
On Mon, Jun 24, 2024 at 5:20 PM Thomas Zimmermann wrote: > Replace FB_BLANK_ constants with their counterparts from the > backlight subsystem. The values are identical, so there's no > change in functionality. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH v2 05/17] backlight: gpio-backlight: Use backlight power constants
On Mon, Jun 24, 2024 at 5:20 PM Thomas Zimmermann wrote: > Replace FB_BLANK_ constants with their counterparts from the > backlight subsystem. The values are identical, so there's no > change in functionality. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Linus Walleij Yours, Linus Walleij
Re: [PATCH] drm/radeon: fix null pointer dereference in radeon_add_common_modes
> In radeon_add_common_modes(), the return value of drm_cvt_mode() is > assigned to mode, which will lead to a possible NULL pointer dereference > on failure of drm_cvt_mode(). Add a check to avoid npd. 1. Can a wording approach (like the following) be a better change description? A null pointer is stored in the local variable “mode” after a call of the function “drm_cvt_mode” failed. This pointer was passed to a subsequent call of the function “drm_mode_probed_add” where an undesirable dereference will be performed then. Thus add a corresponding return value check. 2. Would you like to add any tags (like “Fixes” and “Cc”) accordingly? 3. How do you think about to append parentheses to the function name in the summary phrase? Regards, Markus
[PATCH] drm/msm/adreno: fix a743 and a740 cx mem init
Disable the call to qcom_scm_gpu_init_regs() for a730 and a740
after init failures on the HDK8550 and HDK8450 platforms:
msm_dpu ae01000.display-controller: failed to load adreno gpu
msm_dpu ae01000.display-controller: failed to bind 3d0.gpu (ops a3xx_ops
[msm]): -5
msm_dpu ae01000.display-controller: adev bind failed: -5
While debugging, it happens the call to:
qcom_scm_gpu_init_regs(QCOM_SCM_GPU_ALWAYS_EN_REQ)
returns -5 and makes the gpu fail to initialize.
Remove the scm call since it's not done downstream either and
works fine without.
Fixes: 14b27d5df3ea ("drm/msm/a7xx: Initialize a750 "software fuse"")
Signed-off-by: Neil Armstrong
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index c98cdb1e9326..3ba45b804983 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1741,14 +1741,9 @@ static int a7xx_cx_mem_init(struct a6xx_gpu *a6xx_gpu)
REG_A7XX_CX_MISC_SW_FUSE_VALUE);
adreno_gpu->has_ray_tracing =
!!(fuse_val & A7XX_CX_MISC_SW_FUSE_VALUE_RAYTRACING);
- } else {
- if (adreno_is_a740(adreno_gpu)) {
- /* Raytracing is always enabled on a740 */
- adreno_gpu->has_ray_tracing = true;
- }
-
- if (qcom_scm_is_available())
- return
qcom_scm_gpu_init_regs(QCOM_SCM_GPU_ALWAYS_EN_REQ);
+ } else if (adreno_is_a740(adreno_gpu)) {
+ /* Raytracing is always enabled on a740 */
+ adreno_gpu->has_ray_tracing = true;
}
return 0;
---
base-commit: 62c97045b8f720c2eac807a5f38e26c9ed512371
change-id:
20240626-topic-sm8x50-upstream-fix-a730-a730-gpu-raytracing-init-46ac3f4cdd29
Best regards,
--
Neil Armstrong
Re: [PATCH] drm/msm/adreno: fix a743 and a740 cx mem init
On Wed, Jun 26, 2024 at 1:04 PM Neil Armstrong
wrote:
>
> Disable the call to qcom_scm_gpu_init_regs() for a730 and a740
> after init failures on the HDK8550 and HDK8450 platforms:
> msm_dpu ae01000.display-controller: failed to load adreno gpu
> msm_dpu ae01000.display-controller: failed to bind 3d0.gpu (ops a3xx_ops
> [msm]): -5
> msm_dpu ae01000.display-controller: adev bind failed: -5
>
> While debugging, it happens the call to:
> qcom_scm_gpu_init_regs(QCOM_SCM_GPU_ALWAYS_EN_REQ)
> returns -5 and makes the gpu fail to initialize.
>
> Remove the scm call since it's not done downstream either and
> works fine without.
>
> Fixes: 14b27d5df3ea ("drm/msm/a7xx: Initialize a750 "software fuse"")
> Signed-off-by: Neil Armstrong
> ---
Reviewed-by: Connor Abbott
Re: [PATCH] drm/qxl: fix null pointer dereference in qxl_add_mode
> In qxl_add_mode(), the return value of drm_cvt_mode() is assigned to mode, > which will lead to a possible NULL pointer dereference on failure of > drm_cvt_mode(). Add a check to avoid npd. 1. Can a wording approach (like the following) be a better change description? A null pointer is stored in the local variable “mode” after a call of the function “drm_cvt_mode” failed. This pointer was used in subsequent statements where an undesirable dereference will be performed then. Thus add a corresponding return value check. 2. Would you like to add any tags (like “Fixes” and “Cc”) accordingly? 3. How do you think about to append parentheses to the function name in the summary phrase? Regards, Markus
Re: [PATCH 1/3] dt-bindings: display: panel: Rename WL-355608-A8 panel
On 26/06/2024 13:17, Ryan Walklin wrote:
The WL-355608-A8 is a 3.5" 640x480@60Hz RGB LCD display from an unknown
OEM used in a number of handheld gaming devices made by Anbernic.
Previously committed using the OEM serial without a vendor prefix,
however the preference is to use the integrating device vendor and name
where the OEM is unknown.
Alter the filename and compatible string to reflect the convention.
Fixes: f08aac40639c ("drm: panel: nv3052c: Add WL-355608-A8 panel")
Signed-off-by: Ryan Walklin
---
.../{wl-355608-a8.yaml => anbernic,rg35xx-panel.yaml} | 8
1 file changed, 4 insertions(+), 4 deletions(-)
rename Documentation/devicetree/bindings/display/panel/{wl-355608-a8.yaml =>
anbernic,rg35xx-panel.yaml} (81%)
diff --git a/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
similarity index 81%
rename from Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
rename to
Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
index 397c26be9bda5..a7d5ad0f29389 100644
--- a/Documentation/devicetree/bindings/display/panel/wl-355608-a8.yaml
+++ b/Documentation/devicetree/bindings/display/panel/anbernic,rg35xx-panel.yaml
@@ -1,10 +1,10 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/display/panel/wl-355608-a8.yaml#
+$id: http://devicetree.org/schemas/display/panel/anbernic,rg35xx-panel.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: WL-355608-A8 3.5" (640x480 pixels) 24-bit IPS LCD panel
+title: Anbernic RG35XX (WL-355608-A8) 3.5" 640x480 24-bit IPS LCD panel
maintainers:
- Ryan Walklin
@@ -15,7 +15,7 @@ allOf:
properties:
compatible:
-const: wl-355608-a8
+const: anbernic,rg35xx-panel
reg:
maxItems: 1
@@ -41,7 +41,7 @@ examples:
#size-cells = <0>;
panel@0 {
-compatible = "wl-355608-a8";
+compatible = "anbernic,rg35xx-panel";
Can it be more specific ? because there's a lot of rg35xx defined in bindings:
anbernic,rg351m
anbernic,rg351v
anbernic,rg353p
anbernic,rg353ps
anbernic,rg353v
anbernic,rg353vs
anbernic,rg35xx-2024
anbernic,rg35xx-plus
anbernic,rg35xx-h
Neil
reg = <0>;
spi-3wire;
Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi wrapped functions
Hi,
On Wed, Jun 26, 2024 at 10:22:41AM +0530, Tejas Vipin wrote:
> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi:
> Introduce mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> ("drm/mipi-dsi: wrap more functions for streamline handling") for
> sitronix-st7703 based panels.
>
> Signed-off-by: Tejas Vipin
> ---
> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 836 +-
> 1 file changed, 400 insertions(+), 436 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> index 77b30e045a57..67e8e45498cb 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> @@ -69,7 +69,7 @@ struct st7703_panel_desc {
> unsigned int lanes;
> unsigned long mode_flags;
> enum mipi_dsi_pixel_format format;
> - int (*init_sequence)(struct st7703 *ctx);
> + void (*init_sequence)(struct mipi_dsi_multi_context *dsi_ctx);
> };
>
> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
> @@ -77,62 +77,58 @@ static inline struct st7703 *panel_to_st7703(struct
> drm_panel *panel)
> return container_of(panel, struct st7703, panel);
> }
>
> -static int jh057n_init_sequence(struct st7703 *ctx)
> +static void jh057n_init_sequence(struct mipi_dsi_multi_context *dsi_ctx)
> {
> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> -
> /*
>* Init sequence was supplied by the panel vendor. Most of the commands
>* resemble the ST7703 but the number of parameters often don't match
>* so it's likely a clone.
>*/
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
> -0xF1, 0x12, 0x83);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
> -0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00,
> 0x00,
> -0x00, 0x00);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
> -0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08,
> 0x70,
> -0x00);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
> -0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00,
> 0x00,
> -0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
> - msleep(20);
> -
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_UNKNOWN_BF, 0x02, 0x11,
> 0x00);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
> -0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5,
> 0x12,
> -0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27,
> 0x38,
> -0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C,
> 0x00,
> -0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31,
> 0x88,
> -0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88,
> 0x64,
> -0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88,
> 0x88,
> -0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00,
> -0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
> -0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00,
> -0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02,
> 0x88,
> -0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88,
> 0x13,
> -0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88,
> 0x88,
> -0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02,
> 0x00,
> -0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00,
> -0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30,
> 0x0A,
> -0xA5, 0x00, 0x00, 0x00, 0x00);
> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
> -0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> 0x37,
> -0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> 0x11,
> -0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C,
> 0x41,
> -0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F,
> 0x10,
> -0x11, 0x18);
> - msleep(20);
> -
> - return 0;
> + mipi_dsi_generic_write_seq_multi(dsi_ctx, ST7703_CMD_SETEXTC,
> + 0xF1,
Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi wrapped functions
Hi Guido,
On Wed, Jun 26, 2024 at 02:25:16PM GMT, Guido Günther wrote:
> [...]
> > - ret = ctx->desc->init_sequence(ctx);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> > - return ret;
> > - }
> > + ctx->desc->init_sequence(&dsi_ctx);
>
> Why no return early here in the error case (same for the other cases
> below) giving us an indication which step went wrong?
Return early is hidden in the wrapped *_multi() function calls.
> >
> > - ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> > - return ret;
> > - }
> > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> >
> > /* It takes the controller 120 msec to wake up after sleep. */
> > - msleep(120);
> > + mipi_dsi_msleep(&dsi_ctx, 120);
> >
> > - ret = mipi_dsi_dcs_set_display_on(dsi);
> > - if (ret)
> > - return ret;
> > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> >
> > - dev_dbg(ctx->dev, "Panel init sequence done\n");
>
> Would be nice to keep the debug message.
>
> > + if (!dsi_ctx.accum_err)
> > + dev_dbg(ctx->dev, "Panel init sequence done\n");
> >
> > - return 0;
> > + return dsi_ctx.accum_err;
> > }
> >
> > static int st7703_disable(struct drm_panel *panel)
> > {
> > struct st7703 *ctx = panel_to_st7703(panel);
> > struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > - int ret;
> > + struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
> >
> > - ret = mipi_dsi_dcs_set_display_off(dsi);
> > - if (ret < 0)
> > - dev_err(ctx->dev, "Failed to turn off the display: %d\n", ret);
> > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>
> Similar to the above: it'd be nice to keep the information which step
> failed.
That's done by the _multi() wrappers.
kind regards,
o.
Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi wrapped functions
Hi,
On 6/26/24 5:55 PM, Guido Günther wrote:
> Hi,
> On Wed, Jun 26, 2024 at 10:22:41AM +0530, Tejas Vipin wrote:
>> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi:
>> Introduce mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
>> ("drm/mipi-dsi: wrap more functions for streamline handling") for
>> sitronix-st7703 based panels.
>>
>> Signed-off-by: Tejas Vipin
>> ---
>> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 836 +-
>> 1 file changed, 400 insertions(+), 436 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> index 77b30e045a57..67e8e45498cb 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> @@ -69,7 +69,7 @@ struct st7703_panel_desc {
>> unsigned int lanes;
>> unsigned long mode_flags;
>> enum mipi_dsi_pixel_format format;
>> -int (*init_sequence)(struct st7703 *ctx);
>> +void (*init_sequence)(struct mipi_dsi_multi_context *dsi_ctx);
>> };
>>
>> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
>> @@ -77,62 +77,58 @@ static inline struct st7703 *panel_to_st7703(struct
>> drm_panel *panel)
>> return container_of(panel, struct st7703, panel);
>> }
>>
>> -static int jh057n_init_sequence(struct st7703 *ctx)
>> +static void jh057n_init_sequence(struct mipi_dsi_multi_context *dsi_ctx)
>> {
>> -struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> -
>> /*
>> * Init sequence was supplied by the panel vendor. Most of the commands
>> * resemble the ST7703 but the number of parameters often don't match
>> * so it's likely a clone.
>> */
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
>> - 0xF1, 0x12, 0x83);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
>> - 0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00,
>> 0x00,
>> - 0x00, 0x00);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
>> - 0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08,
>> 0x70,
>> - 0x00);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
>> - 0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00,
>> 0x00,
>> - 0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
>> -msleep(20);
>> -
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_UNKNOWN_BF, 0x02, 0x11,
>> 0x00);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
>> - 0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5,
>> 0x12,
>> - 0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27,
>> 0x38,
>> - 0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C,
>> 0x00,
>> - 0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31,
>> 0x88,
>> - 0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88,
>> 0x64,
>> - 0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88,
>> 0x88,
>> - 0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
>> - 0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> - 0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02,
>> 0x88,
>> - 0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88,
>> 0x13,
>> - 0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88,
>> 0x88,
>> - 0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02,
>> 0x00,
>> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30,
>> 0x0A,
>> - 0xA5, 0x00, 0x00, 0x00, 0x00);
>> -mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
>> - 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
>> 0x37,
>> - 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
>> 0x11,
>> - 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C,
>> 0x41,
>> - 0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F,
>> 0x10,
>> - 0x11, 0x18);
>> -msleep(20);
>> -
>> -return 0;
>> +mipi_dsi_g
Re: [PATCH] drm/mediatek: Fix bit depth overwritten for mtk_ovl_set bit_depth()
Hi, Jason:
Jason-JH.Lin 於 2024年6月24日 週一 下午5:57寫道：
>
> Refine the value and mask define of bit depth for mtk_ovl_set bit_depth().
> Use cmdq_pkt_write_mask() instead of cmdq_pkt_write() to avoid bit depth
> settings being overwritten.
Applied to mediatek-drm-next [1], thanks.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next
Regards,
Chun-Kuang.
>
> Fixes: fb36c5020c9c ("drm/mediatek: Add support for AR30 and BA30 overlays")
> Signed-off-by: Jason-JH.Lin
> ---
> Based on:
> https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 21 -
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 4b370bc0746d..d35f5b4b22c2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -42,7 +42,11 @@
> #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> #define DISP_REG_OVL_ADDR_MT2701 0x0040
> -#define DISP_REG_OVL_CLRFMT_EXT0x02D0
> +#define DISP_REG_OVL_CLRFMT_EXT0x02d0
> +#define OVL_CON_CLRFMT_BIT_DEPTH_MASK(n) (GENMASK(1, 0) << (4
> * (n)))
> +#define OVL_CON_CLRFMT_BIT_DEPTH(depth, n) ((depth) << (4 * (n)))
> +#define OVL_CON_CLRFMT_8_BIT (0)
> +#define OVL_CON_CLRFMT_10_BIT (1)
> #define DISP_REG_OVL_ADDR_MT8173 0x0f40
> #define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n))
> #define DISP_REG_OVL_HDR_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n) + 0x04)
> @@ -65,10 +69,6 @@
> 0 : OVL_CON_CLRFMT_RGB)
> #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \
> OVL_CON_CLRFMT_RGB : 0)
> -#define OVL_CON_CLRFMT_BIT_DEPTH_MASK(ovl) (0xFF << 4 * (ovl))
> -#define OVL_CON_CLRFMT_BIT_DEPTH(depth, ovl) (depth << 4 * (ovl))
> -#define OVL_CON_CLRFMT_8_BIT 0x00
> -#define OVL_CON_CLRFMT_10_BIT 0x01
> #defineOVL_CON_AEN BIT(8)
> #defineOVL_CON_ALPHA 0xff
> #defineOVL_CON_VIRT_FLIP BIT(9)
> @@ -273,22 +273,17 @@ static void mtk_ovl_set_bit_depth(struct device *dev,
> int idx, u32 format,
> struct cmdq_pkt *cmdq_pkt)
> {
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> - unsigned int reg;
> unsigned int bit_depth = OVL_CON_CLRFMT_8_BIT;
>
> if (!ovl->data->supports_clrfmt_ext)
> return;
>
> - reg = readl(ovl->regs + DISP_REG_OVL_CLRFMT_EXT);
> - reg &= ~OVL_CON_CLRFMT_BIT_DEPTH_MASK(idx);
> -
> if (is_10bit_rgb(format))
> bit_depth = OVL_CON_CLRFMT_10_BIT;
>
> - reg |= OVL_CON_CLRFMT_BIT_DEPTH(bit_depth, idx);
> -
> - mtk_ddp_write(cmdq_pkt, reg, &ovl->cmdq_reg,
> - ovl->regs, DISP_REG_OVL_CLRFMT_EXT);
> + mtk_ddp_write_mask(cmdq_pkt, OVL_CON_CLRFMT_BIT_DEPTH(bit_depth, idx),
> + &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_CLRFMT_EXT,
> + OVL_CON_CLRFMT_BIT_DEPTH_MASK(idx));
> }
>
> void mtk_ovl_config(struct device *dev, unsigned int w,
> --
> 2.18.0
>
Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi wrapped functions
Hi,
On Wed, Jun 26, 2024 at 06:23:51PM +0530, Tejas Vipin wrote:
> Hi,
>
> On 6/26/24 5:55 PM, Guido Günther wrote:
> > Hi,
> > On Wed, Jun 26, 2024 at 10:22:41AM +0530, Tejas Vipin wrote:
> >> Use functions introduced in commit 966e397e4f60 ("drm/mipi-dsi:
> >> Introduce mipi_dsi_*_write_seq_multi()") and commit f79d6d28d8fe
> >> ("drm/mipi-dsi: wrap more functions for streamline handling") for
> >> sitronix-st7703 based panels.
> >>
> >> Signed-off-by: Tejas Vipin
> >> ---
> >> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 836 +-
> >> 1 file changed, 400 insertions(+), 436 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> index 77b30e045a57..67e8e45498cb 100644
> >> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> >> @@ -69,7 +69,7 @@ struct st7703_panel_desc {
> >>unsigned int lanes;
> >>unsigned long mode_flags;
> >>enum mipi_dsi_pixel_format format;
> >> - int (*init_sequence)(struct st7703 *ctx);
> >> + void (*init_sequence)(struct mipi_dsi_multi_context *dsi_ctx);
> >> };
> >>
> >> static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
> >> @@ -77,62 +77,58 @@ static inline struct st7703 *panel_to_st7703(struct
> >> drm_panel *panel)
> >>return container_of(panel, struct st7703, panel);
> >> }
> >>
> >> -static int jh057n_init_sequence(struct st7703 *ctx)
> >> +static void jh057n_init_sequence(struct mipi_dsi_multi_context *dsi_ctx)
> >> {
> >> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> >> -
> >>/*
> >> * Init sequence was supplied by the panel vendor. Most of the commands
> >> * resemble the ST7703 but the number of parameters often don't match
> >> * so it's likely a clone.
> >> */
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
> >> - 0xF1, 0x12, 0x83);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
> >> - 0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00,
> >> 0x00,
> >> - 0x00, 0x00);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
> >> - 0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08,
> >> 0x70,
> >> - 0x00);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
> >> - 0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00,
> >> 0x00,
> >> - 0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
> >> - msleep(20);
> >> -
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_UNKNOWN_BF, 0x02, 0x11,
> >> 0x00);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
> >> - 0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5,
> >> 0x12,
> >> - 0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27,
> >> 0x38,
> >> - 0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C,
> >> 0x00,
> >> - 0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31,
> >> 0x88,
> >> - 0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88,
> >> 0x64,
> >> - 0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88,
> >> 0x88,
> >> - 0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00,
> >> 0x00,
> >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
> >> - 0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00,
> >> 0x00,
> >> - 0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02,
> >> 0x88,
> >> - 0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88,
> >> 0x13,
> >> - 0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88,
> >> 0x88,
> >> - 0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02,
> >> 0x00,
> >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >> 0x00,
> >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30,
> >> 0x0A,
> >> - 0xA5, 0x00, 0x00, 0x00, 0x00);
> >> - mipi_dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
> >> - 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
> >> 0x37,
> >> - 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
> >> 0x11,
> >> - 0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C,
> >> 0x41,
> >> -
Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
> > Move the bridge pre_enable call before crtc enable, and the bridge
> > post_disable call after the crtc disable.
> >
> > The sequence of enable after this patch will look like:
> >
> > bridge[n]_pre_enable
> > ...
> > bridge[1]_pre_enable
> >
> > crtc_enable
> > encoder_enable
> >
> > bridge[1]_enable
> > ...
> > bridge[n]__enable
> >
> > and vice-versa for the bridge chain disable sequence.
> >
> > The definition of bridge pre_enable hook says that,
> > "The display pipe (i.e. clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called".
> >
> > Since CRTC is also a source feeding the bridge, it should not be enabled
> > before the bridges in the pipeline are pre_enabled. Fix that by
> > re-ordering the sequence of bridge pre_enable and bridge post_disable.
> >
> > Signed-off-by: Aradhya Bhatia
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 165 ++--
> > include/drm/drm_atomic_helper.h | 7 ++
> > 2 files changed, 114 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index fb97b51b38f1..e8ad08634f58 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -74,6 +74,7 @@
> >* also shares the &struct drm_plane_helper_funcs function table with the
> > plane
> >* helpers.
> >*/
> > +
>
> Extra change.
>
> > static void
> > drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> > struct drm_plane_state *old_plane_state,
> > @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
> > }
> > static void
> > -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > +disable_encoder_brige_chain(struct drm_device *dev, struct
> > drm_atomic_state *old_state,
> > + enum bridge_chain_operation_type op_type)
> > {
> > struct drm_connector *connector;
> > struct drm_connector_state *old_conn_state, *new_conn_state;
> > - struct drm_crtc *crtc;
> > struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > int i;
> > @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct
> > drm_atomic_state *old_state)
> > if (WARN_ON(!encoder))
> > continue;
> > - funcs = encoder->helper_private;
> > -
> > - drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> > - encoder->base.id, encoder->name);
> > -
> > /*
> > * Each encoder has at most one connector (since we always steal
> > * it away), so we won't call disable hooks twice.
> > */
> > bridge = drm_bridge_chain_get_first_bridge(encoder);
> > - drm_atomic_bridge_chain_disable(bridge, old_state);
> > - /* Right function depends upon target state. */
> > - if (funcs) {
> > - if (funcs->atomic_disable)
> > - funcs->atomic_disable(encoder, old_state);
> > - else if (new_conn_state->crtc && funcs->prepare)
> > - funcs->prepare(encoder);
> > - else if (funcs->disable)
> > - funcs->disable(encoder);
> > - else if (funcs->dpms)
> > - funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > - }
> > + switch (op_type) {
> > + case DRM_ENCODER_BRIDGE_DISABLE:
> > + funcs = encoder->helper_private;
> > +
> > + drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> > + encoder->base.id, encoder->name);
> > +
> > + drm_atomic_bridge_chain_disable(bridge, old_state);
> > +
> > + /* Right function depends upon target state. */
> > + if (funcs) {
> > + if (funcs->atomic_disable)
> > + funcs->atomic_disable(encoder,
> > old_state);
> > + else if (new_conn_state->crtc && funcs->prepare)
> > + funcs->prepare(encoder);
> > + else if (funcs->disable)
> > + funcs->disable(encoder);
> > + else if (funcs->dpms)
> > + funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > + }
> > +
> > + break;
> > +
> > + case DRM_BRIDGE_POST_DISABLE:
> > + drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > - drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > + break;
> > +
> > +
Re: [PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()
Hi Tomi,
Thanks for reviewing the patches!
On 26/06/24 15:55, Tomi Valkeinen wrote:
> Hi,
>
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>> Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
>> during the _suspend().
>>
>> Also power-off the Phy from bridge_disable.
>>
>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>> Signed-off-by: Aradhya Bhatia
>> ---
>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 --
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 5159c3f0853e..d89c32bae2b9 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct
>> drm_bridge *bridge)
>> if (dsi->platform_ops && dsi->platform_ops->disable)
>> dsi->platform_ops->disable(dsi);
>> + phy_power_off(dsi->dphy);
>> + dsi->link_initialized = false;
>> + dsi->phy_initialized = false;
>> +
>> pm_runtime_put(dsi->base.dev);
>> }
>> @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>> DPHY_CMN_PDN | DPHY_PLL_PDN,
>> dsi->regs + MCTL_DPHY_CFG0);
>> - phy_init(dsi->dphy);
>> phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
>> phy_configure(dsi->dphy, &output->phy_opts);
>> phy_power_on(dsi->dphy);
>> @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct
>> device *dev)
>> clk_prepare_enable(dsi->dsi_p_clk);
>> clk_prepare_enable(dsi->dsi_sys_clk);
>> + phy_init(dsi->dphy);
>> +
>> return 0;
>> }
>> @@ -1127,10 +1132,11 @@ static int __maybe_unused
>> cdns_dsi_suspend(struct device *dev)
>> {
>> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>> + phy_exit(dsi->dphy);
>> +
>> clk_disable_unprepare(dsi->dsi_sys_clk);
>> clk_disable_unprepare(dsi->dsi_p_clk);
>> reset_control_assert(dsi->dsi_p_rst);
>> - dsi->link_initialized = false;
>> return 0;
>> }
>>
>
> So with this patch, phy_init/exit will be called in the resume/suspend
> functions. That looks fine.
>
> But the phy_power_on/phy_power_off looks odd to me. Here you add
> phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But
> phy_power_on() is called in cdns_dsi_hs_init(), and that is called in
> cdns_dsi_bridge_enable() (which sounds fine), but also in
> cdns_dsi_bridge_pre_enable().
>
> So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable()
> is extra, as it effectively does nothing (it exists right away if
> dsi->phy_initialized == true)?
That's right. When cdns_dsi_hs_init() is called from
cdns_dsi_bridge_enable(), it is simply expected to return since
phy_initialized is true.
I am not aware about the exact reasoning behind this, but this gets
addressed when I convert the _bridge_enable() to _bridge_pre_enable()
and drop the older _bridge_pre_enable() entirely.
Regards
Aradhya
