[PATCH] drm/msm: remove unnecessary NULL check
This NULL check was required when it was added, but we shuffled the code
around in commit 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation
to the _probe function") and now it's not. The inconsistent NULL
checking triggers a Smatch warning:
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
variable dereferenced before check 'mdp5_kms' (see line 782)
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 11d9fc2c6bf5..ec933d597e20 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -844,8 +844,7 @@ static int mdp5_init(struct platform_device *pdev, struct
drm_device *dev)
return 0;
fail:
- if (mdp5_kms)
- mdp5_destroy(mdp5_kms);
+ mdp5_destroy(mdp5_kms);
return ret;
}
--
2.39.2
[PATCH] drm/i915/gvt: Optimize mmio_offset_compare() for efficiency
The original code used conditional branching in the mmio_offset_compare function to compare two values and return -1, 1, or 0 based on the result. However, the list_sort comparison function only needs results <0, >0, or =0. This patch optimizes the code to make the comparison branchless, improving efficiency and reducing code size. This change reduces the number of comparison operations from 1-2 to a single subtraction operation, thereby saving the number of instructions. Signed-off-by: Kuan-Wei Chiu --- drivers/gpu/drm/i915/gvt/debugfs.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c index baccbf1761b7..998d82a259c8 100644 --- a/drivers/gpu/drm/i915/gvt/debugfs.c +++ b/drivers/gpu/drm/i915/gvt/debugfs.c @@ -48,11 +48,7 @@ static int mmio_offset_compare(void *priv, ma = container_of(a, struct diff_mmio, node); mb = container_of(b, struct diff_mmio, node); - if (ma->offset < mb->offset) - return -1; - else if (ma->offset > mb->offset) - return 1; - return 0; + return ma->offset - mb->offset; } static inline int mmio_diff_handler(struct intel_gvt *gvt, -- 2.25.1
[RFC] drm: bridge: samsung-dsim: Recalculate timings when rounding HFP up
When using video sync pulses, the HFP, HBP, and HSA are divided between
the available lanes if there is more than one lane. For certain
timings and lane configurations, the HFP may not be evenly divisible
and it gets rounded down which can cause certain resolutions and
refresh rates to not sync.
ie. 720p60 on some monitors show hss of 1390 and hdisplay of 1280. This
yields an HFP of 110. When taking the HFP of 110 divides along 4 lanes,
the result is 27.5 which rounds down to 27 and causes some monitors not
to sync.
The solultion is to round HFP up by finding the remainder of HFP /
the number of lanes and increasing the hsync_start, hsync_end, and
htotal to compensate when there is a remainder.
Signed-off-by: Adam Ford
---
This adds support for 720p60 in the i.MX8M Plus.
NXP uses a look-up table in their downstream code to accomplish this.
Using this calculation, the driver can adjust without the need for a
complicated table and should be flexible for different timings and
resolutions depending on the monitor.
I don't have a DSI analyzer, and this appears to only work on
i.MX8M Plus but not Mini and Nano for some reason, despite their
having a similar DSI bridge.
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index be5914caa17d..5564e85f6b63 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1628,6 +1628,26 @@ static int samsung_dsim_atomic_check(struct drm_bridge
*bridge,
adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC |
DRM_MODE_FLAG_PVSYNC);
}
+ /*
+* When using video sync pulses, the HFP, HBP, and HSA are divided
between
+* the available lanes if there is more than one lane. For certain
+* timings and lane configurations, the HFP may not be evenly divisible.
+* This can cause HFP to round down, and it ends up being too small
which can
+* cause some monitors to not sync properly. In these instances, round
HFP up
+* and adjust htotal to compensate. Through trial and error, it appears
that
+* the HBP and HSA do not appear to need the same correction that HFP
does.
+*/
+ if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE && dsi->lanes > 1)
{
+ int hfp = adjusted_mode->hsync_start - adjusted_mode->hdisplay;
+ int remainder = hfp % dsi->lanes;
+
+ if (remainder) {
+ adjusted_mode->hsync_start += remainder;
+ adjusted_mode->hsync_end += remainder;
+ adjusted_mode->htotal += remainder;
+ }
+ }
+
return 0;
}
--
2.40.1
Re: [PATCH v10 4/7] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver
Hi Sandor,
thanks for the updated series.
Am Freitag, 13. Oktober 2023, 05:24:23 CEST schrieb Sandor Yu:
> Add a new DRM DisplayPort and HDMI bridge driver for Candence MHDP8501
> used in i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort
> standards according embedded Firmware running in the uCPU.
>
> For iMX8MQ SOC, the DisplayPort/HDMI FW was loaded and activated by
> SOC's ROM code. Bootload binary included respective specific firmware
> is required.
>
> Driver will check display connector type and
> then load the corresponding driver.
>
> Signed-off-by: Sandor Yu
> Tested-by: Alexander Stein
> ---
> v9->v10:
> - struct cdns_mhdp_device is renamed to cdns_mhdp8501_device.
> - update for mhdp helper driver is introduced.
> Remove head file cdns-mhdp-mailbox.h and add cdns-mhdp-helper.h
> Add struct cdns_mhdp_base base to struct cdns_mhdp8501_device.
> Init struct cdns_mhdp_base base when driver probe.
>
> drivers/gpu/drm/bridge/cadence/Kconfig| 16 +
> drivers/gpu/drm/bridge/cadence/Makefile | 2 +
> .../drm/bridge/cadence/cdns-mhdp8501-core.c | 316
> .../drm/bridge/cadence/cdns-mhdp8501-core.h | 365 +
> .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c | 708 ++
> .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c | 673 +
> 6 files changed, 2080 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.h
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
>
> [...]
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file mode 100644
> index 0..73d1c35a74599
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> @@ -0,0 +1,673 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cadence MHDP8501 HDMI bridge driver
> + *
> + * Copyright (C) 2019-2023 NXP Semiconductor, Inc.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "cdns-mhdp8501-core.h"
> +
> +/**
> + * cdns_hdmi_infoframe_set() - fill the HDMI AVI infoframe
> + * @mhdp: phandle to mhdp device.
> + * @entry_id: The packet memory address in which the data is written.
> + * @packet_len: 32, only 32 bytes now.
> + * @packet: point to InfoFrame Packet.
> + * packet[0] = 0
> + * packet[1-3] = HB[0-2] InfoFrame Packet Header
> + * packet[4-31 = PB[0-27] InfoFrame Packet Contents
> + * @packet_type: Packet Type of InfoFrame in HDMI Specification.
> + *
> + */
> +static void cdns_hdmi_infoframe_set(struct cdns_mhdp8501_device *mhdp,
> + u8 entry_id, u8 packet_len,
> + u8 *packet, u8 packet_type)
> +{
> + u32 packet32, len32;
> + u32 val, i;
> +
> + /* only support 32 bytes now */
> + if (packet_len != 32)
> + return;
> +
> + /* invalidate entry */
> + val = F_ACTIVE_IDLE_TYPE(1) | F_PKT_ALLOC_ADDRESS(entry_id);
> + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
SOURCE_PIF_PKT_ALLOC_WR_EN);
> +
> + /* flush fifo 1 */
> + writel(F_FIFO1_FLUSH(1), mhdp->regs + SOURCE_PIF_FIFO1_FLUSH);
> +
> + /* write packet into memory */
> + len32 = packet_len / 4;
> + for (i = 0; i < len32; i++) {
> + packet32 = get_unaligned_le32(packet + 4 * i);
> + writel(F_DATA_WR(packet32), mhdp->regs +
SOURCE_PIF_DATA_WR);
> + }
> +
> + /* write entry id */
> + writel(F_WR_ADDR(entry_id), mhdp->regs + SOURCE_PIF_WR_ADDR);
> +
> + /* write request */
> + writel(F_HOST_WR(1), mhdp->regs + SOURCE_PIF_WR_REQ);
> +
> + /* update entry */
> + val = F_ACTIVE_IDLE_TYPE(1) | F_TYPE_VALID(1) |
> + F_PACKET_TYPE(packet_type) |
F_PKT_ALLOC_ADDRESS(entry_id);
> + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> +
> + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
SOURCE_PIF_PKT_ALLOC_WR_EN);
> +}
> +
> +static int cdns_hdmi_get_edid_block(void *data, u8 *edid,
> + u32 block, size_t length)
> +{
> + struct cdns_mhdp8501_device *mhdp = data;
> + u8 msg[2], reg[5], i;
> + int ret;
> +
> + mutex_lock(&mhdp->mbox_mutex);
> +
> + for (i = 0; i < 4; i++) {
> + msg[0] = block / 2;
> + msg[1] = block % 2;
> +
> + ret = cdns_mhdp_mailbox_send(&mhdp->base,
MB_MODULE_ID_HDMI_TX,
> HDMI_TX_EDID, +sizeof(msg),
msg);
> + if (ret)
> + continue;
> +
> + ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,
MB_MODULE_ID_HDMI_TX,
> + HDMI_TX_EDID,
sizeof(reg) +
Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver
Hi Sandor,
thanks for the updated series.
Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> MHDP8546 mailbox access functions will be share to other mhdp driver
> and Cadence HDP-TX HDMI/DP PHY drivers.
> Create a new mhdp helper driver and move all those functions into.
>
> cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(),
> because it use the DPTX command ID DPTX_WRITE_REGISTER.
>
> New cdns_mhdp_reg_write() is created with the general command ID
> GENERAL_REGISTER_WRITE.
>
> Signed-off-by: Sandor Yu
> ---
> v9->v10:
> *use mhdp helper driver to replace macro functions,
> move maibox access function and mhdp hdmi/dp common API
> functions into the driver.
>
> drivers/gpu/drm/bridge/cadence/Kconfig| 4
> drivers/gpu/drm/bridge/cadence/Makefile | 1 +
> .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 383 +++---
> .../drm/bridge/cadence/cdns-mhdp8546-core.h | 44 +-
> include/drm/bridge/cdns-mhdp-helper.h | 96 +
> 6 files changed, 473 insertions(+), 361 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
>
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> b/drivers/gpu/drm/bridge/cadence/Kconfig index ec35215a20034..0b7b4626a7af0
> 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
> the routing of the DSS DPI signal to the Cadence DSI.
> endif
>
> +config CDNS_MHDP_HELPER
> + tristate
> +
> config DRM_CDNS_MHDP8546
> tristate "Cadence DPI/DP bridge"
> select DRM_DISPLAY_DP_HELPER
> @@ -27,6 +30,7 @@ config DRM_CDNS_MHDP8546
> select DRM_DISPLAY_HELPER
> select DRM_KMS_HELPER
> select DRM_PANEL_BRIDGE
> + select CDNS_MHDP_HELPER
> depends on OF
> help
> Support Cadence DPI to DP bridge. This is an internal
> diff --git a/drivers/gpu/drm/bridge/cadence/Makefile
> b/drivers/gpu/drm/bridge/cadence/Makefile index
> c95fd5b81d137..087dc074820d7 100644
> --- a/drivers/gpu/drm/bridge/cadence/Makefile
> +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> cdns-dsi-y := cdns-dsi-core.o
> cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> cdns-mhdp8546-y := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) += cdns-mhdp8546-j721e.o
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file mode 100644
> index 0..2e3eee40494f0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> @@ -0,0 +1,306 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 NXP Semiconductor, Inc.
> + *
> + */
> +#include
> +#include
> +#include
> +
> +/* Mailbox helper functions */
> +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base)
> +{
> + int ret, empty;
> +
> + WARN_ON(!mutex_is_locked(base->mbox_mutex));
> +
> + ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_EMPTY,
> + empty, !empty, MAILBOX_RETRY_US,
> + MAILBOX_TIMEOUT_US);
> + if (ret < 0)
> + return ret;
> +
> + return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);
No need to export this. You can make this function actually static.
> +
> +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val)
> +{
> + int ret, full;
> +
> + WARN_ON(!mutex_is_locked(base->mbox_mutex));
> +
> + ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> + full, !full, MAILBOX_RETRY_US,
> + MAILBOX_TIMEOUT_US);
> + if (ret < 0)
> + return ret;
> +
> + writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);
No need to export that one as well. You can make this function actually static
as these two functions are only called from the helper itself.
> +
> +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> + u8 module_id, u8 opcode,
> + u16 req_size)
> +{
> + u32 mbox_size, i;
> + u8 header[4];
> + int ret;
> +
> + /* read the header of the message */
> + for (i = 0; i < sizeof(header); i++) {
> + ret = cdns_mhdp_mailbox_read(base);
> + if (ret < 0)
> + return ret;
> +
> + header[i] = ret;
> + }
> +
> + mbox_size = get_unaligned_be16(header + 2);
> +
> + if (op
Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family
Hi Javier,
thanks for this patch.
Am 12.10.23 um 23:38 schrieb Javier Martinez Canillas:
[...]
+static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
+ const struct iosys_map *vmap,
+ struct drm_rect *rect, u8 *buf,
+ u8 *data_array)
+{
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+ unsigned int dst_pitch = drm_rect_width(rect);
+ struct iosys_map dst;
+ int ret = 0;
+
+ /* Align x to display segment boundaries */
+ rect->x1 = round_down(rect->x1, SSD132X_SEGMENT_WIDTH);
+ rect->x2 = min_t(unsigned int, round_up(rect->x2,
SSD132X_SEGMENT_WIDTH),
+ssd130x->width);
+
+ ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+ if (ret)
+ return ret;
+
+ iosys_map_set_vaddr(&dst, buf);
+ drm_fb_xrgb_to_gray8(&dst, &dst_pitch, vmap, fb, rect);
Here's an idea for a follow-up patchset.
You could attempt to integrate the gray8 and mono conversions into
drm_fb_blit(). With some the right parameters, both, ssd130x and ssd132x
could use the same blitting code from BO to buffer.
+
+ drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+ ssd132x_update_rect(ssd130x, rect, buf, data_array);
+
+ return ret;
+}
+
static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
struct drm_atomic_state *state)
{
@@ -677,6 +901,43 @@ static int ssd130x_primary_plane_atomic_check(struct
drm_plane *plane,
return 0;
}
+static int ssd132x_primary_plane_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_state *state)
+{
+ struct drm_device *drm = plane->dev;
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+ struct drm_plane_state *plane_state =
drm_atomic_get_new_plane_state(state, plane);
+ struct ssd130x_plane_state *ssd130x_state =
to_ssd130x_plane_state(plane_state);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state;
+ const struct drm_format_info *fi;
+ unsigned int pitch;
+ int ret;
+
+ if (!crtc)
+ return -EINVAL;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ ret = drm_plane_helper_atomic_check(plane, state);
+ if (ret)
+ return ret;
+
+ fi = drm_format_info(DRM_FORMAT_R8);
+ if (!fi)
+ return -EINVAL;
+
+ pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+ ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+ if (!ssd130x_state->buffer)
+ return -ENOMEM;
It's unrelated to these patches and I know it's been discussed
endlessly, but I have a questions about buffer allocation. That memory
acts as another shadow buffer for the device's memory, such that format
conversion becomes easier.
But then, why is ->buffer part of the plane_state? Shouldn't it be part
of the plane and never be re-allocated? The real size of that buffer is
times (not ). That size is static over the
lifetime of the device. That would represent the semantics much better.
This would allow for additional changes: blit_rect and update_rect would
be much easier to separate: no more segment adjustments for the blit
code; only for updates. If the update code has high latency (IDK), you
could push it into a worker thread to run besides the DRM logic. The gud
and repaper drivers do something to this effect.
+
+ return 0;
+}
+
static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
{
@@ -711,6 +972,40 @@ static void ssd130x_primary_plane_atomic_update(struct
drm_plane *plane,
drm_dev_exit(idx);
}
+static void ssd132x_primary_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_state *state)
+{
+ struct drm_plane_state *plane_state =
drm_atomic_get_new_plane_state(state, plane);
+ struct drm_plane_state *old_plane_state =
drm_atomic_get_old_plane_state(state, plane);
+ struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
plane_state->crtc);
+ struct ssd130x_crtc_state *ssd130x_crtc_state =
to_ssd130x_crtc_state(crtc_state);
+ struct ssd130x_plane_state *ssd130x_plane_state =
to_ssd130x_plane_state(plane_state);
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct drm_atomic_helper_damage_iter iter;
+ struct drm_device *drm = plane->dev;
+ struct drm_rect dst_clip;
+ struct drm_rect damage;
+ i
Re: [PATCH] drm/msm: remove unnecessary NULL check
Hello,
On Fri, Oct 13, 2023 at 10:17:08AM +0300, Dan Carpenter wrote:
> This NULL check was required when it was added, but we shuffled the code
> around in commit 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation
> to the _probe function") and now it's not. The inconsistent NULL
> checking triggers a Smatch warning:
>
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
> variable dereferenced before check 'mdp5_kms' (see line 782)
>
> Signed-off-by: Dan Carpenter
LGTM
Reviewed-by: Uwe Kleine-König
This patch opportunity is valid since commit 1f50db2f3e1e
("drm/msm/mdp5: move resource allocation to the _probe function") but
applies to older trees (where it introduces a bug).
On one hand it's not really a fix, but maybe still add a Fixes: line to
ensure it's not backported to older stables? Hmm, I don't know.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 3/3] drm/rockchip: fix the plane format defination of rk3568/6
On Fri, Oct 13, 2023 at 02:43:31PM +0800, Andy Yan wrote: > Hi Sacha: > > On 10/13/23 14:11, Sascha Hauer wrote: > > On Thu, Oct 12, 2023 at 10:37:05AM +0800, Andy Yan wrote: > > > From: Andy Yan > > > > > > The cluster windows on rk3568/6 only support afbc format, > > > linear format(RGB/YUV) are not supported. > > > The cluster windows on rk3588 support both linear and afbc rgb > > > format, but for yuv format it only support afbc. > > > > > > The esmart windows on rk3588 support uv swap for yuyv, but > > > rk356x does not support it. > > It's a bit hard to track which sentence in the description refers to > > which change in the patch. Could you split this up into multiple patches > > to make this easier reviewable? > > > > Renaming of the formats could also be a separate patch. Patches marked > > with "no functional change" are nice and easy to review. > > > How do you like if I split the patch like bellow: > > PATCH 1 : fix the format When you say "The cluster windows on rk3568/6 only support afbc format, ..." and "The esmart windows on rk3588 support uv swap for yuyv, ..." it sounds like two orthogonal changes which should be done in two patches. > > PATCH 2: rename: s/formats_win_full_10bit/formats_cluster/ > > s/formats_win_full_10bit_yuyv/formats_rk356x_esmart/ > > s/formats_win_little/formats_win_smart/ I'd likely do the rename first, but I guess that's just a matter of taste. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2] drm/msm: remove unnecessary NULL check
This NULL check was required when it was added, but we shuffled the code
around and now it's not. The inconsistent NULL checking triggers a
Smatch warning:
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
variable dereferenced before check 'mdp5_kms' (see line 782)
Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe
function"
Signed-off-by: Dan Carpenter
---
v2: Added a Fixes tag. It's not really a bug fix and so adding the
fixes tag is slightly unfair but it should prevent this patch from
accidentally getting backported before the refactoring and causing an
issue.
Btw, fixes tags are often unfair like this. People look at fixes tags
and think, "the fix introduced a bug" but actually it's really common
that the fix was just not complete. But from a backporting perspective
it makes sense to tie them together.
Plus everyone introduces bugs. If you're not introducing bugs, then
you're probably not writing a lot of code.
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 11d9fc2c6bf5..ec933d597e20 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -844,8 +844,7 @@ static int mdp5_init(struct platform_device *pdev, struct
drm_device *dev)
return 0;
fail:
- if (mdp5_kms)
- mdp5_destroy(mdp5_kms);
+ mdp5_destroy(mdp5_kms);
return ret;
}
--
2.39.2
Re: [PATCH] drm/msm: remove unnecessary NULL check
On Fri, Oct 13, 2023 at 10:01:49AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Oct 13, 2023 at 10:17:08AM +0300, Dan Carpenter wrote:
> > This NULL check was required when it was added, but we shuffled the code
> > around in commit 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation
> > to the _probe function") and now it's not. The inconsistent NULL
> > checking triggers a Smatch warning:
> >
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
> > variable dereferenced before check 'mdp5_kms' (see line 782)
> >
> > Signed-off-by: Dan Carpenter
>
> LGTM
>
> Reviewed-by: Uwe Kleine-König
>
> This patch opportunity is valid since commit 1f50db2f3e1e
> ("drm/msm/mdp5: move resource allocation to the _probe function") but
> applies to older trees (where it introduces a bug).
> On one hand it's not really a fix, but maybe still add a Fixes: line to
> ensure it's not backported to older stables? Hmm, I don't know.
Sure. Being extra safe is good.
regards,
dan carpenter
Re: [PATCH] drm/loongson: Add support for the DC in LS2K1000 SoC
在 2023-10-12星期四的 00:26 +0800，Sui Jingfeng写道：
> LS2K1000 is a low end SoC (two core 1.0gHz), it don't has dedicated
> VRAM.
> It requires the framebuffer to be phisically continguous to scanout.
> The
> display controller in LS2K1000 don't has built-in GPIO hardware, the
> structure (register bit field) of its pixel, DC, GPU, DDR PLL are
> also
> defferent from other model. But for the display controller itself,
> Most of
> hardware features of it are same with ls7a1000.
>
> Below is a simple dts for it.
Why don't you write a proper, YAML-formatted binding?
This will help handling the correctness of device trees, and a binding
is required to allow the driver to enter.
>
> aliases {
> i2c0 = &i2c0;
> i2c1 = &i2c1;
> };
>
> reserved-memory {
> #address-cells = <2>;
> #size-cells = <2>;
> ranges;
>
> display_reserved: framebuffer@3000 {
> compatible = "shared-dma-pool";
> reg = <0x0 0x2000 0x0 0x0800>; /* 128M */
> linux,cma-default;
> };
> };
>
> i2c0: i2c-gpio-0 {
> compatible = "i2c-gpio";
> scl-gpios = <&gpio0 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> sda-gpios = <&gpio0 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> i2c-gpio,delay-us = <5>; /* ~100 kHz */
> status = "okay";
> };
>
> i2c1: i2c-gpio-1 {
> compatible = "i2c-gpio";
> scl-gpios = <&gpio0 33 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> sda-gpios = <&gpio0 32 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> i2c-gpio,delay-us = <5>; /* ~100 kHz */
> status = "okay";
> };
>
> display-controller@6,0 {
> reg = <0x3000 0x0 0x0 0x0 0x0>;
> interrupt-parent = <&liointc0>;
> interrupts = <28 IRQ_TYPE_LEVEL_HIGH>
> memory-region = <&display_reserved>;
Is a system-wide CMA pool enough for this usage? And for a display
controller, will 128M be too much? (I assume the Vivante GPU do not
require contingous memory).
> status = "okay";
> };
>
> This patch is tested on ls2k1000 evaluation board.
>
> $ dmesg | grep ":00:06.0"
>
> loongson :00:06.0: Found LS2K1000 SoC, revision: 0
> loongson :00:06.0: [drm] dc: 250MHz, ddr: 400MHz, gpu: 228MHz
> loongson :00:06.0: [drm] Using of reserved mem:
> 800@0x2000
> loongson :00:06.0: [drm] VRAM: 8192 pages ready
> loongson :00:06.0: [drm] GTT: 32768 pages ready
> loongson :00:06.0: [drm] display pipe-0 has a DVO
> loongson :00:06.0: [drm] display pipe-1 has a DVO
> loongson :00:06.0: [drm] Total 2 outputs
> loongson :00:06.0: [drm] registered irq: 28
> [drm] Initialized loongson 1.0.0 20220701 for :00:06.0 on minor
> 0
> loongson :00:06.0: [drm] fb0: loongsondrmfb frame buffer device
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/loongson/Makefile | 1 +
> drivers/gpu/drm/loongson/loongson_device.c | 59 +++
> drivers/gpu/drm/loongson/lsdc_drv.c | 44 -
> drivers/gpu/drm/loongson/lsdc_drv.h | 9 ++
> drivers/gpu/drm/loongson/lsdc_gfxpll.c | 11 +-
> drivers/gpu/drm/loongson/lsdc_gfxpll.h | 4 +
> drivers/gpu/drm/loongson/lsdc_gfxpll_2k1000.c | 141
> drivers/gpu/drm/loongson/lsdc_i2c.c | 41 +
> drivers/gpu/drm/loongson/lsdc_i2c.h | 4 +
> drivers/gpu/drm/loongson/lsdc_pixpll.c | 153
> +-
> drivers/gpu/drm/loongson/lsdc_pixpll.h | 4 +
> drivers/gpu/drm/loongson/lsdc_probe.c | 27
> drivers/gpu/drm/loongson/lsdc_probe.h | 2 +
> drivers/gpu/drm/loongson/lsdc_regs.h | 36 +
> 14 files changed, 528 insertions(+), 8 deletions(-)
> create mode 100644 drivers/gpu/drm/loongson/lsdc_gfxpll_2k1000.c
>
> diff --git a/drivers/gpu/drm/loongson/Makefile
> b/drivers/gpu/drm/loongson/Makefile
> index 91e72bd900c1..d6e709e19fba 100644
> --- a/drivers/gpu/drm/loongson/Makefile
> +++ b/drivers/gpu/drm/loongson/Makefile
> @@ -7,6 +7,7 @@ loongson-y := \
> lsdc_drv.o \
> lsdc_gem.o \
> lsdc_gfxpll.o \
> + lsdc_gfxpll_2k1000.o \
> lsdc_i2c.o \
> lsdc_irq.o \
> lsdc_output_7a1000.o \
> diff --git a/drivers/gpu/drm/loongson/loongson_device.c
> b/drivers/gpu/drm/loongson/loongson_device.c
> index 9986c8a2a255..67274d9e3b12 100644
> --- a/drivers/gpu/drm/loongson/loongson_device.c
> +++ b/drivers/gpu/drm/loongson/loongson_device.c
> @@ -6,6 +6,7 @@
> #include
>
> #include "lsdc_drv.h"
> +#include "lsdc_probe.h"
>
> static const struct lsdc_kms_funcs ls7a1000_kms_funcs = {
> .create_i2c = lsdc_create_i2c_chan,
> @@ -25,6 +26,20 @@ static const struct lsdc_kms_funcs
> ls7a2000_kms_funcs = {
> .crtc_init = ls7a2000_crtc_init,
> };
>
> +/*
> + * Most of hardware features of ls2k1000 are same with ls7a1000, we
> take the
> + * ls7a1000_kms_funcs as a prototype,
[v4 0/3] Break out as separate driver from boe-tv101wum-nl6 panel driver
Linus series proposed to break out ili9882t as separate driver, but he didn't have time for that extensive rework of the driver. As discussed by Linus and Doug [1], keep macro using the "struct panel_init_cmd" until we get some resolution about the binary size issue. [1]: https://lore.kernel.org/all/20230703-fix-boe-tv101wum-nl6-v3-0-bd6e9432c...@linaro.org In [v1 2/2], Doug suggested move the ili9882t_enter_sleep_mode function to ili9882t_unprepare. I tried this scheme and the test failed. I will continue to investigate the rootcause with ilitek, which may take a long time . So if possible, apply this patch first. If there are new solutions later, I will continue upstream. [v1 2/2] https://lore.kernel.org/all/CAD=FV=XtqPJ77dx8uRb0=tmvc3cvgh5x+7mujexgcg228kz...@mail.gmail.com/ Changes in v4: - PATCH 2/3: Change usleep_range(5,51000) to msleep(50);. - Link to v3: https://lore.kernel.org/all/20231012121004.2127918-1-yangco...@huaqin.corp-partner.google.com/ Changes in v3: - PATCH 1/3: Remove "init_cmd_length" and "linux/of_device.h" . - PATCH 2/3: Change usleep_range(5,51000). - PATCH 3/3: Add a little background for commit. - Link to v2: https://lore.kernel.org/all/20231010121402.3687948-1-yangco...@huaqin.corp-partner.google.com/ Changes in v2: - PATCH 1/3: fix Doug comments,define "_INIT_SWITCH_PAGE_CMD" and remove the "shutdown". - PATCH 2/3: Modify ili9882t_switch_page function instead of hardcoding. - PATCH 3/3: Enable new config in defconfig. - Link to v1: https://lore.kernel.org/all/20231007060639.725350-1-yangco...@huaqin.corp-partner.google.com/ Cong Yang (3): drm/panel: ili9882t: Break out as separate driver drm/panel: ili9882t: Avoid blurred screen from fast sleep arm64: defconfig: Enable ILITEK_ILI9882T panel arch/arm64/configs/defconfig | 1 + drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile| 1 + .../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 371 - drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 779 ++ 5 files changed, 790 insertions(+), 371 deletions(-) create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c -- 2.25.1
[v4 1/3] drm/panel: ili9882t: Break out as separate driver
The Starry ILI9882t-based panel should never have been part of the boe
tv101wum driver, it is clearly based on the Ilitek ILI9882t display
controller and if you look at the custom command sequences for the
panel these clearly contain the signature Ilitek page switch (0xff)
commands. The hardware has nothing in common with the other panels
supported by this driver.
Break this out into a separate driver and config symbol instead.
If the placement here is out of convenience for using similar code,
we should consider creating a helper library instead.
Co-developed-by: Linus Walleij
Signed-off-by: Linus Walleij
Reviewed-by: Linus Walleij
Reviewed-by: Douglas Anderson
Signed-off-by: Cong Yang
---
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile| 1 +
.../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 371 -
drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 759 ++
4 files changed, 769 insertions(+), 371 deletions(-)
create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index ecb22ea326cb..99e14dc212ec 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -203,6 +203,15 @@ config DRM_PANEL_ILITEK_ILI9881C
Say Y if you want to enable support for panels based on the
Ilitek ILI9881c controller.
+config DRM_PANEL_ILITEK_ILI9882T
+ tristate "Ilitek ILI9882t-based panels"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ help
+ Say Y if you want to enable support for panels based on the
+ Ilitek ILI9882t controller.
+
config DRM_PANEL_INNOLUX_EJ030NA
tristate "Innolux EJ030NA 320x480 LCD panel"
depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index e14ce55a0875..d10c3de51c6d 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_DRM_PANEL_HIMAX_HX8394) += panel-himax-hx8394.o
obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o
obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
+obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9882T) += panel-ilitek-ili9882t.o
obj-$(CONFIG_DRM_PANEL_INNOLUX_EJ030NA) += panel-innolux-ej030na.o
obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
obj-$(CONFIG_DRM_PANEL_JADARD_JD9365DA_H3) += panel-jadard-jd9365da-h3.o
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 5ac926281d2c..4f370bc6dca8 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1370,346 +1370,6 @@ static const struct panel_init_cmd
starry_himax83102_j02_init_cmd[] = {
{},
};
-static const struct panel_init_cmd starry_ili9882t_init_cmd[] = {
- _INIT_DELAY_CMD(5),
- _INIT_DCS_CMD(0xFF, 0x98, 0x82, 0x01),
- _INIT_DCS_CMD(0x00, 0x42),
- _INIT_DCS_CMD(0x01, 0x11),
- _INIT_DCS_CMD(0x02, 0x00),
- _INIT_DCS_CMD(0x03, 0x00),
-
- _INIT_DCS_CMD(0x04, 0x01),
- _INIT_DCS_CMD(0x05, 0x11),
- _INIT_DCS_CMD(0x06, 0x00),
- _INIT_DCS_CMD(0x07, 0x00),
-
- _INIT_DCS_CMD(0x08, 0x80),
- _INIT_DCS_CMD(0x09, 0x81),
- _INIT_DCS_CMD(0x0A, 0x71),
- _INIT_DCS_CMD(0x0B, 0x00),
-
- _INIT_DCS_CMD(0x0C, 0x00),
- _INIT_DCS_CMD(0x0E, 0x1A),
-
- _INIT_DCS_CMD(0x24, 0x00),
- _INIT_DCS_CMD(0x25, 0x00),
- _INIT_DCS_CMD(0x26, 0x00),
- _INIT_DCS_CMD(0x27, 0x00),
-
- _INIT_DCS_CMD(0x2C, 0xD4),
- _INIT_DCS_CMD(0xB9, 0x40),
-
- _INIT_DCS_CMD(0xB0, 0x11),
-
- _INIT_DCS_CMD(0xE6, 0x32),
- _INIT_DCS_CMD(0xD1, 0x30),
-
- _INIT_DCS_CMD(0xD6, 0x55),
-
- _INIT_DCS_CMD(0xD0, 0x01),
- _INIT_DCS_CMD(0xE3, 0x93),
- _INIT_DCS_CMD(0xE4, 0x00),
- _INIT_DCS_CMD(0xE5, 0x80),
-
- _INIT_DCS_CMD(0x31, 0x07),
- _INIT_DCS_CMD(0x32, 0x07),
- _INIT_DCS_CMD(0x33, 0x07),
- _INIT_DCS_CMD(0x34, 0x07),
- _INIT_DCS_CMD(0x35, 0x07),
- _INIT_DCS_CMD(0x36, 0x01),
- _INIT_DCS_CMD(0x37, 0x00),
- _INIT_DCS_CMD(0x38, 0x28),
- _INIT_DCS_CMD(0x39, 0x29),
- _INIT_DCS_CMD(0x3A, 0x11),
- _INIT_DCS_CMD(0x3B, 0x13),
- _INIT_DCS_CMD(0x3C, 0x15),
- _INIT_DCS_CMD(0x3D, 0x17),
- _INIT_DCS_CMD(0x3E, 0x09),
- _INIT_DCS_CMD(0x3F, 0x0D),
- _INIT_DCS_CMD(0x40, 0x02),
- _INIT_DCS_CMD(0x41, 0x02),
- _INIT_DCS_CMD(0x42, 0x02),
- _INIT_DCS_CMD(0x43, 0x02),
- _INIT_DCS_CMD(0x44, 0x02),
- _INIT_DCS_CMD(0x45, 0x02),
- _INIT_DCS_CMD(0x46, 0x02),
-
- _INIT_DCS_CMD(0x47, 0x07),
- _INIT_DCS_CMD(0x48, 0x07),
- _INIT_DCS_CMD(0x49, 0x07),
- _INIT_DCS_
[v4 2/3] drm/panel: ili9882t: Avoid blurred screen from fast sleep
At present, we have found that there may be a problem of blurred
screen during fast sleep/resume. The direct cause of the blurred
screen is that the IC does not receive 0x28/0x10. Because of the
particularity of the IC, before the panel enters sleep hid must
stop scanning, as i2c_hid_core_suspend before ili9882t_disable.
If move the ili9882t_enter_sleep_mode function to ili9882t_unprepare,
touch reset will pull low before panel entersleep, which does not meet
the timing requirements.. So in order to solve this problem, the IC
can handle it through the exception mechanism when it cannot receive
0x28/0x10 command. Handling exceptions requires a reset 50ms delay.
Refer to vendor detailed analysis [1].
Ilitek vendor also suggested switching the page before entering sleep to
avoid panel IC not receiving 0x28/0x10 command.
Note: 0x28 is display off, 0x10 is sleep in.
[1]: https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence
Signed-off-by: Cong Yang
---
drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 22 ++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
index 93a40c2f1483..267a5307041c 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
@@ -463,6 +463,24 @@ static int ili9882t_init_dcs_cmd(struct ili9882t *ili)
return 0;
}
+static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
+{
+ int ret;
+ const struct panel_init_cmd cmd = _INIT_SWITCH_PAGE_CMD(page);
+
+ ret = mipi_dsi_dcs_write(dsi, cmd.data[0],
+cmd.len <= 1 ? NULL :
+&cmd.data[1],
+cmd.len - 1);
+ if (ret) {
+ dev_err(&dsi->dev,
+ "error switching panel controller page (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
{
struct mipi_dsi_device *dsi = ili->dsi;
@@ -484,8 +502,10 @@ static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
static int ili9882t_disable(struct drm_panel *panel)
{
struct ili9882t *ili = to_ili9882t(panel);
+ struct mipi_dsi_device *dsi = ili->dsi;
int ret;
+ ili9882t_switch_page(dsi, 0x00);
ret = ili9882t_enter_sleep_mode(ili);
if (ret < 0) {
dev_err(panel->dev, "failed to set panel off: %d\n", ret);
@@ -546,7 +566,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
gpiod_set_value(ili->enable_gpio, 1);
usleep_range(1000, 2000);
gpiod_set_value(ili->enable_gpio, 0);
- usleep_range(1000, 2000);
+ msleep(50);
gpiod_set_value(ili->enable_gpio, 1);
usleep_range(6000, 1);
--
2.25.1
[v4 3/3] arm64: defconfig: Enable ILITEK_ILI9882T panel
DRM_PANEL_ILITEK_ILI9882T is being split out from DRM_PANEL_BOE_TV101WUM_NL6. Since the arm64 defconfig had the BOE panel driver enabled, let's also enable the Ilitek driver. Reviewed-by: Douglas Anderson Signed-off-by: Cong Yang --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 0777bcae9104..c3453dcbad3e 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -813,6 +813,7 @@ CONFIG_DRM_PANEL_BOE_TV101WUM_NL6=m CONFIG_DRM_PANEL_LVDS=m CONFIG_DRM_PANEL_SIMPLE=m CONFIG_DRM_PANEL_EDP=m +CONFIG_DRM_PANEL_ILITEK_ILI9882T=m CONFIG_DRM_PANEL_MANTIX_MLAF057WE51=m CONFIG_DRM_PANEL_RAYDIUM_RM67191=m CONFIG_DRM_PANEL_SITRONIX_ST7703=m -- 2.25.1
Re: [PATCH] drm/atomic: Perform blocking commits on workqueue
On Thu, Oct 12, 2023 at 02:19:41PM -0400, Ray Strode wrote:
> Hi,
>
> On Mon, Oct 09, 2023 at 02:36:17PM +0200, Christian König wrote:
> > > > > To be clear, my take is, if driver code is running in process context
> > > > > and needs to wait for periods of time on the order of or in excess of
> > > > > a typical process time slice it should be sleeping during the waiting.
> > > > > If the operation is at a point where it can be cancelled without side
> > > > > effects, the sleeping should be INTERRUPTIBLE. If it's past the point
> > > > > of no return, sleeping should be UNINTERRUPTIBLE. At no point, in my
> > > > > opinion, should kernel code busy block a typical process for dozens of
> > > > > milliseconds while keeping the process RUNNING. I don't think this is
> > > > > a controversial take.
> > > > Exactly that's what I completely disagree on.
>
> Okay if we can't agree that it's not okay for user space (or the
> kernel running in the context of user space) to busy loop a cpu core
> at 100% utilization throughout and beyond the process's entire
> scheduled time slice then we really are at an impasse. I gotta say i'm
> astonished that this seemingly indefensible behavior is somehow a
> point of contention, but I'm not going to keep arguing about it beyond
> this email.
>
> I mean we're not talking about scientific computing, or code
> compilation, or seti@home. We're talking about nearly the equivalent
> of `while (1) __asm__ ("nop");`
I don't think anyone said this shouldn't be fixed or improved.
What I'm saying is that the atomic ioctl is not going to make guarantees
that it will not take up to much cpu time (for some extremely vague value
of "too much") to the point that userspace can configure it's compositor
in a way that it _will_ get killed if we _ever_ violate this rule.
We should of course try to do as good as job as possible, but that's not
what you're asking for. You're asking for a hard real-time guarantee with
the implication if we ever break it, it's a regression, and the kernel has
to bend over backwards with tricks like in your patch to make it work.
It's that hard real time guarantee of "the kernel will _never_ violate
this cpu time bound" that people are objecting against. Fixing the worst
offenders I don't think will see any pushback at all.
> > > The key point here is that the patch puts the work into the background
> > > just
> > > to avoid that it is accounted to the thread issuing it, and that in turn
> > > is
> > > not valid as far as I can see.
> >
> > Yeah it's that aspect I'm really worried about, because we essentially
> > start to support some gurantees that a) most drivers can't uphold without
> > a huge amount of work, some of the DC state recomputations are _really_
> > expensive b) without actually making the semantics clear, it's just
> > duct-tape.
>
> If DC plane state computation (or whatever) is really taking 50ms or
> 200ms, then it probably should be done in chunks so it doesn't get
> preempted at an inopportune point? Look, this is not my wheelhouse,
> this is your wheelhouse, and I don't want to keep debating forever. It
> seems there is a discrepancy between our understandings of implied
> acceptable behavior.
>
> > Yes compositors want to run kms in real-time, and yes that results in fun
> > if you try to strictly account for cpu time spent. Especially if your
> > policy is to just nuke the real time thread instead of demoting it to
> > SCHED_NORMAL for a time.
>
> So I ended up going with this suggestion for blocking modesets:
>
> https://gitlab.gnome.org/GNOME/mutter/-/commit/5d3e31a49968fc0da04e98c0f9d624ea5095c9e0
>
> But *this* feels like duct tape: You've already said there's no
> guarantee the problem won't also happen during preliminary computation
> during non-blocking commits or via other drm entry points. So it
> really does seem like a fix that won't age well. I won't be surprised
> if in ~3 years (or whatever) in some RHEL release there's a customer
> bug leading to the real-time thread getting blocklisted for obscure
> server display hardware because it's causing the session to tank on a
> production machine.
Yes the atomic ioctl makes no guarantees across drivers and hw platforms
of guaranteed "we will never violate, you can kill your compositor if you
do" cpu bound limits. We'll try to not suck too badly, and we'll try to
focus on fixing the suck where it upsets the people the most.
But there is fundamentally no hard real time guarantee in atomic. At least
not with the current uapi.
> > I think if we want more than hacks here we need to answer two questions:
> > - which parts of the kms api are real time
> > - what exactly do we guarantee with that
>
> imo, this isn't just about real-time versus non-real-time. It's no
> more acceptable for non-real-time mutter to be using 100% CPU doing
> busywaits than it is for real-time mutter to be using 100% cpu doing
> busywaits.
The problem isn't about wasting cpu time. It's about y
Re: [PATCH v2] drm/msm: remove unnecessary NULL check
On Fri, Oct 13, 2023 at 11:25:15AM +0300, Dan Carpenter wrote:
> This NULL check was required when it was added, but we shuffled the code
> around and now it's not. The inconsistent NULL checking triggers a
> Smatch warning:
>
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
> variable dereferenced before check 'mdp5_kms' (see line 782)
>
> Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe
> function"
> Signed-off-by: Dan Carpenter
Reviewed-by: Uwe Kleine-König
(already provided for (implicit) v1, but wasn't picked up)
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] drm/atomic: Perform blocking commits on workqueue
On 10/13/23 11:41, Daniel Vetter wrote:
> On Thu, Oct 12, 2023 at 02:19:41PM -0400, Ray Strode wrote:
>> On Mon, Oct 09, 2023 at 02:36:17PM +0200, Christian König wrote:
>> To be clear, my take is, if driver code is running in process context
>> and needs to wait for periods of time on the order of or in excess of
>> a typical process time slice it should be sleeping during the waiting.
>> If the operation is at a point where it can be cancelled without side
>> effects, the sleeping should be INTERRUPTIBLE. If it's past the point
>> of no return, sleeping should be UNINTERRUPTIBLE. At no point, in my
>> opinion, should kernel code busy block a typical process for dozens of
>> milliseconds while keeping the process RUNNING. I don't think this is
>> a controversial take.
> Exactly that's what I completely disagree on.
>>
>> Okay if we can't agree that it's not okay for user space (or the
>> kernel running in the context of user space) to busy loop a cpu core
>> at 100% utilization throughout and beyond the process's entire
>> scheduled time slice then we really are at an impasse. I gotta say i'm
>> astonished that this seemingly indefensible behavior is somehow a
>> point of contention, but I'm not going to keep arguing about it beyond
>> this email.
>>
>> I mean we're not talking about scientific computing, or code
>> compilation, or seti@home. We're talking about nearly the equivalent
>> of `while (1) __asm__ ("nop");`
>
> I don't think anyone said this shouldn't be fixed or improved.
>
> What I'm saying is that the atomic ioctl is not going to make guarantees
> that it will not take up to much cpu time (for some extremely vague value
> of "too much") to the point that userspace can configure it's compositor
> in a way that it _will_ get killed if we _ever_ violate this rule.
>
> We should of course try to do as good as job as possible, but that's not
> what you're asking for. You're asking for a hard real-time guarantee with
> the implication if we ever break it, it's a regression, and the kernel has
> to bend over backwards with tricks like in your patch to make it work.
I don't think mutter really needs or wants such a hard real-time guarantee.
What it needs is a fighting chance to react before the kernel kills its process.
The intended mechanism for this is SIGXCPU, but that can't work if the kernel
is stuck in a busy-loop. Ray's patch seems like one way to avoid that.
That said, as long as SIGXCPU can work as intended with the non-blocking
commits mutter uses for everything except modesets, mutter's workaround of
dropping RT priority for the blocking commits seems acceptable for the time
being.
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
Re: [PATCH] drm/loongson: Add support for the DC in LS2K1000 SoC
Hi,
On 2023/10/13 16:22, Icenowy Zheng wrote:
在 2023-10-12星期四的 00:26 +0800，Sui Jingfeng写道：
LS2K1000 is a low end SoC (two core 1.0gHz), it don't has dedicated
VRAM.
It requires the framebuffer to be phisically continguous to scanout.
The
display controller in LS2K1000 don't has built-in GPIO hardware, the
structure (register bit field) of its pixel, DC, GPU, DDR PLL are
also
defferent from other model. But for the display controller itself,
Most of
hardware features of it are same with ls7a1000.
Below is a simple dts for it.
Why don't you write a proper, YAML-formatted binding?
This patch use only one DT property, that is the "memory-region =
<&display_reserved>;".
But the memory-region property is a common property, I means that everyone know
how to
use this property. I'm not sure the if YAML documentation is strictly required
now. And
the checkpatch.pl show no warnings to me.
This will help handling the correctness of device trees, and a binding
is required to allow the driver to enter.
Yeah, this may be true. But I have forget the rules and I don't know
what the status now. I remember that a reviewer told me to drop dts and
write(provide) generic code only in the past. Maybe the arch-specific
maintainers want to provide a dts. I don't know what's the status for now, I am waiting some feedback.
aliases {
i2c0 = &i2c0;
i2c1 = &i2c1;
};
reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;
display_reserved: framebuffer@3000 {
compatible = "shared-dma-pool";
reg = <0x0 0x2000 0x0 0x0800>; /* 128M */
linux,cma-default;
};
};
i2c0: i2c-gpio-0 {
compatible = "i2c-gpio";
scl-gpios = <&gpio0 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
sda-gpios = <&gpio0 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
i2c-gpio,delay-us = <5>; /* ~100 kHz */
status = "okay";
};
i2c1: i2c-gpio-1 {
compatible = "i2c-gpio";
scl-gpios = <&gpio0 33 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
sda-gpios = <&gpio0 32 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
i2c-gpio,delay-us = <5>; /* ~100 kHz */
status = "okay";
};
display-controller@6,0 {
reg = <0x3000 0x0 0x0 0x0 0x0>;
interrupt-parent = <&liointc0>;
interrupts = <28 IRQ_TYPE_LEVEL_HIGH>
memory-region = <&display_reserved>;
Is a system-wide CMA pool enough for this usage? And for a display
controller, will 128M be too much? (I assume the Vivante GPU do not
require contingous memory).
Yes, Vivante GPU do not require continuous memory. As the GPU has IOMMU
which map the scatter physical pages to continuous virtual address.
But loongson display control don't has such a luxury. The dumb framebuffer
has to be physically contiguous to scanout.
128M is not too much, as we have plenty system RAM. 64M is also OK.
status = "okay";
};
This patch is tested on ls2k1000 evaluation board.
$ dmesg | grep ":00:06.0"
loongson :00:06.0: Found LS2K1000 SoC, revision: 0
loongson :00:06.0: [drm] dc: 250MHz, ddr: 400MHz, gpu: 228MHz
loongson :00:06.0: [drm] Using of reserved mem:
800@0x2000
loongson :00:06.0: [drm] VRAM: 8192 pages ready
loongson :00:06.0: [drm] GTT: 32768 pages ready
loongson :00:06.0: [drm] display pipe-0 has a DVO
loongson :00:06.0: [drm] display pipe-1 has a DVO
loongson :00:06.0: [drm] Total 2 outputs
loongson :00:06.0: [drm] registered irq: 28
[drm] Initialized loongson 1.0.0 20220701 for :00:06.0 on minor
0
loongson :00:06.0: [drm] fb0: loongsondrmfb frame buffer device
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/loongson/Makefile | 1 +
drivers/gpu/drm/loongson/loongson_device.c | 59 +++
drivers/gpu/drm/loongson/lsdc_drv.c | 44 -
drivers/gpu/drm/loongson/lsdc_drv.h | 9 ++
drivers/gpu/drm/loongson/lsdc_gfxpll.c | 11 +-
drivers/gpu/drm/loongson/lsdc_gfxpll.h | 4 +
drivers/gpu/drm/loongson/lsdc_gfxpll_2k1000.c | 141
drivers/gpu/drm/loongson/lsdc_i2c.c | 41 +
drivers/gpu/drm/loongson/lsdc_i2c.h | 4 +
drivers/gpu/drm/loongson/lsdc_pixpll.c | 153
+-
drivers/gpu/drm/loongson/lsdc_pixpll.h | 4 +
drivers/gpu/drm/loongson/lsdc_probe.c | 27
drivers/gpu/drm/loongson/lsdc_probe.h | 2 +
drivers/gpu/drm/loongson/lsdc_regs.h | 36 +
14 files changed, 528 insertions(+), 8 deletions(-)
create mode 100644 drivers/gpu/drm/loongson/lsdc_gfxpll_2k1000.c
diff --git a/drivers/gpu/drm/loongson/Makefile
b/drivers/gpu/drm/loongson/Makefile
index 91e72bd900c1..d6e709e19fba 100644
--- a/drivers/gpu/drm/loongson/Makefile
+++ b/drivers/gpu/drm/loongson/Makefile
@@ -7,6 +7,7 @@ loongson-y := \
lsdc_drv.o \
Re: [PATCH] drm/edid: add 8 bpc quirk to the BenQ GW2765
On Thu, Oct 12, 2023 at 02:49:27PM -0400, Hamza Mahfooz wrote:
> The BenQ GW2765 reports that it supports higher (> 8) bpc modes, but
> when trying to set them we end up with a black screen. So, limit it to 8
> bpc modes.
Bad cable/etc was ruled out as the cause?
>
> Cc: sta...@vger.kernel.org # 6.5+
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2610
> Signed-off-by: Hamza Mahfooz
> ---
> drivers/gpu/drm/drm_edid.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 0454da505687..bca2af4fe1fc 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -123,6 +123,9 @@ static const struct edid_quirk {
> /* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
> EDID_QUIRK('A', 'E', 'O', 0, EDID_QUIRK_FORCE_6BPC),
>
> + /* BenQ GW2765 */
> + EDID_QUIRK('B', 'N', 'Q', 0x78d6, EDID_QUIRK_FORCE_8BPC),
> +
> /* BOE model on HP Pavilion 15-n233sl reports 8 bpc, but is a 6 bpc
> panel */
> EDID_QUIRK('B', 'O', 'E', 0x78b, EDID_QUIRK_FORCE_6BPC),
>
> --
> 2.42.0
--
Ville Syrjälä
Intel
[PATCH] drm/i915: Flush WC GGTT only on required platforms
gen8_ggtt_invalidate() is only needed for limitted set of platforms
where GGTT is mapped as WC otherwise this can cause unwanted
side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
valid.
Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Joonas Lahtinen
Cc: Jani Nikula
Cc: Jonathan Cavitt
Cc: John Harrison
Cc: Andi Shyti
Cc: # v6.2+
Suggested-by: Matt Roper
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 4d7d88b92632..c2858d434bce 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
{
+ struct drm_i915_private *i915 = ggtt->vm.i915;
struct intel_uncore *uncore = ggtt->vm.gt->uncore;
/*
* Note that as an uncached mmio write, this will flush the
* WCB of the writes into the GGTT before it triggers the invalidate.
+*
+* Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
*/
- intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
+ intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
GFX_FLSH_CNTL_EN);
}
static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
--
2.41.0
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
Hi Nirmoy,
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
> gen8_ggtt_invalidate() is only needed for limitted set of platforms
/limitted/limited/
> where GGTT is mapped as WC otherwise this can cause unwanted
> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> valid.
>
> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: Joonas Lahtinen
> Cc: Jani Nikula
> Cc: Jonathan Cavitt
> Cc: John Harrison
> Cc: Andi Shyti
> Cc: # v6.2+
> Suggested-by: Matt Roper
> Signed-off-by: Nirmoy Das
Acked-by: Andi Shyti
Andi
Re: [PATCH] drm/i915/gvt: Optimize mmio_offset_compare() for efficiency
On Fri, Oct 13, 2023 at 07:04:49AM +0800, Kuan-Wei Chiu wrote: > The original code used conditional branching in the mmio_offset_compare > function to compare two values and return -1, 1, or 0 based on the > result. However, the list_sort comparison function only needs results > <0, >0, or =0. This patch optimizes the code to make the comparison > branchless, improving efficiency and reducing code size. This change > reduces the number of comparison operations from 1-2 to a single > subtraction operation, thereby saving the number of instructions. > > Signed-off-by: Kuan-Wei Chiu > --- > drivers/gpu/drm/i915/gvt/debugfs.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c > b/drivers/gpu/drm/i915/gvt/debugfs.c > index baccbf1761b7..998d82a259c8 100644 > --- a/drivers/gpu/drm/i915/gvt/debugfs.c > +++ b/drivers/gpu/drm/i915/gvt/debugfs.c > @@ -48,11 +48,7 @@ static int mmio_offset_compare(void *priv, > > ma = container_of(a, struct diff_mmio, node); > mb = container_of(b, struct diff_mmio, node); > - if (ma->offset < mb->offset) > - return -1; > - else if (ma->offset > mb->offset) > - return 1; > - return 0; > + return ma->offset - mb->offset; Those are unsigned ints effectively, so this only works due to the return value being the same size signed int. Might be better to add some explicit casts. > } > > static inline int mmio_diff_handler(struct intel_gvt *gvt, > -- > 2.25.1 -- Ville Syrjälä Intel
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
On 10/13/2023 12:35 PM, Andi Shyti wrote:
Hi Nirmoy,
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
gen8_ggtt_invalidate() is only needed for limitted set of platforms
/limitted/limited/
Added " autocmd FileType gitcommit setlocal spell" to my vim config.
Wish I knew about it
before.
where GGTT is mapped as WC otherwise this can cause unwanted
side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
valid.
Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Joonas Lahtinen
Cc: Jani Nikula
Cc: Jonathan Cavitt
Cc: John Harrison
Cc: Andi Shyti
Cc: # v6.2+
Suggested-by: Matt Roper
Signed-off-by: Nirmoy Das
Acked-by: Andi Shyti
Thanks,
Nirmoy
Andi
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
> gen8_ggtt_invalidate() is only needed for limitted set of platforms
> where GGTT is mapped as WC
I know there is supposed to be some kind hw snooping of the ggtt
pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
has no other side effects we depend on?
> otherwise this can cause unwanted
> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> valid.
>
> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: Joonas Lahtinen
> Cc: Jani Nikula
> Cc: Jonathan Cavitt
> Cc: John Harrison
> Cc: Andi Shyti
> Cc: # v6.2+
> Suggested-by: Matt Roper
> Signed-off-by: Nirmoy Das
> ---
> drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 4d7d88b92632..c2858d434bce 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
>
> static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> {
> + struct drm_i915_private *i915 = ggtt->vm.i915;
> struct intel_uncore *uncore = ggtt->vm.gt->uncore;
>
> /*
>* Note that as an uncached mmio write, this will flush the
>* WCB of the writes into the GGTT before it triggers the invalidate.
> + *
> + * Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
>*/
> - intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> + if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> + intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> GFX_FLSH_CNTL_EN);
> }
>
> static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> --
> 2.41.0
--
Ville Syrjälä
Intel
Re: [PATCH] drm/edid: add 8 bpc quirk to the BenQ GW2765
On 10/13/23 06:30, Ville Syrjälä wrote:
On Thu, Oct 12, 2023 at 02:49:27PM -0400, Hamza Mahfooz wrote:
The BenQ GW2765 reports that it supports higher (> 8) bpc modes, but
when trying to set them we end up with a black screen. So, limit it to 8
bpc modes.
Bad cable/etc was ruled out as the cause?
Yup, the issue was also reproduced by two different people with same
aforementioned monitor.
Cc: sta...@vger.kernel.org # 6.5+
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2610
Signed-off-by: Hamza Mahfooz
---
drivers/gpu/drm/drm_edid.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0454da505687..bca2af4fe1fc 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -123,6 +123,9 @@ static const struct edid_quirk {
/* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
EDID_QUIRK('A', 'E', 'O', 0, EDID_QUIRK_FORCE_6BPC),
+ /* BenQ GW2765 */
+ EDID_QUIRK('B', 'N', 'Q', 0x78d6, EDID_QUIRK_FORCE_8BPC),
+
/* BOE model on HP Pavilion 15-n233sl reports 8 bpc, but is a 6 bpc
panel */
EDID_QUIRK('B', 'O', 'E', 0x78b, EDID_QUIRK_FORCE_6BPC),
--
2.42.0
--
Hamza
Re: [PATCH drm-misc-next v6 1/6] drm/gpuvm: add common dma-resv per struct drm_gpuvm
On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote:
> Provide a common dma-resv for GEM objects not being used outside of
> this
> GPU-VM. This is used in a subsequent patch to generalize dma-resv,
> external and evicted object handling and GEM validation.
>
> Signed-off-by: Danilo Krummrich
> ---
> drivers/gpu/drm/drm_gpuvm.c | 56
> +-
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 13 +-
> include/drm/drm_gpuvm.h | 35 +++-
> 3 files changed, 99 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c
> b/drivers/gpu/drm/drm_gpuvm.c
> index 02ecb45a2544..ebda9d594165 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -61,6 +61,15 @@
> * contained within struct drm_gpuva already. Hence, for inserting
> &drm_gpuva
> * entries from within dma-fence signalling critical sections it is
> enough to
> * pre-allocate the &drm_gpuva structures.
> + *
> + * &drm_gem_objects which are private to a single VM can share a
> common
> + * &dma_resv in order to improve locking efficiency (e.g. with
> &drm_exec).
> + * For this purpose drivers must pass a &drm_gem_object to
> drm_gpuvm_init(), in
> + * the following called 'root object', which serves as the container
Nit: Perhaps resv object altough it might typically be the root page-
table object, that doesn't have any meaning to drm_gpuvm, which uses it
solely as a container for the resv?
> of the
> + * GPUVM's shared &dma_resv. This root object can be a driver
> specific
> + * &drm_gem_object, such as the &drm_gem_object containing the root
> page table,
> + * but it can also be a 'dummy' object, which can be allocated with
> + * drm_gpuvm_root_object_alloc().
> */
>
> /**
> @@ -652,9 +661,47 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm,
> !drm_gpuvm_in_kernel_node(gpuvm, addr, range);
> }
>
> +static void
> +drm_gpuvm_gem_object_free(struct drm_gem_object *obj)
> +{
> + drm_gem_object_release(obj);
> + kfree(obj);
> +}
> +
> +static const struct drm_gem_object_funcs drm_gpuvm_object_funcs = {
> + .free = drm_gpuvm_gem_object_free,
> +};
> +
> +/**
> + * drm_gpuvm_root_object_alloc() - allocate a dummy &drm_gem_object
> + * @drm: the drivers &drm_device
> + *
> + * Allocates a dummy &drm_gem_object which can be passed to
> drm_gpuvm_init() in
> + * order to serve as root GEM object providing the &drm_resv shared
> across
> + * &drm_gem_objects local to a single GPUVM.
> + *
> + * Returns: the &drm_gem_object on success, NULL on failure
> + */
> +struct drm_gem_object *
> +drm_gpuvm_root_object_alloc(struct drm_device *drm)
> +{
> + struct drm_gem_object *obj;
> +
> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> + if (!obj)
> + return NULL;
> +
> + obj->funcs = &drm_gpuvm_object_funcs;
> + drm_gem_private_object_init(drm, obj, 0);
> +
> + return obj;
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_root_object_alloc);
> +
> /**
> * drm_gpuvm_init() - initialize a &drm_gpuvm
> * @gpuvm: pointer to the &drm_gpuvm to initialize
> + * @r_obj: the root &drm_gem_object providing the GPUVM's common
> &dma_resv
> * @name: the name of the GPU VA space
> * @start_offset: the start offset of the GPU VA space
> * @range: the size of the GPU VA space
> @@ -668,7 +715,7 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm,
> * &name is expected to be managed by the surrounding driver
> structures.
> */
> void
> -drm_gpuvm_init(struct drm_gpuvm *gpuvm,
> +drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_gem_object
> *r_obj,
> const char *name,
> u64 start_offset, u64 range,
> u64 reserve_offset, u64 reserve_range,
> @@ -683,6 +730,9 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm,
>
> gpuvm->name = name ? name : "unknown";
> gpuvm->ops = ops;
> + gpuvm->r_obj = r_obj;
> +
> + drm_gem_object_get(r_obj);
>
> memset(&gpuvm->kernel_alloc_node, 0, sizeof(struct
> drm_gpuva));
>
> @@ -713,7 +763,9 @@ drm_gpuvm_destroy(struct drm_gpuvm *gpuvm)
> __drm_gpuva_remove(&gpuvm->kernel_alloc_node);
>
> WARN(!RB_EMPTY_ROOT(&gpuvm->rb.tree.rb_root),
> - "GPUVA tree is not empty, potentially leaking memory.");
> + "GPUVA tree is not empty, potentially leaking
> memory.\n");
Should we cache the drm device in struct drm_gpuvm and use drm_warn()
here instead of WARN?
> +
> + drm_gem_object_put(gpuvm->r_obj);
> }
> EXPORT_SYMBOL_GPL(drm_gpuvm_destroy);
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 5cf892c50f43..4dea847ef989 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1808,8 +1808,9 @@ int
> nouveau_uvmm_init(struct nouveau_uvmm *uvmm, struct nouveau_cli
> *cli,
> u64 kernel_managed_addr, u64 kernel_manag
Re: [PATCH drm-misc-next v6 2/6] drm/gpuvm: add drm_gpuvm_flags to drm_gpuvm
On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote:
> Introduce flags for struct drm_gpuvm, this required by subsequent
> commits.
>
> Signed-off-by: Danilo Krummrich
> ---
> drivers/gpu/drm/drm_gpuvm.c | 4 +++-
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +-
> include/drm/drm_gpuvm.h | 17 -
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c
> b/drivers/gpu/drm/drm_gpuvm.c
> index ebda9d594165..6368dfdbe9dd 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -703,6 +703,7 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_root_object_alloc);
> * @gpuvm: pointer to the &drm_gpuvm to initialize
> * @r_obj: the root &drm_gem_object providing the GPUVM's common
> &dma_resv
> * @name: the name of the GPU VA space
> + * @flags: the &drm_gpuvm_flags for this GPUVM
NIT: It looks like kerneldoc guidelines recommends using &enum
drm_gpuvm_flags in new code
> * @start_offset: the start offset of the GPU VA space
> * @range: the size of the GPU VA space
> * @reserve_offset: the start of the kernel reserved GPU VA area
> @@ -716,7 +717,7 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_root_object_alloc);
> */
> void
> drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_gem_object
> *r_obj,
> - const char *name,
> + const char *name, enum drm_gpuvm_flags flags,
> u64 start_offset, u64 range,
> u64 reserve_offset, u64 reserve_range,
> const struct drm_gpuvm_ops *ops)
> @@ -729,6 +730,7 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct
> drm_gem_object *r_obj,
> gpuvm->mm_range = range;
>
> gpuvm->name = name ? name : "unknown";
> + gpuvm->flags = flags;
> gpuvm->ops = ops;
> gpuvm->r_obj = r_obj;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 4dea847ef989..93ad2ba7ec8b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1843,7 +1843,7 @@ nouveau_uvmm_init(struct nouveau_uvmm *uvmm,
> struct nouveau_cli *cli,
> uvmm->kernel_managed_addr = kernel_managed_addr;
> uvmm->kernel_managed_size = kernel_managed_size;
>
> - drm_gpuvm_init(&uvmm->base, r_obj, cli->name,
> + drm_gpuvm_init(&uvmm->base, r_obj, cli->name, 0,
> NOUVEAU_VA_SPACE_START,
> NOUVEAU_VA_SPACE_END,
> kernel_managed_addr, kernel_managed_size,
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 0aec14d8b259..13539f32c2e2 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -183,6 +183,16 @@ static inline bool drm_gpuva_invalidated(struct
> drm_gpuva *va)
> return va->flags & DRM_GPUVA_INVALIDATED;
> }
>
> +/**
> + * enum drm_gpuvm_flags - flags for struct drm_gpuvm
> + */
> +enum drm_gpuvm_flags {
> + /**
> + * @DRM_GPUVM_USERBITS: user defined bits
> + */
> + DRM_GPUVM_USERBITS = (1 << 0),
BIT(0)
> +};
> +
> /**
> * struct drm_gpuvm - DRM GPU VA Manager
> *
> @@ -201,6 +211,11 @@ struct drm_gpuvm {
> */
> const char *name;
>
> + /**
> + * @flags: the &drm_gpuvm_flags of this GPUVM
enum?
> + */
> + enum drm_gpuvm_flags flags;
> +
> /**
> * @mm_start: start of the VA space
> */
> @@ -246,7 +261,7 @@ struct drm_gpuvm {
> };
>
> void drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_gem_object
> *r_obj,
> - const char *name,
> + const char *name, enum drm_gpuvm_flags flags,
> u64 start_offset, u64 range,
> u64 reserve_offset, u64 reserve_range,
> const struct drm_gpuvm_ops *ops);
Reviewed-by: Thomas Hellström
Re: [PATCH drm-misc-next v6 1/6] drm/gpuvm: add common dma-resv per struct drm_gpuvm
On 10/13/23 13:38, Thomas Hellström wrote:
On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote:
Provide a common dma-resv for GEM objects not being used outside of
this
GPU-VM. This is used in a subsequent patch to generalize dma-resv,
external and evicted object handling and GEM validation.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/drm_gpuvm.c | 56
+-
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 13 +-
include/drm/drm_gpuvm.h | 35 +++-
3 files changed, 99 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpuvm.c
b/drivers/gpu/drm/drm_gpuvm.c
index 02ecb45a2544..ebda9d594165 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -61,6 +61,15 @@
* contained within struct drm_gpuva already. Hence, for inserting
&drm_gpuva
* entries from within dma-fence signalling critical sections it is
enough to
* pre-allocate the &drm_gpuva structures.
+ *
+ * &drm_gem_objects which are private to a single VM can share a
common
+ * &dma_resv in order to improve locking efficiency (e.g. with
&drm_exec).
+ * For this purpose drivers must pass a &drm_gem_object to
drm_gpuvm_init(), in
+ * the following called 'root object', which serves as the container
Nit: Perhaps resv object altough it might typically be the root page-
table object, that doesn't have any meaning to drm_gpuvm, which uses it
solely as a container for the resv?
With "root" I didn't want to refer to the object representing the root
page-table object, but being *the* object every other (internal) object
needs to keep a reference to. Maybe I should be more explicit here and say
that drivers need to make sure every internal object requires a reference
to take a reference to this root object.
of the
+ * GPUVM's shared &dma_resv. This root object can be a driver
specific
+ * &drm_gem_object, such as the &drm_gem_object containing the root
page table,
+ * but it can also be a 'dummy' object, which can be allocated with
+ * drm_gpuvm_root_object_alloc().
*/
/**
@@ -652,9 +661,47 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm,
!drm_gpuvm_in_kernel_node(gpuvm, addr, range);
}
+static void
+drm_gpuvm_gem_object_free(struct drm_gem_object *obj)
+{
+ drm_gem_object_release(obj);
+ kfree(obj);
+}
+
+static const struct drm_gem_object_funcs drm_gpuvm_object_funcs = {
+ .free = drm_gpuvm_gem_object_free,
+};
+
+/**
+ * drm_gpuvm_root_object_alloc() - allocate a dummy &drm_gem_object
+ * @drm: the drivers &drm_device
+ *
+ * Allocates a dummy &drm_gem_object which can be passed to
drm_gpuvm_init() in
+ * order to serve as root GEM object providing the &drm_resv shared
across
+ * &drm_gem_objects local to a single GPUVM.
+ *
+ * Returns: the &drm_gem_object on success, NULL on failure
+ */
+struct drm_gem_object *
+drm_gpuvm_root_object_alloc(struct drm_device *drm)
+{
+ struct drm_gem_object *obj;
+
+ obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ if (!obj)
+ return NULL;
+
+ obj->funcs = &drm_gpuvm_object_funcs;
+ drm_gem_private_object_init(drm, obj, 0);
+
+ return obj;
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_root_object_alloc);
+
/**
* drm_gpuvm_init() - initialize a &drm_gpuvm
* @gpuvm: pointer to the &drm_gpuvm to initialize
+ * @r_obj: the root &drm_gem_object providing the GPUVM's common
&dma_resv
* @name: the name of the GPU VA space
* @start_offset: the start offset of the GPU VA space
* @range: the size of the GPU VA space
@@ -668,7 +715,7 @@ drm_gpuvm_range_valid(struct drm_gpuvm *gpuvm,
* &name is expected to be managed by the surrounding driver
structures.
*/
void
-drm_gpuvm_init(struct drm_gpuvm *gpuvm,
+drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_gem_object
*r_obj,
const char *name,
u64 start_offset, u64 range,
u64 reserve_offset, u64 reserve_range,
@@ -683,6 +730,9 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm,
gpuvm->name = name ? name : "unknown";
gpuvm->ops = ops;
+ gpuvm->r_obj = r_obj;
+
+ drm_gem_object_get(r_obj);
memset(&gpuvm->kernel_alloc_node, 0, sizeof(struct
drm_gpuva));
@@ -713,7 +763,9 @@ drm_gpuvm_destroy(struct drm_gpuvm *gpuvm)
__drm_gpuva_remove(&gpuvm->kernel_alloc_node);
WARN(!RB_EMPTY_ROOT(&gpuvm->rb.tree.rb_root),
- "GPUVA tree is not empty, potentially leaking memory.");
+ "GPUVA tree is not empty, potentially leaking
memory.\n");
Should we cache the drm device in struct drm_gpuvm and use drm_warn()
here instead of WARN?
I'd guess the additional backtrace of WARN() isn't overly useful in this
case. However, it might be a bit more obvious in dmesg due to its
verboseness. Not a strong opinion on that, though.
+
+ drm_gem_object_put(gpuvm->r_obj);
}
EXPORT_SYMBOL_GPL(drm_gpuvm_destroy);
diff --git a/drivers/gpu/dr
Re: [PATCH drm-misc-next v6 4/6] drm/gpuvm: track/lock/validate external/evicted objects
On 10/10/23 08:26, Thomas Hellström wrote:
On 10/9/23 16:45, Danilo Krummrich wrote:
On 10/9/23 15:36, Thomas Hellström wrote:
On 10/9/23 01:32, Danilo Krummrich wrote:
Currently the DRM GPUVM offers common infrastructure to track GPU VA
allocations and mappings, generically connect GPU VA mappings to their
backing buffers and perform more complex mapping operations on the GPU VA
space.
However, there are more design patterns commonly used by drivers, which
can potentially be generalized in order to make the DRM GPUVM represent
a basis for GPU-VM implementations. In this context, this patch aims
at generalizing the following elements.
1) Provide a common dma-resv for GEM objects not being used outside of
this GPU-VM.
2) Provide tracking of external GEM objects (GEM objects which are
shared with other GPU-VMs).
3) Provide functions to efficiently lock all GEM objects dma-resv the
GPU-VM contains mappings of.
4) Provide tracking of evicted GEM objects the GPU-VM contains mappings
of, such that validation of evicted GEM objects is accelerated.
5) Provide some convinience functions for common patterns.
Big thanks to Boris Brezillon for his help to figure out locking for
drivers updating the GPU VA space within the fence signalling path.
Suggested-by: Matthew Brost
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/drm_gpuvm.c | 646
include/drm/drm_gpuvm.h | 246 ++
2 files changed, 892 insertions(+)
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index 28282283ddaf..6977bd30eca5 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -82,6 +82,21 @@
* &drm_gem_object list of &drm_gpuvm_bos for an existing instance of this
* particular combination. If not existent a new instance is created and
linked
* to the &drm_gem_object.
+ *
+ * &drm_gpuvm_bo structures, since unique for a given &drm_gpuvm, are also used
+ * as entry for the &drm_gpuvm's lists of external and evicted objects. Those
+ * list are maintained in order to accelerate locking of dma-resv locks and
+ * validation of evicted objects bound in a &drm_gpuvm. For instance, all
+ * &drm_gem_object's &dma_resv of a given &drm_gpuvm can be locked by calling
+ * drm_gpuvm_exec_lock(). Once locked drivers can call drm_gpuvm_validate() in
+ * order to validate all evicted &drm_gem_objects. It is also possible to lock
+ * additional &drm_gem_objects by providing the corresponding parameters to
+ * drm_gpuvm_exec_lock() as well as open code the &drm_exec loop while making
+ * use of helper functions such as drm_gpuvm_prepare_range() or
+ * drm_gpuvm_prepare_objects().
+ *
+ * Every bound &drm_gem_object is treated as external object when its &dma_resv
+ * structure is different than the &drm_gpuvm's common &dma_resv structure.
*/
/**
@@ -429,6 +444,20 @@
* Subsequent calls to drm_gpuvm_bo_obtain() for the same &drm_gpuvm and
* &drm_gem_object must be able to observe previous creations and destructions
* of &drm_gpuvm_bos in order to keep instances unique.
+ *
+ * The &drm_gpuvm's lists for keeping track of external and evicted objects are
+ * protected against concurrent insertion / removal and iteration internally.
+ *
+ * However, drivers still need ensure to protect concurrent calls to functions
+ * iterating those lists, namely drm_gpuvm_prepare_objects() and
+ * drm_gpuvm_validate().
+ *
+ * Alternatively, drivers can set the &DRM_GPUVM_RESV_PROTECTED flag to
indicate
+ * that the corresponding &dma_resv locks are held in order to protect the
+ * lists. If &DRM_GPUVM_RESV_PROTECTED is set, internal locking is disabled and
+ * the corresponding lockdep checks are enabled. This is an optimization for
+ * drivers which are capable of taking the corresponding &dma_resv locks and
+ * hence do not require internal locking.
*/
/**
@@ -641,6 +670,195 @@
* }
*/
+/**
+ * get_next_vm_bo_from_list() - get the next vm_bo element
+ * @__gpuvm: The GPU VM
+ * @__list_name: The name of the list we're iterating on
+ * @__local_list: A pointer to the local list used to store already iterated
items
+ * @__prev_vm_bo: The previous element we got from
drm_gpuvm_get_next_cached_vm_bo()
+ *
+ * This helper is here to provide lockless list iteration. Lockless as in, the
+ * iterator releases the lock immediately after picking the first element from
+ * the list, so list insertion deletion can happen concurrently.
+ *
+ * Elements popped from the original list are kept in a local list, so removal
+ * and is_empty checks can still happen while we're iterating the list.
+ */
+#define get_next_vm_bo_from_list(__gpuvm, __list_name, __local_list,
__prev_vm_bo) \
+ ({ \
+ struct drm_gpuvm_bo *__vm_bo = NULL; \
+ \
+ drm_gpuvm_bo_put(__prev_vm_bo); \
+
Re: [PATCH drm-misc-next v6 4/6] drm/gpuvm: track/lock/validate external/evicted objects
On 10/10/23 08:40, Thomas Hellström wrote:
On 10/9/23 01:32, Danilo Krummrich wrote:
Currently the DRM GPUVM offers common infrastructure to track GPU VA
allocations and mappings, generically connect GPU VA mappings to their
backing buffers and perform more complex mapping operations on the GPU VA
space.
However, there are more design patterns commonly used by drivers, which
can potentially be generalized in order to make the DRM GPUVM represent
a basis for GPU-VM implementations. In this context, this patch aims
at generalizing the following elements.
1) Provide a common dma-resv for GEM objects not being used outside of
this GPU-VM.
2) Provide tracking of external GEM objects (GEM objects which are
shared with other GPU-VMs).
3) Provide functions to efficiently lock all GEM objects dma-resv the
GPU-VM contains mappings of.
4) Provide tracking of evicted GEM objects the GPU-VM contains mappings
of, such that validation of evicted GEM objects is accelerated.
5) Provide some convinience functions for common patterns.
Big thanks to Boris Brezillon for his help to figure out locking for
drivers updating the GPU VA space within the fence signalling path.
Suggested-by: Matthew Brost
Signed-off-by: Danilo Krummrich
+/**
+ * drm_gpuvm_resv_add_fence - add fence to private and all extobj
+ * dma-resv
+ * @gpuvm: the &drm_gpuvm to add a fence to
+ * @exec: the &drm_exec locking context
+ * @fence: fence to add
+ * @private_usage: private dma-resv usage
+ * @extobj_usage: extobj dma-resv usage
+ */
+void
+drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
+ struct drm_exec *exec,
+ struct dma_fence *fence,
+ enum dma_resv_usage private_usage,
+ enum dma_resv_usage extobj_usage)
+{
+ struct drm_gem_object *obj;
+ unsigned long index;
+
+ drm_exec_for_each_locked_object(exec, index, obj) {
+ dma_resv_assert_held(obj->resv);
+ dma_resv_add_fence(obj->resv, fence,
+ drm_gpuvm_is_extobj(gpuvm, obj) ?
+ private_usage : extobj_usage);
It looks like private_usage and extobj_usage are mixed up above?
Good catch, will fix.
+ }
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_resv_add_fence);
+
Thanks,
Thomas
Re: [PATCH v1] drm/msm/dp: do not reinitialize phy unless retry during link training
On Tue, Oct 03, 2023 at 11:10:59AM +0200, Johan Hovold wrote:
> On Tue, Aug 08, 2023 at 03:19:50PM -0700, Kuogee Hsieh wrote:
> > DP PHY re-initialization done using dp_ctrl_reinitialize_mainlink() will
> > cause PLL unlocked initially and then PLL gets locked at the end of
> > initialization. PLL_UNLOCKED interrupt will fire during this time if the
> > interrupt mask is enabled.
> > However currently DP driver link training implementation incorrectly
> > re-initializes PHY unconditionally during link training as the PHY was
> > already configured in dp_ctrl_enable_mainlink_clocks().
> >
> > Fix this by re-initializing the PHY only if the previous link training
> > failed.
> >
> > [drm:dp_aux_isr] *ERROR* Unexpected DP AUX IRQ 0x0100 when not busy
> >
> > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> > Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/30
> > Signed-off-by: Kuogee Hsieh
>
> This fixes the above warning and avoids the unnecessary PHY power-off
> and power-on during boot of the ThinkPad X13s:
>
> Reviewed-by: Johan Hovold
> Tested-by: Johan Hovold
>
> I guess this one should go to stable as well:
>
> Cc: sta...@vger.kernel.org# 5.10
>
> Is anyone planning on getting this fixed in 6.6-rc? I noticed that this
> one still hasn't shown up linux-next.
For the record, this one showed up in a PR from Rob and has now been
forwarded to Linus.
No stable tag included, but I guess we can ping the stable team unless
AUTOSEL picks this up.
Johan
Re: [PATCH v3 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems
On Tue, 10 Oct 2023, Shyam Sundar S K wrote:
> PMF driver sends changing inputs from each subystem to TA for evaluating
> the conditions in the policy binary.
>
> Add initial support of plumbing in the PMF driver for Smart PC to get
> information from other subsystems in the kernel.
>
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/platform/x86/amd/pmf/Makefile | 2 +-
> drivers/platform/x86/amd/pmf/pmf.h| 18
> drivers/platform/x86/amd/pmf/spc.c| 119 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 3 +
> 4 files changed, 141 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/amd/pmf/spc.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile
> b/drivers/platform/x86/amd/pmf/Makefile
> index d2746ee7369f..6b26e48ce8ad 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -7,4 +7,4 @@
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> auto-mode.o cnqf.o \
> - tee-if.o
> + tee-if.o spc.o
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 51c0e17f7720..88ee3c705913 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -150,6 +150,21 @@ struct smu_pmf_metrics {
> u16 infra_gfx_maxfreq; /* in MHz */
> u16 skin_temp; /* in centi-Celsius */
> u16 device_state;
> + u16 curtemp; /* in centi-Celsius */
> + u16 filter_alpha_value;
> + u16 avg_gfx_clkfrequency;
> + u16 avg_fclk_frequency;
> + u16 avg_gfx_activity;
> + u16 avg_socclk_frequency;
> + u16 avg_vclk_frequency;
> + u16 avg_vcn_activity;
> + u16 avg_dram_reads;
> + u16 avg_dram_writes;
> + u16 avg_socket_power;
> + u16 avg_core_power[2];
> + u16 avg_core_c0residency[16];
> + u16 spare1;
> + u32 metrics_counter;
> } __packed;
>
> enum amd_stt_skin_temp {
> @@ -596,4 +611,7 @@ extern const struct attribute_group
> cnqf_feature_attribute_group;
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> +
> +/* Smart PC - TA interfaces */
> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c
> b/drivers/platform/x86/amd/pmf/spc.c
> new file mode 100644
> index ..91a7f1da911c
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - Smart PC Capabilities
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Authors: Shyam Sundar S K
> + * Patil Rajesh Reddy
> + */
> +
> +#include
> +#include
> +#include
> +#include "pmf.h"
> +
> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in)
> +{
> + u16 max, avg = 0;
> + int i;
> +
> + memset(dev->buf, 0, sizeof(dev->m_table));
> + amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> + memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> +
> + in->ev_info.socket_power = dev->m_table.apu_power +
> dev->m_table.dgpu_power;
> + in->ev_info.skin_temperature = dev->m_table.skin_temp;
> +
> + /* Get the avg and max C0 residency of all the cores */
> + max = dev->m_table.avg_core_c0residency[0];
> + for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> + avg += dev->m_table.avg_core_c0residency[i];
> + if (dev->m_table.avg_core_c0residency[i] > max)
> + max = dev->m_table.avg_core_c0residency[i];
> + }
> +
> + in->ev_info.avg_c0residency = avg /
> ARRAY_SIZE(dev->m_table.avg_core_c0residency);
Not saying the current is wrong and the difference might be
insignificantly small... But you might want to consider using
DIV_ROUND_CLOSEST() instead of the truncating divide (I'm not sure which
is the best here so I leave it up to you).
> + in->ev_info.max_c0residency = max;
> + in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> +}
> +
> +static const char * const pmf_battery_supply_name[] = {
> + "BATT",
> + "BAT0",
> +};
> +
> +static int amd_pmf_get_battery_prop(enum power_supply_property prop)
> +{
> + union power_supply_propval value;
> + struct power_supply *psy;
> + int i, ret = -EINVAL;
Unnecessary to initialize ret here.
> + for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
> + psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
> + if (!psy)
> + continue;
> +
> + ret = power_supply_get_property(psy, prop, &value);
> + if (ret) {
> + power_supply_put(psy);
> +
[PATCH v2 0/4] Some cleanup of vop2 driver
From: Andy Yan This is a preparation for the upcoming support for rk3588 vop. Patch 1 remove unused struct Patch 2 remove NR_LAYERS macro to support more layers on rk3588 Patch 3 are plane format fix Patch 4 is a format rename Changes in v2: - split rename to another patch Andy Yan (4): drm/rockchip: remove unused struct in vop2 drm/rockchip: remove NR_LAYERS macro on vop2 drm/rockchip: fix the plane format defination of rk3568/6 drm/rockchip: rename windows format for vop2 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 6 +-- drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 3 -- drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 52 +++- 3 files changed, 30 insertions(+), 31 deletions(-) -- 2.34.1
[PATCH v2 1/4] drm/rockchip: remove unused struct in vop2
From: Andy Yan
These structs are undefined and un used.
Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
Signed-off-by: Andy Yan
Reviewed-by: Sascha Hauer
---
(no changes since v1)
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 3 ---
2 files changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 583df4d22f7e..3c524ca23d53 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -160,7 +160,6 @@ struct vop2_video_port {
struct vop2 *vop2;
struct clk *dclk;
unsigned int id;
- const struct vop2_video_port_regs *regs;
const struct vop2_video_port_data *data;
struct completion dsp_hold_completion;
@@ -2274,7 +2273,6 @@ static int vop2_create_crtcs(struct vop2 *vop2)
vp = &vop2->vps[i];
vp->vop2 = vop2;
vp->id = vp_data->id;
- vp->regs = vp_data->regs;
vp->data = vp_data;
snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp->id);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
index f1234a151130..56fd31e05238 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -134,16 +134,13 @@ struct vop2_video_port_data {
u16 cubic_lut_len;
struct vop_rect max_output;
const u8 pre_scan_max_dly[4];
- const struct vop2_video_port_regs *regs;
unsigned int offset;
};
struct vop2_data {
u8 nr_vps;
- const struct vop2_ctrl *ctrl;
const struct vop2_win_data *win;
const struct vop2_video_port_data *vp;
- const struct vop_csc_table *csc_table;
struct vop_rect max_input;
struct vop_rect max_output;
--
2.34.1
[PATCH v2 2/4] drm/rockchip: remove NR_LAYERS macro on vop2
From: Andy Yan
There are 8 layers on rk3588, so a fix defined macro is
not appropriate.
Signed-off-by: Andy Yan
Reviewed-by: Sascha Hauer
---
(no changes since v1)
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 3c524ca23d53..57c05c6b246c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2251,8 +2251,6 @@ static struct vop2_video_port
*find_vp_without_primary(struct vop2 *vop2)
return NULL;
}
-#define NR_LAYERS 6
-
static int vop2_create_crtcs(struct vop2 *vop2)
{
const struct vop2_data *vop2_data = vop2->data;
@@ -2371,7 +2369,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
struct vop2_video_port *vp = &vop2->vps[i];
if (vp->crtc.port)
- vp->nlayers = NR_LAYERS / nvps;
+ vp->nlayers = vop2_data->win_size / nvps;
}
return 0;
--
2.34.1
[PATCH v2 3/4] drm/rockchip: fix the plane format defination of rk3568/6
From: Andy Yan
Add the missing 10 bit RGB format for cluster window.
The cluster windows on rk3568/6 only support afbc format,
so change the linear yuv format NV12/16/24 to non-Linear
YUV420_8BIT/YUV420_10BIT/YUYV/Y210.
Add NV15 for esmart windows.
Also add some comments.
Signed-off-by: Andy Yan
---
Changes in v2:
- split rename to another patch
drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 22 +---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
index 62b573f282a7..05aee588e8c9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
@@ -16,6 +16,10 @@
#include "rockchip_drm_vop2.h"
static const uint32_t formats_win_full_10bit[] = {
+ DRM_FORMAT_XRGB2101010,
+ DRM_FORMAT_ARGB2101010,
+ DRM_FORMAT_XBGR2101010,
+ DRM_FORMAT_ABGR2101010,
DRM_FORMAT_XRGB,
DRM_FORMAT_ARGB,
DRM_FORMAT_XBGR,
@@ -24,9 +28,10 @@ static const uint32_t formats_win_full_10bit[] = {
DRM_FORMAT_BGR888,
DRM_FORMAT_RGB565,
DRM_FORMAT_BGR565,
- DRM_FORMAT_NV12,
- DRM_FORMAT_NV16,
- DRM_FORMAT_NV24,
+ DRM_FORMAT_YUV420_8BIT, /* yuv420_8bit non-Linear mode only */
+ DRM_FORMAT_YUV420_10BIT, /* yuv420_10bit non-Linear mode only */
+ DRM_FORMAT_YUYV, /* yuv422_8bit non-Linear mode only*/
+ DRM_FORMAT_Y210, /* yuv422_10bit non-Linear mode only */
};
static const uint32_t formats_win_full_10bit_yuyv[] = {
@@ -38,11 +43,12 @@ static const uint32_t formats_win_full_10bit_yuyv[] = {
DRM_FORMAT_BGR888,
DRM_FORMAT_RGB565,
DRM_FORMAT_BGR565,
- DRM_FORMAT_NV12,
- DRM_FORMAT_NV16,
- DRM_FORMAT_NV24,
- DRM_FORMAT_YVYU,
- DRM_FORMAT_VYUY,
+ DRM_FORMAT_NV12, /* yuv420_8bit linear mode, 2 plane */
+ DRM_FORMAT_NV15, /* yuv420_10bit linear mode, 2 plane, no padding */
+ DRM_FORMAT_NV16, /* yuv422_8bit linear mode, 2 plane */
+ DRM_FORMAT_NV24, /* yuv444_8bit linear mode, 2 plane */
+ DRM_FORMAT_YVYU, /* yuv422_8bit[YVYU] linear mode */
+ DRM_FORMAT_VYUY, /* yuv422_8bit[VYUY] linear mode */
};
static const uint32_t formats_win_lite[] = {
--
2.34.1
[PATCH v2 4/4] drm/rockchip: rename windows format for vop2
From: Andy Yan
formats_win_full_10bit is for cluster window,
formats_win_full_10bit_yuyv is for rk356x esmart, rk3588 esmart window
will support more format.
formats_win_lite is for smart window.
Rename it based the windows type may let meaning is clearer
Signed-off-by: Andy Yan
---
(no changes since v1)
drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 30 ++--
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
index 05aee588e8c9..574cb013639e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
@@ -15,7 +15,7 @@
#include "rockchip_drm_vop2.h"
-static const uint32_t formats_win_full_10bit[] = {
+static const uint32_t formats_cluster[] = {
DRM_FORMAT_XRGB2101010,
DRM_FORMAT_ARGB2101010,
DRM_FORMAT_XBGR2101010,
@@ -34,7 +34,7 @@ static const uint32_t formats_win_full_10bit[] = {
DRM_FORMAT_Y210, /* yuv422_10bit non-Linear mode only */
};
-static const uint32_t formats_win_full_10bit_yuyv[] = {
+static const uint32_t formats_rk356x_esmart[] = {
DRM_FORMAT_XRGB,
DRM_FORMAT_ARGB,
DRM_FORMAT_XBGR,
@@ -51,7 +51,7 @@ static const uint32_t formats_win_full_10bit_yuyv[] = {
DRM_FORMAT_VYUY, /* yuv422_8bit[VYUY] linear mode */
};
-static const uint32_t formats_win_lite[] = {
+static const uint32_t formats_smart[] = {
DRM_FORMAT_XRGB,
DRM_FORMAT_ARGB,
DRM_FORMAT_XBGR,
@@ -150,8 +150,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
.name = "Smart0-win0",
.phys_id = ROCKCHIP_VOP2_SMART0,
.base = 0x1c00,
- .formats = formats_win_lite,
- .nformats = ARRAY_SIZE(formats_win_lite),
+ .formats = formats_smart,
+ .nformats = ARRAY_SIZE(formats_smart),
.format_modifiers = format_modifiers,
.layer_sel_id = 3,
.supported_rotations = DRM_MODE_REFLECT_Y,
@@ -162,8 +162,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
}, {
.name = "Smart1-win0",
.phys_id = ROCKCHIP_VOP2_SMART1,
- .formats = formats_win_lite,
- .nformats = ARRAY_SIZE(formats_win_lite),
+ .formats = formats_smart,
+ .nformats = ARRAY_SIZE(formats_smart),
.format_modifiers = format_modifiers,
.base = 0x1e00,
.layer_sel_id = 7,
@@ -175,8 +175,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
}, {
.name = "Esmart1-win0",
.phys_id = ROCKCHIP_VOP2_ESMART1,
- .formats = formats_win_full_10bit_yuyv,
- .nformats = ARRAY_SIZE(formats_win_full_10bit_yuyv),
+ .formats = formats_rk356x_esmart,
+ .nformats = ARRAY_SIZE(formats_rk356x_esmart),
.format_modifiers = format_modifiers,
.base = 0x1a00,
.layer_sel_id = 6,
@@ -188,8 +188,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
}, {
.name = "Esmart0-win0",
.phys_id = ROCKCHIP_VOP2_ESMART0,
- .formats = formats_win_full_10bit_yuyv,
- .nformats = ARRAY_SIZE(formats_win_full_10bit_yuyv),
+ .formats = formats_rk356x_esmart,
+ .nformats = ARRAY_SIZE(formats_rk356x_esmart),
.format_modifiers = format_modifiers,
.base = 0x1800,
.layer_sel_id = 2,
@@ -202,8 +202,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
.name = "Cluster0-win0",
.phys_id = ROCKCHIP_VOP2_CLUSTER0,
.base = 0x1000,
- .formats = formats_win_full_10bit,
- .nformats = ARRAY_SIZE(formats_win_full_10bit),
+ .formats = formats_cluster,
+ .nformats = ARRAY_SIZE(formats_cluster),
.format_modifiers = format_modifiers_afbc,
.layer_sel_id = 0,
.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270
|
@@ -217,8 +217,8 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
.name = "Cluster1-win0",
.phys_id = ROCKCHIP_VOP2_CLUSTER1,
.base = 0x1200,
- .formats = formats_win_full_10bit,
- .nformats = ARRAY_SIZE(formats_win_full_10bit),
+ .formats = formats_cluster,
+ .nformats = ARRAY_SIZE(formats_cluster),
.format_modifiers = format_modifiers_afbc,
.layer_sel_id = 1,
.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270
|
--
2.34.1
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
Hi Ville,
On 10/13/2023 12:50 PM, Ville Syrjälä wrote:
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
gen8_ggtt_invalidate() is only needed for limitted set of platforms
where GGTT is mapped as WC
I know there is supposed to be some kind hw snooping of the ggtt
pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
has no other side effects we depend on?
I spent some time searching through the gfxspec. This GFX_FLSH_CNTL
register only seems to be for
invalidating TLB for GUnit and (from git log ) we started to do that to
enable WC based GGTT updates.
So if I am not missing anything obvious then this should be safe.
Regards,
Nirmoy
otherwise this can cause unwanted
side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
valid.
Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Joonas Lahtinen
Cc: Jani Nikula
Cc: Jonathan Cavitt
Cc: John Harrison
Cc: Andi Shyti
Cc: # v6.2+
Suggested-by: Matt Roper
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 4d7d88b92632..c2858d434bce 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
{
+ struct drm_i915_private *i915 = ggtt->vm.i915;
struct intel_uncore *uncore = ggtt->vm.gt->uncore;
/*
* Note that as an uncached mmio write, this will flush the
* WCB of the writes into the GGTT before it triggers the invalidate.
+*
+* Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
*/
- intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
+ intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
GFX_FLSH_CNTL_EN);
}
static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
--
2.41.0
Re: [PATCH drm-misc-next v6 3/6] drm/gpuvm: add an abstraction for a VM / BO combination
On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote: > Add an abstraction layer between the drm_gpuva mappings of a > particular > drm_gem_object and this GEM object itself. The abstraction represents > a > combination of a drm_gem_object and drm_gpuvm. The drm_gem_object > holds > a list of drm_gpuvm_bo structures (the structure representing this > abstraction), while each drm_gpuvm_bo contains list of mappings of > this > GEM object. > > This has multiple advantages: > > 1) We can use the drm_gpuvm_bo structure to attach it to various > lists > of the drm_gpuvm. This is useful for tracking external and evicted > objects per VM, which is introduced in subsequent patches. > > 2) Finding mappings of a certain drm_gem_object mapped in a certain > drm_gpuvm becomes much cheaper. > > 3) Drivers can derive and extend the structure to easily represent > driver specific states of a BO for a certain GPUVM. > > The idea of this abstraction was taken from amdgpu, hence the credit > for > this idea goes to the developers of amdgpu. > > Cc: Christian König > Signed-off-by: Danilo Krummrich > --- > drivers/gpu/drm/drm_gpuvm.c | 332 +-- > -- > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 64 +++-- > include/drm/drm_gem.h | 32 +-- > include/drm/drm_gpuvm.h | 177 - > 4 files changed, 521 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gpuvm.c > b/drivers/gpu/drm/drm_gpuvm.c > index 6368dfdbe9dd..28282283ddaf 100644 > --- a/drivers/gpu/drm/drm_gpuvm.c > +++ b/drivers/gpu/drm/drm_gpuvm.c > @@ -70,6 +70,18 @@ > * &drm_gem_object, such as the &drm_gem_object containing the root > page table, > * but it can also be a 'dummy' object, which can be allocated with > * drm_gpuvm_root_object_alloc(). > + * > + * In order to connect a struct drm_gpuva its backing > &drm_gem_object each NIT: Same as previous patch regarding kerneldoc references > + * &drm_gem_object maintains a list of &drm_gpuvm_bo structures, and > each > + * &drm_gpuvm_bo contains a list of &&drm_gpuva structures. > + * > + * A &drm_gpuvm_bo is an abstraction that represents a combination > of a > + * &drm_gpuvm and a &drm_gem_object. Every such combination should > be unique. > + * This is ensured by the API through drm_gpuvm_bo_obtain() and > + * drm_gpuvm_bo_obtain_prealloc() which first look into the > corresponding > + * &drm_gem_object list of &drm_gpuvm_bos for an existing instance > of this > + * particular combination. If not existent a new instance is created > and linked > + * to the &drm_gem_object. > */ > > /** > @@ -395,21 +407,28 @@ > /** > * DOC: Locking > * > - * Generally, the GPU VA manager does not take care of locking > itself, it is > - * the drivers responsibility to take care about locking. Drivers > might want to > - * protect the following operations: inserting, removing and > iterating > - * &drm_gpuva objects as well as generating all kinds of operations, > such as > - * split / merge or prefetch. > - * > - * The GPU VA manager also does not take care of the locking of the > backing > - * &drm_gem_object buffers GPU VA lists by itself; drivers are > responsible to > - * enforce mutual exclusion using either the GEMs dma_resv lock or > alternatively > - * a driver specific external lock. For the latter see also > - * drm_gem_gpuva_set_lock(). > - * > - * However, the GPU VA manager contains lockdep checks to ensure > callers of its > - * API hold the corresponding lock whenever the &drm_gem_objects GPU > VA list is > - * accessed by functions such as drm_gpuva_link() or > drm_gpuva_unlink(). > + * In terms of managing &drm_gpuva entries DRM GPUVM does not take > care of > + * locking itself, it is the drivers responsibility to take care > about locking. > + * Drivers might want to protect the following operations: > inserting, removing > + * and iterating &drm_gpuva objects as well as generating all kinds > of > + * operations, such as split / merge or prefetch. > + * > + * DRM GPUVM also does not take care of the locking of the backing > + * &drm_gem_object buffers GPU VA lists and &drm_gpuvm_bo > abstractions by > + * itself; drivers are responsible to enforce mutual exclusion using > either the > + * GEMs dma_resv lock or alternatively a driver specific external > lock. For the > + * latter see also drm_gem_gpuva_set_lock(). > + * > + * However, DRM GPUVM contains lockdep checks to ensure callers of > its API hold > + * the corresponding lock whenever the &drm_gem_objects GPU VA list > is accessed > + * by functions such as drm_gpuva_link() or drm_gpuva_unlink(), but > also > + * drm_gpuvm_bo_obtain() and drm_gpuvm_bo_put(). > + * > + * The latter is required since on creation and destruction of a > &drm_gpuvm_bo > + * the &drm_gpuvm_bo is attached / removed from the &drm_gem_objects > gpuva list. > + * Subsequent calls to drm_gpuvm_bo_obtain() for the same &drm_gpuvm > and > + * &drm_gem_object m
Re: [PATCH drm-misc-next v6 3/6] drm/gpuvm: add an abstraction for a VM / BO combination
On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote: > Add an abstraction layer between the drm_gpuva mappings of a > particular > drm_gem_object and this GEM object itself. The abstraction represents > a > combination of a drm_gem_object and drm_gpuvm. The drm_gem_object > holds > a list of drm_gpuvm_bo structures (the structure representing this > abstraction), while each drm_gpuvm_bo contains list of mappings of > this > GEM object. > > This has multiple advantages: > > 1) We can use the drm_gpuvm_bo structure to attach it to various > lists > of the drm_gpuvm. This is useful for tracking external and evicted > objects per VM, which is introduced in subsequent patches. > > 2) Finding mappings of a certain drm_gem_object mapped in a certain > drm_gpuvm becomes much cheaper. > > 3) Drivers can derive and extend the structure to easily represent > driver specific states of a BO for a certain GPUVM. > > The idea of this abstraction was taken from amdgpu, hence the credit > for > this idea goes to the developers of amdgpu. > > Cc: Christian König > Signed-off-by: Danilo Krummrich > --- > drivers/gpu/drm/drm_gpuvm.c | 332 +-- > -- > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 64 +++-- > include/drm/drm_gem.h | 32 +-- > include/drm/drm_gpuvm.h | 177 - > 4 files changed, 521 insertions(+), 84 deletions(-) Forgot to mention, there are a couple of checkpatch.pl --strict issues with this patch that might need looking at. Thanks, Thomas
Re: [PATCH v7 11/20] drm/imagination: Add GEM and VM related code
Hi Sarah,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-intel/for-linux-next
drm-tip/drm-tip next-20231012]
[cannot apply to drm-exynos/exynos-drm-next drm-intel/for-linux-next-fixes
linus/master v6.6-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Sarah-Walker/sizes-h-Add-entries-between-SZ_32G-and-SZ_64T/20231010-221057
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20231010133738.35274-12-sarah.walker%40imgtec.com
patch subject: [PATCH v7 11/20] drm/imagination: Add GEM and VM related code
config: arm64-allyesconfig
(https://download.01.org/0day-ci/archive/20231013/202310132017.ss9eaovp-...@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20231013/202310132017.ss9eaovp-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202310132017.ss9eaovp-...@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/imagination/pvr_mmu.c:9:
>> drivers/gpu/drm/imagination/pvr_rogue_fwif.h:56:52: warning:
>> 'rogue_fwif_log_group_name_value_map' defined but not used
>> [-Wunused-const-variable=]
56 | static const struct rogue_fwif_log_group_map_entry
rogue_fwif_log_group_name_value_map[] = {
|
^~~
--
>> drivers/gpu/drm/imagination/pvr_mmu.c:1331: warning: Incorrect use of
>> kernel-doc format: * @curr_page - A reference to a single physical
>> page as indexed by
>> drivers/gpu/drm/imagination/pvr_mmu.c:1345: warning: Function parameter or
>> member 'curr_page' not described in 'pvr_mmu_op_context'
>> drivers/gpu/drm/imagination/pvr_mmu.c:1744: warning: Function parameter or
>> member 'ctx' not described in 'pvr_mmu_get_root_table_dma_addr'
>> drivers/gpu/drm/imagination/pvr_mmu.c:1744: warning: Excess function
>> parameter 'root' description in 'pvr_mmu_get_root_table_dma_addr'
>> drivers/gpu/drm/imagination/pvr_mmu.c:2156: warning: Excess function
>> parameter 'ptr' description in 'pvr_page_destroy'
vim +/rogue_fwif_log_group_name_value_map +56
drivers/gpu/drm/imagination/pvr_rogue_fwif.h
b4ef9acd8e065c Sarah Walker 2023-10-10 51
b4ef9acd8e065c Sarah Walker 2023-10-10 52 /*
b4ef9acd8e065c Sarah Walker 2023-10-10 53 * Macro for use with the
ROGUE_FWIF_LOG_GROUP_MAP_ENTRY type to create a lookup
b4ef9acd8e065c Sarah Walker 2023-10-10 54 * table where needed. Keep log
group names short, no more than 20 chars.
b4ef9acd8e065c Sarah Walker 2023-10-10 55 */
b4ef9acd8e065c Sarah Walker 2023-10-10 @56 static const struct
rogue_fwif_log_group_map_entry rogue_fwif_log_group_name_value_map[] = {
b4ef9acd8e065c Sarah Walker 2023-10-10 57 {"none",
ROGUE_FWIF_LOG_TYPE_NONE},
b4ef9acd8e065c Sarah Walker 2023-10-10 58 {"main",
ROGUE_FWIF_LOG_TYPE_GROUP_MAIN},
b4ef9acd8e065c Sarah Walker 2023-10-10 59 {"mts",
ROGUE_FWIF_LOG_TYPE_GROUP_MTS},
b4ef9acd8e065c Sarah Walker 2023-10-10 60 {"cleanup",
ROGUE_FWIF_LOG_TYPE_GROUP_CLEANUP},
b4ef9acd8e065c Sarah Walker 2023-10-10 61 {"csw",
ROGUE_FWIF_LOG_TYPE_GROUP_CSW},
b4ef9acd8e065c Sarah Walker 2023-10-10 62 {"bif",
ROGUE_FWIF_LOG_TYPE_GROUP_BIF},
b4ef9acd8e065c Sarah Walker 2023-10-10 63 {"pm",
ROGUE_FWIF_LOG_TYPE_GROUP_PM},
b4ef9acd8e065c Sarah Walker 2023-10-10 64 {"rtd",
ROGUE_FWIF_LOG_TYPE_GROUP_RTD},
b4ef9acd8e065c Sarah Walker 2023-10-10 65 {"spm",
ROGUE_FWIF_LOG_TYPE_GROUP_SPM},
b4ef9acd8e065c Sarah Walker 2023-10-10 66 {"pow",
ROGUE_FWIF_LOG_TYPE_GROUP_POW},
b4ef9acd8e065c Sarah Walker 2023-10-10 67 {"hwr",
ROGUE_FWIF_LOG_TYPE_GROUP_HWR},
b4ef9acd8e065c Sarah Walker 2023-10-10 68 {"hwp",
ROGUE_FWIF_LOG_TYPE_GROUP_HWP},
b4ef9acd8e065c Sarah Walker 2023-10-10 69 {"rpm",
ROGUE_FWIF_LOG_TYPE_GROUP_RPM},
b4ef9acd8e065c Sarah Walker 2023-10-10 70 {"dma",
ROGUE_FWIF_LOG_TYPE_GROUP_DMA},
b4ef9acd8e065c Sarah Walker 2023-10-10 71 {"misc",
ROGUE_FWIF_LOG_TYPE_GROUP_MISC},
b4ef9acd8e065c Sarah Walker 2023-10-10 72 {"debug",
ROGUE_FWIF_LOG_TYPE_GROUP_DEBUG}
b4ef9acd8e065c Sarah Walker 2023-10-10 73 };
b4ef9acd8e065c Sarah Walker 2023-10-10 74
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
On Fri, Oct 13, 2023 at 02:28:21PM +0200, Nirmoy Das wrote:
> Hi Ville,
>
> On 10/13/2023 12:50 PM, Ville Syrjälä wrote:
> > On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
> >> gen8_ggtt_invalidate() is only needed for limitted set of platforms
> >> where GGTT is mapped as WC
> > I know there is supposed to be some kind hw snooping of the ggtt
> > pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
> > has no other side effects we depend on?
>
> I spent some time searching through the gfxspec. This GFX_FLSH_CNTL
> register only seems to be for
>
> invalidating TLB for GUnit and (from git log ) we started to do that to
> enable WC based GGTT updates.
>
>
> So if I am not missing anything obvious then this should be safe.
OK.
The only code related complaint I have is that you are now duplicating
that same platform check in two different places. It's always better to
have a single point of truth instead of two or more, so that there is
no risk of introducing bugs due to mismatches.
>
>
> Regards,
>
> Nirmoy
>
> >
> >> otherwise this can cause unwanted
> >> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> >> valid.
> >>
> >> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> >> Cc: Rodrigo Vivi
> >> Cc: Tvrtko Ursulin
> >> Cc: Joonas Lahtinen
> >> Cc: Jani Nikula
> >> Cc: Jonathan Cavitt
> >> Cc: John Harrison
> >> Cc: Andi Shyti
> >> Cc: # v6.2+
> >> Suggested-by: Matt Roper
> >> Signed-off-by: Nirmoy Das
> >> ---
> >> drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> index 4d7d88b92632..c2858d434bce 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> @@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
> >>
> >> static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> >> {
> >> + struct drm_i915_private *i915 = ggtt->vm.i915;
> >>struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> >>
> >>/*
> >> * Note that as an uncached mmio write, this will flush the
> >> * WCB of the writes into the GGTT before it triggers the invalidate.
> >> + *
> >> + * Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
> >> */
> >> - intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> >> + if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> >> + intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> >> GFX_FLSH_CNTL_EN);
> >> }
> >>
> >> static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> >> --
> >> 2.41.0
--
Ville Syrjälä
Intel
Re: [PATCH drm-misc-next v6 1/6] drm/gpuvm: add common dma-resv per struct drm_gpuvm
On Fri, 2023-10-13 at 13:51 +0200, Danilo Krummrich wrote:
> On 10/13/23 13:38, Thomas Hellström wrote:
> > On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote:
> > > Provide a common dma-resv for GEM objects not being used outside
> > > of
> > > this
> > > GPU-VM. This is used in a subsequent patch to generalize dma-
> > > resv,
> > > external and evicted object handling and GEM validation.
> > >
> > > Signed-off-by: Danilo Krummrich
> > > ---
> > > drivers/gpu/drm/drm_gpuvm.c | 56
> > > +-
> > > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 13 +-
> > > include/drm/drm_gpuvm.h | 35 +++-
> > > 3 files changed, 99 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c
> > > b/drivers/gpu/drm/drm_gpuvm.c
> > > index 02ecb45a2544..ebda9d594165 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -61,6 +61,15 @@
> > > * contained within struct drm_gpuva already. Hence, for
> > > inserting
> > > &drm_gpuva
> > > * entries from within dma-fence signalling critical sections
> > > it is
> > > enough to
> > > * pre-allocate the &drm_gpuva structures.
> > > + *
> > > + * &drm_gem_objects which are private to a single VM can share a
> > > common
> > > + * &dma_resv in order to improve locking efficiency (e.g. with
> > > &drm_exec).
> > > + * For this purpose drivers must pass a &drm_gem_object to
> > > drm_gpuvm_init(), in
> > > + * the following called 'root object', which serves as the
> > > container
> >
> > Nit: Perhaps resv object altough it might typically be the root
> > page-
> > table object, that doesn't have any meaning to drm_gpuvm, which
> > uses it
> > solely as a container for the resv?
>
> With "root" I didn't want to refer to the object representing the
> root
> page-table object, but being *the* object every other (internal)
> object
> needs to keep a reference to.
OK, yes but the reason they need a reference is because of the shared
resv, so IMO resv_object is a good fit. (I later noticed there's even
the function name drm_gpuvm_resv_obj()). And it will probably get
confused with the driver's "root" page table object, but up to you.
> Maybe I should be more explicit here and say
> that drivers need to make sure every internal object requires a
> reference
> to take a reference to this root object.
>
> >
> > > of the
> > > + * GPUVM's shared &dma_resv. This root object can be a driver
> > > specific
> > > + * &drm_gem_object, such as the &drm_gem_object containing the
> > > root
> > > page table,
> > > + * but it can also be a 'dummy' object, which can be allocated
> > > with
> > > + * drm_gpuvm_root_object_alloc().
> > > */
> > >
> > > /**
> > > @@ -652,9 +661,47 @@ drm_gpuvm_range_valid(struct drm_gpuvm
> > > *gpuvm,
> > > !drm_gpuvm_in_kernel_node(gpuvm, addr, range);
> > > }
> > >
> > > +static void
> > > +drm_gpuvm_gem_object_free(struct drm_gem_object *obj)
> > > +{
> > > + drm_gem_object_release(obj);
> > > + kfree(obj);
> > > +}
> > > +
> > > +static const struct drm_gem_object_funcs drm_gpuvm_object_funcs
> > > = {
> > > + .free = drm_gpuvm_gem_object_free,
> > > +};
> > > +
> > > +/**
> > > + * drm_gpuvm_root_object_alloc() - allocate a dummy
> > > &drm_gem_object
> > > + * @drm: the drivers &drm_device
> > > + *
> > > + * Allocates a dummy &drm_gem_object which can be passed to
> > > drm_gpuvm_init() in
> > > + * order to serve as root GEM object providing the &drm_resv
> > > shared
> > > across
> > > + * &drm_gem_objects local to a single GPUVM.
> > > + *
> > > + * Returns: the &drm_gem_object on success, NULL on failure
> > > + */
> > > +struct drm_gem_object *
> > > +drm_gpuvm_root_object_alloc(struct drm_device *drm)
> > > +{
> > > + struct drm_gem_object *obj;
> > > +
> > > + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > > + if (!obj)
> > > + return NULL;
> > > +
> > > + obj->funcs = &drm_gpuvm_object_funcs;
> > > + drm_gem_private_object_init(drm, obj, 0);
> > > +
> > > + return obj;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gpuvm_root_object_alloc);
> > > +
> > > /**
> > > * drm_gpuvm_init() - initialize a &drm_gpuvm
> > > * @gpuvm: pointer to the &drm_gpuvm to initialize
> > > + * @r_obj: the root &drm_gem_object providing the GPUVM's common
> > > &dma_resv
> > > * @name: the name of the GPU VA space
> > > * @start_offset: the start offset of the GPU VA space
> > > * @range: the size of the GPU VA space
> > > @@ -668,7 +715,7 @@ drm_gpuvm_range_valid(struct drm_gpuvm
> > > *gpuvm,
> > > * &name is expected to be managed by the surrounding driver
> > > structures.
> > > */
> > > void
> > > -drm_gpuvm_init(struct drm_gpuvm *gpuvm,
> > > +drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_gem_object
> > > *r_obj,
> > > const char *name,
> > > u64 start_offset, u64 range,
> >
Re: [PATCH drm-misc-next v6 4/6] drm/gpuvm: track/lock/validate external/evicted objects
On Fri, 2023-10-13 at 14:04 +0200, Danilo Krummrich wrote: > On 10/10/23 08:26, Thomas Hellström wrote: > > > > On 10/9/23 16:45, Danilo Krummrich wrote: > > > On 10/9/23 15:36, Thomas Hellström wrote: > > > > > > > > On 10/9/23 01:32, Danilo Krummrich wrote: > > > > > Currently the DRM GPUVM offers common infrastructure to track > > > > > GPU VA > > > > > allocations and mappings, generically connect GPU VA mappings > > > > > to their > > > > > backing buffers and perform more complex mapping operations > > > > > on the GPU VA > > > > > space. > > > > > > > > > > However, there are more design patterns commonly used by > > > > > drivers, which > > > > > can potentially be generalized in order to make the DRM GPUVM > > > > > represent > > > > > a basis for GPU-VM implementations. In this context, this > > > > > patch aims > > > > > at generalizing the following elements. > > > > > > > > > > 1) Provide a common dma-resv for GEM objects not being used > > > > > outside of > > > > > this GPU-VM. > > > > > > > > > > 2) Provide tracking of external GEM objects (GEM objects > > > > > which are > > > > > shared with other GPU-VMs). > > > > > > > > > > 3) Provide functions to efficiently lock all GEM objects dma- > > > > > resv the > > > > > GPU-VM contains mappings of. > > > > > > > > > > 4) Provide tracking of evicted GEM objects the GPU-VM > > > > > contains mappings > > > > > of, such that validation of evicted GEM objects is > > > > > accelerated. > > > > > > > > > > 5) Provide some convinience functions for common patterns. > > > > > > > > > > Big thanks to Boris Brezillon for his help to figure out > > > > > locking for > > > > > drivers updating the GPU VA space within the fence signalling > > > > > path. > > > > > > > > > > Suggested-by: Matthew Brost > > > > > Signed-off-by: Danilo Krummrich > > > > > --- > > > > > drivers/gpu/drm/drm_gpuvm.c | 646 > > > > > > > > > > include/drm/drm_gpuvm.h | 246 ++ > > > > > 2 files changed, 892 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_gpuvm.c > > > > > b/drivers/gpu/drm/drm_gpuvm.c > > > > > index 28282283ddaf..6977bd30eca5 100644 > > > > > --- a/drivers/gpu/drm/drm_gpuvm.c > > > > > +++ b/drivers/gpu/drm/drm_gpuvm.c > > > > > @@ -82,6 +82,21 @@ > > > > > * &drm_gem_object list of &drm_gpuvm_bos for an existing > > > > > instance of this > > > > > * particular combination. If not existent a new instance > > > > > is created and linked > > > > > * to the &drm_gem_object. > > > > > + * > > > > > + * &drm_gpuvm_bo structures, since unique for a given > > > > > &drm_gpuvm, are also used > > > > > + * as entry for the &drm_gpuvm's lists of external and > > > > > evicted objects. Those > > > > > + * list are maintained in order to accelerate locking of > > > > > dma-resv locks and > > > > > + * validation of evicted objects bound in a &drm_gpuvm. For > > > > > instance, all > > > > > + * &drm_gem_object's &dma_resv of a given &drm_gpuvm can be > > > > > locked by calling > > > > > + * drm_gpuvm_exec_lock(). Once locked drivers can call > > > > > drm_gpuvm_validate() in > > > > > + * order to validate all evicted &drm_gem_objects. It is > > > > > also possible to lock > > > > > + * additional &drm_gem_objects by providing the > > > > > corresponding parameters to > > > > > + * drm_gpuvm_exec_lock() as well as open code the &drm_exec > > > > > loop while making > > > > > + * use of helper functions such as drm_gpuvm_prepare_range() > > > > > or > > > > > + * drm_gpuvm_prepare_objects(). > > > > > + * > > > > > + * Every bound &drm_gem_object is treated as external object > > > > > when its &dma_resv > > > > > + * structure is different than the &drm_gpuvm's common > > > > > &dma_resv structure. > > > > > */ > > > > > /** > > > > > @@ -429,6 +444,20 @@ > > > > > * Subsequent calls to drm_gpuvm_bo_obtain() for the same > > > > > &drm_gpuvm and > > > > > * &drm_gem_object must be able to observe previous > > > > > creations and destructions > > > > > * of &drm_gpuvm_bos in order to keep instances unique. > > > > > + * > > > > > + * The &drm_gpuvm's lists for keeping track of external and > > > > > evicted objects are > > > > > + * protected against concurrent insertion / removal and > > > > > iteration internally. > > > > > + * > > > > > + * However, drivers still need ensure to protect concurrent > > > > > calls to functions > > > > > + * iterating those lists, namely drm_gpuvm_prepare_objects() > > > > > and > > > > > + * drm_gpuvm_validate(). > > > > > + * > > > > > + * Alternatively, drivers can set the > > > > > &DRM_GPUVM_RESV_PROTECTED flag to indicate > > > > > + * that the corresponding &dma_resv locks are held in order > > > > > to protect the > > > > > + * lists. If &DRM_GPUVM_RESV_PROTECTED is set, internal > > > > > locking is disabled and > > > > > + * the corresponding lockdep checks are enabled. This is an > > > > > optimization
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
On 10/13/2023 2:55 PM, Ville Syrjälä wrote:
On Fri, Oct 13, 2023 at 02:28:21PM +0200, Nirmoy Das wrote:
Hi Ville,
On 10/13/2023 12:50 PM, Ville Syrjälä wrote:
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
gen8_ggtt_invalidate() is only needed for limitted set of platforms
where GGTT is mapped as WC
I know there is supposed to be some kind hw snooping of the ggtt
pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
has no other side effects we depend on?
I spent some time searching through the gfxspec. This GFX_FLSH_CNTL
register only seems to be for
invalidating TLB for GUnit and (from git log ) we started to do that to
enable WC based GGTT updates.
So if I am not missing anything obvious then this should be safe.
OK.
The only code related complaint I have is that you are now duplicating
that same platform check in two different places. It's always better to
have a single point of truth instead of two or more, so that there is
no risk of introducing bugs due to mismatches.
I agree. I will resend with a static helper function to detect that.
Thanks,
Nirmoy
Regards,
Nirmoy
otherwise this can cause unwanted
side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
valid.
Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Joonas Lahtinen
Cc: Jani Nikula
Cc: Jonathan Cavitt
Cc: John Harrison
Cc: Andi Shyti
Cc: # v6.2+
Suggested-by: Matt Roper
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 4d7d88b92632..c2858d434bce 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
{
+ struct drm_i915_private *i915 = ggtt->vm.i915;
struct intel_uncore *uncore = ggtt->vm.gt->uncore;
/*
* Note that as an uncached mmio write, this will flush the
* WCB of the writes into the GGTT before it triggers the invalidate.
+*
+* Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
*/
- intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
+ intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
GFX_FLSH_CNTL_EN);
}
static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
--
2.41.0
[PATCH 0/4] drm/i915: Fix LUT rounding
From: Ville Syrjälä The current LUT rounding is generating weird results. Adjust it to follow the OpenGL int<->float conversion rules. Ville Syrjälä (4): drm: Fix color LUT rounding drm/i915: Adjust LUT rounding rules drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack() drm/i915: Fix glk+ degamma LUT conversions drivers/gpu/drm/i915/display/intel_color.c | 70 +++--- include/drm/drm_color_mgmt.h | 18 +++--- 2 files changed, 42 insertions(+), 46 deletions(-) -- 2.41.0
[PATCH 1/4] drm: Fix color LUT rounding
From: Ville Syrjälä
The current implementation of drm_color_lut_extract()
generates weird results. Eg. if we go through all the
values for 16->8bpc conversion we see the following pattern:
inout (count)
0 - 7f -> 0 (128)
80 - 17f -> 1 (256)
180 - 27f -> 2 (256)
280 - 37f -> 3 (256)
...
fb80 - fc7f -> fc (256)
fc80 - fd7f -> fd (256)
fd80 - fe7f -> fe (256)
fe80 - -> ff (384)
So less values map to 0 and more values map 0xff, which
doesn't seem particularly great.
To get just the same number of input values to map to
the same output values we'd just need to drop the rounding
entrirely. But perhaps a better idea would be to follow the
OpenGL int<->float conversion rules, in which case we get
the following results:
inout (count)
0 - 80 -> 0 (129)
81 - 181 -> 1 (257)
182 - 282 -> 2 (257)
283 - 383 -> 3 (257)
...
fc7c - fd7c -> fc (257)
fd7d - fe7d -> fd (257)
fe7e - ff7e -> fe (257)
ff7f - -> ff (129)
Note that since the divisor is constant the compiler
is able to optimize away the integer division in most
cases. The only exception is the _ULL() case on 32bit
architectures since that gets emitted as inline asm
via do_div() and thus the compiler doesn't get to
optimize it.
Signed-off-by: Ville Syrjälä
---
include/drm/drm_color_mgmt.h | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 81c298488b0c..6be3cbe18944 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -36,20 +36,16 @@ struct drm_plane;
*
* Extract a degamma/gamma LUT value provided by user (in the form of
* &drm_color_lut entries) and round it to the precision supported by the
- * hardware.
+ * hardware, following OpenGL int<->float conversion rules.
*/
static inline u32 drm_color_lut_extract(u32 user_input, int bit_precision)
{
- u32 val = user_input;
- u32 max = 0x >> (16 - bit_precision);
-
- /* Round only if we're not using full precision. */
- if (bit_precision < 16) {
- val += 1UL << (16 - bit_precision - 1);
- val >>= 16 - bit_precision;
- }
-
- return clamp_val(val, 0, max);
+ if (bit_precision > 16)
+ return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(user_input, (1 <<
bit_precision) - 1),
+(1 << 16) - 1);
+ else
+ return DIV_ROUND_CLOSEST(user_input * ((1 << bit_precision) -
1),
+(1 << 16) - 1);
}
u64 drm_color_ctm_s31_32_to_qm_n(u64 user_input, u32 m, u32 n);
--
2.41.0
[PATCH 2/4] drm/i915: Adjust LUT rounding rules
From: Ville Syrjälä
drm_color_lut_extract() rounding was changed to follow the
OpenGL int<->float conversion rules. Adjust intel_color_lut_pack()
to match.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 2a2a163ea652..b01f463af861 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -785,14 +785,12 @@ static void chv_assign_csc(struct intel_crtc_state
*crtc_state)
/* convert hw value with given bit_precision to lut property val */
static u32 intel_color_lut_pack(u32 val, int bit_precision)
{
- u32 max = 0x >> (16 - bit_precision);
-
- val = clamp_val(val, 0, max);
-
- if (bit_precision < 16)
- val <<= 16 - bit_precision;
-
- return val;
+ if (bit_precision > 16)
+ return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(val, (1 << 16) - 1),
+(1 << bit_precision) - 1);
+ else
+ return DIV_ROUND_CLOSEST(val * ((1 << 16) - 1),
+(1 << bit_precision) - 1);
}
static u32 i9xx_lut_8(const struct drm_color_lut *color)
--
2.41.0
[PATCH 3/4] drm/i915: s/clamp()/min()/ in i965_lut_11p6_max_pack()
From: Ville Syrjälä
Use min() instead of clamp() since the color values
involved are unsigned. No functional changes.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index b01f463af861..a4b30614bd63 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -909,7 +909,7 @@ static void i965_lut_10p6_pack(struct drm_color_lut *entry,
u32 ldw, u32 udw)
static u16 i965_lut_11p6_max_pack(u32 val)
{
/* PIPEGCMAX is 11.6, clamp to 10.6 */
- return clamp_val(val, 0, 0x);
+ return min(val, 0xu);
}
static u32 ilk_lut_10(const struct drm_color_lut *color)
--
2.41.0
[PATCH 4/4] drm/i915: Fix glk+ degamma LUT conversions
From: Ville Syrjälä
The current implementation of change_lut_val_precision() is just
a convoluted way of shifting by 8. Implement the proper rounding
by just using drm_color_lut_extract() and intel_color_lut_pack()
like everyone else does.
And as the uapi can't handle >=1.0 values but the hardware
can we need to clamp the results appropriately in the readout
path.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 54 +++---
1 file changed, 28 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index a4b30614bd63..1cfbb3650304 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1526,14 +1526,27 @@ static int glk_degamma_lut_size(struct drm_i915_private
*i915)
return 35;
}
-/*
- * change_lut_val_precision: helper function to upscale or downscale lut
values.
- * Parameters 'to' and 'from' needs to be less than 32. This should be
sufficient
- * as currently there are no lut values exceeding 32 bit.
- */
-static u32 change_lut_val_precision(u32 lut_val, int to, int from)
+static u32 glk_degamma_lut(const struct drm_color_lut *color)
+{
+ return color->green;
+}
+
+static void glk_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
+{
+ /* PRE_CSC_GAMC_DATA is 3.16, clamp to 0.16 */
+ entry->red = entry->green = entry->blue = min(val, 0xu);
+}
+
+static u32 mtl_degamma_lut(const struct drm_color_lut *color)
+{
+ return drm_color_lut_extract(color->green, 24);
+}
+
+static void mtl_degamma_lut_pack(struct drm_color_lut *entry, u32 val)
{
- return mul_u32_u32(lut_val, (1 << to)) / (1 << from);
+ /* PRE_CSC_GAMC_DATA is 3.24, clamp to 0.16 */
+ entry->red = entry->green = entry->blue =
+ intel_color_lut_pack(min(val, 0xffu), 24);
}
static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
@@ -1570,20 +1583,16 @@ static void glk_load_degamma_lut(const struct
intel_crtc_state *crtc_state,
* ToDo: Extend to max 7.0. Enable 32 bit input value
* as compared to just 16 to achieve this.
*/
- u32 lut_val;
-
- if (DISPLAY_VER(i915) >= 14)
- lut_val = change_lut_val_precision(lut[i].green, 24,
16);
- else
- lut_val = lut[i].green;
-
ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
- lut_val);
+ DISPLAY_VER(i915) >= 14 ?
+ mtl_degamma_lut(&lut[i]) :
glk_degamma_lut(&lut[i]));
}
/* Clamp values > 1.0. */
while (i++ < glk_degamma_lut_size(i915))
- ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 << 16);
+ ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
+ DISPLAY_VER(i915) >= 14 ?
+ 1 << 24 : 1 << 16);
ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0);
}
@@ -3573,17 +3582,10 @@ static struct drm_property_blob
*glk_read_degamma_lut(struct intel_crtc *crtc)
for (i = 0; i < lut_size; i++) {
u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
- /*
-* For MTL and beyond, convert back the 24 bit lut values
-* read from HW to 16 bit values to maintain parity with
-* userspace values
-*/
if (DISPLAY_VER(dev_priv) >= 14)
- val = change_lut_val_precision(val, 16, 24);
-
- lut[i].red = val;
- lut[i].green = val;
- lut[i].blue = val;
+ mtl_degamma_lut_pack(&lut[i], val);
+ else
+ glk_degamma_lut_pack(&lut[i], val);
}
intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
--
2.41.0
Re: [PATCH drm-misc-next v6 4/6] drm/gpuvm: track/lock/validate external/evicted objects
Hi, On Mon, 2023-10-09 at 01:32 +0200, Danilo Krummrich wrote: > Currently the DRM GPUVM offers common infrastructure to track GPU VA > allocations and mappings, generically connect GPU VA mappings to > their > backing buffers and perform more complex mapping operations on the > GPU VA > space. > > However, there are more design patterns commonly used by drivers, > which > can potentially be generalized in order to make the DRM GPUVM > represent > a basis for GPU-VM implementations. In this context, this patch aims > at generalizing the following elements. > > 1) Provide a common dma-resv for GEM objects not being used outside > of > this GPU-VM. > > 2) Provide tracking of external GEM objects (GEM objects which are > shared with other GPU-VMs). > > 3) Provide functions to efficiently lock all GEM objects dma-resv the > GPU-VM contains mappings of. > > 4) Provide tracking of evicted GEM objects the GPU-VM contains > mappings > of, such that validation of evicted GEM objects is accelerated. > > 5) Provide some convinience functions for common patterns. > > Big thanks to Boris Brezillon for his help to figure out locking for > drivers updating the GPU VA space within the fence signalling path. > > Suggested-by: Matthew Brost > Signed-off-by: Danilo Krummrich > --- > drivers/gpu/drm/drm_gpuvm.c | 646 > > include/drm/drm_gpuvm.h | 246 ++ > 2 files changed, 892 insertions(+) > There's a checkpatch.pl warning and a number of random macro CHECKs if using --strict. Also the overall s/Returns:/Return/ (and possibly function line break). > diff --git a/drivers/gpu/drm/drm_gpuvm.c > b/drivers/gpu/drm/drm_gpuvm.c > index 28282283ddaf..6977bd30eca5 100644 > --- a/drivers/gpu/drm/drm_gpuvm.c > +++ b/drivers/gpu/drm/drm_gpuvm.c > @@ -82,6 +82,21 @@ > * &drm_gem_object list of &drm_gpuvm_bos for an existing instance > of this > * particular combination. If not existent a new instance is created > and linked > * to the &drm_gem_object. > + * > + * &drm_gpuvm_bo structures, since unique for a given &drm_gpuvm, > are also used > + * as entry for the &drm_gpuvm's lists of external and evicted > objects. Those > + * list are maintained in order to accelerate locking of dma-resv > locks and s/list/lists/ > + * validation of evicted objects bound in a &drm_gpuvm. For > instance, all > + * &drm_gem_object's &dma_resv of a given &drm_gpuvm can be locked > by calling > + * drm_gpuvm_exec_lock(). Once locked drivers can call > drm_gpuvm_validate() in > + * order to validate all evicted &drm_gem_objects. It is also > possible to lock > + * additional &drm_gem_objects by providing the corresponding > parameters to > + * drm_gpuvm_exec_lock() as well as open code the &drm_exec loop > while making > + * use of helper functions such as drm_gpuvm_prepare_range() or > + * drm_gpuvm_prepare_objects(). > + * > + * Every bound &drm_gem_object is treated as external object when > its &dma_resv > + * structure is different than the &drm_gpuvm's common &dma_resv > structure. > */ > > /** > @@ -429,6 +444,20 @@ > * Subsequent calls to drm_gpuvm_bo_obtain() for the same &drm_gpuvm > and > * &drm_gem_object must be able to observe previous creations and > destructions > * of &drm_gpuvm_bos in order to keep instances unique. > + * > + * The &drm_gpuvm's lists for keeping track of external and evicted > objects are > + * protected against concurrent insertion / removal and iteration > internally. > + * > + * However, drivers still need ensure to protect concurrent calls to > functions > + * iterating those lists, namely drm_gpuvm_prepare_objects() and > + * drm_gpuvm_validate(). > + * > + * Alternatively, drivers can set the &DRM_GPUVM_RESV_PROTECTED flag > to indicate > + * that the corresponding &dma_resv locks are held in order to > protect the > + * lists. If &DRM_GPUVM_RESV_PROTECTED is set, internal locking is > disabled and > + * the corresponding lockdep checks are enabled. This is an > optimization for > + * drivers which are capable of taking the corresponding &dma_resv > locks and > + * hence do not require internal locking. > */ > > /** > @@ -641,6 +670,195 @@ > * } > */ > > +/** > + * get_next_vm_bo_from_list() - get the next vm_bo element macros use a different kerneldoc syntax: https://return42.github.io/linuxdoc/linuxdoc-howto/kernel-doc-syntax.html#macro > + * @__gpuvm: The GPU VM > + * @__list_name: The name of the list we're iterating on > + * @__local_list: A pointer to the local list used to store already > iterated items > + * @__prev_vm_bo: The previous element we got from > drm_gpuvm_get_next_cached_vm_bo() > + * > + * This helper is here to provide lockless list iteration. Lockless > as in, the > + * iterator releases the lock immediately after picking the first > element from > + * the list, so list insertion deletion can happen concurrently. > + * > + * Elements popped from the original list are kept in a
Re: [PATCH] drm/amdgpu/vkms: fix a possible null pointer dereference
On Thu, Oct 12, 2023 at 10:32 PM Ma Ke wrote:
>
> In amdgpu_vkms_conn_get_modes(), the return value of drm_cvt_mode()
> is assigned to mode, which will lead to a NULL pointer dereference
> on failure of drm_cvt_mode(). Add a check to avoid null pointer
> dereference.
>
> Signed-off-by: Ma Ke
Applied. Thanks!
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> index 7148a216ae2f..db6fc0cb18eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> @@ -239,6 +239,8 @@ static int amdgpu_vkms_conn_get_modes(struct
> drm_connector *connector)
>
> for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
> 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.37.2
>
[PATCH v2] drm/i915: Flush WC GGTT only on required platforms
gen8_ggtt_invalidate() is only needed for limited set of platforms
where GGTT is mapped as WC otherwise this can cause unwanted
side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
valid.
v2: Add a func to detect wc ggtt detection (Ville)
Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Joonas Lahtinen
Cc: Jani Nikula
Cc: Jonathan Cavitt
Cc: John Harrison
Cc: Andi Shyti
Cc: Ville Syrjälä
Cc: # v6.2+
Suggested-by: Matt Roper
Signed-off-by: Nirmoy Das
Acked-by: Andi Shyti
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 35 +++-
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 4d7d88b92632..401667f83f96 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -195,6 +195,21 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
spin_unlock_irq(&uncore->lock);
}
+static bool needs_wc_ggtt_mapping(struct drm_i915_private *i915)
+{
+ /*
+* On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
+* will be dropped. For WC mappings in general we have 64 byte burst
+* writes when the WC buffer is flushed, so we can't use it, but have to
+* resort to an uncached mapping. The WC issue is easily caught by the
+* readback check when writing GTT PTE entries.
+*/
+ if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
+ return true;
+
+ return false;
+}
+
static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
{
struct intel_uncore *uncore = ggtt->vm.gt->uncore;
@@ -202,8 +217,12 @@ static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
/*
* Note that as an uncached mmio write, this will flush the
* WCB of the writes into the GGTT before it triggers the invalidate.
+*
+* Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
*/
- intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ if (needs_wc_ggtt_mapping(ggtt->vm.i915))
+ intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
+ GFX_FLSH_CNTL_EN);
}
static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
@@ -1126,17 +1145,11 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt,
u64 size)
GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
gen6_gttmmadr_size(i915));
phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
gen6_gttadr_offset(i915);
- /*
-* On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
-* will be dropped. For WC mappings in general we have 64 byte burst
-* writes when the WC buffer is flushed, so we can't use it, but have to
-* resort to an uncached mapping. The WC issue is easily caught by the
-* readback check when writing GTT PTE entries.
-*/
- if (IS_GEN9_LP(i915) || GRAPHICS_VER(i915) >= 11)
- ggtt->gsm = ioremap(phys_addr, size);
- else
+ if (needs_wc_ggtt_mapping(i915))
ggtt->gsm = ioremap_wc(phys_addr, size);
+ else
+ ggtt->gsm = ioremap(phys_addr, size);
+
if (!ggtt->gsm) {
drm_err(&i915->drm, "Failed to map the ggtt page table\n");
return -ENOMEM;
--
2.41.0
Re: [PATCH] drm/atomic: Perform blocking commits on workqueue
Hi
On Fri, Oct 13, 2023 at 5:41 AM Daniel Vetter wrote:
> > I mean we're not talking about scientific computing, or code
> > compilation, or seti@home. We're talking about nearly the equivalent
> > of `while (1) __asm__ ("nop");`
>
> I don't think anyone said this shouldn't be fixed or improved.
Yea but we apparently disagree that it would be an improvement to stop
discrediting user space for driver problems.
You see, to me, there are two problems 1) The behavior itself is not
nice and should be fixed 2) The blame/accounting/attribution for a
driver problem is getting directed to user space. I think both issues
should be fixed. One brought the other issue to light, but both
problems, in my mind, are legitimate and should be addressed. You
think fixing the second problem is some tactic to paper over the first
problem. Christian thinks the second problem isn't a problem at all
but the correct design. So none of us are completely aligned and I
don't see anyone changing their mind anytime soon.
> What I'm saying is that the atomic ioctl is not going to make guarantees
> that it will not take up to much cpu time (for some extremely vague value
> of "too much") to the point that userspace can configure it's compositor
> in a way that it _will_ get killed if we _ever_ violate this rule.
yea I find that strange, all kernel tasks have a certain implied
baseline responsibility to be good citizens to the system.
And how user space is configured seems nearly immaterial.
But we're talking in circles.
> Fixing the worst offenders I don't think will see any pushback at all.
Yea we all agree fixing this one busy loop is a problem but we don't
agree on where the cpu time blame should go.
> > But *this* feels like duct tape: You've already said there's no
> > guarantee the problem won't also happen during preliminary computation
> > during non-blocking commits or via other drm entry points. So it
> > really does seem like a fix that won't age well. I won't be surprised
> > if in ~3 years (or whatever) in some RHEL release there's a customer
> > bug leading to the real-time thread getting blocklisted for obscure
> > server display hardware because it's causing the session to tank on a
> > production machine.
>
> Yes the atomic ioctl makes no guarantees across drivers and hw platforms
> of guaranteed "we will never violate, you can kill your compositor if you
> do" cpu bound limits. We'll try to not suck too badly, and we'll try to
> focus on fixing the suck where it upsets the people the most.
>
> But there is fundamentally no hard real time guarantee in atomic. At least
> not with the current uapi.
So in your mind mutter should get out of the real-time thread business entirely?
> The problem isn't about wasting cpu time. It's about you having set up the
> system in a way so that mutter gets killed if we ever waste too much cpu
> time, ever.
mutter is not set up in a way to kill itself if the driver wastes too
much cpu time,
ever. mutter is set up in a way to kill itself if it, itself, wastes
too much cpu time, ever.
The fact that the kernel is killing it when a kernel driver is wasting
cpu time is the
bug that led to the patch in the first place!
> The latter is flat out not a guarantee the kernel currently makes, and I
> see no practical feasible way to make that happen. And pretending we do
> make this guarantee will just result in frustrated users filling bugs that
> they desktop session died and developers closing them as "too hard to
> fix".
I think all three of us agree busy loops are not nice (though maybe we
aren't completely aligned on severity). And I'll certainly concede
that fixing all the busy loops can be hard. Some of the cpu-bound code
paths may even be doing legitimate computation. I still assert that
if the uapi calls into driver code that might potentially be doing
something slow where it can't sleep, it should be doing it on a
workqueue or thread. That seems orthogonal to fixing all the places
where the drivers aren't acting nicely.
> Maybe as a bit more useful outcome of this entire discussion: Could you
> sign up to write a documentation patch for the atomic ioctl that adds a
> paragraph stating extremely clearly that this ioctl does not make hard
> real time guarantees, but only best effort soft realtime, and even then
> priority of the effort is focused entirely on the !ALLOW_MODESET case,
> because that is the one users care about the most.
I don't think I'm the best positioned to write such documentation. I
still think what the kernel is doing is wrong here and I don't even
fully grok what you mean by best effort.
--Ray
Re: [PATCH v3 1/2] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()
On Thu, Sep 21, 2023 at 12:48 PM Ian Ray wrote:
>
> Migrate away from custom EDID parsing and validity checks.
>
> Note: This is a follow-up to the original RFC by Jani [1]. The first
> submission in this series should have been marked v2.
>
> [1]
> https://patchwork.freedesktop.org/patch/msgid/20230901102400.552254-1-jani.nik...@intel.com
>
> Co-developed-by: Jani Nikula
> Signed-off-by: Jani Nikula
> Signed-off-by: Ian Ray
> ---
> .../drm/bridge/megachips-stdp-ge-b850v3-fw.c | 57
> --
> 1 file changed, 9 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> index 460db3c..e93083b 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> @@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
>
> static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
>
> -static u8 *stdp2690_get_edid(struct i2c_client *client)
> +static int stdp2690_read_block(void *context, u8 *buf, unsigned int block,
> size_t len)
> {
> + struct i2c_client *client = context;
> struct i2c_adapter *adapter = client->adapter;
> - unsigned char start = 0x00;
> - unsigned int total_size;
> - u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> + unsigned char start = block * EDID_LENGTH;
>
> struct i2c_msg msgs[] = {
> {
> @@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
> }, {
> .addr = client->addr,
> .flags = I2C_M_RD,
> - .len= EDID_LENGTH,
> - .buf= block,
> + .len= len,
> + .buf= buf,
> }
> };
>
> - if (!block)
> - return NULL;
> + if (i2c_transfer(adapter, msgs, 2) != 2)
> + return -1;
>
> - if (i2c_transfer(adapter, msgs, 2) != 2) {
> - DRM_ERROR("Unable to read EDID.\n");
> - goto err;
> - }
> -
> - if (!drm_edid_block_valid(block, 0, false, NULL)) {
> - DRM_ERROR("Invalid EDID data\n");
> - goto err;
> - }
> -
> - total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> - if (total_size > EDID_LENGTH) {
> - kfree(block);
> - block = kmalloc(total_size, GFP_KERNEL);
> - if (!block)
> - return NULL;
> -
> - /* Yes, read the entire buffer, and do not skip the first
> -* EDID_LENGTH bytes.
> -*/
> - start = 0x00;
> - msgs[1].len = total_size;
> - msgs[1].buf = block;
> -
> - if (i2c_transfer(adapter, msgs, 2) != 2) {
> - DRM_ERROR("Unable to read EDID extension blocks.\n");
> - goto err;
> - }
> - if (!drm_edid_block_valid(block, 1, false, NULL)) {
> - DRM_ERROR("Invalid EDID data\n");
> - goto err;
> - }
> - }
> -
> - return block;
> -
> -err:
> - kfree(block);
> - return NULL;
> + return 0;
> }
>
> static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
> @@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct
> drm_bridge *bridge,
>
> client = ge_b850v3_lvds_ptr->stdp2690_i2c;
>
> - return (struct edid *)stdp2690_get_edid(client);
> + return drm_do_get_edid(connector, stdp2690_read_block, client);
> }
>
> static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
Reviewed-by: Robert Foss
Re: [PATCH v3 2/2] MAINTAINERS: Update entry for megachips-stdpxxxx-ge-b850v3-fw
On Thu, Sep 21, 2023 at 12:48 PM Ian Ray wrote: > > Replace Martin, who has left GE. > > Signed-off-by: Ian Ray > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index bf0f54c..31bb835 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13524,7 +13524,7 @@ F: drivers/usb/mtu3/ > > MEGACHIPS STDP-GE-B850V3-FW LVDS/DP++ BRIDGES > M: Peter Senna Tschudin > -M: Martin Donnelly > +M: Ian Ray > M: Martyn Welch > S: Maintained > F: > Documentation/devicetree/bindings/display/bridge/megachips-stdp-ge-b850v3-fw.txt > -- > 2.10.1 > Reviewed-by: Robert Foss
Re: [PATCH v3 1/2] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()
On Thu, 21 Sep 2023 13:47:50 +0300, Ian Ray wrote: > Migrate away from custom EDID parsing and validity checks. > > Note: This is a follow-up to the original RFC by Jani [1]. The first > submission in this series should have been marked v2. > > [1] > https://patchwork.freedesktop.org/patch/msgid/20230901102400.552254-1-jani.nik...@intel.com > > [...] Applied, thanks! [1/2] drm/bridge: megachips-stdp-ge-b850v3-fw: switch to drm_do_get_edid() https://cgit.freedesktop.org/drm/drm-misc/commit/?id=e0eb7db49764 [2/2] MAINTAINERS: Update entry for megachips-stdp-ge-b850v3-fw https://cgit.freedesktop.org/drm/drm-misc/commit/?id=e755d439c1b7 Rob
Re: DRM FB emulation initialisation leaving the display disabled
Hi Thomas
Thanks for the response.
On Thu, 12 Oct 2023 at 08:03, Thomas Zimmermann wrote:
>
> Hi Dave
>
> Am 11.10.23 um 17:52 schrieb Dave Stevenson:
> > Hi Thomas (and everyone else)
> >
> > Maxime has suggested you're the person for DRM framebuffer emulation.
> >
> > I'm getting some unexpected behaviour when there are multiple DRM
> > drivers in play. In this case it happens to be vc4 and the tiny
> > hx8357d SPI display driver, but this affects most of the tiny DRM
> > drivers and also the DSI and DPI outputs on the Pi5.
> > We get different behaviour depending on whether vc4 or hx8357d
> > initialises first.
> >
> > If hx8357d loads first and registers as /dev/fb0 through the fb
> > emulation, then we get fbcon enabling the display and putting the
> > emulated framebuffer on it. vc4 then loads, registers /dev/fb1, and
> > through the hotplug handler it enables the display
> > (drm_fb_helper_hotplug_event calls, drm_fb_helper_set_par, which calls
> > __drm_fb_helper_restore_fbdev_mode_unlocked).
> >
> > If vc4 loads first and claims /dev/fb0, fbcon initalises and enables
> > the display. hx8357d then loads and registers as /dev/fb1. fbcon is
> > not configured for that fb, and there is no subsequent hotplug event
> > (SPI displays aren't hotpluggable), so we have a fully configured
> > framebuffer exposed to userspace but the display itself isn't enabled
> > so we don't see anything :-(
> > Open and close /dev/dri/card1 and the lastclose hook calls
> > drm_fb_helper_restore_fbdev_mode_unlocked and we get the display
> > enabled.
>
> What you're describing sounds like the recent bug report at
>
>https://gitlab.freedesktop.org/drm/amd/-/issues/2649
>
> which had similar symptoms with amdgpu. IIRC the console didn't
> initialize if the DRM hotplugging event happened before fbdev emulation
> was ready. DRM's fbdev code would then not see the hotplugged connector.
>
> Do you have commit 27655b9bb9f0 ("drm/client: Send hotplug event after
> registering a client") in your tree? (It's been ported into various
> stable branches as well.)
I was about to switch from my 6.1.55 build to the latest just to
ensure it hadn't been fixed, but that commit hit the 6.1 stable branch
in 6.1.43, so I do have it.
Other priorities have overtaken my investigation of this, but at least
knowing that it should enable the outputs (and nominally through the
hotplug hook) I can have another look at what is actually going on.
Thanks again
Dave
> Best regard
> Thomas
>
> >
> > Is it intentional that we're left in this limbo state with the display
> > not enabled if fbcon isn't enabled on a framebuffer?
> >
> > We're trying to get people to transition from the fbdev drivers to DRM
> > equivalents, but this seems to be a backwards step if there is an
> > extra step required to get the display enabled. Many users are still
> > just using the framebuffer.
> >
> > Any input is much appreciated.
> >
> > Thanks,
> >Dave
>
> --
> 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)
[PATCH] drm/ttm: Drain workqueue before sys manager release
In rare cases, a delayed destruction of a BO with a system resource
could stay in the workqueue until drain_workqueue() is called
in ttm_device_fini(). An attempt to free a resource from an already
released manager results in NULL pointer dereference. Move the step
of draining and destroying the workqueue so it happens before
the ttm_sys_manager cleanup.
Fixes: 9bff18d13473 ("drm/ttm: use per BO cleanup workers")
Signed-off-by: Karolina Stolarek
---
Some background: I stumbled upon this issue when testing
ttm_bo_pipeline_gutting() with BO with an active dma_resv fence. In ~2% of
the runs, the delayed destruction of the ghost wouldn't happen until the
drain_queue() step. man->func->free(man, *res) got called via
ttm_bo_cleanup_memtype_use(), the manager and its functions were nowhere to
be seen, resulting in a nulptr deref.
drivers/gpu/drm/ttm/ttm_device.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
index 7726a72befc5..753126581620 100644
--- a/drivers/gpu/drm/ttm/ttm_device.c
+++ b/drivers/gpu/drm/ttm/ttm_device.c
@@ -232,6 +232,9 @@ void ttm_device_fini(struct ttm_device *bdev)
struct ttm_resource_manager *man;
unsigned i;
+ drain_workqueue(bdev->wq);
+ destroy_workqueue(bdev->wq);
+
man = ttm_manager_type(bdev, TTM_PL_SYSTEM);
ttm_resource_manager_set_used(man, false);
ttm_set_driver_manager(bdev, TTM_PL_SYSTEM, NULL);
@@ -240,9 +243,6 @@ void ttm_device_fini(struct ttm_device *bdev)
list_del(&bdev->device_list);
mutex_unlock(&ttm_global_mutex);
- drain_workqueue(bdev->wq);
- destroy_workqueue(bdev->wq);
-
spin_lock(&bdev->lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i)
if (list_empty(&man->lru[0]))
--
2.25.1
[PATCH libdrm v4 0/9] util, modetest: add support for low-color frame buffer formats
Hi all,
A long outstanding issue with the DRM subsystem has been the lack of
support for low-color displays, as used typically on older desktop
systems, and on small embedded displays.
This patch series adds support for color-indexed frame buffer formats
with 2, 4, and 16 colors. It has been tested on ARAnyM using a
work-in-progress Atari DRM driver.
Changes compared to v3[1]:
- Rename util_smpte_index_gamma() to util_smpte_fill_lut(), and its
first parameter from size to ncolors,
- Move smpte_color_lut[] down,
- Kill FILL_COLOR() macro,
- Add and use EXPAND_COLOR() macro,
- Replace FILL_COLOR() use by bw_color_lut[],
- Replace FILL_COLOR() use by pentile_color_lut[],
- Add missing C[12] to oneline-summary,
- Do not remove memset() of full lut, else some entries may stay
uninitialized.
Changes compared to v2[2]:
- Add Acked-by,
- Add Wikipedia link,
- Dropped "[RFC] drm_fourcc: Add DRM_FORMAT_C[124]", as these were
added in commit 329eebcf32793361 ("drm_fourcc: sync drm_fourcc with
latest drm-next kernel") in libdrm-2.4.115.
Changes compared to v1[3]:
- SMPTE color LUT accuracy,
- Factor out smpte color LUT,
- Restructure patches,
- Improve descriptions.
- Store number of colors for indexed formats,
- Add SMPTE pattern support for the C1 and C2 formats.
I have also updated the merge request at [4].
Thanks for your comments!
[1] https://lore.kernel.org/r/cover.1690537375.git.ge...@linux-m68k.org
[2] https://lore.kernel.org/r/cover.1657302034.git.ge...@linux-m68k.org/
[3] https://lore.kernel.org/r/cover.1646683737.git.ge...@linux-m68k.org/
[4] https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/314
Geert Uytterhoeven (9):
util: improve SMPTE color LUT accuracy
util: factor out and optimize C8 SMPTE color LUT
util: add support for DRM_FORMAT_C[124]
util: store number of colors for indexed formats
util: add SMPTE pattern support for C4 format
util: add SMPTE pattern support for C1 format
util: add SMPTE pattern support for C2 format
modetest: add support for DRM_FORMAT_C[124]
modetest: add SMPTE pattern support for C[124] formats
tests/modetest/buffers.c | 15 ++
tests/modetest/modetest.c | 9 +-
tests/util/format.c | 5 +-
tests/util/format.h | 1 +
tests/util/pattern.c | 430 ++
tests/util/pattern.h | 2 +-
6 files changed, 415 insertions(+), 47 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 libdrm v4 3/9] util: add support for DRM_FORMAT_C[124]
Add support for creating buffers using the new color-indexed frame
buffer formats with two, four, and sixteen colors.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- No changes,
v3:
- Add Acked-by,
v2:
- Split off changes to tests/util/format.c.
---
tests/util/format.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/util/format.c b/tests/util/format.c
index f825027227ddba24..b3d2abdc8e67eed0 100644
--- a/tests/util/format.c
+++ b/tests/util/format.c
@@ -40,6 +40,9 @@
static const struct util_format_info format_info[] = {
/* Indexed */
+ { DRM_FORMAT_C1, "C1" },
+ { DRM_FORMAT_C2, "C2" },
+ { DRM_FORMAT_C4, "C4" },
{ DRM_FORMAT_C8, "C8" },
/* YUV packed */
{ DRM_FORMAT_UYVY, "UYVY", MAKE_YUV_INFO(YUV_YCbCr | YUV_CY, 2, 2, 2) },
--
2.34.1
[PATCH libdrm v4 8/9] modetest: add support for DRM_FORMAT_C[124]
Add support for creating buffers using the new color-indexed frame
buffer formats with two, four, and sixteen colors.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- No changes,
v3:
- Add Acked-by,
v2:
- Split off changes to tests/modetest/buffers.c.
---
tests/modetest/buffers.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/tests/modetest/buffers.c b/tests/modetest/buffers.c
index c122fb3fe9429190..65f1cfb32ab9eeae 100644
--- a/tests/modetest/buffers.c
+++ b/tests/modetest/buffers.c
@@ -124,6 +124,18 @@ bo_create(int fd, unsigned int format,
int ret;
switch (format) {
+ case DRM_FORMAT_C1:
+ bpp = 1;
+ break;
+
+ case DRM_FORMAT_C2:
+ bpp = 2;
+ break;
+
+ case DRM_FORMAT_C4:
+ bpp = 4;
+ break;
+
case DRM_FORMAT_C8:
case DRM_FORMAT_NV12:
case DRM_FORMAT_NV21:
@@ -292,6 +304,9 @@ bo_create(int fd, unsigned int format,
planes[2] = virtual + offsets[2];
break;
+ case DRM_FORMAT_C1:
+ case DRM_FORMAT_C2:
+ case DRM_FORMAT_C4:
case DRM_FORMAT_C8:
case DRM_FORMAT_ARGB:
case DRM_FORMAT_XRGB:
--
2.34.1
[PATCH libdrm v4 5/9] util: add SMPTE pattern support for C4 format
Add support for drawing the SMPTE pattern in a buffer using the C4
indexed format.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- No changes,
v3:
- Add Acked-by,
v2:
- Use new smpte_top[],
- Split off changes to tests/util/pattern.c.
---
tests/util/pattern.c | 42 ++
1 file changed, 42 insertions(+)
diff --git a/tests/util/pattern.c b/tests/util/pattern.c
index fc457c4ac61e404a..17074265dc60033c 100644
--- a/tests/util/pattern.c
+++ b/tests/util/pattern.c
@@ -672,6 +672,46 @@ static const struct drm_color_lut smpte_color_lut[] = {
#undef EXPAND_COLOR
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel)
+{
+ if (x & 1)
+ mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);
+ else
+ mem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);
+}
+
+static void fill_smpte_c4(void *mem, unsigned int width, unsigned int height,
+ unsigned int stride)
+{
+ unsigned int x;
+ unsigned int y;
+
+ for (y = 0; y < height * 6 / 9; ++y) {
+ for (x = 0; x < width; ++x)
+ write_pixel_4(mem, x, smpte_top[x * 7 / width]);
+ mem += stride;
+ }
+
+ for (; y < height * 7 / 9; ++y) {
+ for (x = 0; x < width; ++x)
+ write_pixel_4(mem, x, smpte_middle[x * 7 / width]);
+ mem += stride;
+ }
+
+ for (; y < height; ++y) {
+ for (x = 0; x < width * 5 / 7; ++x)
+ write_pixel_4(mem, x,
+ smpte_bottom[x * 4 / (width * 5 / 7)]);
+ for (; x < width * 6 / 7; ++x)
+ write_pixel_4(mem, x,
+ smpte_bottom[(x - width * 5 / 7) * 3 /
+ (width / 7) + 4]);
+ for (; x < width; ++x)
+ write_pixel_4(mem, x, smpte_bottom[7]);
+ mem += stride;
+ }
+}
+
static void fill_smpte_c8(void *mem, unsigned int width, unsigned int height,
unsigned int stride)
{
@@ -723,6 +763,8 @@ static void fill_smpte(const struct util_format_info *info,
void *planes[3],
unsigned char *u, *v;
switch (info->format) {
+ case DRM_FORMAT_C4:
+ return fill_smpte_c4(planes[0], width, height, stride);
case DRM_FORMAT_C8:
return fill_smpte_c8(planes[0], width, height, stride);
case DRM_FORMAT_UYVY:
--
2.34.1
[PATCH libdrm v4 6/9] util: add SMPTE pattern support for C1 format
Add support for drawing the SMPTE pattern in a buffer using the C1
indexed format.
As only two colors are available, the pattern is drawn in black and
white, using Floyd-Steinberg dithering[1].
[1] https://en.wikipedia.org/wiki/Floyd%E2%80%93Steinberg_dithering
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
Dithering example at
https://drive.google.com/file/d/1waJczErrIaEKRhBCCU1ynxRG8agpo0Xx/view
v4:
- Replace FILL_COLOR() use by bw_color_lut[],
v3:
- Add Acked-by,
- Add Wikipedia link,
v2:
New.
---
tests/util/pattern.c | 174 ++-
1 file changed, 171 insertions(+), 3 deletions(-)
diff --git a/tests/util/pattern.c b/tests/util/pattern.c
index 17074265dc60033c..2362555356e0dd71 100644
--- a/tests/util/pattern.c
+++ b/tests/util/pattern.c
@@ -654,6 +654,11 @@ static unsigned int smpte_bottom[8] = {
#define EXPAND_COLOR(r, g, b) { (r) * 0x101, (g) * 0x101, (b) * 0x101 }
+static const struct drm_color_lut bw_color_lut[] = {
+ EXPAND_COLOR( 0, 0, 0),/* black */
+ EXPAND_COLOR(255, 255, 255),/* white */
+};
+
static const struct drm_color_lut smpte_color_lut[] = {
[SMPTE_COLOR_GREY] =EXPAND_COLOR(192, 192, 192),
[SMPTE_COLOR_YELLOW] = EXPAND_COLOR(192, 192, 0),
@@ -672,6 +677,164 @@ static const struct drm_color_lut smpte_color_lut[] = {
#undef EXPAND_COLOR
+/*
+ * Floyd-Steinberg dithering
+ */
+
+struct fsd {
+ unsigned int width;
+ unsigned int x;
+ unsigned int i;
+ int red;
+ int green;
+ int blue;
+ int error[];
+};
+
+static struct fsd *fsd_alloc(unsigned int width)
+{
+ unsigned int n = 3 * (width + 1);
+ struct fsd *fsd = malloc(sizeof(*fsd) + n * sizeof(fsd->error[0]));
+
+ fsd->width = width;
+ fsd->x = 0;
+ fsd->i = 0;
+ memset(fsd->error, 0, n * sizeof(fsd->error[0]));
+
+ return fsd;
+}
+
+static inline int clamp(int val, int min, int max)
+{
+ if (val < min)
+ return min;
+ if (val > max)
+ return max;
+ return val;
+}
+
+static void fsd_dither(struct fsd *fsd, struct drm_color_lut *color)
+{
+ unsigned int i = fsd->i;
+
+ fsd->red = (int)color->red + (fsd->error[3 * i] + 8) / 16;
+ fsd->green = (int)color->green + (fsd->error[3 * i + 1] + 8) / 16;
+ fsd->blue = (int)color->blue + (fsd->error[3 * i + 2] + 8) / 16;
+
+ color->red = clamp(fsd->red, 0, 65535);
+ color->green = clamp(fsd->green, 0, 65535);
+ color->blue = clamp(fsd->blue, 0, 65535);
+}
+
+static void fsd_update(struct fsd *fsd, const struct drm_color_lut *actual)
+{
+ int error_red = fsd->red - (int)actual->red;
+ int error_green = fsd->green - (int)actual->green;
+ int error_blue = fsd->blue - (int)actual->blue;
+ unsigned int width = fsd->width;
+ unsigned int i = fsd->i, j;
+ unsigned int n = width + 1;
+
+ /* Distribute errors over neighboring pixels */
+ if (fsd->x == width - 1) {
+ /* Last pixel on this scanline */
+ /* South East: initialize to zero */
+ fsd->error[3 * i] = 0;
+ fsd->error[3 * i + 1] = 0;
+ fsd->error[3 * i + 2] = 0;
+ } else {
+ /* East: accumulate error */
+ j = (i + 1) % n;
+ fsd->error[3 * j] += 7 * error_red;
+ fsd->error[3 * j + 1] += 7 * error_green;
+ fsd->error[3 * j + 2] += 7 * error_blue;
+
+ /* South East: initial error */
+ fsd->error[3 * i] = error_red;
+ fsd->error[3 * i + 1] = error_green;
+ fsd->error[3 * i + 2] = error_blue;
+ }
+ /* South West: accumulate error */
+ j = (i + width - 1) % n;
+ fsd->error[3 * j] += 3 * error_red;
+ fsd->error[3 * j + 1] += 3 * error_green;
+ fsd->error[3 * j + 2] += 3 * error_blue;
+
+ /* South: accumulate error */
+ j = (i + width) % n;
+ fsd->error[3 * j] += 5 * error_red;
+ fsd->error[3 * j + 1] += 5 * error_green;
+ fsd->error[3 * j + 2] += 5 * error_blue;
+
+ fsd->x = (fsd->x + 1) % width;
+ fsd->i = (fsd->i + 1) % n;
+}
+
+static void write_pixel_1(uint8_t *mem, unsigned int x, unsigned int pixel)
+{
+ unsigned int shift = 7 - (x & 7);
+ unsigned int mask = 1U << shift;
+
+ mem[x / 8] = (mem[x / 8] & ~mask) | ((pixel << shift) & mask);
+}
+
+static void write_color_1(struct fsd *fsd, uint8_t *mem, unsigned int x,
+ unsigned int index)
+{
+ struct drm_color_lut color = smpte_color_lut[index];
+ unsigned int pixel;
+
+ fsd_dither(fsd, &color);
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ if (3 * color.red + 6 * color.green + color.blue >= 10 * 32768) {
+ pixel = 1;
+ color.red = color.green = color.blue = 65535;
+
[PATCH libdrm v4 7/9] util: add SMPTE pattern support for C2 format
Add support for drawing the SMPTE pattern in a buffer using the C2
indexed format.
As only four colors are available, resolution is halved, and the pattern
is drawn in a PenTile RG-GB matrix, using Floyd-Steinberg dithering.
The magnitude of the green subpixels is reduced, as there are twice as
many green subpixels as red or blue subpixels.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
Dithering example at
https://drive.google.com/file/d/1g5O8XeacrjrC8rgaVENvR65YeI6QvmtO/view
v4:
- Replace FILL_COLOR() use by pentile_color_lut[],
v3:
- Add Acked-by,
v2:
- New.
---
tests/util/pattern.c | 100 ++-
1 file changed, 99 insertions(+), 1 deletion(-)
diff --git a/tests/util/pattern.c b/tests/util/pattern.c
index 2362555356e0dd71..f69c5206d96eff02 100644
--- a/tests/util/pattern.c
+++ b/tests/util/pattern.c
@@ -659,6 +659,14 @@ static const struct drm_color_lut bw_color_lut[] = {
EXPAND_COLOR(255, 255, 255),/* white */
};
+static const struct drm_color_lut pentile_color_lut[] = {
+ /* PenTile RG-GB */
+ EXPAND_COLOR( 0, 0, 0),/* black */
+ EXPAND_COLOR(255, 0, 0),/* red */
+ EXPAND_COLOR( 0, 207, 0),/* green */
+ EXPAND_COLOR( 0, 0, 255),/* blue */
+};
+
static const struct drm_color_lut smpte_color_lut[] = {
[SMPTE_COLOR_GREY] =EXPAND_COLOR(192, 192, 192),
[SMPTE_COLOR_YELLOW] = EXPAND_COLOR(192, 192, 0),
@@ -835,6 +843,92 @@ static void fill_smpte_c1(void *mem, unsigned int width,
unsigned int height,
free(fsd);
}
+static void write_pixel_2(uint8_t *mem, unsigned int x, unsigned int pixel)
+{
+ unsigned int shift = 6 - 2 * (x & 3);
+ unsigned int mask = 3U << shift;
+
+ mem[x / 4] = (mem[x / 4] & ~mask) | ((pixel << shift) & mask);
+}
+
+static void write_color_2(struct fsd *fsd, uint8_t *mem, unsigned int stride,
+ unsigned int x, unsigned int index)
+{
+ struct drm_color_lut color = smpte_color_lut[index];
+ unsigned int r, g, b;
+
+ fsd_dither(fsd, &color);
+
+ if (color.red >= 32768) {
+ r = 1;
+ color.red = 65535;
+ } else {
+ r = 0;
+ color.red = 0;
+ }
+ if (color.green >= 32768) {
+ g = 2;
+ color.green = 65535;
+ } else {
+ g = 0;
+ color.green = 0;
+ }
+ if (color.blue >= 32768) {
+ b = 3;
+ color.blue = 65535;
+ } else {
+ b = 0;
+ color.blue = 0;
+ }
+
+ fsd_update(fsd, &color);
+
+ /* Use PenTile RG-GB */
+ write_pixel_2(mem, 2 * x, r);
+ write_pixel_2(mem, 2 * x + 1, g);
+ write_pixel_2(mem + stride, 2 * x, g);
+ write_pixel_2(mem + stride, 2 * x + 1, b);
+}
+
+static void fill_smpte_c2(void *mem, unsigned int width, unsigned int height,
+ unsigned int stride)
+{
+ struct fsd *fsd = fsd_alloc(width);
+ unsigned int x;
+ unsigned int y;
+
+ /* Half resolution for PenTile RG-GB */
+ width /= 2;
+ height /= 2;
+
+ for (y = 0; y < height * 6 / 9; ++y) {
+ for (x = 0; x < width; ++x)
+ write_color_2(fsd, mem, stride, x, smpte_top[x * 7 /
width]);
+ mem += 2 * stride;
+ }
+
+ for (; y < height * 7 / 9; ++y) {
+ for (x = 0; x < width; ++x)
+ write_color_2(fsd, mem, stride, x, smpte_middle[x * 7 /
width]);
+ mem += 2 * stride;
+ }
+
+ for (; y < height; ++y) {
+ for (x = 0; x < width * 5 / 7; ++x)
+ write_color_2(fsd, mem, stride, x,
+ smpte_bottom[x * 4 / (width * 5 / 7)]);
+ for (; x < width * 6 / 7; ++x)
+ write_color_2(fsd, mem, stride, x,
+ smpte_bottom[(x - width * 5 / 7) * 3 /
+ (width / 7) + 4]);
+ for (; x < width; ++x)
+ write_color_2(fsd, mem, stride, x, smpte_bottom[7]);
+ mem += 2 * stride;
+ }
+
+ free(fsd);
+}
+
static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel)
{
if (x & 1)
@@ -916,8 +1010,10 @@ void util_smpte_fill_lut(unsigned int ncolors, struct
drm_color_lut *lut)
}
memset(lut, 0, ncolors * sizeof(struct drm_color_lut));
- if (ncolors < ARRAY_SIZE(smpte_color_lut))
+ if (ncolors < ARRAY_SIZE(pentile_color_lut))
memcpy(lut, bw_color_lut, sizeof(bw_color_lut));
+ else if (ncolors < ARRAY_SIZE(smpte_color_lut))
+ memcpy(lut, pentile_color_lut, sizeof(pentile_color_lut));
else
memcpy(lut, smpte_color_lut, sizeof(smpte_color_lut));
[PATCH libdrm v4 2/9] util: factor out and optimize C8 SMPTE color LUT
The color LUT for the SMPTE pattern in indexed mode contains 22 entries,
although only 13 are non-unique.
Reduce the size of the color LUT by dropping duplicate entries, so it
can be reused for formats supporting e.g. 16 colors. Rename the
function util_smpte_c8_gamma() to util_smpte_fill_lut(), and its first
parameter size to ncolors, to match their actual use.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- Rename util_smpte_index_gamma() to util_smpte_fill_lut(), and its
first parameter from size to ncolors,
- Move smpte_color_lut[] down,
- Kill FILL_COLOR() macro,
- Add and use EXPAND_COLOR() macro,
v3:
- Add Acked-by,
v2:
- Factor out smpte color LUT.
---
tests/modetest/modetest.c | 2 +-
tests/util/pattern.c | 122 +-
tests/util/pattern.h | 2 +-
3 files changed, 82 insertions(+), 44 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 861a06ebb27bd170..9504fbd8af59ff21 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1155,7 +1155,7 @@ static void set_gamma(struct device *dev, unsigned
crtc_id, unsigned fourcc)
if (fourcc == DRM_FORMAT_C8) {
/* TODO: Add C8 support for more patterns */
- util_smpte_c8_gamma(256, gamma_lut);
+ util_smpte_fill_lut(256, gamma_lut);
drmModeCreatePropertyBlob(dev->fd, gamma_lut,
sizeof(gamma_lut), &blob_id);
} else {
/*
diff --git a/tests/util/pattern.c b/tests/util/pattern.c
index 7d4f6610015e7464..fc457c4ac61e404a 100644
--- a/tests/util/pattern.c
+++ b/tests/util/pattern.c
@@ -605,6 +605,73 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info
*rgb, void *mem,
}
}
+enum smpte_colors {
+ SMPTE_COLOR_GREY,
+ SMPTE_COLOR_YELLOW,
+ SMPTE_COLOR_CYAN,
+ SMPTE_COLOR_GREEN,
+ SMPTE_COLOR_MAGENTA,
+ SMPTE_COLOR_RED,
+ SMPTE_COLOR_BLUE,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_IN_PHASE,
+ SMPTE_COLOR_SUPER_WHITE,
+ SMPTE_COLOR_QUADRATURE,
+ SMPTE_COLOR_3PC5,
+ SMPTE_COLOR_11PC5,
+};
+
+static unsigned int smpte_top[7] = {
+ SMPTE_COLOR_GREY,
+ SMPTE_COLOR_YELLOW,
+ SMPTE_COLOR_CYAN,
+ SMPTE_COLOR_GREEN,
+ SMPTE_COLOR_MAGENTA,
+ SMPTE_COLOR_RED,
+ SMPTE_COLOR_BLUE,
+};
+
+static unsigned int smpte_middle[7] = {
+ SMPTE_COLOR_BLUE,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_MAGENTA,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_CYAN,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_GREY,
+};
+
+static unsigned int smpte_bottom[8] = {
+ SMPTE_COLOR_IN_PHASE,
+ SMPTE_COLOR_SUPER_WHITE,
+ SMPTE_COLOR_QUADRATURE,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_3PC5,
+ SMPTE_COLOR_BLACK,
+ SMPTE_COLOR_11PC5,
+ SMPTE_COLOR_BLACK,
+};
+
+#define EXPAND_COLOR(r, g, b) { (r) * 0x101, (g) * 0x101, (b) * 0x101 }
+
+static const struct drm_color_lut smpte_color_lut[] = {
+ [SMPTE_COLOR_GREY] =EXPAND_COLOR(192, 192, 192),
+ [SMPTE_COLOR_YELLOW] = EXPAND_COLOR(192, 192, 0),
+ [SMPTE_COLOR_CYAN] =EXPAND_COLOR( 0, 192, 192),
+ [SMPTE_COLOR_GREEN] = EXPAND_COLOR( 0, 192, 0),
+ [SMPTE_COLOR_MAGENTA] = EXPAND_COLOR(192, 0, 192),
+ [SMPTE_COLOR_RED] = EXPAND_COLOR(192, 0, 0),
+ [SMPTE_COLOR_BLUE] =EXPAND_COLOR( 0, 0, 192),
+ [SMPTE_COLOR_BLACK] = EXPAND_COLOR( 19, 19, 19),
+ [SMPTE_COLOR_IN_PHASE] =EXPAND_COLOR( 0, 33, 76),
+ [SMPTE_COLOR_SUPER_WHITE] = EXPAND_COLOR(255, 255, 255),
+ [SMPTE_COLOR_QUADRATURE] = EXPAND_COLOR( 50, 0, 106),
+ [SMPTE_COLOR_3PC5] =EXPAND_COLOR( 9, 9, 9),
+ [SMPTE_COLOR_11PC5] = EXPAND_COLOR( 29, 29, 29),
+};
+
+#undef EXPAND_COLOR
+
static void fill_smpte_c8(void *mem, unsigned int width, unsigned int height,
unsigned int stride)
{
@@ -613,69 +680,40 @@ static void fill_smpte_c8(void *mem, unsigned int width,
unsigned int height,
for (y = 0; y < height * 6 / 9; ++y) {
for (x = 0; x < width; ++x)
- ((uint8_t *)mem)[x] = x * 7 / width;
+ ((uint8_t *)mem)[x] = smpte_top[x * 7 / width];
mem += stride;
}
for (; y < height * 7 / 9; ++y) {
for (x = 0; x < width; ++x)
- ((uint8_t *)mem)[x] = 7 + (x * 7 / width);
+ ((uint8_t *)mem)[x] = smpte_middle[x * 7 / width];
mem += stride;
}
for (; y < height; ++y) {
for (x = 0; x < width * 5 / 7; ++x)
((uint8_t *)mem)[x] =
- 14 + (x * 4 / (width * 5 / 7));
+ smpte_bottom[x * 4 / (width * 5 / 7)];
for (; x < wi
[PATCH libdrm v4 4/9] util: store number of colors for indexed formats
Store the number of available colors for color-indexed frame
buffer formats in the format_info[] array. This avoids the need of test
code for having to use switch statements all the time to obtain the
number of colors, or to check if a mode is color-indexed or not.
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- No changes,
v3:
- Add Acked-by,
v2:
- New.
---
tests/util/format.c | 8
tests/util/format.h | 1 +
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/util/format.c b/tests/util/format.c
index b3d2abdc8e67eed0..b99cc9c3599d9237 100644
--- a/tests/util/format.c
+++ b/tests/util/format.c
@@ -40,10 +40,10 @@
static const struct util_format_info format_info[] = {
/* Indexed */
- { DRM_FORMAT_C1, "C1" },
- { DRM_FORMAT_C2, "C2" },
- { DRM_FORMAT_C4, "C4" },
- { DRM_FORMAT_C8, "C8" },
+ { DRM_FORMAT_C1, "C1", .ncolors = 2 },
+ { DRM_FORMAT_C2, "C2", .ncolors = 4 },
+ { DRM_FORMAT_C4, "C4", .ncolors = 16 },
+ { DRM_FORMAT_C8, "C8", .ncolors = 256 },
/* YUV packed */
{ DRM_FORMAT_UYVY, "UYVY", MAKE_YUV_INFO(YUV_YCbCr | YUV_CY, 2, 2, 2) },
{ DRM_FORMAT_VYUY, "VYUY", MAKE_YUV_INFO(YUV_YCrCb | YUV_CY, 2, 2, 2) },
diff --git a/tests/util/format.h b/tests/util/format.h
index 2ce1c021fd78d51d..b847c9f2933b3cde 100644
--- a/tests/util/format.h
+++ b/tests/util/format.h
@@ -55,6 +55,7 @@ struct util_yuv_info {
struct util_format_info {
uint32_t format;
const char *name;
+ unsigned int ncolors;
const struct util_rgb_info rgb;
const struct util_yuv_info yuv;
};
--
2.34.1
[PATCH libdrm v4 9/9] modetest: add SMPTE pattern support for C[124] formats
Add support for drawing the SMPTE pattern in buffers using a
color-indexed frame buffer formats with two, four, or sixteen colors.
Note that this still uses 256 as the CLUT size, as
DRM_IOCTL_MODE_SETGAMMA enforces that the size matches against the
(fixed) gamma size, while the CLUT size depends on the format.
Move clearing the color LUT entries from util_smpte_index_gamma() to its
caller, as only the caller knows how many entries there really are
(currently DRM always assumes 256 entries).
Signed-off-by: Geert Uytterhoeven
Acked-by: Sam Ravnborg
---
v4:
- Add missing C[12] to oneline-summary,
- Do not remove memset() of full lut, else some entries may stay
uninitialized,
v3:
- Add Acked-by,
v2:
- Split off changes to tests/modetest/modetest.c,
- Add C1 and C2 support.
The linuxdoc comments say userspace can query the gamma size:
* drm_mode_gamma_set_ioctl - set the gamma table
*
* Set the gamma table of a CRTC to the one passed in by the user. Userspace can
* inquire the required gamma table size through drm_mode_gamma_get_ioctl.
* drm_mode_gamma_get_ioctl - get the gamma table
*
* Copy the current gamma table into the storage provided. This also provides
* the gamma table size the driver expects, which can be used to size the
* allocated storage.
but the code doesn't seem to support that in an easy way (like setting
red/green/blue to NULL on input, retrieving gamma_size on output), only
by providing big enough buffers for red/green/blue, and looping over
gamma_size until -EINVAL is no longer returned.
---
tests/modetest/modetest.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 9504fbd8af59ff21..9b1aa537be8716cf 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1149,13 +1149,16 @@ static bool add_property_optional(struct device *dev,
uint32_t obj_id,
static void set_gamma(struct device *dev, unsigned crtc_id, unsigned fourcc)
{
unsigned blob_id = 0;
+ const struct util_format_info *info;
/* TODO: support 1024-sized LUTs, when the use-case arises */
struct drm_color_lut gamma_lut[256];
int i, ret;
- if (fourcc == DRM_FORMAT_C8) {
- /* TODO: Add C8 support for more patterns */
- util_smpte_fill_lut(256, gamma_lut);
+ info = util_format_info_find(fourcc);
+ if (info->ncolors) {
+ memset(gamma_lut, 0, sizeof(gamma_lut));
+ /* TODO: Add index support for more patterns */
+ util_smpte_fill_lut(info->ncolors, gamma_lut);
drmModeCreatePropertyBlob(dev->fd, gamma_lut,
sizeof(gamma_lut), &blob_id);
} else {
/*
--
2.34.1
[PATCH libdrm v4 1/9] util: improve SMPTE color LUT accuracy
Fill in the LSB when converting color components from 8-bit to 16-bit. Signed-off-by: Geert Uytterhoeven Acked-by: Sam Ravnborg --- v4: - No changes, v3: - Add Acked-by, v2: - New. --- tests/util/pattern.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/util/pattern.c b/tests/util/pattern.c index bd0989e6dbc6aa27..7d4f6610015e7464 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -646,9 +646,9 @@ void util_smpte_c8_gamma(unsigned size, struct drm_color_lut *lut) memset(lut, 0, size * sizeof(struct drm_color_lut)); #define FILL_COLOR(idx, r, g, b) \ - lut[idx].red = (r) << 8; \ - lut[idx].green = (g) << 8; \ - lut[idx].blue = (b) << 8 + lut[idx].red = (r) * 0x101; \ + lut[idx].green = (g) * 0x101; \ + lut[idx].blue = (b) * 0x101 FILL_COLOR( 0, 192, 192, 192); /* grey */ FILL_COLOR( 1, 192, 192, 0 ); /* yellow */ -- 2.34.1
Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family
Thomas Zimmermann writes:
Hello Thomas,
Thanks a lot for your feedback.
> Hi Javier,
>
> thanks for this patch.
>
> Am 12.10.23 um 23:38 schrieb Javier Martinez Canillas:
> [...]
>>
>> +static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
>> +const struct iosys_map *vmap,
>> +struct drm_rect *rect, u8 *buf,
>> +u8 *data_array)
>> +{
>> +struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>> +unsigned int dst_pitch = drm_rect_width(rect);
>> +struct iosys_map dst;
>> +int ret = 0;
>> +
>> +/* Align x to display segment boundaries */
>> +rect->x1 = round_down(rect->x1, SSD132X_SEGMENT_WIDTH);
>> +rect->x2 = min_t(unsigned int, round_up(rect->x2,
>> SSD132X_SEGMENT_WIDTH),
>> + ssd130x->width);
>> +
>> +ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +if (ret)
>> +return ret;
>> +
>> +iosys_map_set_vaddr(&dst, buf);
>> +drm_fb_xrgb_to_gray8(&dst, &dst_pitch, vmap, fb, rect);
>
> Here's an idea for a follow-up patchset.
>
> You could attempt to integrate the gray8 and mono conversions into
> drm_fb_blit(). With some the right parameters, both, ssd130x and ssd132x
> could use the same blitting code from BO to buffer.
>
Yeah, I considered that but as mentioned in the commit message want to see
what are the needs of the SSD133x controller family (I bought a SSD1331
display but haven't had time to play with it yet) before trying to factor
out the common bits in helper functions.
[...]
>> +
>> +ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
>> +if (!ssd130x_state->buffer)
>> +return -ENOMEM;
>
> It's unrelated to these patches and I know it's been discussed
> endlessly, but I have a questions about buffer allocation. That memory
> acts as another shadow buffer for the device's memory, such that format
> conversion becomes easier.
>
Correct.
> But then, why is ->buffer part of the plane_state? Shouldn't it be part
> of the plane and never be re-allocated? The real size of that buffer is
> times (not ). That size is static over the
> lifetime of the device. That would represent the semantics much better.
>
> This would allow for additional changes: blit_rect and update_rect would
> be much easier to separate: no more segment adjustments for the blit
> code; only for updates. If the update code has high latency (IDK), you
> could push it into a worker thread to run besides the DRM logic. The gud
> and repaper drivers do something to this effect.
>
>
The idea of making it part of the plane state is that this buffer could be
optional, for example in the case of user-space using the native display
format instead of the emulated XRGB.
In that case, an intermediate buffer won't be used because the shadow-plane
format will already be the native one (e.g: R1) and there won't be a need
to do any format conversion (only the conversion to the data format as is
expected by the controller).
Take a look to Geert's patch adding R1 support to ssd130x for an example:
https://lore.kernel.org/all/72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.ge...@linux-m68k.org/
That's why it was decided that making it part of the plane state follows
better the KMS model, because when using R1 this buffer won't even be
allocated in the primary plane .atomic_check handler.
[...]
>> +drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>> +drm_atomic_for_each_plane_damage(&iter, &damage) {
>> +dst_clip = plane_state->dst;
>> +
>> +if (!drm_rect_intersect(&dst_clip, &damage))
>> +continue;
>> +
>> +ssd132x_fb_blit_rect(fb, &shadow_plane_state->data[0],
>> &dst_clip,
>> + ssd130x_plane_state->buffer,
>> + ssd130x_crtc_state->data_array);
>> +}
>
> Here's another idea for a another follow-up patchset:
>
> You are allocating state->buffer to cover the whole display, right? It's
> times IIRC. Maybe it would make sense to split the
> damage loop into two loops and inline the driver's blit_rect() function.
> Something like that
>
>begin_cpu_access()
>
>for_each(damage) {
> drm_fb_blit( "from GEM BO to buffer" )
>}
>
>end_cpu_access()
>
>for_each(damge) {
> update_rect( "from buffer to device" )
>}
>
> With the changes from the other comments, the first loop could become
> entirely device-neutral AFAICT.
>
Regardless, splitting the blit and update rect might make sense and is an
intersesting idea. I need to explore this, thanks for the suggestion.
As you mention that these could be follow-up changes, I assume that you
agree with the current approach. Should I expect your review / ack for
this patch-set?
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Implement per-key keyboard backlight as auxdisplay?
Hi, coming from the leds mailing list I'm writing with Pavel how to best handle per-key RGB keyboards. His suggestion was that it could be implemented as an aux display, but he also suggested that I ask first if this fits. The specific keyboard RGB controller I want to implement takes 6*21 rgb values. However not every one is actually mapped to a physical key. e.g. the bottom row needs less entries because of the space bar. Additionally the keys are ofc not in a straight line from top to bottom. Best regards, Werner
Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family
Hi
Am 13.10.23 um 16:57 schrieb Javier Martinez Canillas:
Thomas Zimmermann writes:
Hello Thomas,
Thanks a lot for your feedback.
Hi Javier,
thanks for this patch.
Am 12.10.23 um 23:38 schrieb Javier Martinez Canillas:
[...]
+static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
+ const struct iosys_map *vmap,
+ struct drm_rect *rect, u8 *buf,
+ u8 *data_array)
+{
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+ unsigned int dst_pitch = drm_rect_width(rect);
+ struct iosys_map dst;
+ int ret = 0;
+
+ /* Align x to display segment boundaries */
+ rect->x1 = round_down(rect->x1, SSD132X_SEGMENT_WIDTH);
+ rect->x2 = min_t(unsigned int, round_up(rect->x2,
SSD132X_SEGMENT_WIDTH),
+ssd130x->width);
+
+ ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+ if (ret)
+ return ret;
+
+ iosys_map_set_vaddr(&dst, buf);
+ drm_fb_xrgb_to_gray8(&dst, &dst_pitch, vmap, fb, rect);
Here's an idea for a follow-up patchset.
You could attempt to integrate the gray8 and mono conversions into
drm_fb_blit(). With some the right parameters, both, ssd130x and ssd132x
could use the same blitting code from BO to buffer.
Yeah, I considered that but as mentioned in the commit message want to see
what are the needs of the SSD133x controller family (I bought a SSD1331
display but haven't had time to play with it yet) before trying to factor
out the common bits in helper functions.
[...]
+
+ ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+ if (!ssd130x_state->buffer)
+ return -ENOMEM;
It's unrelated to these patches and I know it's been discussed
endlessly, but I have a questions about buffer allocation. That memory
acts as another shadow buffer for the device's memory, such that format
conversion becomes easier.
Correct.
But then, why is ->buffer part of the plane_state? Shouldn't it be part
of the plane and never be re-allocated? The real size of that buffer is
times (not ). That size is static over the
lifetime of the device. That would represent the semantics much better.
This would allow for additional changes: blit_rect and update_rect would
be much easier to separate: no more segment adjustments for the blit
code; only for updates. If the update code has high latency (IDK), you
could push it into a worker thread to run besides the DRM logic. The gud
and repaper drivers do something to this effect.
The idea of making it part of the plane state is that this buffer could be
optional, for example in the case of user-space using the native display
format instead of the emulated XRGB.
In that case, an intermediate buffer won't be used because the shadow-plane
format will already be the native one (e.g: R1) and there won't be a need
to do any format conversion (only the conversion to the data format as is
expected by the controller).
Take a look to Geert's patch adding R1 support to ssd130x for an example:
https://lore.kernel.org/all/72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.ge...@linux-m68k.org/
That's why it was decided that making it part of the plane state follows
better the KMS model, because when using R1 this buffer won't even be
allocated in the primary plane .atomic_check handler.
[...]
+ drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+ drm_atomic_for_each_plane_damage(&iter, &damage) {
+ dst_clip = plane_state->dst;
+
+ if (!drm_rect_intersect(&dst_clip, &damage))
+ continue;
+
+ ssd132x_fb_blit_rect(fb, &shadow_plane_state->data[0],
&dst_clip,
+ssd130x_plane_state->buffer,
+ssd130x_crtc_state->data_array);
+ }
Here's another idea for a another follow-up patchset:
You are allocating state->buffer to cover the whole display, right? It's
times IIRC. Maybe it would make sense to split the
damage loop into two loops and inline the driver's blit_rect() function.
Something like that
begin_cpu_access()
for_each(damage) {
drm_fb_blit( "from GEM BO to buffer" )
}
end_cpu_access()
for_each(damge) {
update_rect( "from buffer to device" )
}
With the changes from the other comments, the first loop could become
entirely device-neutral AFAICT.
Regardless, splitting the blit and update rect might make sense and is an
intersesting idea. I need to explore this, thanks for the suggestion.
As you mention that these could be follow-up changes, I assume that you
agree with the current approach. Should I expect your review / ack for
this patch-set?
Please take my ack for this patchset
Acked-by: Thomas Zimmermann
Best regards
Thomas
Best regards
Thomas
--
Thomas Zimmermann
Graphics Dr
Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family
Thomas Zimmermann writes: Hello Thomas, [...] >> As you mention that these could be follow-up changes, I assume that you >> agree with the current approach. Should I expect your review / ack for >> this patch-set? > > Please take my ack for this patchset > > Acked-by: Thomas Zimmermann > Perfect, thanks a lot! > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
[PATCH] drm/bridge: analogix: anx78xx: Check return value
In anx78xx_start() err value of anx78xx_clear_bits()
overwriting without check.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Signed-off-by: Artem Chernyshev
---
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
index 800555aef97f..c966e661b9b9 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
@@ -537,6 +537,10 @@ static int anx78xx_start(struct anx78xx *anx78xx)
SP_POWERDOWN_CTRL_REG,
SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD |
SP_LINK_PD);
+ if (err) {
+ DRM_ERROR("Failed to clear bits: %d\n", err);
+ goto err_poweroff;
+ }
err = anx78xx_enable_interrupts(anx78xx);
if (err) {
--
2.37.3
[PATCH v2] drm/i915/gvt: Optimize mmio_offset_compare() for efficiency
The original code used conditional branching in the mmio_offset_compare function to compare two values and return -1, 1, or 0 based on the result. However, the list_sort comparison function only needs results <0, >0, or =0. This patch optimizes the code to make the comparison branchless, improving efficiency and reducing code size. This change reduces the number of comparison operations from 1-2 to a single subtraction operation, thereby saving the number of instructions. Signed-off-by: Kuan-Wei Chiu --- v1 -> v2: - Add explicit type cast in case the sizes of u32 and int differ. drivers/gpu/drm/i915/gvt/debugfs.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c index baccbf1761b7..d85d8a3b5ae5 100644 --- a/drivers/gpu/drm/i915/gvt/debugfs.c +++ b/drivers/gpu/drm/i915/gvt/debugfs.c @@ -48,11 +48,7 @@ static int mmio_offset_compare(void *priv, ma = container_of(a, struct diff_mmio, node); mb = container_of(b, struct diff_mmio, node); - if (ma->offset < mb->offset) - return -1; - else if (ma->offset > mb->offset) - return 1; - return 0; + return (int)ma->offset - (int)mb->offset; } static inline int mmio_diff_handler(struct intel_gvt *gvt, -- 2.25.1
Re: [PATCH v2] drm/i915: Flush WC GGTT only on required platforms
On Fri, Oct 13, 2023 at 03:44:39PM +0200, Nirmoy Das wrote:
> gen8_ggtt_invalidate() is only needed for limited set of platforms
> where GGTT is mapped as WC otherwise this can cause unwanted
> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> valid.
>
> v2: Add a func to detect wc ggtt detection (Ville)
>
> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: Joonas Lahtinen
> Cc: Jani Nikula
> Cc: Jonathan Cavitt
> Cc: John Harrison
> Cc: Andi Shyti
> Cc: Ville Syrjälä
> Cc: # v6.2+
> Suggested-by: Matt Roper
> Signed-off-by: Nirmoy Das
> Acked-by: Andi Shyti
Reviewed-by: Matt Roper
Interestingly, bspec 151 indicates that we probably shouldn't have been
using a CPU:WC mapping for the GGTT on gen9bc platforms either (i.e.,
the GTT part of the GTTMMADR has the same "64-bits or less" restriction
listed as later platforms). But we've been using WC without issue for
the last 8 years, so I guess it's not worth changing it now.
Matt
> ---
> drivers/gpu/drm/i915/gt/intel_ggtt.c | 35 +++-
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 4d7d88b92632..401667f83f96 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -195,6 +195,21 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
> spin_unlock_irq(&uncore->lock);
> }
>
> +static bool needs_wc_ggtt_mapping(struct drm_i915_private *i915)
> +{
> + /*
> + * On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
> + * will be dropped. For WC mappings in general we have 64 byte burst
> + * writes when the WC buffer is flushed, so we can't use it, but have to
> + * resort to an uncached mapping. The WC issue is easily caught by the
> + * readback check when writing GTT PTE entries.
> + */
> + if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> + return true;
> +
> + return false;
> +}
> +
> static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> {
> struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> @@ -202,8 +217,12 @@ static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> /*
>* Note that as an uncached mmio write, this will flush the
>* WCB of the writes into the GGTT before it triggers the invalidate.
> + *
> + * Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
>*/
> - intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> + if (needs_wc_ggtt_mapping(ggtt->vm.i915))
> + intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> + GFX_FLSH_CNTL_EN);
> }
>
> static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> @@ -1126,17 +1145,11 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt,
> u64 size)
> GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
> gen6_gttmmadr_size(i915));
> phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> gen6_gttadr_offset(i915);
>
> - /*
> - * On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
> - * will be dropped. For WC mappings in general we have 64 byte burst
> - * writes when the WC buffer is flushed, so we can't use it, but have to
> - * resort to an uncached mapping. The WC issue is easily caught by the
> - * readback check when writing GTT PTE entries.
> - */
> - if (IS_GEN9_LP(i915) || GRAPHICS_VER(i915) >= 11)
> - ggtt->gsm = ioremap(phys_addr, size);
> - else
> + if (needs_wc_ggtt_mapping(i915))
> ggtt->gsm = ioremap_wc(phys_addr, size);
> + else
> + ggtt->gsm = ioremap(phys_addr, size);
> +
> if (!ggtt->gsm) {
> drm_err(&i915->drm, "Failed to map the ggtt page table\n");
> return -ENOMEM;
> --
> 2.41.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
On Fri, Oct 13, 2023 at 02:28:21PM +0200, Nirmoy Das wrote:
> Hi Ville,
>
> On 10/13/2023 12:50 PM, Ville Syrjälä wrote:
> > On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
> > > gen8_ggtt_invalidate() is only needed for limitted set of platforms
> > > where GGTT is mapped as WC
> > I know there is supposed to be some kind hw snooping of the ggtt
> > pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
> > has no other side effects we depend on?
>
> I spent some time searching through the gfxspec. This GFX_FLSH_CNTL register
> only seems to be for
>
> invalidating TLB for GUnit and (from git log ) we started to do that to
> enable WC based GGTT updates.
Might be good to cite the relevant git commits in the commit message to
make this clear.
-Sima
>
>
> So if I am not missing anything obvious then this should be safe.
>
>
> Regards,
>
> Nirmoy
>
> >
> > > otherwise this can cause unwanted
> > > side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> > > valid.
> > >
> > > Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> > > Cc: Rodrigo Vivi
> > > Cc: Tvrtko Ursulin
> > > Cc: Joonas Lahtinen
> > > Cc: Jani Nikula
> > > Cc: Jonathan Cavitt
> > > Cc: John Harrison
> > > Cc: Andi Shyti
> > > Cc: # v6.2+
> > > Suggested-by: Matt Roper
> > > Signed-off-by: Nirmoy Das
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > index 4d7d88b92632..c2858d434bce 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > @@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
> > > static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> > > {
> > > + struct drm_i915_private *i915 = ggtt->vm.i915;
> > > struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> > > /*
> > >* Note that as an uncached mmio write, this will flush the
> > >* WCB of the writes into the GGTT before it triggers the
> > > invalidate.
> > > + *
> > > + * Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
> > >*/
> > > - intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> > > + if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> > > + intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> > > GFX_FLSH_CNTL_EN);
> > > }
> > > static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> > > --
> > > 2.41.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v7 04/23] dt-bindings: display: mediatek: padding: Add MT8188
Hi Shawn, On Fri, 6 Oct 2023 at 08:38, Hsiao Chien Sung wrote: > + Padding provides ability to add pixels to width and height of a layer with > + specified colors. Due to hardware design, Mixer in VDOSYS1 requires > + width of a layer to be 2-pixel-align, or 4-pixel-align when ETHDR is > enabled, > + we need Padding to deal with odd width. > + Please notice that even if the Padding is in bypass mode, settings in > + register must be cleared to 0, or undefined behaviors could happen. If I understand the driver correctly, padding is automatically applied to compensate for unaligned dimensions. The first/last rows/columns of the overlay area will be filled with a specified colour (black?) to preserve the area. This is unfortunately not OK to do transparently. Userspace must be aware of this policy decision and specifically request it. If not, the atomic request check should fail and tell userspace that the requested configuration is not possible to achieve. Cheers, Daniel
Re: [PATCH] drm/ci: Default to UART for logging
On Fri, 6 Oct 2023 at 18:32, Rob Clark wrote: > ssh logging is the default for mesa, as it is generally more reliable. > But if there are kernel issues, especially at boot, UART logging is > infinitely more useful. Hmm, we should still be capturing the UART boot logs regardless. Those go into a collapsed 'LAVA boot' section but they don't just disappear ... ?
Re: Warning notice "Memory manager not clean during takedown" on RB5 runs
Hello Aishwarya, On Fri, 13 Oct 2023 at 19:05, Aishwarya TCV wrote: > > Hi Dmitry > > > On 17/08/2023 15:59, Dmitry Baryshkov wrote: > > Add displayport altmode declaration to the Type-C controller node to > > enable DP altmode negotiation. > > > > Signed-off-by: Dmitry Baryshkov > > --- > > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 7 +++ > > 1 file changed, 7 insertions(+) > > > > At present RB5 runs against next-master are consistently throwing > warning messages (attached below with call trace) on "Memory manager not > clean during takedown". I can send full logs if required. However, the > runs are booting successfully so the logs are quite big. Tried running > with older dtb built, where these > warnings was not observed. Thank you for the report. These patches should be fixed by the patchset at https://patchwork.freedesktop.org/series/125010/ -- With best wishes Dmitry
Re: [v3 2/3] drm/panel: ili9882t: Avoid blurred screen from fast sleep
Hi, On Thu, Oct 12, 2023 at 8:56 PM cong yang wrote: > > Hi, > > On Fri, Oct 13, 2023 at 10:28 AM Doug Anderson wrote: > > > > Hi, > > > > On Thu, Oct 12, 2023 at 6:12 PM cong yang > > wrote: > > > > > > Hi, > > > > > > On Thu, Oct 12, 2023 at 11:15 PM Doug Anderson > > > wrote: > > > > > > > > Hi, > > > > > > > > On Thu, Oct 12, 2023 at 5:10 AM Cong Yang > > > > wrote: > > > > > > > > > > At present, we have found that there may be a problem of blurred > > > > > screen during fast sleep/resume. The direct cause of the blurred > > > > > screen is that the IC does not receive 0x28/0x10. Because of the > > > > > particularity of the IC, before the panel enters sleep hid must > > > > > stop scanning, as i2c_hid_core_suspend before ili9882t_disable. > > > > > If move the ili9882t_enter_sleep_mode function to ili9882t_unprepare, > > > > > touch reset will pull low before panel entersleep, which does not meet > > > > > the timing requirements.. > > > > > > > > The above makes me believe that the reset GPIO should be moved out of > > > > the input driver and into the panel driver. I could just imagine that > > > > the kernel might have some reason it wants to suspend the i2c hid > > > > device. If that causes the panel to suddenly start failing then that > > > > would be bad... I think we should fix this. > > > > > > Thanks, I will confirm with ilitek in further analysis and use "move > > > the ili9882t_enter_sleep_mode > > > function to ili9882t_unprepare". Is the test failure really because > > > the touch reset timing > > > does not match? There is also a separate reset GPIO on the panel. > > > Shouldn't touch reset not > > > affect the panel? > > > > > > If we find a better solution I will continue upstream,。 So is it > > > possible to apply this plan now? > > > > I wouldn't be too upset at applying the current code as long as you're > > going to continue to investigate. We can always continue to iterate on > > it and having something working reasonably well is better than nothing > > at all. However, I probably would wait at least 1 week before applying > > any patch from you just simply out of courtesy to give others on the > > mailing list time to express their comments. ...presumably we could > > get to the bottom of the problem in that 1 week time anyway... > > > > I'm not trying to be an obstinate pain here--I'm merely trying to make > > sure that whatever we land will continue to work across kernel uprevs, > > even if driver probe order / timing changes in the kernel. If the > > panel is really so tied to the touchscreen device's reset GPIO timing > > then it worries me. What happens, for instance, if you disable the > > touchscreen CONFIG in the kernel? Does the panel still work, or is > > that extra reset GPIO totally critical to the functioning of the > > panel. If it's totally critical then it probably makes sense to move > > to the panel driver given that the touchscreen is a panel follower > > anyway... > > Thanks. It looks like the panel works fine after I disable the touch screen > device. So the panel may not depend on touch screen reset. > Need to continue investigating the root cause for current status. Ah, OK. So I guess the issue is that the ideal case involves more interleaving of things? Right now, I think this is what happens is at power off: 1. We call the "disable" of the panel code which enters sleep mode. 2. As panel follower, the touchscreen gets called to power off _before_ the panel's unprepare stage. This is when we assert the touchscreen reset GPIO. 3. We call the "unprepare" of the panel code which deasserts the "enable" pin of the panel and then disables regulators. The proper sequence is: 1. Stop i2c hid scanning 2. Panel enter sleep 3. Assert touchscreen reset 4. Deassert the "enable" pin of the panel and disable regulators. Ick. I guess the only way we'd be able to really make this work would be to extend panel follower to notify followers before _both_ the disable and the unprepare. I guess I can put that on my todo list and we can see what folks think. Looking closely at it, I agree that I don't think we want to move the "touchscreen reset" functionality into the panel even if it would probably work. That feels ugly. -Doug
Re: [PATCH v5 6/7] drm/ssd130x: Fix atomic_check for disabled planes
Thomas Zimmermann writes:
Hello Thomas,
> The plane's atomic_check returns -EINVAL if the CRTC has not been
> set. This is the case for disabled planes, for which atomic_check
> should return 0. For disabled planes, it also omits the mandatory
> call to drm_atomic_helper_check_plane_state().
>
> Replace the test with the boiler-plate code that first invokes
> drm_atomic_helper_check_plane_state() and then tests for the plane
> to be visible. Return early for non-visible planes.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private
> CRTC state")
> Reviewed-by: Javier Martinez Canillas
> Tested-by: Javier Martinez Canillas
> Cc: Geert Uytterhoeven
> Cc: Javier Martinez Canillas
> Cc: Maxime Ripard
> ---
I've picked this since it's an unrelated fix to the rest of the series.
Pushed to drm-misc (drm-misc-next) because the offending commit is not
in mainline yet. Thanks!
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v5 3/7] drm/sched: Move schedule policy to scheduler
On 2023-10-11 19:58, Matthew Brost wrote:
> Rather than a global modparam for scheduling policy, move the scheduling
> policy to scheduler so user can control each scheduler policy.
>
> v2:
> - s/DRM_SCHED_POLICY_MAX/DRM_SCHED_POLICY_COUNT (Luben)
> - Only include policy in scheduler (Luben)
> v3:
> - use a ternary operator as opposed to an if-control (Luben)
> - s/DRM_SCHED_POLICY_DEFAULT/DRM_SCHED_POLICY_UNSET/ (Luben)
> - s/default_drm_sched_policy/drm_sched_policy_default/ (Luben)
> - Update commit message (Boris)
> - Fix v3d build (CI)
> - s/bad_policies/drm_sched_policy_mismatch/ (Luben)
> - Don't update modparam doc (Luben)
> v4:
> - Fix alignment in msm_ringbuffer_new (Luben / checkpatch)
>
> Reviewed-by: Luben Tuikov
> Signed-off-by: Matthew Brost
Hi,
Forgot to mention this, but it is a very important process to note,
is that one should _never_ add someone else's R-V tag, _*UNLESS*_
a) there's an email from the person giving their review or ack, and
b) you're the one pushing the patch set into the tree.
If you're not the one pushing it into the tree, the maintainer will
add their R-V (after their reply-to follow-up email--see below),
including a Link: tag when they do "git am" after it's been all reviewed.
And there's a reason for this.
The reason is that when kernel maintainers (especially DRM via dim[1]) push
patches into the kernel, we want to add a Link: tag [2,3] pointing to
the thread where a) the patch was posted and b) where the reviewer gave
their Reviewed-by to the patch in a reply-all email, and at this moment
there is no such email for this patch.
When a maintainer says "Do X, Y, Z, for an immediate R-V", this means
do those things, post it, and get a reply from the maintainer with an
R-V. This records how it happened and is very helpful when doing
data mining on how and why the code changed, via what patches, etc.
I suspect there might be a v6, and we can do the R-V/Ack the right way
at that time. No big deal, but it's good to know in one place as it
is a bit scatter here and there in the kernel-doc.
[1] https://gitlab.freedesktop.org/drm/maintainer-tools/
[2] git am --message-id
[3] https://docs.kernel.org/maintainer/
--
Regards,
Luben
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> drivers/gpu/drm/etnaviv/etnaviv_sched.c| 3 ++-
> drivers/gpu/drm/lima/lima_sched.c | 3 ++-
> drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sched.c| 3 ++-
> drivers/gpu/drm/panfrost/panfrost_job.c| 3 ++-
> drivers/gpu/drm/scheduler/sched_entity.c | 24 ++
> drivers/gpu/drm/scheduler/sched_main.c | 19 -
> drivers/gpu/drm/v3d/v3d_sched.c| 15 +-
> include/drm/gpu_scheduler.h| 20 --
> 10 files changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index b54c4d771104..e4e6f91450a4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2283,6 +2283,7 @@ static int amdgpu_device_init_schedulers(struct
> amdgpu_device *adev)
> ring->num_hw_submission, 0,
> timeout, adev->reset_domain->wq,
> ring->sched_score, ring->name,
> +DRM_SCHED_POLICY_UNSET,
> adev->dev);
> if (r) {
> DRM_ERROR("Failed to create scheduler on ring %s.\n",
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> index 618a804ddc34..15b0e2f1abe5 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> @@ -137,7 +137,8 @@ int etnaviv_sched_init(struct etnaviv_gpu *gpu)
> ret = drm_sched_init(&gpu->sched, &etnaviv_sched_ops, NULL,
>etnaviv_hw_jobs_limit, etnaviv_job_hang_limit,
>msecs_to_jiffies(500), NULL, NULL,
> - dev_name(gpu->dev), gpu->dev);
> + dev_name(gpu->dev), DRM_SCHED_POLICY_UNSET,
> + gpu->dev);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/lima/lima_sched.c
> b/drivers/gpu/drm/lima/lima_sched.c
> index 8d858aed0e56..50c2075228aa 100644
> --- a/drivers/gpu/drm/lima/lima_sched.c
> +++ b/drivers/gpu/drm/lima/lima_sched.c
> @@ -491,7 +491,8 @@ int lima_sched_pipe_init(struct lima_sched_pipe *pipe,
> const char *name)
> return drm_sched_init(&pipe->base, &lima_sched_ops, NULL, 1,
> lima_job_hang_limit,
> msecs_to_jiffies(timeout), NULL,
> - NULL, name, pipe->ldev->dev);
> + NULL, name, DRM_SCHED_PO
[pull] amdgpu, amdkfd, radeon, UAPI drm-next-6.7
Hi Dave, Daniel, New stuff for 6.7. The following changes since commit 3698a75f5a98d0a6599e2878ab25d30a82dd836a: Merge tag 'drm-intel-next-fixes-2023-08-24' of git://anongit.freedesktop.org/drm/drm-intel into drm-next (2023-08-25 12:55:55 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-next-6.7-2023-10-13 for you to fetch changes up to cd90511557fdfb394bb4ac4c3b539b007383914c: drm/amdgpu/vkms: fix a possible null pointer dereference (2023-10-13 11:36:25 -0400) amd-drm-next-6.7-2023-10-13: amdgpu: - DC replay fixes - Misc code cleanups and spelling fixes - Documentation updates - RAS EEPROM Updates - FRU EEPROM Updates - IP discovery updates - SR-IOV fixes - RAS updates - DC PQ fixes - SMU 13.0.6 updates - GC 11.5 Support - NBIO 7.11 Support - GMC 11 Updates - Reset fixes - SMU 11.5 Updates - SMU 13.0 OD support - Use flexible arrays for bo list handling - W=1 Fixes - SubVP fixes - DPIA fixes - DCN 3.5 Support - Devcoredump fixes - VPE 6.1 support - VCN 4.0 Updates - S/G display fixes - DML fixes - DML2 Support - MST fixes - VRR fixes - Enable seamless boot in more cases - Enable content type property for HDMI - OLED fixes - Rework and clean up GPUVM TLB flushing - DC ODM fixes - DP 2.x fixes - AGP aperture fixes - SDMA firmware loading cleanups - Cyan Skillfish GPU clock counter fix - GC 11 GART fix - Cache GPU fault info for userspace queries - DC cursor check fixes - eDP fixes - DC FP handling fixes - Variable sized array fixes - SMU 13.0.x fixes - IB start and size alignment fixes for VCN - SMU 14 Support - Suspend and resume sequence rework - vkms fix amdkfd: - GC 11 fixes - GC 10 fixes - Doorbell fixes - CWSR fixes - SVM fixes - Clean up GC info enumeration - Rework memory limit handling - Coherent memory handling fixes - Use partial migrations in GPU faults - TLB flush fixes - DMA unmap fixes - GC 9.4.3 fixes - SQ interrupt fix - GTT mapping fix - GC 11.5 Support radeon: - Misc code cleanups - W=1 Fixes - Fix possible buffer overflow - Fix possible NULL pointer dereference UAPI: - Add EXT_COHERENT memory allocation flags. These allow for system scope atomics. Proposed userspace: https://github.com/RadeonOpenCompute/ROCT-Thunk-Interface/pull/88 - Add support for new VPE engine. This is a memory to memory copy engine with advanced scaling, CSC, and color management features Proposed mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25713 - Add INFO IOCTL interface to query GPU faults Proposed Mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23238 Proposed libdrm MR: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/298 Aaron Liu (4): drm/amdgpu: add golden setting for gc_11_5_0 drm/amdgpu: add imu firmware support for gc_11_5_0 drm/amdgpu: add mes firmware support for gc_11_5_0 drm/amdgpu/discovery: enable DCN 3.5.0 support Agustin Gutierrez (1): drm/amd/display: Optimize OLED T7 delay Alex Deucher (20): drm/amd/pm: fix debugfs pm_info output drm/amd/pm: fix error flow in sensor fetching drm/amdgpu/soc21: don't remap HDP registers for SR-IOV drm/amdgpu/nbio4.3: set proper rmmio_remap.reg_offset for SR-IOV drm/amdgpu: add vcn_doorbell_range callback for nbio 7.11 drm/amdgpu: add remap_hdp_registers callback for nbio 7.11 drm/amdgpu: add VPE IP discovery info to HW IP info query drm/amd/display: fix some style issues drm/amdgpu/gmc6-8: properly disable the AGP aperture drm/amdgpu/gmc: set a default disable value for AGP drm/amdgpu/gmc11: disable AGP on GC 11.5 drm/amdkfd: reduce stack size in kfd_topology_add_device() drm/amdkfd: drop struct kfd_cu_info drm/amdgpu/gmc: add a way to force a particular placement for GART drm/amdgpu/gmc11: set gart placement GC11 drm/amdgpu: add cached GPU fault structure to vm struct drm/amdgpu: cache gpuvm fault information for gmc7+ drm/amdgpu: add new INFO ioctl query for the last GPU page fault drm/amdgpu: refine fault cache updates drm/amdgpu: Enable SMU 13.0.0 optimizations when ROCm is active (v2) Alex Hung (31): drm/amd/display: skip audio config for virtual signal drm/amd/display: Remove unwanted drm edid references drm/amd/display: Initialize writeback connector drm/amd/display: Hande writeback request from userspace drm/amd/display: Add writeback enable/disable in dc drm/amd/display: Fix writeback_info never got updated drm/amd/display: Validate hw_points_num before using it drm/amd/display: Fix writeback_info is not removed drm/amd/display: Add writeback enable field (wb_enabled) drm/amd/display: Setup for mmhubbub3_warmup_mcif with big buffer drm/amd/display: Add new set_fc_enable
Re: [PATCH] drm/ci: Default to UART for logging
On Fri, Oct 13, 2023 at 9:28 AM Daniel Stone wrote: > > On Fri, 6 Oct 2023 at 18:32, Rob Clark wrote: > > ssh logging is the default for mesa, as it is generally more reliable. > > But if there are kernel issues, especially at boot, UART logging is > > infinitely more useful. > > Hmm, we should still be capturing the UART boot logs regardless. Those > go into a collapsed 'LAVA boot' section but they don't just disappear > ... ? Hmm, I wasn't seeing anything in the raw log, which doesn't collapse sections.. That said, I still think uart is preferable to ssh for kernel CI.. we aren't running jobs at the scale of mesa CI jobs so even if we get UART flakes 1/1000 (or even 1/100) times, that is an acceptable trade-off for the fact that uart can still work when things are too fubar for ssh. BR, -R
BUG: KASAN: slab-use-after-free in drm_connector_cleanup
Following kasan bug was noticed on arm64 bcm2711-rpi-4-b device running Linux next 6.6.0-rc5-next-20231011 with given config. This was first noticed on 6.6.0-rc2-next-20230918. This is an intermittent issue. Links to build and test logs provided. Reported-by: Linux Kernel Functional Testing Boot log: - [ 209.991842] Bluetooth: Core ver 2.22 [ 209.996014] NET: Registered PF_BLUETOOTH protocol family [ 210.002564] Bluetooth: HCI device and connection manager initialized [ 210.009204] Bluetooth: HCI socket layer initialized [ 210.014443] Bluetooth: L2CAP socket layer initialized [ 210.019906] Bluetooth: SCO socket layer initialized [ 210.175889] KTAP version 1 [ 210.178771] 1..3 [ 210.184030] KTAP version 1 [ 210.187234] # Subtest: vc4-pv-muxing-combinations [ 210.192491] # module: vc4 [ 210.192572] 1..2 [ 210.197937] KTAP version 1 [ 210.201544] # Subtest: drm_vc4_test_pv_muxing [ 210.218416] Bluetooth: HCI UART driver ver 2.3 [ 210.236869] [drm] Initialized vc4 0.0.0 20140616 for drm-kunit-mock-device on minor 1 [ 210.241063] Bluetooth: HCI UART protocol H4 registered [ 210.320009] Bluetooth: HCI UART protocol LL registered [ 210.464457] Bluetooth: HCI UART protocol Broadcom registered [ 210.470871] hci_uart_bcm serial0-0: supply vbat not found, using dummy regulator [ 210.472120] Bluetooth: HCI UART protocol QCA registered [ 210.480123] hci_uart_bcm serial0-0: supply vddio not found, using dummy regulator [ 210.490971] Bluetooth: HCI UART protocol Marvell registered Debian GNU/Linux 12 runner-vwmj3eza-project-40964107-concurrent-0 ttyS0 runner-vwmj3eza-project-40964107-concurrent-0 login: [ 2root 10.623188] cfg80211: Loading compiled-in X.509 certificates for regulatory database [ 210.658616] == [ 210.666006] BUG: KASAN: slab-use-after-free in drm_connector_cleanup+0x30/0x488 drm [ 210.675144] Read of size 8 at addr 000113a8e0a8 by task kunit_try_catch/1750 [ 210.682685] [ 210.684219] CPU: 1 PID: 1750 Comm: kunit_try_catch Tainted: GB N 6.6.0-rc5-next-20231011 #1 [ 210.694056] Hardware name: Raspberry Pi 4 Model B (DT) [ 210.699323] Call trace: r[ 210.701824] dump_backtrace (arch/arm64/kernel/stacktrace.c:235) [ 210.705757] show_stack (arch/arm64/kernel/stacktrace.c:242) [ 210.709160] dump_stack_lvl (lib/dump_stack.c:107) [ 210.712917] print_report (mm/kasan/report.c:365 mm/kasan/report.c:475) [ 210.716560] kasan_report (mm/kasan/report.c:590) o[ 210.720201] __asan_load8 (mm/kasan/generic.c:260) o[ 210.723827] drm_connector_cleanup+0x30/0x488 drm [ 210.729265] drm_connector_cleanup_action+0x1c/0x30 drm t[ 210.735228] drm_managed_release+0x128/0x228 drm [ 210.740570] drm_dev_put.part.0+0xb4/0xf8 drm [ 210.742616] Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7' [ 210.745649] devm_drm_dev_init_release+0x1c/0x38 drm [ 210.756667] devm_action_release (drivers/base/devres.c:722) [ 210.760840] release_nodes (drivers/base/devres.c:506) [ 210.764569] devres_release_all (drivers/base/devres.c:535) [ 210.765145] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2 [ 210.768737] device_unbind_cleanup (drivers/base/dd.c:551) [ 210.768759] device_release_driver_internal (drivers/base/dd.c:1280 drivers/base/dd.c:1295) [ 210.768775] device_release_driver (drivers/base/dd.c:1319) [ 210.768788] bus_remove_device (include/linux/kobject.h:193 drivers/base/base.h:73 drivers/base/bus.c:581) [ 210.768801] device_del (drivers/base/core.c:3815) [ 210.768812] platform_device_del.part.0 (drivers/base/platform.c:753) [ 210.768824] platform_device_del (drivers/base/platform.c:764) [ 210.768834] kunit_action_platform_device_del+0x18/0x30 drm_kunit_helpers [ 210.810264] cfg80211: failed to load regulatory.db [ 210.815438] __kunit_action_free (lib/kunit/resource.c:92) [ 210.815460] kunit_remove_resource (include/kunit/resource.h:120 include/linux/kref.h:65 include/kunit/resource.h:138 lib/kunit/resource.c:59 lib/kunit/resource.c:48) [ 210.815474] kunit_cleanup (lib/kunit/test.c:815 (discriminator 1)) [ 210.815486] kunit_try_run_case_cleanup (lib/kunit/test.c:412) [ 210.815499] kunit_generic_run_threadfn_adapter (lib/kunit/try-catch.c:30) [ 210.815514] kthread (kernel/kthread.c:388) [ 210.815526] ret_from_fork (arch/arm64/kernel/entry.S:858) [ 210.815540] [ 210.815544] Allocated by task 1745: [ 210.815552] kasan_save_stack (mm/kasan/common.c:46) [ 210.815567] kasan_set_track (mm/kasan/common.c:52 (discriminator 1)) [ 210.815579] kasan_save_alloc_info (mm/kasan/generic.c:512) [ 210.815592] __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) [ 210.815603] __kmalloc (include/linux/kasan.h:198 mm/slab_common.c:1004 mm/slab_common.c:1017) [ 210.815613] kunit_kmalloc_array (include/linux/slab.h:637 lib/kunit/test.c:779) [ 210.815625] vc4_dummy_output+0xac/0x228 vc4 [ 210.882798] __mock_device+0x24c/0x4b0 vc4 [
[PATCH 0/5] Add Support for RK3566 Powkiddy RGB30
From: Chris Morgan Add support for the Powkiddy RGB30 handheld gaming console. Chris Morgan (5): dt-bindings: vendor-prefixes: document Powkiddy dt-bindings: panel: Add Powkiddy RGB30 panel compatible drm/panel: st7703: Add Powkiddy RGB30 Panel Support dt-bindings: arm64: rockchip: add Powkiddy RGB30 arm64: dts: rockchip: add support for Powkiddy RGB30 .../devicetree/bindings/arm/rockchip.yaml | 5 + .../display/panel/rocktech,jh057n00900.yaml | 2 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + arch/arm64/boot/dts/rockchip/Makefile | 1 + .../dts/rockchip/rk3566-powkiddy-rgb30.dts| 152 ++ drivers/gpu/drm/panel/panel-sitronix-st7703.c | 89 ++ 6 files changed, 251 insertions(+) create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts -- 2.34.1
[PATCH 2/5] dt-bindings: panel: Add Powkiddy RGB30 panel compatible
From: Chris Morgan The Powkiddy RGB30 panel is a 4 inch 720x720 MIPI-DSI LCD panel. It appears to be based on the ST7703 LCD controller (this is assumed from the init sequence similarity between this and other displays). Powkiddy would not share the part number or name for the display from the bill of materials and there were no obvious external markings, so name the panel for the device (Powkiddy RGB30). Signed-off-by: Chris Morgan --- .../devicetree/bindings/display/panel/rocktech,jh057n00900.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.yaml b/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.yaml index 5ea74426b1d5..97cccd8a8479 100644 --- a/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.yaml +++ b/Documentation/devicetree/bindings/display/panel/rocktech,jh057n00900.yaml @@ -22,6 +22,8 @@ properties: enum: # Anberic RG353V-V2 5.0" 640x480 TFT LCD panel - anbernic,rg353v-panel-v2 + # Powkiddy RGB30 3.0" 720x720 TFT LCD panel + - powkiddy,rgb30-panel # Rocktech JH057N00900 5.5" 720x1440 TFT LCD panel - rocktech,jh057n00900 # Xingbangda XBD599 5.99" 720x1440 TFT LCD panel -- 2.34.1
[PATCH 4/5] dt-bindings: arm64: rockchip: add Powkiddy RGB30
From: Chris Morgan The Powkiddy RGB30 is a portable handheld console from Powkiddy which uses the Rockchip RK3566 SoC. Signed-off-by: Chris Morgan --- Documentation/devicetree/bindings/arm/rockchip.yaml | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml index ca5389862887..a349bf4da6bc 100644 --- a/Documentation/devicetree/bindings/arm/rockchip.yaml +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml @@ -669,6 +669,11 @@ properties: - const: pine64,soquartz - const: rockchip,rk3566 + - description: Powkiddy RGB30 +items: + - const: powkiddy,rgb30 + - const: rockchip,rk3566 + - description: Radxa Compute Module 3(CM3) items: - enum: -- 2.34.1
[PATCH 1/5] dt-bindings: vendor-prefixes: document Powkiddy
From: Chris Morgan Document Powkiddy (https://powkiddy.com/). Signed-off-by: Chris Morgan --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 573578db9509..25fd2dc378f5 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1081,6 +1081,8 @@ patternProperties: description: Powertip Tech. Corp. "^powervr,.*": description: PowerVR (deprecated, use img) + "^powkiddy,.*": +description: Powkiddy "^primux,.*": description: Primux Trading, S.L. "^probox2,.*": -- 2.34.1
[PATCH 5/5] arm64: dts: rockchip: add support for Powkiddy RGB30
From: Chris Morgan
The Powkiddy RGB30 is a portable game device based on the Rockchip
RK3566 SoC. It has GPIO buttons on the face and sides for input, stereo
speakers, a 720x720 4 inch DSI display, a USB-C host port and a USB-C
peripheral port, dual SD card slots, WiFi, Bluetooth, and 1GB of RAM.
Working/Tested:
- SDMMC
- UART (for debugging)
- Buttons
- Charging/battery/PMIC
- Speaker/Headphones
- USB
- WiFi
- Bluetooth
- Display (at 59.04hz)
Not Working/TODO:
- Display does not resume from suspend properly. I'm working with the
manufacturer to resolve this.
Signed-off-by: Chris Morgan
---
arch/arm64/boot/dts/rockchip/Makefile | 1 +
.../dts/rockchip/rk3566-powkiddy-rgb30.dts| 152 ++
2 files changed, 153 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
diff --git a/arch/arm64/boot/dts/rockchip/Makefile
b/arch/arm64/boot/dts/rockchip/Makefile
index e7728007fd1b..3f01b429a3aa 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -77,6 +77,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-anbernic-rg353vs.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-anbernic-rg503.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.1.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.2.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-powkiddy-rgb30.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-a.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-b.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-radxa-cm3-io.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
new file mode 100644
index ..c7828c99a1bb
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/dts-v1/;
+
+#include
+#include
+#include
+#include "rk3566-anbernic-rg353x.dtsi"
+
+/ {
+ model = "RGB30";
+ compatible = "powkiddy,rgb30", "rockchip,rk3566";
+
+ aliases {
+ mmc1 = &sdmmc0;
+ mmc2 = &sdmmc1;
+ mmc3 = &sdmmc2;
+ };
+
+ battery: battery {
+ compatible = "simple-battery";
+ charge-full-design-microamp-hours = <3151000>;
+ charge-term-current-microamp = <30>;
+ constant-charge-current-max-microamp = <200>;
+ constant-charge-voltage-max-microvolt = <425>;
+ factory-internal-resistance-micro-ohms = <117000>;
+ voltage-max-design-microvolt = <4172000>;
+ voltage-min-design-microvolt = <340>;
+
+ ocv-capacity-celsius = <20>;
+ ocv-capacity-table-0 = <4172000 100>, <4092000 95>, <4035000
90>, <399 85>,
+ <3939000 80>, <3895000 75>, <3852000
70>, <3807000 65>,
+ <3762000 60>, <3713000 55>, <3672000
50>, <3647000 45>,
+ <3629000 40>, <3613000 35>, <3598000
30>, <3578000 25>,
+ <355 20>, <3519000 15>, <3479000
10>, <3438000 5>,
+ <340 0>;
+ };
+
+ /*
+* Channels reversed for speakers. Headphones automatically switch via
hardware when
+* detected with no ability to control output in software. Headphones
appear to be mono
+* (each output channel receives all audio). No microphone support on
3.5mm jack.
+*/
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "rk817_ext";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,mclk-fs = <256>;
+ simple-audio-card,widgets =
+ "Headphone", "Headphones";
+ simple-audio-card,routing =
+ "Headphones", "HPOL",
+ "Headphones", "HPOR";
+
+ simple-audio-card,codec {
+ sound-dai = <&rk817>;
+ };
+
+ simple-audio-card,cpu {
+ sound-dai = <&i2s1_8ch>;
+ };
+ };
+};
+
+/delete-node/ &adc_keys;
+
+&cru {
+ assigned-clocks = <&pmucru CLK_RTC_32K>, <&cru PLL_GPLL>,
+ <&pmucru PLL_PPLL>, <&cru PLL_VPLL>;
+ assigned-clock-rates = <32768>, <12>,
+ <2>, <10800>;
+};
+
+&gpio_keys_control {
+ button-r1 {
+ gpios = <&gpio3 RK_PB3 GPIO_ACTIVE_LOW>;
+ label = "TR";
+ linux,code = ;
+ };
+
+ button-r2 {
+ gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_LOW>;
+ label = "TR2";
+ linux,code = ;
+ };
+};
+
+/delete-node/ &{/i2c@fdd4/regulator@40};
+
+&i2c0 {
+ vdd_cpu: regulator@1c {
+ compatible = "tcs,t
[PATCH 3/5] drm/panel: st7703: Add Powkiddy RGB30 Panel Support
From: Chris Morgan
The Powkiddy RGB30 4 inch panel is a 4 inch 720x720 DSI panel used in
the Powkiddy RGB30 handheld gaming device. Add support for it.
TODO: The panel seems to not resume properly from suspend. I've
confirmed on the other ST7703 based devices it works correctly.
Signed-off-by: Chris Morgan
---
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 89 +++
1 file changed, 89 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
index 6a3945639535..a9892a4447fb 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
@@ -433,6 +433,94 @@ static const struct st7703_panel_desc rg353v2_desc = {
.init_sequence = rg353v2_init_sequence,
};
+static int rgb30panel_init_sequence(struct st7703 *ctx)
+{
+ struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+ /* Init sequence extracted from Powkiddy RGB30 BSP kernel. */
+
+ /*
+* For some reason this specific panel must be taken out of sleep
+* before the full init sequence, or else it will not display.
+*/
+ mipi_dsi_dcs_exit_sleep_mode(dsi);
+ msleep(250);
+
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETEXTC, 0xf1, 0x12, 0x83);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETMIPI, 0x33, 0x81, 0x05, 0xf9,
+ 0x0e, 0x0e, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x44, 0x25, 0x00, 0x90, 0x0a, 0x00,
+ 0x00, 0x01, 0x4f, 0x01, 0x00, 0x00, 0x37);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER_EXT, 0x25, 0x22, 0xf0,
+ 0x63);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_UNKNOWN_BF, 0x02, 0x11, 0x00);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETRGBIF, 0x10, 0x10, 0x28,
+ 0x28, 0x03, 0xff, 0x00, 0x00, 0x00, 0x00);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETSCR, 0x73, 0x73, 0x50, 0x50,
+ 0x00, 0x00, 0x12, 0x70, 0x00);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVDC, 0x46);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0b);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETDISP, 0x3c, 0x12, 0x30);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETEQ, 0x07, 0x07, 0x0b, 0x0b,
+ 0x03, 0x0b, 0x00, 0x00, 0x00, 0x00, 0xff, 0x00,
+ 0xc0, 0x10);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER, 0x36, 0x00, 0x32,
+ 0x32, 0x77, 0xf1, 0xcc, 0xcc, 0x77, 0x77, 0x33,
+ 0x33);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP, 0x0a, 0x0a);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM, 0x88, 0x88);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP1, 0xc8, 0x10, 0x0a, 0x10,
+ 0x0f, 0xa1, 0x80, 0x12, 0x31, 0x23, 0x47, 0x86,
+ 0xa1, 0x80, 0x47, 0x08, 0x00, 0x00, 0x0d, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x0d, 0x00, 0x00, 0x00,
+ 0x48, 0x02, 0x8b, 0xaf, 0x46, 0x02, 0x88, 0x88,
+ 0x88, 0x88, 0x88, 0x48, 0x13, 0x8b, 0xaf, 0x57,
+ 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP2, 0x96, 0x12, 0x01, 0x01,
+ 0x01, 0x78, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x4f, 0x31, 0x8b, 0xa8, 0x31, 0x75, 0x88, 0x88,
+ 0x88, 0x88, 0x88, 0x4f, 0x20, 0x8b, 0xa8, 0x20,
+ 0x64, 0x88, 0x88, 0x88, 0x88, 0x88, 0x23, 0x00,
+ 0x00, 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x40, 0xa1, 0x80, 0x00, 0x00, 0x00,
+ 0x00);
+ mipi_dsi_dcs_write_seq(dsi, ST7703_CMD_SETGAMMA, 0x00, 0x0a, 0x0f,
+ 0x29, 0x3b, 0x3f, 0x42, 0x39, 0x06, 0x0d, 0x10,
+ 0x13, 0x15, 0x14, 0x15, 0x10, 0x17, 0x00, 0x0a,
+ 0x0f, 0x29, 0x3b, 0x3f, 0x42, 0x39, 0x06, 0x0d,
+ 0x10, 0x13, 0x15, 0x14, 0x15, 0x10, 0x17);
+
+ return 0;
+}
+
+static const struct drm_display_mode rgb30panel_mode = {
+ .hdisplay = 720,
+ .hsync_start= 720 + 45,
+ .hsync_end = 720 + 45 + 4,
+ .htotal = 720 + 45 + 4 + 45,
+ .vdisplay = 720,
+ .vsync_start= 720 + 15,
+ .vsync_end = 720 + 15 + 3,
+ .v
Re: [PATCH 5/9] dma-buf: heaps: mtk_sec_heap: Initialise tee session
Sorry for the delayed reply, needed to get some more info. This really wouldn't be possible due to the limitation on the number of regions...for example only 32 regions can be defined on some SoCs, and you'd run out of regions really fast trying to do this. That's why this is creating heaps for those regions and then allocations are performed within the defined region is the preferred strategy. On Thu, Sep 28, 2023 at 11:54 PM Benjamin Gaignard < benjamin.gaign...@collabora.com> wrote: > > Le 28/09/2023 à 19:48, Jeffrey Kardatzke a écrit : > > On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard > > wrote: > >> > >> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit : > >>> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard > >>> wrote: > Le 27/09/2023 à 15:46, Joakim Bech a écrit : > > On Mon, Sep 25, 2023 at 12:49:50PM +, Yong Wu (吴勇) wrote: > >> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote: > >>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto: > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno > wrote: > > Il 11/09/23 04:30, Yong Wu ha scritto: > >> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't > >> work > >> here since this is not a platform driver, therefore initialise > >> the > >> TEE > >> context/session while we allocate the first secure buffer. > >> > >> Signed-off-by: Yong Wu > >> --- > >> drivers/dma-buf/heaps/mtk_secure_heap.c | 61 > >> + > >> 1 file changed, 61 insertions(+) > >> > >> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c > >> b/drivers/dma- > >> buf/heaps/mtk_secure_heap.c > >> index bbf1c8dce23e..e3da33a3d083 100644 > >> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c > >> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c > >> @@ -10,6 +10,12 @@ > >> #include > >> #include > >> #include > >> +#include > >> +#include > >> + > >> +#define TZ_TA_MEM_UUID "4477588a-8476-11e2-ad15- > >> e41f1390d676" > >> + > > Is this UUID the same for all SoCs and all TZ versions? > Yes. It is the same for all SoCs and all TZ versions currently. > > >>> That's good news! > >>> > >>> Is this UUID used in any userspace component? (example: Android > >>> HALs?) > >> No. Userspace never use it. If userspace would like to allocate this > >> secure buffer, it can achieve through the existing dmabuf IOCTL via > >> /dev/dma_heap/mtk_svp node. > >> > > In general I think as mentioned elsewhere in comments, that there > isn't > > that much here that seems to be unique for MediaTek in this patch > > series, so I think it worth to see whether this whole patch set can > be > > made more generic. Having said that, the UUID is always unique for a > > certain Trusted Application. So, it's not entirely true saying that > the > > UUID is the same for all SoCs and all TrustZone versions. It might be > > true for a family of MediaTek devices and the TEE in use, but not > > generically. > > > > So, if we need to differentiate between different TA implementations, > > then we need different UUIDs. If it would be possible to make this > patch > > set generic, then it sounds like a single UUID would be sufficient, > but > > that would imply that all TA's supporting such a generic UUID would > be > > implemented the same from an API point of view. Which also means that > > for example Trusted Application function ID's needs to be the same > etc. > > Not impossible to achieve, but still not easy (different TEE follows > > different specifications) and it's not typically something we've > done in > > the past. > > > > Unfortunately there is no standardized database of TA's describing > what > > they implement and support. > > > > As an alternative, we could implement a query call in the TEE > answering, > > "What UUID does your TA have that implements secure unmapped heap?". > > I.e., something that reminds of a lookup table. Then we wouldn't > have to > > carry this in UAPI, DT or anywhere else. > Joakim does a TA could offer a generic API and hide the hardware > specific > details (like kernel uAPI does for drivers) ? > >>> It would have to go through another layer (like the tee driver) to be > >>> a generic API. The main issue with TAs is that they have UUIDs you > >>> need to connect to and specific codes for each function; so we should > >>> abstract at a layer above where those exist in the dma-heap code. > Aside that question I wonder what are the needs to perform a 'secure' > playback. > I have in mind 2 requirements: > - secure memory regions, whic
Re: [PATCH 5/9] dma-buf: heaps: mtk_sec_heap: Initialise tee session
Sorry for the delayed reply, needed to get some more info. This really wouldn't be possible due to the limitation on the number of regions...for example only 32 regions can be defined on some SoCs, and you'd run out of regions really fast trying to do this. That's why this is creating heaps for those regions and then allocations are performed within the defined region is the preferred strategy. On Thu, Sep 28, 2023 at 11:54 PM Benjamin Gaignard wrote: > > > Le 28/09/2023 à 19:48, Jeffrey Kardatzke a écrit : > > On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard > > wrote: > >> > >> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit : > >>> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard > >>> wrote: > Le 27/09/2023 à 15:46, Joakim Bech a écrit : > > On Mon, Sep 25, 2023 at 12:49:50PM +, Yong Wu (吴勇) wrote: > >> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote: > >>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto: > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno > wrote: > > Il 11/09/23 04:30, Yong Wu ha scritto: > >> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't > >> work > >> here since this is not a platform driver, therefore initialise > >> the > >> TEE > >> context/session while we allocate the first secure buffer. > >> > >> Signed-off-by: Yong Wu > >> --- > >> drivers/dma-buf/heaps/mtk_secure_heap.c | 61 > >> + > >> 1 file changed, 61 insertions(+) > >> > >> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c > >> b/drivers/dma- > >> buf/heaps/mtk_secure_heap.c > >> index bbf1c8dce23e..e3da33a3d083 100644 > >> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c > >> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c > >> @@ -10,6 +10,12 @@ > >> #include > >> #include > >> #include > >> +#include > >> +#include > >> + > >> +#define TZ_TA_MEM_UUID "4477588a-8476-11e2-ad15- > >> e41f1390d676" > >> + > > Is this UUID the same for all SoCs and all TZ versions? > Yes. It is the same for all SoCs and all TZ versions currently. > > >>> That's good news! > >>> > >>> Is this UUID used in any userspace component? (example: Android > >>> HALs?) > >> No. Userspace never use it. If userspace would like to allocate this > >> secure buffer, it can achieve through the existing dmabuf IOCTL via > >> /dev/dma_heap/mtk_svp node. > >> > > In general I think as mentioned elsewhere in comments, that there isn't > > that much here that seems to be unique for MediaTek in this patch > > series, so I think it worth to see whether this whole patch set can be > > made more generic. Having said that, the UUID is always unique for a > > certain Trusted Application. So, it's not entirely true saying that the > > UUID is the same for all SoCs and all TrustZone versions. It might be > > true for a family of MediaTek devices and the TEE in use, but not > > generically. > > > > So, if we need to differentiate between different TA implementations, > > then we need different UUIDs. If it would be possible to make this patch > > set generic, then it sounds like a single UUID would be sufficient, but > > that would imply that all TA's supporting such a generic UUID would be > > implemented the same from an API point of view. Which also means that > > for example Trusted Application function ID's needs to be the same etc. > > Not impossible to achieve, but still not easy (different TEE follows > > different specifications) and it's not typically something we've done in > > the past. > > > > Unfortunately there is no standardized database of TA's describing what > > they implement and support. > > > > As an alternative, we could implement a query call in the TEE answering, > > "What UUID does your TA have that implements secure unmapped heap?". > > I.e., something that reminds of a lookup table. Then we wouldn't have to > > carry this in UAPI, DT or anywhere else. > Joakim does a TA could offer a generic API and hide the hardware specific > details (like kernel uAPI does for drivers) ? > >>> It would have to go through another layer (like the tee driver) to be > >>> a generic API. The main issue with TAs is that they have UUIDs you > >>> need to connect to and specific codes for each function; so we should > >>> abstract at a layer above where those exist in the dma-heap code. > Aside that question I wonder what are the needs to perform a 'secure' > playback. > I have in mind 2 requirements: > - secure memory regions, which means configure the hardware to ensure > t
Re: [PATCH 5/7] drm/sun4i: dw-hdmi: Split driver registration
Dne četrtek, 05. oktober 2023 ob 10:43:14 CEST je Maxime Ripard napisal(a): > On Mon, Sep 25, 2023 at 05:07:45PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 25. september 2023 ob 09:47:15 CEST je Maxime Ripard > > napisal(a): > > > On Sun, Sep 24, 2023 at 09:26:02PM +0200, Jernej Skrabec wrote: > > > > There is no reason to register two drivers in same place. Using macro > > > > lowers amount of boilerplate code. > > > > > > There's one actually: you can't have several module_init functions in > > > the some module, and both files are compiled into the same module. > > > > Yeah, I figured as much. However, I think code clean up is good enough > > reason > > to add hidden option in Kconfig and extra entry in Makefile (in v2). > > > > Do you agree? > > Yeah, I don't know. Adding more modules makes it more difficult to > handle (especially autoloading) without a clear argument why. > Module_init is simple enough as it is currently, maybe we should just > add a comment to make it clearer? I'll just drop this patch then. While I think autoloading works pretty good these days and cleaner code is nice, it can certainly cause some issues while packaging. Best regards, Jernej
Re: Implement per-key keyboard backlight as auxdisplay?
Hi!
> > The specific keyboard RGB controller I want to implement takes 6*21 rgb
> > values. However not every one is actually mapped to a physical key. e.g. the
> > bottom row needs less entries because of the space bar. Additionally the
> > keys are ofc not in a straight line from top to bottom.
>
> So... a bit of rationale. The keyboard does not really fit into the
> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> a matrix of them.
>
> We do see various strange displays these days -- they commonly have
> rounded corners and holes in them. I'm not sure how that's currently
> supported, but I believe it is reasonable to view keyboard as a
> display with slightly weird placing of pixels.
>
> Plus, I'd really like to play tetris on one of those :-).
>
> So, would presenting them as auxdisplay be acceptable? Or are there
> better options?
Oh and... My existing keyboard membrane-based Chicony, and it is from
time when PS/2 was still in wide use. I am slowly looking for a new
keyboard. If you know of one with nice mechanical switches, RGB
backlight with known protocol, and hopefully easily available in Czech
republic, let me know.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
signature.asc
Description: PGP signature
Re: Implement per-key keyboard backlight as auxdisplay?
Hi!
> coming from the leds mailing list I'm writing with Pavel how to best handle
> per-key RGB keyboards.
>
> His suggestion was that it could be implemented as an aux display, but he
> also suggested that I ask first if this fits.
Thanks for doing this.
> The specific keyboard RGB controller I want to implement takes 6*21 rgb
> values. However not every one is actually mapped to a physical key. e.g. the
> bottom row needs less entries because of the space bar. Additionally the
> keys are ofc not in a straight line from top to bottom.
So... a bit of rationale. The keyboard does not really fit into the
LED subsystem; LEDs are expected to be independent ("hdd led") and not
a matrix of them.
We do see various strange displays these days -- they commonly have
rounded corners and holes in them. I'm not sure how that's currently
supported, but I believe it is reasonable to view keyboard as a
display with slightly weird placing of pixels.
Plus, I'd really like to play tetris on one of those :-).
So, would presenting them as auxdisplay be acceptable? Or are there
better options?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
signature.asc
Description: PGP signature
[PATCH v2 1/2] video: fbdev: core: cfbcopyarea: fix sloppy typing
In cfb_copyarea(), the local variable bits_per_line is needlessly typed as
*unsigned long* -- which is a 32-bit type on the 32-bit arches and a 64-bit
type on the 64-bit arches; that variable's value is derived from the __u32
typed fb_fix_screeninfo::line_length field (multiplied by 8u) and a 32-bit
*unsigned int* type should still be enough to store the # of bits per line.
Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.
Signed-off-by: Sergey Shtylyov
---
drivers/video/fbdev/core/cfbcopyarea.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/cfbcopyarea.c
b/drivers/video/fbdev/core/cfbcopyarea.c
index 6d4bfeecee35..5b80bf3dae50 100644
--- a/drivers/video/fbdev/core/cfbcopyarea.c
+++ b/drivers/video/fbdev/core/cfbcopyarea.c
@@ -382,7 +382,7 @@ void cfb_copyarea(struct fb_info *p, const struct
fb_copyarea *area)
{
u32 dx = area->dx, dy = area->dy, sx = area->sx, sy = area->sy;
u32 height = area->height, width = area->width;
- unsigned long const bits_per_line = p->fix.line_length*8u;
+ unsigned int const bits_per_line = p->fix.line_length * 8u;
unsigned long __iomem *base = NULL;
int bits = BITS_PER_LONG, bytes = bits >> 3;
unsigned dst_idx = 0, src_idx = 0, rev_copy = 0;
--
2.26.3
[PATCH v2 0/2] Fix sloppy typing in the FB area copying routines
Here are 2 patches against the 'master' branch of Linus' 'linux.git' repo...
In {cfb|sys}_copyarea(), the local variable bits_per_line is needlessly typed
as *unsigned long* -- which is a 32-bit type on the 32-bit arches and a 64-bit
type on the 64-bit arches;; that variable's value is derived from the __u32
typed fb_fix_screeninfo::line_length field (multiplied by 8u) and a 32-bit
*unsigned int* type should still be enough to store the # of bits per line.
Sergey Shtylyov (2):
video: fbdev: core: cfbcopyarea: fix sloppy typing
video: fbdev: core: syscopyarea: fix sloppy typing
drivers/video/fbdev/core/cfbcopyarea.c | 2 +-
drivers/video/fbdev/core/syscopyarea.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.26.3
