Re: [PATCH] drm: bridge: it66121: Added it66121 chip external screen status judgment
I tested it on Loongarch and MIPS, and the results were fine。 -- Original -- From: "Neil Armstrong"
[PATCH] drm/bridge: anx7625: Fix not correct get property counts
The property length which returns from "of_get_property", divided by sizeof(int) to get the total property counts. Fixes: fd0310b6fe7d ("drm/bridge: anx7625: add MIPI DPI input feature") Signed-off-by: Xin Ji --- drivers/gpu/drm/bridge/analogix/anx7625.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index c6a9a02ed762..87081d5b408d 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -1594,6 +1594,7 @@ static int anx7625_get_swing_setting(struct device *dev, if (of_get_property(dev->of_node, "analogix,lane0-swing", &num_regs)) { + num_regs /= sizeof(int); if (num_regs > DP_TX_SWING_REG_CNT) num_regs = DP_TX_SWING_REG_CNT; @@ -1604,6 +1605,7 @@ static int anx7625_get_swing_setting(struct device *dev, if (of_get_property(dev->of_node, "analogix,lane1-swing", &num_regs)) { + num_regs /= sizeof(int); if (num_regs > DP_TX_SWING_REG_CNT) num_regs = DP_TX_SWING_REG_CNT; -- 2.25.1
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915: Improve long running OCL w/a for GuC submission
On 09/03/2022 21:16, John Harrison wrote: On 3/8/2022 01:41, Tvrtko Ursulin wrote: On 03/03/2022 22:37, john.c.harri...@intel.com wrote: From: John Harrison A workaround was added to the driver to allow OpenCL workloads to run 'forever' by disabling pre-emption on the RCS engine for Gen12. It is not totally unbound as the heartbeat will kick in eventually and cause a reset of the hung engine. However, this does not work well in GuC submission mode. In GuC mode, the pre-emption timeout is how GuC detects hung contexts and triggers a per engine reset. Thus, disabling the timeout means also losing all per engine reset ability. A full GT reset will still occur when the heartbeat finally expires, but that is a much more destructive and undesirable mechanism. The purpose of the workaround is actually to give OpenCL tasks longer to reach a pre-emption point after a pre-emption request has been issued. This is necessary because Gen12 does not support mid-thread pre-emption and OpenCL can have long running threads. So, rather than disabling the timeout completely, just set it to a 'long' value. v2: Review feedback from Tvrtko - must hard code the 'long' value instead of determining it algorithmically. So make it an extra CONFIG definition. Also, remove the execlist centric comment from the existing pre-emption timeout CONFIG option given that it applies to more than just execlists. Signed-off-by: John Harrison Reviewed-by: Daniele Ceraolo Spurio (v1) Acked-by: Michal Mrozek --- drivers/gpu/drm/i915/Kconfig.profile | 26 +++ drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 ++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile index 39328567c200..7cc38d25ee5c 100644 --- a/drivers/gpu/drm/i915/Kconfig.profile +++ b/drivers/gpu/drm/i915/Kconfig.profile @@ -57,10 +57,28 @@ config DRM_I915_PREEMPT_TIMEOUT default 640 # milliseconds help How long to wait (in milliseconds) for a preemption event to occur - when submitting a new context via execlists. If the current context - does not hit an arbitration point and yield to HW before the timer - expires, the HW will be reset to allow the more important context - to execute. + when submitting a new context. If the current context does not hit + an arbitration point and yield to HW before the timer expires, the + HW will be reset to allow the more important context to execute. + + This is adjustable via + /sys/class/drm/card?/engine/*/preempt_timeout_ms + + May be 0 to disable the timeout. + + The compiled in default may get overridden at driver probe time on + certain platforms and certain engines which will be reflected in the + sysfs control. + +config DRM_I915_PREEMPT_TIMEOUT_COMPUTE + int "Preempt timeout for compute engines (ms, jiffy granularity)" + default 7500 # milliseconds + help + How long to wait (in milliseconds) for a preemption event to occur + when submitting a new context to a compute capable engine. If the + current context does not hit an arbitration point and yield to HW + before the timer expires, the HW will be reset to allow the more + important context to execute. This is adjustable via /sys/class/drm/card?/engine/*/preempt_timeout_ms diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 4185c7338581..cc0954ad836a 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -438,9 +438,14 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id, engine->props.timeslice_duration_ms = CONFIG_DRM_I915_TIMESLICE_DURATION; - /* Override to uninterruptible for OpenCL workloads. */ + /* + * Mid-thread pre-emption is not available in Gen12. Unfortunately, + * some OpenCL workloads run quite long threads. That means they get + * reset due to not pre-empting in a timely manner. So, bump the + * pre-emption timeout value to be much higher for compute engines. + */ if (GRAPHICS_VER(i915) == 12 && (engine->flags & I915_ENGINE_HAS_RCS_REG_STATE)) - engine->props.preempt_timeout_ms = 0; + engine->props.preempt_timeout_ms = CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE; I wouldn't go as far as adding a config option since as it is it only applies to Gen12 but Kconfig text says nothing about that. And I am not saying you should add a Gen12 specific config option, that would be weird. So IMO just drop it. You were the one arguing that the driver was illegally overriding the user's explicitly chosen settings, including the compile time config This is a bit out of context and illegally don't think used, so misrepresents the earlier discussion. And I certainly did not suggest a kconfig option.
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? Regards, Christian. BR, -R
[PULL] drm-intel-fixes
Hi Dave, Daniel, One PSR2 fix for the next release candidate, if there will be one. Regards, Tvrtko drm-intel-fixes-2022-03-10: - Fix PSR2 when selective fetch is enabled and cursor at (-1, -1) (Jouni Högander) The following changes since commit ffb217a13a2eaf6d5bd974fc83036a53ca69f1e2: Linux 5.17-rc7 (2022-03-06 14:28:31 -0800) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2022-03-10 for you to fetch changes up to 804f468853179b9b58af05c153c411931aa5b310: drm/i915/psr: Set "SF Partial Frame Enable" also on full update (2022-03-07 14:45:31 +) - Fix PSR2 when selective fetch is enabled and cursor at (-1, -1) (Jouni Högander) Jouni Högander (1): drm/i915/psr: Set "SF Partial Frame Enable" also on full update drivers/gpu/drm/i915/display/intel_psr.c | 16 ++-- drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 15 insertions(+), 2 deletions(-)
Re: DSI Bridge switching
On Wed, Mar 09, 2022 at 06:45:10PM -0600, Adam Ford wrote: > On Wed, Mar 9, 2022 at 1:11 PM Jagan Teki wrote: > > > > or a Hi All, > > > > On Thu, Oct 14, 2021 at 6:45 PM Jagan Teki > > wrote: > > > > > > Hi Laurent, > > > > > > On Fri, Oct 8, 2021 at 7:07 PM Laurent Pinchart > > > wrote: > > > > > > > > Hello, > > > > > > > > On Fri, Oct 08, 2021 at 03:27:43PM +0200, Andrzej Hajda wrote: > > > > > Hi, > > > > > > > > > > Removed my invalid email (I will update files next week). > > > > > > > > > > On 08.10.2021 13:14, Jagan Teki wrote: > > > > > > Hi, > > > > > > > > > > > > I think this seems to be a known use case for industrial these days > > > > > > with i.mx8m. > > > > > > > > > > > > The host DSI would configure with two bridges one for DSI to LVDS > > > > > > (SN65DSI83) and another for DSI to HDMI Out (ADV7535). Technically > > > > > > we > > > > > > can use only one bridge at a time as host DSI support single out > > > > > > port. > > > > > > So we can have two separate device tree files for LVDS and HDMI and > > > > > > load them static. > > > > > > > > > > > > But, one of the use cases is to support both of them in single dts, > > > > > > and > > > > > > - Turn On LVDS (default) > > > > > > - Turn Off LVDS then Turn On HDMI when cable plug-in > > > > > > > > > > Are you sure it will work from hardware PoV? Do you have some demuxer? > > > > > isolation of pins? > > > > > > > > It may be in the category of "you shouldn't do this, but it actually > > > > works". I've seen the same being done with two CSI-2 camera sensors > > > > connected to the same receiver, with one of them being held in reset at > > > > all times. > > > > > > Yes. Here the design has 2 MIPI D-PHY switches. Each switch take 2 > > > input data lanes and 1 clock lane from SoC and produces 4 data lanes > > > and 2 clock lanes and from switch output 2 lanes and 1 clock are > > > inputting to HDMI bridge and other 2 lanes and 1 clock is inputting to > > > LVDS. So 1st pair of 1st switch and 1st pair of 2nd switch goes to > > > HDMI and 2nd pair of 1st switch and 2nd pair of 2nd switch does to > > > LVDS. > > > > > > However, routing of these lanes are controlled by SEL, OE GPIO pins. > > > So at a time we can access only single bridge. > > > > > > > > > > > > > The HDMI event can be detected via some HDMI-INT GPIO on-board > > > > > > design. > > > > > > > > > > > > The possible solution, I'm thinking of adding LVDS on port 1, HDMI > > > > > > on > > > > > > port 2 in the DSI host node, and trying to attach the respective > > > > > > bridge based on HDMI-INT like repeating the bridge attachment cycle > > > > > > based on the HDMI-INT. > > > > > > > > > > I think more appropriate would be to share the same port, but provide > > > > > two endpoints inside this port - we have two hardware sharing the same > > > > > physical port. > > > > > > > > That sounds like the correct DT description to me. > > > > > > > > > > Can it be possible to do bridge attachment at runtime? something > > > > > > like > > > > > > a bridge hotplug event? or any other possible solutions? > > > > > > > > > > > > Any suggestions? > > > > > > > > > > Practically it is possible, see exynos_dsi + panels, or exynos_dsi + > > > > > some toshiba bridge - panel and bridge are dynamically 'plugged' and > > > > > 'unplugged' from exynos_drm, but they do not use bridge chain for this > > > > > and some other reasons. (un|re|)plugging should be performed of course > > > > > when pipeline is off (connector disconnected). I am not sure about > > > > > bridges added to bridge chain - you need to inspect all opses to > > > > > ensure > > > > > it can be done safely. > > > > > > > > > > And the main issue: Daniel does not like it :) > > > > > > > > Neither do I :-) Could it be handled with two DRM connectors that are > > > > mutually exclusive ? > > > > > > How about adding lvds-connector, hdmi-connector on the pipeline and > > > select them based on the switch SEL GPIO? does it make sense to do > > > this implementation via display-connector.c > > > > I have somehow managed to make runtime switching possible between LVDS > > and HDMI with the help of DRM bridges. > > > > | => ADV7535=> > > HDMI-A Connector > > DSI Host => display-switch => | > > |=> SN65DSI83 => > > Panel-Simple > > > > display-switch here is a bridge driver that can switch or attach the > > downstream bridge flow based on HDMI HPD here. One potential problem > > is that when we switch from LVDS to HDMI Out the HDMI Out is displayed > > with the resolution that LVDS has and it is unable to display higher > > HDMI resolutions even though it supports it. Does anyone aware of > > changing the resolution at runtime, please let me know? > > > > Technically, the display-switch hardware does available in various forms > > 1. MIPI Switch PI3WVR626 > > 2. Conventional Mux Switch > > 3. Converter bridge
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Maxime, On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote: > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > On 3/8/22 11:07, Jagan Teki wrote: > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut wrote: > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > Hi, > > > > > > > > [...] > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs > > > > > > chipone_bridge_funcs = { > > > > > >static int chipone_parse_dt(struct chipone *icn) > > > > > >{ > > > > > > struct device *dev = icn->dev; > > > > > > + struct device_node *endpoint; > > > > > > struct drm_panel *panel; > > > > > > + int dsi_lanes; > > > > > > int ret; > > > > > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, "vdd1"); > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone > > > > > > *icn) > > > > > > return PTR_ERR(icn->enable_gpio); > > > > > > } > > > > > > > > > > > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, > > > > > > 0); > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, > > > > > > "data-lanes"); > > > > > > + icn->host_node = of_graph_get_remote_port_parent(endpoint); > > > > > > + of_node_put(endpoint); > > > > > > + > > > > > > + if (!icn->host_node) > > > > > > + return -ENODEV; > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner > > > > > Display pipeline in R16 [1]. > > > > > > > > > > We need to have a helper to return host_node for non-ports as I have > > > > > done it for drm_of_find_bridge. > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are > > > > required, see: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ? > > > > > > port@0 is optional as some of the DSI host OF-graph represent the > > > bridge or panel as child nodes instead of ports. (i think dt-binding > > > has to fix it to make port@0 optional) > > > > The current upstream DT binding document says: > > > > required: > > - port@0 > > - port@1 > > > > So port@0 is mandatory. > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is > correct. If the bridge supports DCS, there's no reason to use the OF > graph in the first place: the bridge node will be a child node of the > MIPI-DSI controller (and there's no obligation to use the OF-graph for a > MIPI-DSI controller). > > I believe port@0 should be made optional (or downright removed if > MIPI-DCS in the only control bus). I think we should make ports mandatory in all cases actually. The DT parent-child hierarchy is meant to model control relations between devices, so a DSI device controlled through DCS should be a child of the DSI controller. No disagreement there. The OF graph is meant to model data connections. While a DSI device controlled through DCS will use the same DSI link for data transfer, the two concepts are different. We have taken shortcuts and decided to not use OF graph for some DSI devices (not necessarily as a well thought decision, it was sometimes just not considered). This has led to different issues that we're having to deal with today, making it more difficult to develop generic code. Going forward, I think new bindings should always use OF graph to model the data connection. -- Regards, Laurent Pinchart
Re: [v4 2/5] drm/edid: parse multiple CEA extension block
On Wed, Mar 02, 2022 at 05:35:08PM +0800, Lee Shawn C wrote: > Try to find and parse more CEA ext blocks if edid->extensions > is greater than one. > > v2: split prvious patch to two. And do CEA block parsing > in this one. > v3: simplify this patch based on previous change. > > Cc: Jani Nikula > Cc: Ville Syrjala > Cc: Ankit Nautiyal > Signed-off-by: Lee Shawn C > --- > drivers/gpu/drm/drm_edid.c | 36 ++-- > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 375e70d9de86..c4a47465ba76 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4319,16 +4319,24 @@ static void drm_parse_y420cmdb_bitmap(struct > drm_connector *connector, > static int > add_cea_modes(struct drm_connector *connector, struct edid *edid) > { > - const u8 *cea, *db, *hdmi = NULL, *video = NULL; > - u8 dbl, hdmi_len, video_len = 0; > + const u8 *cea, *db; > + u8 dbl, hdmi_len; > int modes = 0, ext_index = 0; > + int i, start, end; I think everything here apart from modes and ext_index can be moved into the loop. Apart from that 1-2 look fine to me. intel-gfx wasn't cc:d however so we have no ci results for any of this. > > - cea = drm_find_cea_extension(edid, &ext_index); > - if (cea && cea_revision(cea) >= 3) { > - int i, start, end; > + for (;;) { > + const u8 *hdmi = NULL, *video = NULL; > + u8 video_len = 0; > + > + cea = drm_find_cea_extension(edid, &ext_index); > + if (!cea) > + break; > + > + if (cea_revision(cea) < 3) > + continue; > > if (cea_db_offsets(cea, &start, &end)) > - return 0; > + continue; > > for_each_cea_db(cea, i, start, end) { > db = &cea[i]; > @@ -4350,15 +4358,15 @@ add_cea_modes(struct drm_connector *connector, struct > edid *edid) > dbl - 1); > } > } > - } > > - /* > - * We parse the HDMI VSDB after having added the cea modes as we will > - * be patching their flags when the sink supports stereo 3D. > - */ > - if (hdmi) > - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, > - video_len); > + /* > + * We parse the HDMI VSDB after having added the cea modes as > we will > + * be patching their flags when the sink supports stereo 3D. > + */ > + if (hdmi) > + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, > video, > + video_len); > + } > > return modes; > } > -- > 2.17.1 -- Ville Syrjälä Intel
Re: [v4 5/5] drm/edid: check for HF-SCDB block
On Wed, Mar 02, 2022 at 05:35:11PM +0800, Lee Shawn C wrote: > Find HF-SCDB information in CEA extensions block. And retrieve > Max_TMDS_Character_Rate that support by sink device. > > Cc: Jani Nikula > Cc: Ville Syrjala > Cc: Ankit Nautiyal > Signed-off-by: Lee Shawn C > --- > drivers/gpu/drm/drm_edid.c | 36 > 1 file changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 2b8ddc956ce2..d6b48c543c23 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3350,6 +3350,7 @@ add_detailed_modes(struct drm_connector *connector, > struct edid *edid, > #define EXT_VIDEO_DATA_BLOCK_420 0x0E > #define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F > #define EXT_VIDEO_HF_EEODB_DATA_BLOCK0x78 > +#define EXT_VIDEO_HF_SCDB_DATA_BLOCK 0x79 > #define EDID_BASIC_AUDIO (1 << 6) > #define EDID_CEA_YCRCB444(1 << 5) > #define EDID_CEA_YCRCB422(1 << 4) > @@ -4277,6 +4278,20 @@ static bool cea_db_is_vcdb(const u8 *db) > return true; > } > > +static bool cea_db_is_hf_scdb(const u8 *db) > +{ > + if (cea_db_tag(db) != USE_EXTENDED_TAG) > + return false; > + > + if (cea_db_payload_len(db) < 7) > + return false; > + > + if (cea_db_extended_tag(db) != EXT_VIDEO_HF_SCDB_DATA_BLOCK) > + return false; > + > + return true; > +} > + > static bool cea_db_is_y420cmdb(const u8 *db) > { > if (cea_db_tag(db) != USE_EXTENDED_TAG) > @@ -4987,6 +5002,25 @@ static void drm_parse_vcdb(struct drm_connector > *connector, const u8 *db) > info->rgb_quant_range_selectable = true; > } > > +static void drm_parse_hf_scdb(struct drm_connector *connector, const u8 *db) > +{ > + struct drm_display_info *info = &connector->display_info; > + u32 max_tmds_clock; > + > + DRM_DEBUG_KMS("HF-SCDB version 0x%02x\n", db[4]); > + > + max_tmds_clock = db[5] * 5000; > + if (info->max_tmds_clock < max_tmds_clock) { > + info->max_tmds_clock = max_tmds_clock; > + DRM_DEBUG_KMS("HF-SCDB: max TMDS clock %d kHz\n", > + info->max_tmds_clock); > + } > + > + /* > + * ToDo: Parse the remaining SCDB data if needed > + */ If I'm reading the spec right this block should contain the exact same stuff as the HF-VSDB. We should reuse the same code for parsing both. > +} > + > static > void drm_get_max_frl_rate(int max_frl_rate, u8 *max_lanes, u8 > *max_rate_per_lane) > { > @@ -5282,6 +5316,8 @@ static void drm_parse_cea_ext(struct drm_connector > *connector, > drm_parse_y420cmdb_bitmap(connector, db); > if (cea_db_is_vcdb(db)) > drm_parse_vcdb(connector, db); > + if (cea_db_is_hf_scdb(db)) > + drm_parse_hf_scdb(connector, db); > if (cea_db_is_hdmi_hdr_metadata_block(db)) > drm_parse_hdr_metadata_block(connector, db); > } > -- > 2.17.1 -- Ville Syrjälä Intel
Re: [PATCH v14 03/22] dt-bindings: mediatek: add ethdr definition for mt8195
Il 10/03/22 04:54, Nancy.Lin ha scritto: Add vdosys1 ETHDR definition. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- .../display/mediatek/mediatek,ethdr.yaml | 158 ++ 1 file changed, 158 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
Re: [PATCH v14 08/22] soc: mediatek: change the mutex defines and the mutex_mod type
Il 10/03/22 04:55, Nancy.Lin ha scritto: This is a preparation for adding support for mt8195 vdosys1 mutex. The vdosys1 path component contains ovl_adaptor, merge5, and dp_intf1. Ovl_adaptor is composed of several sub-elements, so change it to support multi-bit control. Signed-off-by: Nancy.Lin Reviewed-by: AngeloGioacchino Del Regno --- drivers/soc/mediatek/mtk-mutex.c | 264 +++ 1 file changed, 132 insertions(+), 132 deletions(-)
Re: [PATCH v14 10/22] drm/mediatek: add display MDP RDMA support for MT8195
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add MDP_RDMA driver for MT8195. MDP_RDMA is the DMA engine of the ovl_adaptor component. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/Makefile | 3 +- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 7 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 315 drivers/gpu/drm/mediatek/mtk_mdp_rdma.h | 20 ++ 6 files changed, 346 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/mediatek/mtk_mdp_rdma.c create mode 100644 drivers/gpu/drm/mediatek/mtk_mdp_rdma.h
Re: [PATCH v14 11/22] drm/mediatek: add display merge advance config API for MT8195
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add merge new advance config API. The original merge API is mtk_ddp_comp_funcs function prototype. The API interface parameters cannot be modified, so add a new config API for extension. This is the preparation for ovl_adaptor merge control. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 ++ drivers/gpu/drm/mediatek/mtk_disp_merge.c | 52 --- 2 files changed, 48 insertions(+), 7 deletions(-)
Re: [PATCH v14 12/22] drm/mediatek: add display merge start/stop API for cmdq support
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add merge start/stop API for cmdq support. The ovl_adaptor merges are configured with each drm plane update. Need to enable/disable merge with cmdq making sure all the settings taken effect in the same vblank. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 2 ++ drivers/gpu/drm/mediatek/mtk_disp_merge.c | 20 +--- 2 files changed, 19 insertions(+), 3 deletions(-)
Re: [PATCH v14 13/22] drm/mediatek: add display merge mute/unmute support for MT8195
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add merge mute/unmute setting for MT8195. MT8195 Vdosys1 merge1~merge4 support HW mute function. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_disp_merge.c | 13 + 1 file changed, 13 insertions(+)
Re: [PATCH v14 14/22] drm/mediatek: add display merge async reset control
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add merge async reset control in mtk_merge_stop. Async hw doesn't do self reset on each sof signal(start of frame), so need to reset the async to clear the hw status for the next merge start. Signed-off-by: Nancy.Lin Reviewed-by: CK Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_disp_merge.c | 4 1 file changed, 4 insertions(+)
Re: [PATCH v14 15/22] drm/mediatek: add ETHDR support for MT8195
Il 10/03/22 04:55, Nancy.Lin ha scritto: ETHDR is a part of ovl_adaptor. ETHDR is designed for HDR video and graphics conversion in the external display path. It handles multiple HDR input types and performs tone mapping, color space/color format conversion, and then combine different layers, output the required HDR or SDR signal to the subsequent display path. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/Makefile | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + drivers/gpu/drm/mediatek/mtk_ethdr.c | 376 + drivers/gpu/drm/mediatek/mtk_ethdr.h | 23 ++ 5 files changed, 402 insertions(+) create mode 100644 drivers/gpu/drm/mediatek/mtk_ethdr.c create mode 100644 drivers/gpu/drm/mediatek/mtk_ethdr.h
Re: [PATCH v14 16/22] drm/mediatek: add mediatek-drm plane color encoding info
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add plane color encoding information for color space conversion. It's a preparation for adding support for mt8195 ovl_adaptor mdp_rdma csc control. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/mtk_drm_plane.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_plane.h | 1 + 2 files changed, 2 insertions(+)
Re: [PATCH v14 17/22] drm/mediatek: add ovl_adaptor support for MT8195
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add ovl_adaptor driver for MT8195. Ovl_adaptor is an encapsulated module and designed for simplified DRM control flow. This module is composed of 8 RDMAs, 4 MERGEs and an ETHDR. Two RDMAs merge into one layer, so this module support 4 layers. Signed-off-by: Nancy.Lin Reviewed-by: Chun-Kuang Hu Reviewed-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/mediatek/Makefile | 1 + drivers/gpu/drm/mediatek/mtk_disp_drv.h | 17 + .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 443 ++ drivers/gpu/drm/mediatek/mtk_drm_drv.h| 1 + 4 files changed, 462 insertions(+) create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
Re: [PATCH v14 22/22] arm64: dts: mt8195: add display node for vdosys1
Il 10/03/22 04:55, Nancy.Lin ha scritto: Add display node for vdosys1. Signed-off-by: Nancy.Lin --- arch/arm64/boot/dts/mediatek/mt8195.dtsi | 223 +++ 1 file changed, 223 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi index dbca699bba05..e650ec759235 100644 --- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi ..snip.. + + ethdr0: ethdr@1c114000 { + compatible = "mediatek,mt8195-disp-ethdr"; + reg = <0 0x1c114000 0 0x1000>, + <0 0x1c115000 0 0x1000>, + <0 0x1c117000 0 0x1000>, + <0 0x1c119000 0 0x1000>, + <0 0x1c11A000 0 0x1000>, + <0 0x1c11B000 0 0x1000>, + <0 0x1c11C000 0 0x1000>; Hello Nancy, looks like you partially forgot to use lower-case hex here... 0x1c11a000 0x1c11b000 0x1c11c000 + reg-names = "mixer", "vdo_fe0", "vdo_fe1", "gfx_fe0", "gfx_fe1", + "vdo_be", "adl_ds"; + mediatek,gce-client-reg = <&gce0 SUBSYS_1c11 0x4000 0x1000>, + <&gce0 SUBSYS_1c11 0x5000 0x1000>, + <&gce0 SUBSYS_1c11 0x7000 0x1000>, + <&gce0 SUBSYS_1c11 0x9000 0x1000>, + <&gce0 SUBSYS_1c11 0xA000 0x1000>, + <&gce0 SUBSYS_1c11 0xB000 0x1000>, + <&gce0 SUBSYS_1c11 0xC000 0x1000>; ...and here too: 0xa000 0xb000 0xc000 Please fix that, after which, you can add my Reviewed-by: AngeloGioacchino Del Regno + clocks = <&vdosys1 CLK_VDO1_DISP_MIXER>, +<&vdosys1 CLK_VDO1_HDR_VDO_FE0>, +<&vdosys1 CLK_VDO1_HDR_VDO_FE1>, +<&vdosys1 CLK_VDO1_HDR_GFX_FE0>, +<&vdosys1 CLK_VDO1_HDR_GFX_FE1>, +<&vdosys1 CLK_VDO1_HDR_VDO_BE>, +<&vdosys1 CLK_VDO1_26M_SLOW>, +<&vdosys1 CLK_VDO1_HDR_VDO_FE0_DL_ASYNC>, +<&vdosys1 CLK_VDO1_HDR_VDO_FE1_DL_ASYNC>, +<&vdosys1 CLK_VDO1_HDR_GFX_FE0_DL_ASYNC>, +<&vdosys1 CLK_VDO1_HDR_GFX_FE1_DL_ASYNC>, +<&vdosys1 CLK_VDO1_HDR_VDO_BE_DL_ASYNC>, +<&topckgen CLK_TOP_ETHDR>; + clock-names = "mixer", "vdo_fe0", "vdo_fe1", "gfx_fe0", "gfx_fe1", + "vdo_be", "adl_ds", "vdo_fe0_async", "vdo_fe1_async", + "gfx_fe0_async", "gfx_fe1_async","vdo_be_async", + "ethdr_top"; + power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>; + iommus = <&iommu_vpp M4U_PORT_L3_HDR_DS>, +<&iommu_vpp M4U_PORT_L3_HDR_ADL>; + interrupts = ; /* disp mixer */ + resets = <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_FE0_DL_ASYNC>, +<&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_FE1_DL_ASYNC>, +<&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_GFX_FE0_DL_ASYNC>, +<&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_GFX_FE1_DL_ASYNC>, +<&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_BE_DL_ASYNC>; + }; + }; };
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote: > On 3/8/22 17:21, Maxime Ripard wrote: > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote: > > > On 3/8/22 14:49, Maxime Ripard wrote: > > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote: > > > > > On 3/8/22 13:51, Maxime Ripard wrote: > > > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > > > > > > On 3/8/22 11:07, Jagan Teki wrote: > > > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs > > > > > > > > > > > chipone_bridge_funcs = { > > > > > > > > > > > static int chipone_parse_dt(struct chipone *icn) > > > > > > > > > > > { > > > > > > > > > > > struct device *dev = icn->dev; > > > > > > > > > > > + struct device_node *endpoint; > > > > > > > > > > > struct drm_panel *panel; > > > > > > > > > > > + int dsi_lanes; > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, > > > > > > > > > > > "vdd1"); > > > > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct > > > > > > > > > > > chipone *icn) > > > > > > > > > > > return PTR_ERR(icn->enable_gpio); > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > + endpoint = > > > > > > > > > > > of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > > > > > > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, > > > > > > > > > > > "data-lanes"); > > > > > > > > > > > + icn->host_node = > > > > > > > > > > > of_graph_get_remote_port_parent(endpoint); > > > > > > > > > > > + of_node_put(endpoint); > > > > > > > > > > > + > > > > > > > > > > > + if (!icn->host_node) > > > > > > > > > > > + return -ENODEV; > > > > > > > > > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the > > > > > > > > > > Allwinner > > > > > > > > > > Display pipeline in R16 [1]. > > > > > > > > > > > > > > > > > > > > We need to have a helper to return host_node for non-ports > > > > > > > > > > as I have > > > > > > > > > > done it for drm_of_find_bridge. > > > > > > > > > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that > > > > > > > > > patch is > > > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and > > > > > > > > > port@1 are > > > > > > > > > required, see: > > > > > > > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > > > > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that > > > > > > > > > ? > > > > > > > > > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent > > > > > > > > the > > > > > > > > bridge or panel as child nodes instead of ports. (i think > > > > > > > > dt-binding > > > > > > > > has to fix it to make port@0 optional) > > > > > > > > > > > > > > The current upstream DT binding document says: > > > > > > > > > > > > > >required: > > > > > > > - port@0 > > > > > > > - port@1 > > > > > > > > > > > > > > So port@0 is mandatory. > > > > > > > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan > > > > > > provided is > > > > > > correct. If the bridge supports DCS, there's no reason to use the OF > > > > > > graph in the first place: the bridge node will be a child node of > > > > > > the > > > > > > MIPI-DSI controller (and there's no obligation to use the OF-graph > > > > > > for a > > > > > > MIPI-DSI controller). > > > > > > > > > > > > I believe port@0 should be made optional (or downright removed if > > > > > > MIPI-DCS in the only control bus). > > > > > > > > > > That's out of scope of this series anyway, so Jagan can implement > > > > > patches > > > > > for that mode if needed. > > > > > > > > Not really? You can't count on the port@0 being there generally > > > > speaking, so you can't count on data-lanes being there either, which > > > > exactly what you're doing in this patch. > > > > > > I can because the upstream DT bindings currently say port@0 must be > > > present, > > > see above. If that requirement should be relaxed, sure, but that's a > > > separate series. > > > > And another upstream DT bindings say that you don't need them at all. > > Which "another upstream DT bindings" do you refer to ? https://git.
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > Hi Maxime, > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote: > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > > On 3/8/22 11:07, Jagan Teki wrote: > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut wrote: > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > > > Hi, > > > > > > > > > > [...] > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs > > > > > > > chipone_bridge_funcs = { > > > > > > >static int chipone_parse_dt(struct chipone *icn) > > > > > > >{ > > > > > > > struct device *dev = icn->dev; > > > > > > > + struct device_node *endpoint; > > > > > > > struct drm_panel *panel; > > > > > > > + int dsi_lanes; > > > > > > > int ret; > > > > > > > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, "vdd1"); > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone > > > > > > > *icn) > > > > > > > return PTR_ERR(icn->enable_gpio); > > > > > > > } > > > > > > > > > > > > > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, > > > > > > > 0); > > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, > > > > > > > "data-lanes"); > > > > > > > + icn->host_node = > > > > > > > of_graph_get_remote_port_parent(endpoint); > > > > > > > + of_node_put(endpoint); > > > > > > > + > > > > > > > + if (!icn->host_node) > > > > > > > + return -ENODEV; > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the Allwinner > > > > > > Display pipeline in R16 [1]. > > > > > > > > > > > > We need to have a helper to return host_node for non-ports as I have > > > > > > done it for drm_of_find_bridge. > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch is > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are > > > > > required, see: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ? > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the > > > > bridge or panel as child nodes instead of ports. (i think dt-binding > > > > has to fix it to make port@0 optional) > > > > > > The current upstream DT binding document says: > > > > > > required: > > > - port@0 > > > - port@1 > > > > > > So port@0 is mandatory. > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is > > correct. If the bridge supports DCS, there's no reason to use the OF > > graph in the first place: the bridge node will be a child node of the > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a > > MIPI-DSI controller). > > > > I believe port@0 should be made optional (or downright removed if > > MIPI-DCS in the only control bus). > > I think we should make ports mandatory in all cases actually. > > The DT parent-child hierarchy is meant to model control relations > between devices, so a DSI device controlled through DCS should be a > child of the DSI controller. No disagreement there. > > The OF graph is meant to model data connections. While a DSI device > controlled through DCS will use the same DSI link for data transfer, the > two concepts are different. We have taken shortcuts and decided to not > use OF graph for some DSI devices (not necessarily as a well thought > decision, it was sometimes just not considered). I disagree. Unless the data path is explicitly stated using the OF-graph or some other binding, it's inferred. We never asked ourselves where the data from an i2c chip, an ethernet controller or an v4l2 output device was coming from. It comes from the parent bus, because it's what makes sense. Making a requirement on the OF-Graph to model this would create a big inconsistency. > This has led to different issues that we're having to deal with today, > making it more difficult to develop generic code. Going forward, I > think new bindings should always use OF graph to model the data > connection. Either way, that discussion is irrelevant. Not all DSI controllers use OF-Graph, a bridge can be attached to any of them, so we can't require OF-Graph support in any bridge. Maxime signature.asc Description: PGP signature
Re: DSI Bridge switching
On Thu, Mar 10, 2022 at 6:15 AM Adam Ford wrote: > > On Wed, Mar 9, 2022 at 1:11 PM Jagan Teki wrote: > > > > or a Hi All, > > > > On Thu, Oct 14, 2021 at 6:45 PM Jagan Teki > > wrote: > > > > > > Hi Laurent, > > > > > > On Fri, Oct 8, 2021 at 7:07 PM Laurent Pinchart > > > wrote: > > > > > > > > Hello, > > > > > > > > On Fri, Oct 08, 2021 at 03:27:43PM +0200, Andrzej Hajda wrote: > > > > > Hi, > > > > > > > > > > Removed my invalid email (I will update files next week). > > > > > > > > > > On 08.10.2021 13:14, Jagan Teki wrote: > > > > > > Hi, > > > > > > > > > > > > I think this seems to be a known use case for industrial these days > > > > > > with i.mx8m. > > > > > > > > > > > > The host DSI would configure with two bridges one for DSI to LVDS > > > > > > (SN65DSI83) and another for DSI to HDMI Out (ADV7535). Technically > > > > > > we > > > > > > can use only one bridge at a time as host DSI support single out > > > > > > port. > > > > > > So we can have two separate device tree files for LVDS and HDMI and > > > > > > load them static. > > > > > > > > > > > > But, one of the use cases is to support both of them in single dts, > > > > > > and > > > > > > - Turn On LVDS (default) > > > > > > - Turn Off LVDS then Turn On HDMI when cable plug-in > > > > > > > > > > Are you sure it will work from hardware PoV? Do you have some demuxer? > > > > > isolation of pins? > > > > > > > > It may be in the category of "you shouldn't do this, but it actually > > > > works". I've seen the same being done with two CSI-2 camera sensors > > > > connected to the same receiver, with one of them being held in reset at > > > > all times. > > > > > > Yes. Here the design has 2 MIPI D-PHY switches. Each switch take 2 > > > input data lanes and 1 clock lane from SoC and produces 4 data lanes > > > and 2 clock lanes and from switch output 2 lanes and 1 clock are > > > inputting to HDMI bridge and other 2 lanes and 1 clock is inputting to > > > LVDS. So 1st pair of 1st switch and 1st pair of 2nd switch goes to > > > HDMI and 2nd pair of 1st switch and 2nd pair of 2nd switch does to > > > LVDS. > > > > > > However, routing of these lanes are controlled by SEL, OE GPIO pins. > > > So at a time we can access only single bridge. > > > > > > > > > > > > > The HDMI event can be detected via some HDMI-INT GPIO on-board > > > > > > design. > > > > > > > > > > > > The possible solution, I'm thinking of adding LVDS on port 1, HDMI > > > > > > on > > > > > > port 2 in the DSI host node, and trying to attach the respective > > > > > > bridge based on HDMI-INT like repeating the bridge attachment cycle > > > > > > based on the HDMI-INT. > > > > > > > > > > I think more appropriate would be to share the same port, but provide > > > > > two endpoints inside this port - we have two hardware sharing the same > > > > > physical port. > > > > > > > > That sounds like the correct DT description to me. > > > > > > > > > > Can it be possible to do bridge attachment at runtime? something > > > > > > like > > > > > > a bridge hotplug event? or any other possible solutions? > > > > > > > > > > > > Any suggestions? > > > > > > > > > > Practically it is possible, see exynos_dsi + panels, or exynos_dsi + > > > > > some toshiba bridge - panel and bridge are dynamically 'plugged' and > > > > > 'unplugged' from exynos_drm, but they do not use bridge chain for this > > > > > and some other reasons. (un|re|)plugging should be performed of course > > > > > when pipeline is off (connector disconnected). I am not sure about > > > > > bridges added to bridge chain - you need to inspect all opses to > > > > > ensure > > > > > it can be done safely. > > > > > > > > > > And the main issue: Daniel does not like it :) > > > > > > > > Neither do I :-) Could it be handled with two DRM connectors that are > > > > mutually exclusive ? > > > > > > How about adding lvds-connector, hdmi-connector on the pipeline and > > > select them based on the switch SEL GPIO? does it make sense to do > > > this implementation via display-connector.c > > > > I have somehow managed to make runtime switching possible between LVDS > > and HDMI with the help of DRM bridges. > > > > | => ADV7535=> > > HDMI-A Connector > > DSI Host => display-switch => | > > |=> SN65DSI83 => > > Panel-Simple > > > > display-switch here is a bridge driver that can switch or attach the > > downstream bridge flow based on HDMI HPD here. One potential problem > > is that when we switch from LVDS to HDMI Out the HDMI Out is displayed > > with the resolution that LVDS has and it is unable to display higher > > HDMI resolutions even though it supports it. Does anyone aware of > > changing the resolution at runtime, please let me know? > > > > Technically, the display-switch hardware does available in various forms > > 1. MIPI Switch PI3WVR626 > > 2. Conventional Mux Switch > > 3. Converter bridge DSI to
Re: [PATCH] drm/vc4: add tracepoints for CL submissions
On Tue, Mar 01, 2022 at 01:58:26PM -0100, Melissa Wen wrote: > On 02/25, Maxime Ripard wrote: > > Hi Melissa, > > > > On Tue, Feb 01, 2022 at 08:26:51PM -0100, Melissa Wen wrote: > > > Trace submit_cl_ioctl and related IRQs for CL submission and bin/render > > > jobs execution. It might be helpful to get a rendering timeline and > > > track job throttling. > > > > > > Signed-off-by: Melissa Wen > > > > I'm not really sure what to do about this patch to be honest. > > > > My understanding is that tracepoints are considered as userspace ABI, > > but I can't really judge whether your traces are good enough or if it's > > something that will bit us some time down the road. > > Thanks for taking a look at this patch. > > So, I followed the same path of tracing CL submissions on v3d. I mean, > tracking submit_cl ioctl, points when a job (bin/render) starts it > execution, and irqs of completion (bin/render job). We used it to > examine job throttling when running Chromium and, therefore, in addition > to have the timeline of jobs execution, I show some data submitted in > the ioctl to make connections. I think these tracers might be useful for > some investigation in the future, but I'm also not sure if all data are > necessary to be displayed. Yeah, I'm sure that it's useful :) I don't see anything wrong with that patch, really. What I meant is that I don't really have the experience to judge if there's anything wrong in the first place :) If you can get someone with more experience with the v3d driver (Emma, Iago maybe?) I'll be definitely be ok merging that patch Maxime signature.asc Description: PGP signature
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
Hi Maxime, On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote: > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote: > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > > > On 3/8/22 11:07, Jagan Teki wrote: > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut wrote: > > > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > [...] > > > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs > > > > > > > > chipone_bridge_funcs = { > > > > > > > >static int chipone_parse_dt(struct chipone *icn) > > > > > > > >{ > > > > > > > > struct device *dev = icn->dev; > > > > > > > > + struct device_node *endpoint; > > > > > > > > struct drm_panel *panel; > > > > > > > > + int dsi_lanes; > > > > > > > > int ret; > > > > > > > > > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, "vdd1"); > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct > > > > > > > > chipone *icn) > > > > > > > > return PTR_ERR(icn->enable_gpio); > > > > > > > > } > > > > > > > > > > > > > > > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, > > > > > > > > 0, 0); > > > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, > > > > > > > > "data-lanes"); > > > > > > > > + icn->host_node = > > > > > > > > of_graph_get_remote_port_parent(endpoint); > > > > > > > > + of_node_put(endpoint); > > > > > > > > + > > > > > > > > + if (!icn->host_node) > > > > > > > > + return -ENODEV; > > > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the > > > > > > > Allwinner > > > > > > > Display pipeline in R16 [1]. > > > > > > > > > > > > > > We need to have a helper to return host_node for non-ports as I > > > > > > > have > > > > > > > done it for drm_of_find_bridge. > > > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that patch > > > > > > is > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and port@1 > > > > > > are > > > > > > required, see: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ? > > > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding > > > > > has to fix it to make port@0 optional) > > > > > > > > The current upstream DT binding document says: > > > > > > > > required: > > > > - port@0 > > > > - port@1 > > > > > > > > So port@0 is mandatory. > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is > > > correct. If the bridge supports DCS, there's no reason to use the OF > > > graph in the first place: the bridge node will be a child node of the > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a > > > MIPI-DSI controller). > > > > > > I believe port@0 should be made optional (or downright removed if > > > MIPI-DCS in the only control bus). > > > > I think we should make ports mandatory in all cases actually. > > > > The DT parent-child hierarchy is meant to model control relations > > between devices, so a DSI device controlled through DCS should be a > > child of the DSI controller. No disagreement there. > > > > The OF graph is meant to model data connections. While a DSI device > > controlled through DCS will use the same DSI link for data transfer, the > > two concepts are different. We have taken shortcuts and decided to not > > use OF graph for some DSI devices (not necessarily as a well thought > > decision, it was sometimes just not considered). > > I disagree. Unless the data path is explicitly stated using the OF-graph > or some other binding, it's inferred. It is today, and for video data, I think it's showing to be a problem :-) > We never asked ourselves where the > data from an i2c chip, an ethernet controller or an v4l2 output device > was coming from. It comes from the parent bus, because it's what makes > sense. Making a requirement on the OF-Graph to model this would create a > big inconsistency. I'm afraid I disagree, especially when it comes to data transfers from device to device. The device tree has never tried to model those until OF graph. > > This has led to different issues that we're having to deal with today, > > making it more difficult to develop generic code. Going forward, I > > think new bindings should always use OF graph
[Bug 215669] [drm:gfx_v10_0_priv_reg_irq [amdgpu]] *ERROR* Illegal register access in command stream
https://bugzilla.kernel.org/show_bug.cgi?id=215669 --- Comment #2 from Andreas Polnas (andreas.polna...@hotmail.com) --- Thanks Alex, I have posted it on the mesa gitlab as an issue shown below: https://gitlab.freedesktop.org/mesa/mesa/-/issues/6113 -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
Hi Doug, Quoting Doug Anderson (2022-03-07 19:52:08) > Hi, > > On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham > wrote: > > > > From: Laurent Pinchart > > > > Implement the bridge connector-related .get_edid() operation, and report > > the related bridge capabilities and type. > > > > Signed-off-by: Laurent Pinchart > > Reviewed-by: Stephen Boyd > > Reviewed-by: Douglas Anderson > > Signed-off-by: Kieran Bingham > > --- > > Changes since v1: > > > > - The connector .get_modes() operation doesn't rely on EDID anymore, > > __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged > > together > > > > Notes from Kieran: > > > > RB Tags collected from: > > > > https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+rene...@ideasonboard.com/ > > > > However this was over a year ago, so let me know if other patches now > > superceed this one or otherwise invalidate this update. > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index c55848588123..ffb6c04f6c46 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct > > drm_bridge *bridge) > > pm_runtime_put_sync(pdata->dev); > > } > > > > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > > + struct drm_connector *connector) > > +{ > > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > + struct edid *edid; > > + > > + pm_runtime_get_sync(pdata->dev); > > + edid = drm_get_edid(connector, &pdata->aux.ddc); > > + pm_runtime_put_autosuspend(pdata->dev); > > + > > + return edid; > > +} > > + > > static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > > .attach = ti_sn_bridge_attach, > > .detach = ti_sn_bridge_detach, > > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs > > ti_sn_bridge_funcs = { > > .enable = ti_sn_bridge_enable, > > .disable = ti_sn_bridge_disable, > > .post_disable = ti_sn_bridge_post_disable, > > + .get_edid = ti_sn_bridge_get_edid, > > }; > > > > static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, > > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device > > *adev, > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > pdata->bridge.of_node = np; > > + pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > + pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; > > This doesn't look right to me. In the eDP case the EDID reading is > driven by the panel. Now that I have the optional connector working based on Sam's series I think this is the last issue to solve before reposting the DP/HPD support. Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort? -- Regards Kieran > > -Doug
Re: [PATCH] drm/vc4: add tracepoints for CL submissions
El 10/3/22 a las 12:12, Maxime Ripard escribió: On Tue, Mar 01, 2022 at 01:58:26PM -0100, Melissa Wen wrote: On 02/25, Maxime Ripard wrote: Hi Melissa, On Tue, Feb 01, 2022 at 08:26:51PM -0100, Melissa Wen wrote: Trace submit_cl_ioctl and related IRQs for CL submission and bin/render jobs execution. It might be helpful to get a rendering timeline and track job throttling. Signed-off-by: Melissa Wen I'm not really sure what to do about this patch to be honest. My understanding is that tracepoints are considered as userspace ABI, but I can't really judge whether your traces are good enough or if it's something that will bit us some time down the road. Thanks for taking a look at this patch. So, I followed the same path of tracing CL submissions on v3d. I mean, tracking submit_cl ioctl, points when a job (bin/render) starts it execution, and irqs of completion (bin/render job). We used it to examine job throttling when running Chromium and, therefore, in addition to have the timeline of jobs execution, I show some data submitted in the ioctl to make connections. I think these tracers might be useful for some investigation in the future, but I'm also not sure if all data are necessary to be displayed. Yeah, I'm sure that it's useful :) I don't see anything wrong with that patch, really. What I meant is that I don't really have the experience to judge if there's anything wrong in the first place :) If you can get someone with more experience with the v3d driver (Emma, Iago maybe?) I'll be definitely be ok merging that patch I've checked this patch and I've been using these tracepoints. They have been working properly. Reviewed-by: Jose Maria Casanova Crespo Regards, Chema Casanova
Re: [Intel-gfx] [PATCH] drm/i915/uapi: Add DRM_I915_QUERY_GEOMETRY_SUBSLICES
On 10/03/2022 05:18, Matt Atwood wrote: Newer platforms have DSS that aren't necessarily available for both geometry and compute, two queries will need to exist. This introduces the first, when passing a valid engine class and engine instance in the flags returns a topology describing geometry. v2: fix white space errors Cc: Ashutosh Dixit Cc: Matt Roper Cc: Joonas Lahtinen UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14143 Signed-off-by: Matt Atwood --- drivers/gpu/drm/i915/i915_query.c | 68 ++- include/uapi/drm/i915_drm.h | 24 +++ 2 files changed, 65 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 2dfbc22857a3..e4f35da28642 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -9,6 +9,7 @@ #include "i915_drv.h" #include "i915_perf.h" #include "i915_query.h" +#include "gt/intel_engine_user.h" #include static int copy_query_item(void *query_hdr, size_t query_sz, @@ -28,36 +29,30 @@ static int copy_query_item(void *query_hdr, size_t query_sz, return 0; } -static int query_topology_info(struct drm_i915_private *dev_priv, - struct drm_i915_query_item *query_item) +static int fill_topology_info(const struct sseu_dev_info *sseu, + struct drm_i915_query_item *query_item, + const u8 *subslice_mask) { - const struct sseu_dev_info *sseu = &to_gt(dev_priv)->info.sseu; struct drm_i915_query_topology_info topo; u32 slice_length, subslice_length, eu_length, total_length; int ret; - if (query_item->flags != 0) - return -EINVAL; + BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask)); if (sseu->max_slices == 0) return -ENODEV; - BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask)); - slice_length = sizeof(sseu->slice_mask); subslice_length = sseu->max_slices * sseu->ss_stride; eu_length = sseu->max_slices * sseu->max_subslices * sseu->eu_stride; total_length = sizeof(topo) + slice_length + subslice_length + eu_length; - ret = copy_query_item(&topo, sizeof(topo), total_length, - query_item); + ret = copy_query_item(&topo, sizeof(topo), total_length, query_item); + if (ret != 0) return ret; - if (topo.flags != 0) - return -EINVAL; - memset(&topo, 0, sizeof(topo)); topo.max_slices = sseu->max_slices; topo.max_subslices = sseu->max_subslices; @@ -69,27 +64,61 @@ static int query_topology_info(struct drm_i915_private *dev_priv, topo.eu_stride = sseu->eu_stride; if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), - &topo, sizeof(topo))) +&topo, sizeof(topo))) return -EFAULT; if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)), - &sseu->slice_mask, slice_length)) +&sseu->slice_mask, slice_length)) return -EFAULT; if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + - sizeof(topo) + slice_length), - sseu->subslice_mask, subslice_length)) +sizeof(topo) + slice_length), +subslice_mask, subslice_length)) return -EFAULT; if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + - sizeof(topo) + - slice_length + subslice_length), - sseu->eu_mask, eu_length)) +sizeof(topo) + +slice_length + subslice_length), +sseu->eu_mask, eu_length)) return -EFAULT; return total_length; } +static int query_topology_info(struct drm_i915_private *dev_priv, + struct drm_i915_query_item *query_item) +{ + const struct sseu_dev_info *sseu = &to_gt(dev_priv)->info.sseu; + + if (query_item->flags != 0) + return -EINVAL; + + return fill_topology_info(sseu, query_item, sseu->subslice_mask); +} + +static int query_geometry_subslices(struct drm_i915_private *i915, + struct drm_i915_query_item *query_item) +{ + const struct sseu_dev_info *sseu; + struct intel_engine_cs *engine; + u8 engine_class, engine_instance; + + if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) + return -ENODEV; + + engine_class = query_item->flags & 0xFF; + engine_instance = (query_item->flags >> 8) & 0xFF; + + engine = intel_engine_lookup_use
[PATCH v2 0/8] Some more bits for small BAR enabling
The leftover bits around dealing with stolen-local memory + small BAR, plus some related fixes. v2: some tweaks based on feedback from Ville -- 2.34.1
[PATCH v2 2/8] drm/i915/stolen: don't treat small BAR as an error
From: Akeem G Abodunrin On client platforms with reduced LMEM BAR, we should be able to continue with driver load with reduced io_size. Instead of using the BAR size to determine the how large stolen should be, we should instead use the ADDR_RANGE register to figure this out(at least on platforms like DG2). For simplicity we don't attempt to support partially mappable stolen. Signed-off-by: Akeem G Abodunrin Co-developed-by: Matthew Auld Signed-off-by: Matthew Auld Cc: Thomas Hellström --- drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 48 -- drivers/gpu/drm/i915/i915_reg.h| 3 ++ 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index 0bf8f61134af..6df1600708a7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -12,6 +12,8 @@ #include "gem/i915_gem_lmem.h" #include "gem/i915_gem_region.h" +#include "gt/intel_gt.h" +#include "gt/intel_region_lmem.h" #include "i915_drv.h" #include "i915_gem_stolen.h" #include "i915_reg.h" @@ -750,9 +752,9 @@ static int init_stolen_lmem(struct intel_memory_region *mem) if (GEM_WARN_ON(resource_size(&mem->region) == 0)) return -ENODEV; - if (!io_mapping_init_wc(&mem->iomap, - mem->io_start, - mem->io_size)) + if (mem->io_size && !io_mapping_init_wc(&mem->iomap, + mem->io_start, + mem->io_size)) return -EIO; /* @@ -773,7 +775,8 @@ static int init_stolen_lmem(struct intel_memory_region *mem) static int release_stolen_lmem(struct intel_memory_region *mem) { - io_mapping_fini(&mem->iomap); + if (mem->io_size) + io_mapping_fini(&mem->iomap); i915_gem_cleanup_stolen(mem->i915); return 0; } @@ -790,25 +793,43 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type, { struct intel_uncore *uncore = &i915->uncore; struct pci_dev *pdev = to_pci_dev(i915->drm.dev); + resource_size_t dsm_size, dsm_base, lmem_size; struct intel_memory_region *mem; + resource_size_t io_start, io_size; resource_size_t min_page_size; - resource_size_t io_start; - resource_size_t lmem_size; - u64 lmem_base; - lmem_base = intel_uncore_read64(uncore, GEN12_DSMBASE); - if (GEM_WARN_ON(lmem_base >= pci_resource_len(pdev, 2))) + if (WARN_ON_ONCE(instance)) return ERR_PTR(-ENODEV); - lmem_size = pci_resource_len(pdev, 2) - lmem_base; - io_start = pci_resource_start(pdev, 2) + lmem_base; + /* Use DSM base address instead for stolen memory */ + dsm_base = intel_uncore_read64(uncore, GEN12_DSMBASE); + if (IS_DG1(uncore->i915)) { + lmem_size = pci_resource_len(pdev, 2); + if (WARN_ON(lmem_size < dsm_base)) + return ERR_PTR(-ENODEV); + } else { + resource_size_t lmem_range; + + lmem_range = intel_gt_read_register(&i915->gt0, XEHPSDV_TILE0_ADDR_RANGE) & 0x; + lmem_size = lmem_range >> XEHPSDV_TILE_LMEM_RANGE_SHIFT; + lmem_size *= SZ_1G; + } + + dsm_size = lmem_size - dsm_base; + if (pci_resource_len(pdev, 2) < lmem_size) { + io_start = 0; + io_size = 0; + } else { + io_start = pci_resource_start(pdev, 2) + dsm_base; + io_size = dsm_size; + } min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : I915_GTT_PAGE_SIZE_4K; - mem = intel_memory_region_create(i915, lmem_base, lmem_size, + mem = intel_memory_region_create(i915, dsm_base, dsm_size, min_page_size, -io_start, lmem_size, +io_start, io_size, type, instance, &i915_region_stolen_lmem_ops); if (IS_ERR(mem)) @@ -822,6 +843,7 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type, drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n", &mem->io_start); + drm_dbg(&i915->drm, "Stolen Local DSM base: %pa\n", &dsm_base); intel_memory_region_set_name(mem, "stolen-local"); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 25ecddc706af..0183823b4c55 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8466,6 +8466,9 @@ enum skl_power_gate { #define SGGI_DIS REG_BIT(15) #define SGR_DIS REG_BIT(13) +#define XEHPSDV_
[PATCH v2 1/8] drm/i915/lmem: don't treat small BAR as an error
Just pass along the probed io_size. The backend should be able to utilize the entire range here, even if some of it is non-mappable. It does leave open with what to do with stolen local-memory. Signed-off-by: Matthew Auld Cc: Thomas Hellström Reviewed-by: Thomas Hellström --- drivers/gpu/drm/i915/gt/intel_region_lmem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c index 6cecfdae07ad..783d81072c3b 100644 --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c @@ -93,6 +93,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) struct intel_memory_region *mem; resource_size_t min_page_size; resource_size_t io_start; + resource_size_t io_size; resource_size_t lmem_size; int err; @@ -124,7 +125,8 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) io_start = pci_resource_start(pdev, 2); - if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2))) + io_size = min(pci_resource_len(pdev, 2), lmem_size); + if (!io_size) return ERR_PTR(-ENODEV); min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : @@ -134,7 +136,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt) lmem_size, min_page_size, io_start, -lmem_size, +io_size, INTEL_MEMORY_LOCAL, 0, &intel_region_lmem_ops); -- 2.34.1
[PATCH v2 3/8] drm/i915/stolen: consider I915_BO_ALLOC_GPU_ONLY
Keep the behaviour consistent with normal lmem, where we assume CPU access if by default required. Signed-off-by: Matthew Auld Cc: Thomas Hellström --- drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index 6df1600708a7..369a2a60bd7a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -695,6 +695,14 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem, if (size == 0) return -EINVAL; + /* +* With discrete devices, where we lack a mappable aperture there is no +* possible way to ever access this memory on the CPU side. +*/ + if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size && + !(flags & I915_BO_ALLOC_GPU_ONLY)) + return -ENOSPC; + stolen = kzalloc(sizeof(*stolen), GFP_KERNEL); if (!stolen) return -ENOMEM; -- 2.34.1
[PATCH v2 5/8] drm/i915/ttm: wire up the object offset
For the ttm backend we can use existing placements fpfn and lpfn to force the allocator to place the object at the requested offset, potentially evicting stuff if the spot is currently occupied. Signed-off-by: Matthew Auld Cc: Thomas Hellström --- .../gpu/drm/i915/gem/i915_gem_object_types.h | 2 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c| 18 ++ drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 3 ++- drivers/gpu/drm/i915/intel_region_ttm.c| 7 ++- drivers/gpu/drm/i915/intel_region_ttm.h| 1 + drivers/gpu/drm/i915/selftests/mock_region.c | 3 +++ 6 files changed, 28 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index fd54eb8f4826..2c88bdb8ff7c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -631,6 +631,8 @@ struct drm_i915_gem_object { struct drm_mm_node *stolen; + resource_size_t bo_offset; + unsigned long scratch; u64 encode; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 5e543ed867a2..e4a06fcf741a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -126,6 +126,8 @@ i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj) static void i915_ttm_place_from_region(const struct intel_memory_region *mr, struct ttm_place *place, + resource_size_t offset, + resource_size_t size, unsigned int flags) { memset(place, 0, sizeof(*place)); @@ -133,7 +135,10 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr, if (flags & I915_BO_ALLOC_CONTIGUOUS) place->flags |= TTM_PL_FLAG_CONTIGUOUS; - if (mr->io_size && mr->io_size < mr->total) { + if (offset != I915_BO_INVALID_OFFSET) { + place->fpfn = offset >> PAGE_SHIFT; + place->lpfn = place->fpfn + (size >> PAGE_SHIFT); + } else if (mr->io_size && mr->io_size < mr->total) { if (flags & I915_BO_ALLOC_GPU_ONLY) { place->flags |= TTM_PL_FLAG_TOPDOWN; } else { @@ -155,12 +160,14 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj, placement->num_placement = 1; i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] : - obj->mm.region, requested, flags); + obj->mm.region, requested, obj->bo_offset, + obj->base.size, flags); /* Cache this on object? */ placement->num_busy_placement = num_allowed; for (i = 0; i < placement->num_busy_placement; ++i) - i915_ttm_place_from_region(obj->mm.placements[i], busy + i, flags); + i915_ttm_place_from_region(obj->mm.placements[i], busy + i, + obj->bo_offset, obj->base.size, flags); if (num_allowed == 0) { *busy = *requested; @@ -802,7 +809,8 @@ static int __i915_ttm_migrate(struct drm_i915_gem_object *obj, struct ttm_placement placement; int ret; - i915_ttm_place_from_region(mr, &requested, flags); + i915_ttm_place_from_region(mr, &requested, obj->bo_offset, + obj->base.size, flags); placement.num_placement = 1; placement.num_busy_placement = 1; placement.placement = &requested; @@ -1159,6 +1167,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, drm_gem_private_object_init(&i915->drm, &obj->base, size); i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags); + obj->bo_offset = offset; + /* Don't put on a region list until we're either locked or fully initialized. */ obj->mm.region = mem; INIT_LIST_HEAD(&obj->mm.region_link); diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index 129f668f21ff..8e4e3f72c1ef 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -71,7 +71,8 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, GEM_BUG_ON(min_page_size < mm->chunk_size); - if (place->flags & TTM_PL_FLAG_CONTIGUOUS) { + if (place->fpfn + bman_res->base.num_pages != place->lpfn && + place->flags & TTM_PL_FLAG_CONTIGUOUS) { unsigned long pages; size = roundup_pow_of_two(size); diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c index 737ef3f4ab54..62ff77445b01 100644 --- a/drivers/gpu/drm/i915/intel_region_ttm.c +++ b/drive
[PATCH v2 4/8] drm/i915: add i915_gem_object_create_region_at()
Add a generic interface for allocating an object at some specific offset, and convert stolen over. Later we will want to hook this up to different backends. Signed-off-by: Matthew Auld Cc: Thomas Hellström --- .../drm/i915/display/intel_plane_initial.c| 4 +- drivers/gpu/drm/i915/gem/i915_gem_create.c| 2 +- drivers/gpu/drm/i915/gem/i915_gem_region.c| 47 ++-- drivers/gpu/drm/i915/gem/i915_gem_region.h| 7 ++ drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 1 + drivers/gpu/drm/i915/gem/i915_gem_stolen.c| 74 --- drivers/gpu/drm/i915/gem/i915_gem_stolen.h| 4 - drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 1 + drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 1 + drivers/gpu/drm/i915/gt/intel_rc6.c | 8 +- drivers/gpu/drm/i915/intel_memory_region.h| 1 + drivers/gpu/drm/i915/selftests/mock_region.c | 1 + 12 files changed, 77 insertions(+), 74 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index e207d12286b5..5227e5b35206 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -3,6 +3,7 @@ * Copyright © 2021 Intel Corporation */ +#include "gem/i915_gem_region.h" #include "i915_drv.h" #include "intel_atomic_plane.h" #include "intel_display.h" @@ -69,7 +70,8 @@ initial_plane_vma(struct drm_i915_private *i915, size * 2 > i915->stolen_usable_size) return NULL; - obj = i915_gem_object_create_stolen_for_preallocated(i915, base, size); + obj = i915_gem_object_create_region_at(i915->mm.stolen_region, + base, size, 0); if (IS_ERR(obj)) return NULL; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index c6eb023d3d86..5802692ea604 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -123,7 +123,7 @@ __i915_gem_object_create_user_ext(struct drm_i915_private *i915, u64 size, */ flags = I915_BO_ALLOC_USER; - ret = mr->ops->init_object(mr, obj, size, 0, flags); + ret = mr->ops->init_object(mr, obj, I915_BO_INVALID_OFFSET, size, 0, flags); if (ret) goto object_free; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c index c9b2e8b91053..3428ddfb2fdb 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c @@ -27,11 +27,12 @@ void i915_gem_object_release_memory_region(struct drm_i915_gem_object *obj) mutex_unlock(&mem->objects.lock); } -struct drm_i915_gem_object * -i915_gem_object_create_region(struct intel_memory_region *mem, - resource_size_t size, - resource_size_t page_size, - unsigned int flags) +static struct drm_i915_gem_object * +__i915_gem_object_create_region(struct intel_memory_region *mem, + resource_size_t offset, + resource_size_t size, + resource_size_t page_size, + unsigned int flags) { struct drm_i915_gem_object *obj; resource_size_t default_page_size; @@ -86,7 +87,7 @@ i915_gem_object_create_region(struct intel_memory_region *mem, if (default_page_size < mem->min_page_size) flags |= I915_BO_ALLOC_PM_EARLY; - err = mem->ops->init_object(mem, obj, size, page_size, flags); + err = mem->ops->init_object(mem, obj, offset, size, page_size, flags); if (err) goto err_object_free; @@ -98,6 +99,40 @@ i915_gem_object_create_region(struct intel_memory_region *mem, return ERR_PTR(err); } +struct drm_i915_gem_object * +i915_gem_object_create_region(struct intel_memory_region *mem, + resource_size_t size, + resource_size_t page_size, + unsigned int flags) +{ + return __i915_gem_object_create_region(mem, I915_BO_INVALID_OFFSET, + size, page_size, flags); +} + +struct drm_i915_gem_object * +i915_gem_object_create_region_at(struct intel_memory_region *mem, +resource_size_t offset, +resource_size_t size, +unsigned int flags) +{ + GEM_BUG_ON(offset == I915_BO_INVALID_OFFSET); + + if (GEM_WARN_ON(!IS_ALIGNED(size, mem->min_page_size)) || + GEM_WARN_ON(!IS_ALIGNED(offset, mem->min_page_size))) + return ERR_PTR(-EINVAL); + + if (range_overflows(offset, size, resource_size(&mem->region))) + return ERR_PTR(-EINVAL); + + if (!(flags
[PATCH v2 6/8] drm/i915/display: Check mappable aperture when pinning preallocated vma
From: CQ Tang When system does not have mappable aperture, ggtt->mappable_end=0. In this case if we pass PIN_MAPPABLE when pinning vma, the pinning code will return -ENOSPC. So conditionally set PIN_MAPPABLE if HAS_GMCH(). Suggested-by: Chris P Wilson Signed-off-by: CQ Tang Cc: Radhakrishna Sripada Cc: Ap Kamal Signed-off-by: Matthew Auld Cc: Thomas Hellström Cc: Ville Syrjälä --- drivers/gpu/drm/i915/display/intel_plane_initial.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index 5227e5b35206..f797fcef18fc 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -51,6 +51,7 @@ initial_plane_vma(struct drm_i915_private *i915, struct drm_i915_gem_object *obj; struct i915_vma *vma; u32 base, size; + u64 pinctl; if (!mem || plane_config->size == 0) return NULL; @@ -101,7 +102,10 @@ initial_plane_vma(struct drm_i915_private *i915, if (IS_ERR(vma)) goto err_obj; - if (i915_ggtt_pin(vma, NULL, 0, PIN_MAPPABLE | PIN_OFFSET_FIXED | base)) + pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base; + if (HAS_GMCH(i915)) + pinctl |= PIN_MAPPABLE; + if (i915_vma_pin(vma, 0, 0, pinctl)) goto err_obj; if (i915_gem_object_is_tiled(obj) && -- 2.34.1
[PATCH v2 7/8] drm/i915: fixup the initial fb base on DG1
The offset we get looks to be the exact start of DSM, but the inital_plane_vma expects the address to be relative. v2(Ville): - The base is actually the pre-programmed GGTT address, which is then meant to 1:1 map to somewhere inside dsm. In the case of dgpu the base looks to just be some offset within lmem, but this also happens to be the exact dsm start, on dg1. Therefore we should only need to fudge the physical address, before allocating from stolen. - Bail if it's not located in dsm. Signed-off-by: Matthew Auld Cc: Thomas Hellström Cc: Ville Syrjälä --- .../drm/i915/display/intel_plane_initial.c| 21 +-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index f797fcef18fc..2aebde02ff57 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -50,7 +50,7 @@ initial_plane_vma(struct drm_i915_private *i915, struct intel_memory_region *mem = i915->mm.stolen_region; struct drm_i915_gem_object *obj; struct i915_vma *vma; - u32 base, size; + u32 base, phys_base, size; u64 pinctl; if (!mem || plane_config->size == 0) @@ -71,8 +71,25 @@ initial_plane_vma(struct drm_i915_private *i915, size * 2 > i915->stolen_usable_size) return NULL; + /* +* On discrete, it looks like the GGTT base address should 1:1 map to +* somewhere in lmem. On DG1 for some reason this intersects with the +* exact start of DSM(possibly due to small lmem size), in which case we +* need to allocate it directly from stolen, which means fudging the +* physical address to be relative to the start of DSM. In such cases +* we might also need to choose between initial fb vs fbc, if space is +* limited. +*/ + phys_base = base; + if (IS_DG1(i915)) { + if (WARN_ON(phys_base < i915->dsm.start)) + return NULL; + + phys_base -= i915->dsm.start; + } + obj = i915_gem_object_create_region_at(i915->mm.stolen_region, - base, size, 0); + phys_base, size, 0); if (IS_ERR(obj)) return NULL; -- 2.34.1
[PATCH v2 8/8] drm/i915: fixup the initial fb on DG2
On DG2+ the initial fb shouldn't be placed anywhere close to DSM, and so should just be allocated directly from LMEM. Signed-off-by: Matthew Auld Cc: Thomas Hellström --- .../drm/i915/display/intel_plane_initial.c| 46 +++ 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c index 2aebde02ff57..12bda6604a1b 100644 --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c @@ -58,6 +58,31 @@ initial_plane_vma(struct drm_i915_private *i915, base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT); + phys_base = base; + if (IS_DGFX(i915)) { + /* +* On discrete, it looks like the GGTT base address should 1:1 +* map to somewhere in lmem. On DG1 for some reason this +* intersects with the exact start of DSM(possibly due to small +* lmem size), in which case we need to allocate it directly +* from stolen, which means fudging the physical address to be +* relative to the start of DSM. In such cases we might also +* need to choose between initial fb vs fbc, if space is +* limited. +* +* On future discrete HW, like DG2, we should be able to just +* allocate directly from lmem it seems. +*/ + if (IS_DG1(i915)) { + if (WARN_ON(phys_base < i915->dsm.start)) + return NULL; + + phys_base -= i915->dsm.start; + } else { + mem = i915->mm.regions[INTEL_REGION_LMEM]; + } + } + size = round_up(plane_config->base + plane_config->size, mem->min_page_size); size -= base; @@ -68,28 +93,11 @@ initial_plane_vma(struct drm_i915_private *i915, * features. */ if (IS_ENABLED(CONFIG_FRAMEBUFFER_CONSOLE) && + mem == i915->mm.stolen_region && size * 2 > i915->stolen_usable_size) return NULL; - /* -* On discrete, it looks like the GGTT base address should 1:1 map to -* somewhere in lmem. On DG1 for some reason this intersects with the -* exact start of DSM(possibly due to small lmem size), in which case we -* need to allocate it directly from stolen, which means fudging the -* physical address to be relative to the start of DSM. In such cases -* we might also need to choose between initial fb vs fbc, if space is -* limited. -*/ - phys_base = base; - if (IS_DG1(i915)) { - if (WARN_ON(phys_base < i915->dsm.start)) - return NULL; - - phys_base -= i915->dsm.start; - } - - obj = i915_gem_object_create_region_at(i915->mm.stolen_region, - phys_base, size, 0); + obj = i915_gem_object_create_region_at(mem, phys_base, size, 0); if (IS_ERR(obj)) return NULL; -- 2.34.1
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On 3/10/22 11:53, Maxime Ripard wrote: On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote: On 3/8/22 17:21, Maxime Ripard wrote: On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote: On 3/8/22 14:49, Maxime Ripard wrote: On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote: On 3/8/22 13:51, Maxime Ripard wrote: On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: On 3/8/22 11:07, Jagan Teki wrote: On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut wrote: On 3/8/22 09:03, Jagan Teki wrote: Hi, [...] @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = { static int chipone_parse_dt(struct chipone *icn) { struct device *dev = icn->dev; + struct device_node *endpoint; struct drm_panel *panel; + int dsi_lanes; int ret; icn->vdd1 = devm_regulator_get_optional(dev, "vdd1"); @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct chipone *icn) return PTR_ERR(icn->enable_gpio); } + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); + dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes"); + icn->host_node = of_graph_get_remote_port_parent(endpoint); + of_node_put(endpoint); + + if (!icn->host_node) + return -ENODEV; The non-ports-based OF graph returns a -19 example on the Allwinner Display pipeline in R16 [1]. We need to have a helper to return host_node for non-ports as I have done it for drm_of_find_bridge. [1] https://patchwork.amarulasolutions.com/patch/1805/ The link points to a patch marked "DO NOT MERGE", maybe that patch is missing the DSI host port@0 OF graph link ? Both port@0 and port@1 are required, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 What is "non-ports-based OF graph" ? I don't see drm_of_find_bridge() in linux-next , what is that ? port@0 is optional as some of the DSI host OF-graph represent the bridge or panel as child nodes instead of ports. (i think dt-binding has to fix it to make port@0 optional) The current upstream DT binding document says: required: - port@0 - port@1 So port@0 is mandatory. In the binding, sure, but fundamentally the DT excerpt Jagan provided is correct. If the bridge supports DCS, there's no reason to use the OF graph in the first place: the bridge node will be a child node of the MIPI-DSI controller (and there's no obligation to use the OF-graph for a MIPI-DSI controller). I believe port@0 should be made optional (or downright removed if MIPI-DCS in the only control bus). That's out of scope of this series anyway, so Jagan can implement patches for that mode if needed. Not really? You can't count on the port@0 being there generally speaking, so you can't count on data-lanes being there either, which exactly what you're doing in this patch. I can because the upstream DT bindings currently say port@0 must be present, see above. If that requirement should be relaxed, sure, but that's a separate series. And another upstream DT bindings say that you don't need them at all. Which "another upstream DT bindings" do you refer to ? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt Yes, there's a conflict. Yes, it's unfortunate. But the generic DSI binding is far more relevant than a single bridge driver. That seems to be the wrong way around, how can generic subsystem-wide binding take precedence over specific driver binding ? This is the binding of the bus. You're part of that bus. You're a child node of that bus, but nothing ever mandates that your parent node uses the same convention. And some don't. And since your bridge can be connected to pretty much any DSI controller, you have to use the lowest common denominator, not make up some new constraints that not all controller will be able to comply with. It seems to me the ICN6211 DT bindings currently further constraint the generic DSI bus bindings, and that seems OK to me. Let me reiterate this again -- if someone wants to relax the requirements currently imposed by the ICN6211 DT bindings, fine, but that can be done in a separate patchset AND that needs DT bindings update. Furthermore, there are no users of this ICN6211 bridge in upstream DTs, so there is currently no bridge which would operate without OF graph using this driver. So figuring it out is very much a prerequisite to that series, especially since those patches effectively make the OF-graph mandatory in some situations, while it was purely aesthetics before. The OF-graph is mandatory per the DT bindings of this driver. If you implement invalid DT which does not contain port@0, it will fail DT validation. If this requirement should be relaxed, sure,
Re: [Intel-gfx] [CI 1/2] drm/mm: Add an iterator to optimally walk over holes for an allocation (v5)
On 05/03/2022 23:36, Vivek Kasireddy wrote: This iterator relies on drm_mm_first_hole() and drm_mm_next_hole() functions to identify suitable holes for an allocation of a given size by efficiently traversing the rbtree associated with the given allocator. It replaces the for loop in drm_mm_insert_node_in_range() and can also be used by drm drivers to quickly identify holes of a certain size within a given range. v2: (Tvrtko) - Prepend a double underscore for the newly exported first/next_hole - s/each_best_hole/each_suitable_hole/g - Mask out DRM_MM_INSERT_ONCE from the mode before calling first/next_hole and elsewhere. v3: (Tvrtko) - Reduce the number of hunks by retaining the "mode" variable name v4: - Typo: s/__drm_mm_next_hole(.., hole/__drm_mm_next_hole(.., pos v5: (Tvrtko) - Fixed another typo: should pass caller_mode instead of mode to the iterator in drm_mm_insert_node_in_range(). Reviewed-by: Tvrtko Ursulin Acked-by: Christian König Suggested-by: Tvrtko Ursulin Signed-off-by: Vivek Kasireddy Are we okay to merge this via drm-intel-gt-next? I haven't spotted any in progress patches touching this area so should be conflict free. Regards, Tvrtko --- drivers/gpu/drm/drm_mm.c | 32 +++- include/drm/drm_mm.h | 36 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 8257f9d4f619..6ff98a0e4df3 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -352,10 +352,10 @@ static struct drm_mm_node *find_hole_addr(struct drm_mm *mm, u64 addr, u64 size) return node; } -static struct drm_mm_node * -first_hole(struct drm_mm *mm, - u64 start, u64 end, u64 size, - enum drm_mm_insert_mode mode) +struct drm_mm_node * +__drm_mm_first_hole(struct drm_mm *mm, + u64 start, u64 end, u64 size, + enum drm_mm_insert_mode mode) { switch (mode) { default: @@ -374,6 +374,7 @@ first_hole(struct drm_mm *mm, hole_stack); } } +EXPORT_SYMBOL(__drm_mm_first_hole); /** * DECLARE_NEXT_HOLE_ADDR - macro to declare next hole functions @@ -410,11 +411,11 @@ static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ DECLARE_NEXT_HOLE_ADDR(next_hole_high_addr, rb_left, rb_right) DECLARE_NEXT_HOLE_ADDR(next_hole_low_addr, rb_right, rb_left) -static struct drm_mm_node * -next_hole(struct drm_mm *mm, - struct drm_mm_node *node, - u64 size, - enum drm_mm_insert_mode mode) +struct drm_mm_node * +__drm_mm_next_hole(struct drm_mm *mm, + struct drm_mm_node *node, + u64 size, + enum drm_mm_insert_mode mode) { switch (mode) { default: @@ -432,6 +433,7 @@ next_hole(struct drm_mm *mm, return &node->hole_stack == &mm->hole_stack ? NULL : node; } } +EXPORT_SYMBOL(__drm_mm_next_hole); /** * drm_mm_reserve_node - insert an pre-initialized node @@ -516,11 +518,11 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, u64 size, u64 alignment, unsigned long color, u64 range_start, u64 range_end, - enum drm_mm_insert_mode mode) + enum drm_mm_insert_mode caller_mode) { struct drm_mm_node *hole; u64 remainder_mask; - bool once; + enum drm_mm_insert_mode mode = caller_mode & ~DRM_MM_INSERT_ONCE; DRM_MM_BUG_ON(range_start > range_end); @@ -533,13 +535,9 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, if (alignment <= 1) alignment = 0; - once = mode & DRM_MM_INSERT_ONCE; - mode &= ~DRM_MM_INSERT_ONCE; - remainder_mask = is_power_of_2(alignment) ? alignment - 1 : 0; - for (hole = first_hole(mm, range_start, range_end, size, mode); -hole; -hole = once ? NULL : next_hole(mm, hole, size, mode)) { + drm_mm_for_each_suitable_hole(hole, mm, range_start, range_end, + size, caller_mode) { u64 hole_start = __drm_mm_hole_node_start(hole); u64 hole_end = hole_start + hole->hole_size; u64 adj_start, adj_end; diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index ac33ba1b18bc..dff6db627807 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -400,6 +400,42 @@ static inline u64 drm_mm_hole_node_end(const struct drm_mm_node *hole_node) 1 : 0; \ pos = list_next_entry(pos, hole_stack)) +struct drm_mm_node * +__drm_mm_first_hole(struct drm_mm *mm, + u64 start, u64 end, u64 size, + enum drm_mm_insert_mode mode); + +struct drm_mm_node * +__drm_mm
Re: [PATCH v6 0/6] drm: exynos: dsi: Convert drm bridge
Hi Jagan, Am 09.03.22 um 15:01 schrieb Jagan Teki: > Hi Frieder, > > On Wed, Mar 9, 2022 at 6:54 PM Frieder Schrempf > wrote: >> >> Hi Jagan, >> >> Am 03.03.22 um 17:36 schrieb Jagan Teki: >>> Updated series about drm bridge conversion of exynos dsi. >>> >>> Previous version can be accessible, here [1]. >>> >>> Patch 1: tc358764 panel_bridge API >>> >>> Patch 2: connector reset >>> >>> Patch 3: bridge attach in MIC >>> >>> Patch 4: panel_bridge API >>> >>> Patch 5: bridge conversion >>> >>> Patch 6: atomic functions >>> >>> [1] >>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fcover%2F1839&data=04%7C01%7Cfrieder.schrempf%40kontron.de%7Cc99f637dd67444dfc38208da01d55963%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637824313083236643%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=qF5bVwelZ35cQQygW3PvPUZkQlyFalUDsyDVjDnngiU%3D&reserved=0 >>> >>> Any inputs? >> >> Thanks for your efforts. I didn't follow the whole history, but I'm >> looking forward and hope to see upstream support for the i.MX8MM DSIM in >> the not too distant future. >> >> Can you give me a short update about the state of this patchset? Are >> there still any major obstacles? >> >> I can't help with testing on Exynos, but if you have the matching >> follow-up patches for i.MX8MM support somewhere around I could do some >> tests with those on i.MX8MM. > > Unfortunately, it is getting slow due to existing exynos dsi drivers. > Idea is to push exynos and then move the bridge as per Mailing-list > discussion. I have initial series to support i.MX8MM on linux-next [1] > which is working on my setup. However I'm waiting for this series to > move further to send those on the mailing list. Indeed I'm solely > relaying on Marek testing to move further as I too don't have Exynos > hardware to validate. Thanks for the status update. Let's hope Marek or others with access to the hardware can provide further testing. And thanks for providing the git tree for i.MX8MM. I will try to do some tests on our hardware. Thanks Frieder
Re: [PATCH 1/2] dt-bindings: drm: bridge: adi,adv7533: Document adi,disable-lanes-override property
On 09/03/2022 16:11, Biju Das wrote: > On Renesas RZ/{G2L,V2L} platforms changing the lanes from 4 to 3 at > lower frequencies causes display instability. On such platforms, it > is better to avoid switching lanes from 4 to 3 as it needs different > set of PLL parameter constraints to make the display stable with 3 > lanes. > > This patch introduces 'adi,disable-lanes-override' property to disable > lane switching at lower frequencies. > > Signed-off-by: Biju Das > Reviewed-by: Lad Prabhakar > --- > .../devicetree/bindings/display/bridge/adi,adv7533.yaml | 5 + > 1 file changed, 5 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > index f36209137c8a..2dc378039d21 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > @@ -84,6 +84,11 @@ properties: >timings for HDMI output. > type: boolean > > + adi,disable-lanes-override: > +description: > + Disables the overriding lanes at lower frequencies. > +type: boolean The bindings should not focus on desired feature/functionality of driver, but hardware. You are now encoding the driver behavior in the bindings. Best regards, Krzysztof
Re: [PATCH v6 0/6] drm: exynos: dsi: Convert drm bridge
Am 10.03.22 um 14:03 schrieb Frieder Schrempf: > Hi Jagan, > > Am 09.03.22 um 15:01 schrieb Jagan Teki: >> Hi Frieder, >> >> On Wed, Mar 9, 2022 at 6:54 PM Frieder Schrempf >> wrote: >>> >>> Hi Jagan, >>> >>> Am 03.03.22 um 17:36 schrieb Jagan Teki: Updated series about drm bridge conversion of exynos dsi. Previous version can be accessible, here [1]. Patch 1: tc358764 panel_bridge API Patch 2: connector reset Patch 3: bridge attach in MIC Patch 4: panel_bridge API Patch 5: bridge conversion Patch 6: atomic functions [1] https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fcover%2F1839&data=04%7C01%7Cfrieder.schrempf%40kontron.de%7Cc99f637dd67444dfc38208da01d55963%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637824313083236643%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=qF5bVwelZ35cQQygW3PvPUZkQlyFalUDsyDVjDnngiU%3D&reserved=0 Any inputs? >>> >>> Thanks for your efforts. I didn't follow the whole history, but I'm >>> looking forward and hope to see upstream support for the i.MX8MM DSIM in >>> the not too distant future. >>> >>> Can you give me a short update about the state of this patchset? Are >>> there still any major obstacles? >>> >>> I can't help with testing on Exynos, but if you have the matching >>> follow-up patches for i.MX8MM support somewhere around I could do some >>> tests with those on i.MX8MM. >> >> Unfortunately, it is getting slow due to existing exynos dsi drivers. >> Idea is to push exynos and then move the bridge as per Mailing-list >> discussion. I have initial series to support i.MX8MM on linux-next [1] >> which is working on my setup. However I'm waiting for this series to >> move further to send those on the mailing list. Indeed I'm solely >> relaying on Marek testing to move further as I too don't have Exynos >> hardware to validate. > > Thanks for the status update. Let's hope Marek or others with access to > the hardware can provide further testing. > > And thanks for providing the git tree for i.MX8MM. I will try to do some > tests on our hardware. Sorry, forgot to say that if you could cc me on future iterations of this patchset and the upcoming i.MX8MM patches, that would be great, thanks!
[Bug 215652] kernel 5.17-rc fail to load radeon DRM "modprobe: ERROR: could not insert 'radeon': Unknown symbol in module, or unknown parameter (see dmesg)"
https://bugzilla.kernel.org/show_bug.cgi?id=215652 --- Comment #8 from Erhard F. (erhar...@mailbox.org) --- Created attachment 300550 --> https://bugzilla.kernel.org/attachment.cgi?id=300550&action=edit kernel dmesg (kernel 5.17-rc7, CONFIG_DRM_RADEON=m, Talos II) Seems this is issue already fixed in -rc7. v5.17-rc7 boots on the Talos II again with radeon drm loaded from disk without an initrd or firmware being built in. Out of curiosity I'll do a bisect next week anyhow to check out which commit fixed the issue. But feel free to close here if it is not appropriate to hold this bug open any longer. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Quoting Kieran Bingham (2022-03-07 17:59:54) > From: Laurent Pinchart > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > routed to a DisplayPort connector. Enable DisplayPort mode when the next > component in the display pipeline is detected as a DisplayPort > connector, and disable eDP features in that case. > > Signed-off-by: Laurent Pinchart > Reworked to set bridge type based on the next bridge/connector. > Signed-off-by: Kieran Bingham > -- > Changes since v1/RFC: > - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to >devm_drm_of_get_bridge" > - eDP/DP mode determined from the next bridge connector type. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 29f5f7123ed9..461f963faa0b 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -60,6 +60,7 @@ > #define SN_LN_ASSIGN_REG 0x59 > #define LN_ASSIGN_WIDTH 2 > #define SN_ENH_FRAME_REG 0x5A > +#define ASSR_CONTROL BIT(0) > #define VSTREAM_ENABLEBIT(3) > #define LN_POLRS_OFFSET 4 > #define LN_POLRS_MASK 0xf0 > @@ -91,6 +92,8 @@ > #define SN_DATARATE_CONFIG_REG 0x94 > #define DP_DATARATE_MASK GENMASK(7, 5) > #define DP_DATARATE(x)((x) << 5) > +#define SN_TRAINING_SETTING_REG0x95 > +#define SCRAMBLE_DISABLE BIT(4) > #define SN_ML_TX_MODE_REG 0x96 > #define ML_TX_MAIN_LINK_OFF 0 > #define ML_TX_NORMAL_MODE BIT(0) > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > *pdata, int dp_rate_idx, > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, >DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > + /* For DisplayPort, use the standard DP scrambler seed. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > + ASSR_CONTROL, 0); > + > /* enable DP PLL */ > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > *pdata, int dp_rate_idx, > goto exit; > } > > + /* For DisplayPort, disable scrambling mode. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > + > /* > * We'll try to link train several times. As part of link training > * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If > @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device > *adev, > pdata->bridge.funcs = &ti_sn_bridge_funcs; > pdata->bridge.of_node = np; > pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > - pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; > + pdata->bridge.type = pdata->next_bridge->type == > DRM_MODE_CONNECTOR_DisplayPort > + ? DRM_MODE_CONNECTOR_DisplayPort : > DRM_MODE_CONNECTOR_eDP; Anyone know if there's any expectation of other types here? Or is it just as safe to do: pdata->bridge.type = pdata->next_bridge->type; To achieve the same effect? -- Kieran > > drm_bridge_add(&pdata->bridge); > > -- > 2.32.0 >
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On Thu, Mar 10, 2022 at 01:47:13PM +0100, Marek Vasut wrote: > On 3/10/22 11:53, Maxime Ripard wrote: > > On Tue, Mar 08, 2022 at 10:41:05PM +0100, Marek Vasut wrote: > > > On 3/8/22 17:21, Maxime Ripard wrote: > > > > On Tue, Mar 08, 2022 at 03:47:22PM +0100, Marek Vasut wrote: > > > > > On 3/8/22 14:49, Maxime Ripard wrote: > > > > > > On Tue, Mar 08, 2022 at 02:27:40PM +0100, Marek Vasut wrote: > > > > > > > On 3/8/22 13:51, Maxime Ripard wrote: > > > > > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > > > > > > > > On 3/8/22 11:07, Jagan Teki wrote: > > > > > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct > > > > > > > > > > > > > drm_bridge_funcs chipone_bridge_funcs = { > > > > > > > > > > > > >static int chipone_parse_dt(struct chipone > > > > > > > > > > > > > *icn) > > > > > > > > > > > > >{ > > > > > > > > > > > > > struct device *dev = icn->dev; > > > > > > > > > > > > > + struct device_node *endpoint; > > > > > > > > > > > > > struct drm_panel *panel; > > > > > > > > > > > > > + int dsi_lanes; > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > icn->vdd1 = > > > > > > > > > > > > > devm_regulator_get_optional(dev, "vdd1"); > > > > > > > > > > > > > @@ -350,15 +359,42 @@ static int > > > > > > > > > > > > > chipone_parse_dt(struct chipone *icn) > > > > > > > > > > > > > return > > > > > > > > > > > > > PTR_ERR(icn->enable_gpio); > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > + endpoint = > > > > > > > > > > > > > of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > > > > > > > > > > > > > + dsi_lanes = > > > > > > > > > > > > > of_property_count_u32_elems(endpoint, "data-lanes"); > > > > > > > > > > > > > + icn->host_node = > > > > > > > > > > > > > of_graph_get_remote_port_parent(endpoint); > > > > > > > > > > > > > + of_node_put(endpoint); > > > > > > > > > > > > > + > > > > > > > > > > > > > + if (!icn->host_node) > > > > > > > > > > > > > + return -ENODEV; > > > > > > > > > > > > > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on > > > > > > > > > > > > the Allwinner > > > > > > > > > > > > Display pipeline in R16 [1]. > > > > > > > > > > > > > > > > > > > > > > > > We need to have a helper to return host_node for > > > > > > > > > > > > non-ports as I have > > > > > > > > > > > > done it for drm_of_find_bridge. > > > > > > > > > > > > > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > > > > > > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe > > > > > > > > > > > that patch is > > > > > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 > > > > > > > > > > > and port@1 are > > > > > > > > > > > required, see: > > > > > > > > > > > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > > > > > > > > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > > > > > > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is > > > > > > > > > > > that ? > > > > > > > > > > > > > > > > > > > > port@0 is optional as some of the DSI host OF-graph > > > > > > > > > > represent the > > > > > > > > > > bridge or panel as child nodes instead of ports. (i think > > > > > > > > > > dt-binding > > > > > > > > > > has to fix it to make port@0 optional) > > > > > > > > > > > > > > > > > > The current upstream DT binding document says: > > > > > > > > > > > > > > > > > > required: > > > > > > > > > - port@0 > > > > > > > > > - port@1 > > > > > > > > > > > > > > > > > > So port@0 is mandatory. > > > > > > > > > > > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan > > > > > > > > provided is > > > > > > > > correct. If the bridge supports DCS, there's no reason to use > > > > > > > > the OF > > > > > > > > graph in the first place: the bridge node will be a child node > > > > > > > > of the > > > > > > > > MIPI-DSI controller (and there's no obligation to use the > > > > > > > > OF-graph for a > > > > > > > > MIPI-DSI controller). > > > > > > > > > > > > > > > > I believe port@0 should be made optional (or downright removed > > > > > > > > if > > > > > > > > MIPI-DCS in the only control bus). > > > > > > > > > > > > > > That's out of scope of this series anyway, so Jagan can implement > > > > > > > patches > >
Re: [PATCH V3 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
On Thu, Mar 10, 2022 at 01:16:57PM +0200, Laurent Pinchart wrote: > On Thu, Mar 10, 2022 at 11:57:38AM +0100, Maxime Ripard wrote: > > On Thu, Mar 10, 2022 at 12:42:12PM +0200, Laurent Pinchart wrote: > > > On Tue, Mar 08, 2022 at 01:51:40PM +0100, Maxime Ripard wrote: > > > > On Tue, Mar 08, 2022 at 11:29:59AM +0100, Marek Vasut wrote: > > > > > On 3/8/22 11:07, Jagan Teki wrote: > > > > > > On Tue, Mar 8, 2022 at 3:19 PM Marek Vasut wrote: > > > > > > > > > > > > > > On 3/8/22 09:03, Jagan Teki wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > @@ -314,7 +321,9 @@ static const struct drm_bridge_funcs > > > > > > > > > chipone_bridge_funcs = { > > > > > > > > >static int chipone_parse_dt(struct chipone *icn) > > > > > > > > >{ > > > > > > > > > struct device *dev = icn->dev; > > > > > > > > > + struct device_node *endpoint; > > > > > > > > > struct drm_panel *panel; > > > > > > > > > + int dsi_lanes; > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > icn->vdd1 = devm_regulator_get_optional(dev, > > > > > > > > > "vdd1"); > > > > > > > > > @@ -350,15 +359,42 @@ static int chipone_parse_dt(struct > > > > > > > > > chipone *icn) > > > > > > > > > return PTR_ERR(icn->enable_gpio); > > > > > > > > > } > > > > > > > > > > > > > > > > > > + endpoint = > > > > > > > > > of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > > > > > > > > > + dsi_lanes = of_property_count_u32_elems(endpoint, > > > > > > > > > "data-lanes"); > > > > > > > > > + icn->host_node = > > > > > > > > > of_graph_get_remote_port_parent(endpoint); > > > > > > > > > + of_node_put(endpoint); > > > > > > > > > + > > > > > > > > > + if (!icn->host_node) > > > > > > > > > + return -ENODEV; > > > > > > > > > > > > > > > > The non-ports-based OF graph returns a -19 example on the > > > > > > > > Allwinner > > > > > > > > Display pipeline in R16 [1]. > > > > > > > > > > > > > > > > We need to have a helper to return host_node for non-ports as I > > > > > > > > have > > > > > > > > done it for drm_of_find_bridge. > > > > > > > > > > > > > > > > [1] https://patchwork.amarulasolutions.com/patch/1805/ > > > > > > > > > > > > > > The link points to a patch marked "DO NOT MERGE", maybe that > > > > > > > patch is > > > > > > > missing the DSI host port@0 OF graph link ? Both port@0 and > > > > > > > port@1 are > > > > > > > required, see: > > > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/bridge/chipone,icn6211.yaml#n53 > > > > > > > > > > > > > > What is "non-ports-based OF graph" ? > > > > > > > > > > > > > > I don't see drm_of_find_bridge() in linux-next , what is that ? > > > > > > > > > > > > port@0 is optional as some of the DSI host OF-graph represent the > > > > > > bridge or panel as child nodes instead of ports. (i think dt-binding > > > > > > has to fix it to make port@0 optional) > > > > > > > > > > The current upstream DT binding document says: > > > > > > > > > > required: > > > > > - port@0 > > > > > - port@1 > > > > > > > > > > So port@0 is mandatory. > > > > > > > > In the binding, sure, but fundamentally the DT excerpt Jagan provided is > > > > correct. If the bridge supports DCS, there's no reason to use the OF > > > > graph in the first place: the bridge node will be a child node of the > > > > MIPI-DSI controller (and there's no obligation to use the OF-graph for a > > > > MIPI-DSI controller). > > > > > > > > I believe port@0 should be made optional (or downright removed if > > > > MIPI-DCS in the only control bus). > > > > > > I think we should make ports mandatory in all cases actually. > > > > > > The DT parent-child hierarchy is meant to model control relations > > > between devices, so a DSI device controlled through DCS should be a > > > child of the DSI controller. No disagreement there. > > > > > > The OF graph is meant to model data connections. While a DSI device > > > controlled through DCS will use the same DSI link for data transfer, the > > > two concepts are different. We have taken shortcuts and decided to not > > > use OF graph for some DSI devices (not necessarily as a well thought > > > decision, it was sometimes just not considered). > > > > I disagree. Unless the data path is explicitly stated using the OF-graph > > or some other binding, it's inferred. > > It is today, and for video data, I think it's showing to be a problem > :-) > > > We never asked ourselves where the > > data from an i2c chip, an ethernet controller or an v4l2 output device > > was coming from. It comes from the parent bus, because it's what makes > > sense. Making a requirement on the OF-Graph to model this would create a > > big inconsistency. > > I'm afraid I disagree, especially when it comes to data tra
RE: [v4 5/5] drm/edid: check for HF-SCDB block
On Thursday, March 10, 2022 6:50 PM, Ville Syrjälä wrote: >On Wed, Mar 02, 2022 at 05:35:11PM +0800, Lee Shawn C wrote: >> Find HF-SCDB information in CEA extensions block. And retrieve >> Max_TMDS_Character_Rate that support by sink device. >> >> Cc: Jani Nikula >> Cc: Ville Syrjala >> Cc: Ankit Nautiyal >> Signed-off-by: Lee Shawn C >> --- >> drivers/gpu/drm/drm_edid.c | 36 >> 1 file changed, 36 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 2b8ddc956ce2..d6b48c543c23 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -3350,6 +3350,7 @@ add_detailed_modes(struct drm_connector *connector, >> struct edid *edid, >> #define EXT_VIDEO_DATA_BLOCK_4200x0E >> #define EXT_VIDEO_CAP_BLOCK_Y420CMDB0x0F >> #define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78 >> +#define EXT_VIDEO_HF_SCDB_DATA_BLOCK0x79 >> #define EDID_BASIC_AUDIO(1 << 6) >> #define EDID_CEA_YCRCB444 (1 << 5) >> #define EDID_CEA_YCRCB422 (1 << 4) >> @@ -4277,6 +4278,20 @@ static bool cea_db_is_vcdb(const u8 *db) >> return true; >> } >> >> +static bool cea_db_is_hf_scdb(const u8 *db) { >> +if (cea_db_tag(db) != USE_EXTENDED_TAG) >> +return false; >> + >> +if (cea_db_payload_len(db) < 7) >> +return false; >> + >> +if (cea_db_extended_tag(db) != EXT_VIDEO_HF_SCDB_DATA_BLOCK) >> +return false; >> + >> +return true; >> +} >> + >> static bool cea_db_is_y420cmdb(const u8 *db) { >> if (cea_db_tag(db) != USE_EXTENDED_TAG) @@ -4987,6 +5002,25 @@ >> static void drm_parse_vcdb(struct drm_connector *connector, const u8 *db) >> info->rgb_quant_range_selectable = true; } >> >> +static void drm_parse_hf_scdb(struct drm_connector *connector, const >> +u8 *db) { >> +struct drm_display_info *info = &connector->display_info; >> +u32 max_tmds_clock; >> + >> +DRM_DEBUG_KMS("HF-SCDB version 0x%02x\n", db[4]); >> + >> +max_tmds_clock = db[5] * 5000; >> +if (info->max_tmds_clock < max_tmds_clock) { >> +info->max_tmds_clock = max_tmds_clock; >> +DRM_DEBUG_KMS("HF-SCDB: max TMDS clock %d kHz\n", >> + info->max_tmds_clock); >> +} >> + >> +/* >> + * ToDo: Parse the remaining SCDB data if needed >> + */ > >If I'm reading the spec right this block should contain the exact same stuff >as the HF-VSDB. We should reuse the same code for parsing both. > Yes, you are right! HF-SCDB contain the same SCDS data packet as VSDB. I will fix it later. Best regards, Shawn >> +} >> + >> static >> void drm_get_max_frl_rate(int max_frl_rate, u8 *max_lanes, u8 >> *max_rate_per_lane) { @@ -5282,6 +5316,8 @@ static void >> drm_parse_cea_ext(struct drm_connector *connector, >> drm_parse_y420cmdb_bitmap(connector, db); >> if (cea_db_is_vcdb(db)) >> drm_parse_vcdb(connector, db); >> +if (cea_db_is_hf_scdb(db)) >> +drm_parse_hf_scdb(connector, db); >> if (cea_db_is_hdmi_hdr_metadata_block(db)) >> drm_parse_hdr_metadata_block(connector, db); >> } >> -- >> 2.17.1 > >-- >Ville Syrjälä >Intel >
Re: [PATCH v2 3/4] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Quoting Kieran Bingham (2022-03-10 14:04:09) > Quoting Kieran Bingham (2022-03-07 17:59:54) > > From: Laurent Pinchart > > > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > > routed to a DisplayPort connector. Enable DisplayPort mode when the next > > component in the display pipeline is detected as a DisplayPort > > connector, and disable eDP features in that case. > > > > Signed-off-by: Laurent Pinchart > > Reworked to set bridge type based on the next bridge/connector. > > Signed-off-by: Kieran Bingham > > -- > > Changes since v1/RFC: > > - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to > >devm_drm_of_get_bridge" > > - eDP/DP mode determined from the next bridge connector type. > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 29f5f7123ed9..461f963faa0b 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -60,6 +60,7 @@ > > #define SN_LN_ASSIGN_REG 0x59 > > #define LN_ASSIGN_WIDTH 2 > > #define SN_ENH_FRAME_REG 0x5A > > +#define ASSR_CONTROL BIT(0) > > #define VSTREAM_ENABLEBIT(3) > > #define LN_POLRS_OFFSET 4 > > #define LN_POLRS_MASK 0xf0 > > @@ -91,6 +92,8 @@ > > #define SN_DATARATE_CONFIG_REG 0x94 > > #define DP_DATARATE_MASK GENMASK(7, 5) > > #define DP_DATARATE(x)((x) << 5) > > +#define SN_TRAINING_SETTING_REG0x95 > > +#define SCRAMBLE_DISABLE BIT(4) > > #define SN_ML_TX_MODE_REG 0x96 > > #define ML_TX_MAIN_LINK_OFF 0 > > #define ML_TX_NORMAL_MODE BIT(0) > > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > > *pdata, int dp_rate_idx, > > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, > >DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > > > + /* For DisplayPort, use the standard DP scrambler seed. */ > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > > + ASSR_CONTROL, 0); > > + > > /* enable DP PLL */ > > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > > > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > > *pdata, int dp_rate_idx, > > goto exit; > > } > > > > + /* For DisplayPort, disable scrambling mode. */ > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > + > > /* > > * We'll try to link train several times. As part of link training > > * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If > > @@ -1260,7 +1273,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device > > *adev, > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > pdata->bridge.of_node = np; > > pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > - pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; > > + pdata->bridge.type = pdata->next_bridge->type == > > DRM_MODE_CONNECTOR_DisplayPort > > + ? DRM_MODE_CONNECTOR_DisplayPort : > > DRM_MODE_CONNECTOR_eDP; > > Anyone know if there's any expectation of other types here? Or is it > just as safe to do: > > pdata->bridge.type = pdata->next_bridge->type; > > To achieve the same effect? Unfortunately, it would create nicer code - but I don't think it's exactly right. It leaves the definition of our bridge type to the connector type of the panel. While the SN65DSI86 can /only/ be either eDP or DisplayPort. So I'll keep this conditional on if the next connector is DRM_MODE_CONNECTOR_DisplayPort. > > -- > Kieran > > > > > > drm_bridge_add(&pdata->bridge); > > > > -- > > 2.32.0 > >
[Bug 215652] kernel 5.17-rc fail to load radeon DRM "modprobe: ERROR: could not insert 'radeon': Unknown symbol in module, or unknown parameter (see dmesg)"
https://bugzilla.kernel.org/show_bug.cgi?id=215652 --- Comment #9 from Alex Deucher (alexdeuc...@gmail.com) --- Only the person that filed the bug can close it. If it's fixed for you, please close it. Thanks! -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] drm: remove min_order BUG_ON check
On 08/03/22 10:31 pm, Matthew Auld wrote: > On 08/03/2022 13:59, Arunpravin wrote: >> >> >> On 07/03/22 10:11 pm, Matthew Auld wrote: >>> On 07/03/2022 14:37, Arunpravin wrote: place BUG_ON(order < min_order) outside do..while loop as it fails Unigine Heaven benchmark. Unigine Heaven has buffer allocation requests for example required pages are 161 and alignment request is 128. To allocate the remaining 33 pages, continues the iteration to find the order value which is 5 and when it compares with min_order = 7, enables the BUG_ON(). To avoid this problem, placed the BUG_ON check outside of do..while loop. Signed-off-by: Arunpravin --- drivers/gpu/drm/drm_buddy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..ed94c56b720f 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, order = fls(pages) - 1; min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); + BUG_ON(order < min_order); >>> >>> Isn't the issue that we are allowing a size that is not aligned to the >>> requested min_page_size? Should we not fix the caller(and throw a normal >>> error here), or perhaps add the round_up() here instead? >>> >> CASE 1: >> when size is not aligned to the requested min_page_size, for instance, >> required size = 161 pages, min_page_size = 128 pages, here we have 3 >> possible options, >> a. AFAIK,This kind of situation is common in any workload,the first >> allocation (i.e) 128 pages is aligned to min_page_size, Should we just >> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does >> know the left over pages are not in min_page_size alignment? > > So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some > arbitrary physical alignment for an object? Is that not meant to apply > to every page/chunk? The above example would only have the correct > physical alignment guaranteed for the first chunk, or so, is this the > expected ABI behaviour? > I gone through the function amdgpu_gem_create_ioctl(), it reads the physical alignment in bytes from userspace, does i915 round up the size value to the alignment or does i915 fails the allocation request if size is not aligned with min_page_size? If not, I think running unigine heaven or similar benchmark triggers BUG_ON() on current version of drm buddy > Also looking at this some more, the other related bug here is the > order-- == min_order check, since it now won't bail when order == 0, > leading to order = -1, if we are unlucky... will add a fix > > Originally, if asking for min_page_size > chunk_size, then the > allocation was meant to fail if it can't fill the resource request with > pages of at least that size(and also alignment). Or at least that was > the original meaning in i915 IIRC. we can follow the same here too, failing the allocation request if size is not aligned with min_page_size? I added a debug print for requested num_pages from userspace and its alignment request and executed unigine heaven, I see many such instances where min_page_size is not aligned to the size, how i915 handles such requests? > >> >> b. There are many such instances in unigine heaven workload (there would >> be many such workloads), throwing a normal error would lower the FPS? is >> it possible to fix at caller application? >> >> c. adding the round_up() is possible, but in every such instances we end >> up allocating extra unused memory. For example, if required pages = 1028 >> and min_page_size = 1024 pages, we end up round up of left over 4 pages >> to the min_page_size, so the total size would be 2048 pages. >> >>> i.e if someone does: >>> >>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags); >> CASE 2: >> I think this case should be detected (i.e) when min_page_size > size, >> should we return -EINVAL? >>> >>> This will still trigger the BUG_ON() even if we move it out of the loop, >>> AFAICT. >>> >> >> Should we just allow the CASE 1 proceed for the allocation and return >> -EINVAL for the CASE 2? >> + do { order = min(order, (unsigned int)fls(pages) - 1); BUG_ON(order > mm->max_order); - BUG_ON(order < min_order); do { if (flags & DRM_BUDDY_RANGE_ALLOCATION) base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
[v5 0/5] enhanced edid driver compatibility
Support to parse multiple CEA extension blocks and HF-EEODB to extend drm edid driver's capability. v4: add one more patch to support HF-SCDB v5: HF-SCDB and HF-VSDBS carry the same SCDS data. Reuse drm_parse_hdmi_forum_vsdb() to parse this packet. Lee Shawn C (5): drm/edid: seek for available CEA block from specific EDID block index drm/edid: parse multiple CEA extension block drm/edid: read HF-EEODB ext block drm/edid: parse HF-EEODB CEA extension block drm/edid: check for HF-SCDB block drivers/gpu/drm/drm_connector.c | 8 +- drivers/gpu/drm/drm_displayid.c | 5 +- drivers/gpu/drm/drm_edid.c | 172 include/drm/drm_edid.h | 4 +- 4 files changed, 142 insertions(+), 47 deletions(-) -- 2.31.1
[v5 2/5] drm/edid: parse multiple CEA extension block
Try to find and parse more CEA ext blocks if edid->extensions is greater than one. v2: split prvious patch to two. And do CEA block parsing in this one. v3: simplify this patch based on previous change. v4: refine patch v3. Cc: Jani Nikula Cc: Ville Syrjala Cc: Ankit Nautiyal Cc: intel-gfx Signed-off-by: Lee Shawn C --- drivers/gpu/drm/drm_edid.c | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 1251226d9284..7b672166fab4 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4319,16 +4319,22 @@ static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector, static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { - const u8 *cea, *db, *hdmi = NULL, *video = NULL; - u8 dbl, hdmi_len, video_len = 0; int modes = 0, ext_index = 0; - cea = drm_find_cea_extension(edid, &ext_index); - if (cea && cea_revision(cea) >= 3) { + for (;;) { + const u8 *cea, *db, *hdmi = NULL, *video = NULL; + u8 dbl, hdmi_len = 0, video_len = 0; int i, start, end; + cea = drm_find_cea_extension(edid, &ext_index); + if (!cea) + break; + + if (cea_revision(cea) < 3) + continue; + if (cea_db_offsets(cea, &start, &end)) - return 0; + continue; for_each_cea_db(cea, i, start, end) { db = &cea[i]; @@ -4350,15 +4356,15 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) dbl - 1); } } - } - /* -* We parse the HDMI VSDB after having added the cea modes as we will -* be patching their flags when the sink supports stereo 3D. -*/ - if (hdmi) - modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, - video_len); + /* +* We parse the HDMI VSDB after having added the cea modes as we will +* be patching their flags when the sink supports stereo 3D. +*/ + if (hdmi) + modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video, + video_len); + } return modes; } -- 2.31.1
[v5 1/5] drm/edid: seek for available CEA block from specific EDID block index
drm_find_cea_extension() always look for a top level CEA block. Pass ext_index from caller then this function to search next available CEA ext block from a specific EDID block pointer. Cc: Jani Nikula Cc: Ville Syrjala Cc: Ankit Nautiyal Cc: intel-gfx Signed-off-by: Lee Shawn C --- drivers/gpu/drm/drm_edid.c | 42 ++ 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 561f53831e29..1251226d9284 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3353,16 +3353,14 @@ const u8 *drm_find_edid_extension(const struct edid *edid, return edid_ext; } -static const u8 *drm_find_cea_extension(const struct edid *edid) +static const u8 *drm_find_cea_extension(const struct edid *edid, int *ext_index) { const struct displayid_block *block; struct displayid_iter iter; const u8 *cea; - int ext_index = 0; - /* Look for a top level CEA extension block */ - /* FIXME: make callers iterate through multiple CEA ext blocks? */ - cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index); + /* Look for a CEA extension block from ext_index */ + cea = drm_find_edid_extension(edid, CEA_EXT, ext_index); if (cea) return cea; @@ -3643,10 +3641,10 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) struct drm_device *dev = connector->dev; struct drm_display_mode *mode, *tmp; LIST_HEAD(list); - int modes = 0; + int modes = 0, ext_index = 0; /* Don't add CEA modes if the CEA extension block is missing */ - if (!drm_find_cea_extension(edid)) + if (!drm_find_cea_extension(edid, &ext_index)) return 0; /* @@ -4321,11 +4319,11 @@ static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector, static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { - const u8 *cea = drm_find_cea_extension(edid); - const u8 *db, *hdmi = NULL, *video = NULL; + const u8 *cea, *db, *hdmi = NULL, *video = NULL; u8 dbl, hdmi_len, video_len = 0; - int modes = 0; + int modes = 0, ext_index = 0; + cea = drm_find_cea_extension(edid, &ext_index); if (cea && cea_revision(cea) >= 3) { int i, start, end; @@ -4562,7 +4560,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) uint8_t *eld = connector->eld; const u8 *cea; const u8 *db; - int total_sad_count = 0; + int total_sad_count = 0, ext_index = 0; int mnl; int dbl; @@ -4571,7 +4569,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (!edid) return; - cea = drm_find_cea_extension(edid); + cea = drm_find_cea_extension(edid, &ext_index); if (!cea) { DRM_DEBUG_KMS("ELD: no CEA Extension found\n"); return; @@ -4655,11 +4653,11 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) */ int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads) { - int count = 0; + int count = 0, ext_index = 0; int i, start, end, dbl; const u8 *cea; - cea = drm_find_cea_extension(edid); + cea = drm_find_cea_extension(edid, &ext_index); if (!cea) { DRM_DEBUG_KMS("SAD: no CEA Extension found\n"); return 0; @@ -4717,11 +4715,11 @@ EXPORT_SYMBOL(drm_edid_to_sad); */ int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb) { - int count = 0; + int count = 0, ext_index = 0; int i, start, end, dbl; const u8 *cea; - cea = drm_find_cea_extension(edid); + cea = drm_find_cea_extension(edid, &ext_index); if (!cea) { DRM_DEBUG_KMS("SAD: no CEA Extension found\n"); return 0; @@ -4814,9 +4812,9 @@ bool drm_detect_hdmi_monitor(struct edid *edid) { const u8 *edid_ext; int i; - int start_offset, end_offset; + int start_offset, end_offset, ext_index = 0; - edid_ext = drm_find_cea_extension(edid); + edid_ext = drm_find_cea_extension(edid, &ext_index); if (!edid_ext) return false; @@ -4853,9 +4851,9 @@ bool drm_detect_monitor_audio(struct edid *edid) const u8 *edid_ext; int i, j; bool has_audio = false; - int start_offset, end_offset; + int start_offset, end_offset, ext_index = 0; - edid_ext = drm_find_cea_extension(edid); + edid_ext = drm_find_cea_extension(edid, &ext_index); if (!edid_ext) goto end; @@ -5177,9 +5175,9 @@ static void drm_parse_cea_ext(struct drm_connector *connector, { struct drm_display_info *info = &connector->display_info; const u8
[v5 3/5] drm/edid: read HF-EEODB ext block
According to HDMI 2.1 spec. "The HDMI Forum EDID Extension Override Data Block (HF-EEODB) is utilized by Sink Devices to provide an alternate method to indicate an EDID Extension Block count larger than 1, while avoiding the need to present a VESA Block Map in the first E-EDID Extension Block." It is a mandatory for HDMI 2.1 protocol compliance as well. This patch help to know how many HF_EEODB blocks report by sink and read allo HF_EEODB blocks back. v2: support to find CEA block, check EEODB block format, and return available block number in drm_edid_read_hf_eeodb_blk_count(). Cc: Jani Nikula Cc: Ville Syrjala Cc: Ankit Nautiyal Cc: intel-gfx Signed-off-by: Lee Shawn C --- drivers/gpu/drm/drm_connector.c | 8 +++- drivers/gpu/drm/drm_edid.c | 71 +++-- include/drm/drm_edid.h | 2 +- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index a50c82bc2b2f..16011023c12e 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2129,7 +2129,7 @@ int drm_connector_update_edid_property(struct drm_connector *connector, const struct edid *edid) { struct drm_device *dev = connector->dev; - size_t size = 0; + size_t size = 0, hf_eeodb_blk_count; int ret; const struct edid *old_edid; @@ -2137,8 +2137,12 @@ int drm_connector_update_edid_property(struct drm_connector *connector, if (connector->override_edid) return 0; - if (edid) + if (edid) { size = EDID_LENGTH * (1 + edid->extensions); + hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count(edid); + if (hf_eeodb_blk_count) + size = EDID_LENGTH * (1 + hf_eeodb_blk_count); + } /* Set the display info, using edid if available, otherwise * resetting the values to defaults. This duplicates the work diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b672166fab4..38b041c8b4ad 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1992,6 +1992,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, { int i, j = 0, valid_extensions = 0; u8 *edid, *new; + size_t hf_eeodb_blk_count; struct edid *override; override = drm_get_override_edid(connector); @@ -2051,7 +2052,35 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, } kfree(edid); + return (struct edid *)new; + } + + hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count((struct edid *)edid); + if (hf_eeodb_blk_count >= 2) { + new = krealloc(edid, (hf_eeodb_blk_count + 1) * EDID_LENGTH, GFP_KERNEL); + if (!new) + goto out; edid = new; + + valid_extensions = hf_eeodb_blk_count - 1; + for (j = 2; j <= hf_eeodb_blk_count; j++) { + u8 *block = edid + j * EDID_LENGTH; + + for (i = 0; i < 4; i++) { + if (get_edid_block(data, block, j, EDID_LENGTH)) + goto out; + if (drm_edid_block_valid(block, j, false, NULL)) + break; + } + + if (i == 4) + valid_extensions--; + } + + if (valid_extensions != hf_eeodb_blk_count - 1) { + DRM_ERROR("Not able to retrieve proper EDID contain HF-EEODB data.\n"); + goto out; + } } return (struct edid *)edid; @@ -3315,15 +3344,17 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define VIDEO_BLOCK 0x02 #define VENDOR_BLOCK0x03 #define SPEAKER_BLOCK 0x04 -#define HDR_STATIC_METADATA_BLOCK 0x6 -#define USE_EXTENDED_TAG 0x07 -#define EXT_VIDEO_CAPABILITY_BLOCK 0x00 +#define EXT_VIDEO_CAPABILITY_BLOCK 0x00 +#define HDR_STATIC_METADATA_BLOCK 0x06 +#define USE_EXTENDED_TAG 0x07 #define EXT_VIDEO_DATA_BLOCK_420 0x0E -#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F +#define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78 #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) #define EDID_CEA_VCDB_QS (1 << 6) +#define HF_EEODB_LENGTH2 /* * Search EDID for CEA extension block. @@ -4273,9 +4304,41 @@ static bool cea_db_is_y420vdb(const u8 *db) return true; } +static bool cea_db_is_hdmi_forum_eeodb(const u8 *db) +{ + if (cea_db_tag(db) != USE_EXTENDED_TAG) + return false; + + if (cea_db_payload_len
[v5 5/5] drm/edid: check for HF-SCDB block
Find HF-SCDB information in CEA extensions block. And retrieve Max_TMDS_Character_Rate that support by sink device. v2: HF-SCDB and HF-VSDBS carry the same SCDS data. Reuse drm_parse_hdmi_forum_vsdb() to parse this packet. Cc: Jani Nikula Cc: Ville Syrjala Cc: Ankit Nautiyal Cc: intel-gfx Signed-off-by: Lee Shawn C --- drivers/gpu/drm/drm_edid.c | 18 +- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 1da1239c21cb..f1d5180ee5a9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3350,6 +3350,7 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define EXT_VIDEO_DATA_BLOCK_420 0x0E #define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F #define EXT_VIDEO_HF_EEODB_DATA_BLOCK 0x78 +#define EXT_VIDEO_HF_SCDB_DATA_BLOCK 0x79 #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) @@ -4277,6 +4278,20 @@ static bool cea_db_is_vcdb(const u8 *db) return true; } +static bool cea_db_is_hdmi_forum_scdb(const u8 *db) +{ + if (cea_db_tag(db) != USE_EXTENDED_TAG) + return false; + + if (cea_db_payload_len(db) < 7) + return false; + + if (cea_db_extended_tag(db) != EXT_VIDEO_HF_SCDB_DATA_BLOCK) + return false; + + return true; +} + static bool cea_db_is_y420cmdb(const u8 *db) { if (cea_db_tag(db) != USE_EXTENDED_TAG) @@ -5272,7 +5287,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, if (cea_db_is_hdmi_vsdb(db)) drm_parse_hdmi_vsdb_video(connector, db); - if (cea_db_is_hdmi_forum_vsdb(db)) + if (cea_db_is_hdmi_forum_vsdb(db) || + cea_db_is_hdmi_forum_scdb(db)) drm_parse_hdmi_forum_vsdb(connector, db); if (cea_db_is_microsoft_vsdb(db)) drm_parse_microsoft_vsdb(connector, db); -- 2.31.1
[v5 4/5] drm/edid: parse HF-EEODB CEA extension block
While adding CEA modes, try to get available EEODB block number. Then based on it to parse numbers of ext blocks, retrieve CEA information and add more CEA modes. Cc: Jani Nikula Cc: Ville Syrjala Cc: Ankit Nautiyal Cc: intel-gfx Signed-off-by: Lee Shawn C --- drivers/gpu/drm/drm_displayid.c | 5 - drivers/gpu/drm/drm_edid.c | 35 +++-- include/drm/drm_edid.h | 2 +- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c index 32da557b960f..dc649a9efaa2 100644 --- a/drivers/gpu/drm/drm_displayid.c +++ b/drivers/gpu/drm/drm_displayid.c @@ -37,7 +37,10 @@ static const u8 *drm_find_displayid_extension(const struct edid *edid, int *length, int *idx, int *ext_index) { - const u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index); + const u8 *displayid = drm_find_edid_extension(edid, + DISPLAYID_EXT, + ext_index, + edid->extensions); const struct displayid_header *base; int ret; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 38b041c8b4ad..1da1239c21cb 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3360,23 +3360,23 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, * Search EDID for CEA extension block. */ const u8 *drm_find_edid_extension(const struct edid *edid, - int ext_id, int *ext_index) + int ext_id, int *ext_index, int ext_blk_num) { const u8 *edid_ext = NULL; int i; /* No EDID or EDID extensions */ - if (edid == NULL || edid->extensions == 0) + if (edid == NULL || edid->extensions == 0 || *ext_index >= ext_blk_num) return NULL; /* Find CEA extension */ - for (i = *ext_index; i < edid->extensions; i++) { + for (i = *ext_index; i < ext_blk_num; i++) { edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1); if (edid_ext[0] == ext_id) break; } - if (i >= edid->extensions) + if (i >= ext_blk_num) return NULL; *ext_index = i + 1; @@ -3384,14 +3384,15 @@ const u8 *drm_find_edid_extension(const struct edid *edid, return edid_ext; } -static const u8 *drm_find_cea_extension(const struct edid *edid, int *ext_index) +static const u8 *drm_find_cea_extension(const struct edid *edid, + int *ext_index, int ext_blk_num) { const struct displayid_block *block; struct displayid_iter iter; const u8 *cea; /* Look for a CEA extension block from ext_index */ - cea = drm_find_edid_extension(edid, CEA_EXT, ext_index); + cea = drm_find_edid_extension(edid, CEA_EXT, ext_index, ext_blk_num); if (cea) return cea; @@ -3675,7 +3676,7 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid) int modes = 0, ext_index = 0; /* Don't add CEA modes if the CEA extension block is missing */ - if (!drm_find_cea_extension(edid, &ext_index)) + if (!drm_find_cea_extension(edid, &ext_index, edid->extensions)) return 0; /* @@ -4327,7 +4328,7 @@ size_t drm_edid_read_hf_eeodb_blk_count(const struct edid *edid) int i, start, end, ext_index = 0; if (edid->extensions) { - cea = drm_find_cea_extension(edid, &ext_index); + cea = drm_find_cea_extension(edid, &ext_index, edid->extensions); if (cea && !cea_db_offsets(cea, &start, &end)) for_each_cea_db(cea, i, start, end) @@ -4383,13 +4384,17 @@ static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { int modes = 0, ext_index = 0; + int ext_blk_num = drm_edid_read_hf_eeodb_blk_count(edid); + + if (!ext_blk_num) + ext_blk_num = edid->extensions; for (;;) { const u8 *cea, *db, *hdmi = NULL, *video = NULL; u8 dbl, hdmi_len = 0, video_len = 0; int i, start, end; - cea = drm_find_cea_extension(edid, &ext_index); + cea = drm_find_cea_extension(edid, &ext_index, ext_blk_num); if (!cea) break; @@ -4638,7 +4643,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (!edid) return; - cea = drm_find_cea_extension(edid, &ext_index); + cea = drm_find_cea_extension(edid, &ext_index, edid->extensions); if (!cea) {
Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
Hi, On Thu, Mar 10, 2022 at 3:43 AM Kieran Bingham wrote: > > Hi Doug, > > Quoting Doug Anderson (2022-03-07 19:52:08) > > Hi, > > > > On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham > > wrote: > > > > > > From: Laurent Pinchart > > > > > > Implement the bridge connector-related .get_edid() operation, and report > > > the related bridge capabilities and type. > > > > > > Signed-off-by: Laurent Pinchart > > > > > > Reviewed-by: Stephen Boyd > > > Reviewed-by: Douglas Anderson > > > Signed-off-by: Kieran Bingham > > > --- > > > Changes since v1: > > > > > > - The connector .get_modes() operation doesn't rely on EDID anymore, > > > __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged > > > together > > > > > > Notes from Kieran: > > > > > > RB Tags collected from: > > > > > > https://lore.kernel.org/all/20210322030128.2283-9-laurent.pinchart+rene...@ideasonboard.com/ > > > > > > However this was over a year ago, so let me know if other patches now > > > superceed this one or otherwise invalidate this update. > > > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > index c55848588123..ffb6c04f6c46 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > @@ -1154,6 +1154,19 @@ static void ti_sn_bridge_post_disable(struct > > > drm_bridge *bridge) > > > pm_runtime_put_sync(pdata->dev); > > > } > > > > > > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > > > + struct drm_connector *connector) > > > +{ > > > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > > + struct edid *edid; > > > + > > > + pm_runtime_get_sync(pdata->dev); > > > + edid = drm_get_edid(connector, &pdata->aux.ddc); > > > + pm_runtime_put_autosuspend(pdata->dev); > > > + > > > + return edid; > > > +} > > > + > > > static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > > > .attach = ti_sn_bridge_attach, > > > .detach = ti_sn_bridge_detach, > > > @@ -1162,6 +1175,7 @@ static const struct drm_bridge_funcs > > > ti_sn_bridge_funcs = { > > > .enable = ti_sn_bridge_enable, > > > .disable = ti_sn_bridge_disable, > > > .post_disable = ti_sn_bridge_post_disable, > > > + .get_edid = ti_sn_bridge_get_edid, > > > }; > > > > > > static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, > > > @@ -1248,6 +1262,8 @@ static int ti_sn_bridge_probe(struct > > > auxiliary_device *adev, > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > pdata->bridge.of_node = np; > > > + pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > + pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; > > > > This doesn't look right to me. In the eDP case the EDID reading is > > driven by the panel. > > Now that I have the optional connector working based on Sam's series I > think this is the last issue to solve before reposting the DP/HPD > support. > > Are you saying that the bridge.ops should only set DRM_BRIDGE_OP_EDID > when pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort? Yes. -Doug
Re: [PATCH] drm/rockchip: remove redundant assignment to pointer connector
Hi, looks like I wasn't in the original recipient list, so only got Nick's answer. Am Mittwoch, 9. März 2022, 00:10:31 CET schrieb Nick Desaulniers: > On Mon, Mar 7, 2022 at 10:17 AM Colin Ian King wrote: > > > > The pointer connector is being assigned a value that is never read, > > it is being re-assigned in the following statement. The assignment > > is redundant and can be removed. > > > > Cleans up clang scan build warning: > > drivers/gpu/drm/rockchip/rockchip_rgb.c:153:2: warning: Value stored > > to 'connector' is never read [deadcode.DeadStores] > > + Author & reviewer of: > Fixes: 2e87bf389e13 ("drm/rockchip: add DRM_BRIDGE_ATTACH_NO_CONNECTOR > flag to drm_bridge_attach") > > > > > Signed-off-by: Colin Ian King > > --- > > drivers/gpu/drm/rockchip/rockchip_rgb.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c > > b/drivers/gpu/drm/rockchip/rockchip_rgb.c > > index 2494b079489d..92a727931a49 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c > > @@ -150,7 +150,6 @@ struct rockchip_rgb *rockchip_rgb_init(struct device > > *dev, > > if (ret) > > goto err_free_encoder; > > > > - connector = &rgb->connector; > > connector = drm_bridge_connector_init(rgb->drm_dev, encoder); I don't think this will work as expected. Yes, the whole thing looks a bit broken right now, but the connector field in the rockchip_rgb struct still exists and rockchip_rgb_fini also still uses it when calling drm_connector_cleanup. Same issue seems to exist in in rockchip_lvds.c with drm_connector_cleanup it seems. I guess drm_bridge_connector_destroy() is responsible for the cleanup so the drm_connector_cleanup call both in rockchip_rgb and rockchip_lvds as well as the local connector elements can go away as well? Heiko > > if (IS_ERR(connector)) { > > DRM_DEV_ERROR(drm_dev->dev, > > -- > > 2.35.1 > > > > > > >
Re: [PATCH] drm: of: Properly try all possible cases for bridge/panel detection
Hi Paul, On Wed, Mar 09, 2022 at 03:32:00PM +0100, Paul Kocialkowski wrote: > While bridge/panel detection was initially relying on the usual > port/ports-based of graph detection, it was recently changed to > perform the lookup on any child node that is not port/ports > instead when such a node is available, with no fallback on the > usual way. > > This results in breaking detection when a child node is present > but does not contain any panel or bridge node, even when the > usual port/ports-based of graph is there. > > In order to support both situations properly, this commit reworks > the logic to try both options and not just one of the two: it will > only return -EPROBE_DEFER when both have failed. > > Signed-off-by: Paul Kocialkowski > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") Thanks, it's in pretty good shape now, but I have a few bike sheds to paint :) > --- > drivers/gpu/drm/drm_of.c | 93 +--- > 1 file changed, 49 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 9d90cd75c457..67f1b7dfc892 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -219,6 +219,35 @@ int drm_of_encoder_active_endpoint(struct device_node > *node, > } > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > +static int drm_of_find_remote_panel_or_bridge(struct device_node *remote, > + struct drm_panel **panel, > + struct drm_bridge **bridge) This function performs its look up directly on the struct device_node passed as argument, so I don't think the "remote" in the name is great. Since it's static, we can just call it find_panel_or_bridge, what do you think? > +{ > + int ret = -EPROBE_DEFER; > + > + if (panel) { > + *panel = of_drm_find_panel(remote); > + if (!IS_ERR(*panel)) > + ret = 0; return 0? > + else > + *panel = NULL; > + > + } > + > + /* No panel found yet, check for a bridge next. */ > + if (bridge) { > + if (ret) { And the return above allows to remove that test > + *bridge = of_drm_find_bridge(remote); > + if (*bridge) > + ret = 0; return 0? > + } else { > + *bridge = NULL; > + } > + > + } > + > + return ret; And here we can just return -EPROBE_DEFER > +} > + > /** > * drm_of_find_panel_or_bridge - return connected panel or bridge device > * @np: device tree node containing encoder output ports > @@ -249,57 +278,33 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > if (panel) > *panel = NULL; > > - /** > - * Devices can also be child nodes when we also control that device > - * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > - * > - * Lookup for a child node of the given parent that isn't either port > - * or ports. > - */ > - for_each_available_child_of_node(np, remote) { > - if (of_node_name_eq(remote, "port") || > - of_node_name_eq(remote, "ports")) > - continue; > - > - goto of_find_panel_or_bridge; > + /* Check for a graph on the device node first. */ > + if (of_graph_is_present(np)) { > + remote = of_graph_get_remote_node(np, port, endpoint); > + if (remote) { > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > + bridge); > + of_node_put(remote); > + } > } > > - /* > - * of_graph_get_remote_node() produces a noisy error message if port > - * node isn't found and the absence of the port is a legit case here, > - * so at first we silently check whether graph presents in the > - * device-tree node. > - */ > - if (!of_graph_is_present(np)) > - return -ENODEV; > - > - remote = of_graph_get_remote_node(np, port, endpoint); > - > -of_find_panel_or_bridge: > - if (!remote) > - return -ENODEV; > + /* Otherwise check for any child node other than port/ports. */ > + if (ret) { > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > > - if (panel) { > - *panel = of_drm_find_panel(remote); > - if (!IS_ERR(*panel)) > - ret = 0; > - else > - *panel = NULL; > - } > + ret = drm_of_find_remote_panel_or_bridge(remote, panel, > +
[PATCH v3 0/3] drm/bridge: ti-sn65dsi86: Support non-eDP DisplayPort connectors
Implement support for non eDP connectors on the TI-SN65DSI86 bridge, and provide IRQ based hotplug detect to identify when the connector is present. no-hpd is extended to be the default behaviour for non DisplayPort connectors. This series is based upon Sam Ravnborgs and Rob Clarks series [0] to support DRM_BRIDGE_STATE_OPS and NO_CONNECTOR support on the SN65DSI86, however some extra modifications have been made on the top of Sam's series to fix compile breakage and the NO_CONNECTOR support. A full branch with these changes is available at [1] [0] https://lore.kernel.org/all/20220206154405.124-1-...@ravnborg.org/ [1] git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git branch: kbingham/drm-misc/next/sn65dsi86/hpd Kieran Bingham (1): drm/bridge: ti-sn65dsi86: Support hotplug detection Laurent Pinchart (2): drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode drm/bridge: ti-sn65dsi86: Implement bridge connector operations drivers/gpu/drm/bridge/ti-sn65dsi86.c | 180 +++--- 1 file changed, 165 insertions(+), 15 deletions(-) -- 2.32.0
[PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
From: Laurent Pinchart Despite the SN65DSI86 being an eDP bridge, on some systems its output is routed to a DisplayPort connector. Enable DisplayPort mode when the next component in the display pipeline is detected as a DisplayPort connector, and disable eDP features in that case. Signed-off-by: Laurent Pinchart Reworked to set bridge type based on the next bridge/connector. Signed-off-by: Kieran Bingham -- Changes since v1/RFC: - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to devm_drm_of_get_bridge" - eDP/DP mode determined from the next bridge connector type. Changes since v2: - Remove setting of Standard DP Scrambler Seed. (It's read-only). - Prevent setting DP_EDP_CONFIGURATION_SET in ti_sn_bridge_atomic_enable() - Use Doug's suggested text for disabling ASSR on DP mode. drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 --- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index c892ecba91c7..93b54fcba8ba 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -62,6 +62,7 @@ #define SN_LN_ASSIGN_REG 0x59 #define LN_ASSIGN_WIDTH 2 #define SN_ENH_FRAME_REG 0x5A +#define ASSR_CONTROL BIT(0) #define VSTREAM_ENABLEBIT(3) #define LN_POLRS_OFFSET 4 #define LN_POLRS_MASK 0xf0 @@ -93,6 +94,8 @@ #define SN_DATARATE_CONFIG_REG 0x94 #define DP_DATARATE_MASK GENMASK(7, 5) #define DP_DATARATE(x)((x) << 5) +#define SN_TRAINING_SETTING_REG0x95 +#define SCRAMBLE_DISABLE BIT(4) #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) @@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, goto exit; } + /* +* eDP panels use an Alternate Scrambler Seed compared to displays +* hooked up via a full DisplayPort connector. SN65DSI86 only supports +* the alternate scrambler seed, not the normal one, so the only way we +* can support full DisplayPort displays is by fully turning off the +* scrambler. +*/ + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); + /* * We'll try to link train several times. As part of link training * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If @@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, /* * The SN65DSI86 only supports ASSR Display Authentication method and -* this method is enabled by default. An eDP panel must support this +* this method is enabled for eDP panels. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel * at DisplayPort address 0x0010A prior to link training. */ - drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, - DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); + if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP) + drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, + DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); /* Set the DP output format (18 bpp or 24 bpp) */ val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0; @@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev, pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = np; + pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort + ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP; drm_bridge_add(&pdata->bridge); -- 2.32.0
[PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
When the SN65DSI86 is used in DisplayPort mode, its output is likely routed to a DisplayPort connector, which can benefit from hotplug detection. Support it in such cases, with polling mode only for now. The implementation is limited to the bridge operations, as the connector operations are legacy and new users should use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Signed-off-by: Laurent Pinchart Signed-off-by: Kieran Bingham --- Changes since v1: - Document the no_hpd field - Rely on the SN_HPD_DISABLE_REG default value in the HPD case - Add a TODO comment regarding IRQ support [Kieran] - Fix spelling s/assrted/asserted/ - Only enable HPD on DisplayPort connector. - Support IRQ based hotplug detect Changes since v2: [Kieran] - Use unsigned int for values read by regmap - Update HPD support warning message - Only enable OP_HPD if IRQ support enabled. - Only register IRQ handler during ti_sn_bridge_probe() - Store IRQ in the struct ti_sn65dsi86 - Register IRQ only when !no-hpd - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++--- 1 file changed, 129 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d581c820e5d8..328a48f09f97 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -70,6 +70,7 @@ #define BPP_18_RGBBIT(0) #define SN_HPD_DISABLE_REG 0x5C #define HPD_DISABLE BIT(0) +#define HPD_DEBOUNCED_STATE BIT(4) #define SN_GPIO_IO_REG 0x5E #define SN_GPIO_INPUT_SHIFT 4 #define SN_GPIO_OUTPUT_SHIFT 0 @@ -106,10 +107,24 @@ #define SN_PWM_EN_INV_REG 0xA5 #define SN_PWM_INV_MASK BIT(0) #define SN_PWM_EN_MASKBIT(1) +#define SN_IRQ_EN_REG 0xE0 +#define IRQ_ENBIT(0) +#define SN_IRQ_HPD_REG 0xE6 +#define IRQ_HPD_ENBIT(0) +#define IRQ_HPD_INSERTION_EN BIT(1) +#define IRQ_HPD_REMOVAL_ENBIT(2) +#define IRQ_HPD_REPLUG_EN BIT(3) +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5) #define SN_AUX_CMD_STATUS_REG 0xF4 #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6) +#define SN_IRQ_HPD_STATUS_REG 0xF5 +#define IRQ_HPD_STATUSBIT(0) +#define IRQ_HPD_INSERTION_STATUS BIT(1) +#define IRQ_HPD_REMOVAL_STATUSBIT(2) +#define IRQ_HPD_REPLUG_STATUS BIT(3) +#define IRQ_PLL_UNLOCKBIT(5) #define MIN_DSI_CLK_FREQ_MHZ 40 @@ -168,6 +183,12 @@ * @pwm_enabled: Used to track if the PWM signal is currently enabled. * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM. * @pwm_refclk_freq: Cache for the reference clock input to the PWM. + * + * @no_hpd: Disable hot-plug detection as instructed by device tree (used + *for instance for eDP panels whose HPD signal won't be asserted + *until the panel is turned on, and is thus not usable for + *downstream device detection). + * @irq: IRQ number for the device. */ struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; @@ -202,6 +223,9 @@ struct ti_sn65dsi86 { atomic_tpwm_pin_busy; #endif unsigned intpwm_refclk_freq; + + boolno_hpd; + int irq; }; static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) ti_sn_bridge_set_refclk_freq(pdata); /* -* HPD on this bridge chip is a bit useless. This is an eDP bridge -* so the HPD is an internal signal that's only there to signal that -* the panel is done powering up. ...but the bridge chip debounces -* this signal by between 100 ms and 400 ms (depending on process, -* voltage, and temperate--I measured it at about 200 ms). One +* As this is an eDP bridge, the output will be connected to a fixed +* panel in most systems. HPD is in that case only an internal signal +* to signal that the panel is done powering up. The bridge chip +* debounces this signal by between 100 ms and 400 ms (depending on +* process, voltage, and temperate--I measured it at about 200 ms). One * particular panel asserted HPD 84 ms after it was powered
[PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
From: Laurent Pinchart Implement the bridge connector-related .get_edid() operation, and report the related bridge capabilities and type. Signed-off-by: Laurent Pinchart Signed-off-by: Kieran Bingham --- Changes since v1: - The connector .get_modes() operation doesn't rely on EDID anymore, __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged together - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS Changes since v2: [Kieran] - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes. drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 + 1 file changed, 17 insertions(+) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 93b54fcba8ba..d581c820e5d8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge, pm_runtime_put_sync(pdata->dev); } +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, + struct drm_connector *connector) +{ + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct edid *edid; + + pm_runtime_get_sync(pdata->dev); + edid = drm_get_edid(connector, &pdata->aux.ddc); + pm_runtime_put_autosuspend(pdata->dev); + + return edid; +} + static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .attach = ti_sn_bridge_attach, .detach = ti_sn_bridge_detach, .mode_valid = ti_sn_bridge_mode_valid, + .get_edid = ti_sn_bridge_get_edid, .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, .atomic_enable = ti_sn_bridge_atomic_enable, .atomic_disable = ti_sn_bridge_atomic_disable, @@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev, pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP; + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) + pdata->bridge.ops = DRM_BRIDGE_OP_EDID; + drm_bridge_add(&pdata->bridge); ret = ti_sn_attach_host(pdata); -- 2.32.0
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: > > > > Am 09.03.22 um 19:12 schrieb Rob Clark: > > On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > > wrote: > >> From: Shashank Sharma > >> > >> This patch adds a new sysfs event, which will indicate > >> the userland about a GPU reset, and can also provide > >> some information like: > >> - process ID of the process involved with the GPU reset > >> - process name of the involved process > >> - the GPU status info (using flags) > >> > >> This patch also introduces the first flag of the flags > >> bitmap, which can be appended as and when required. > > Why invent something new, rather than using the already existing > > devcoredump? > > Yeah, that's a really valid question. > > > I don't think we need (or should encourage/allow) something drm > > specific when there is already an existing solution used by both drm > > and non-drm drivers. Userspace should not have to learn to support > > yet another mechanism to do the same thing. > > Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. BR, -R [1] One small change to allow-list the drm/msm devcore dumps was needed after a privacy review/audit of what is contained in the GPU devcored to ensure it does not contain any PII .. for CrOS on amd devices I'd be happy to walk whoever deals with amd CrOS devices through the process offline. > Regards, > Christian. > > > > > BR, > > -R >
Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
Quoting Kieran Bingham (2022-03-10 15:22:27) > When the SN65DSI86 is used in DisplayPort mode, its output is likely > routed to a DisplayPort connector, which can benefit from hotplug > detection. Support it in such cases, with polling mode only for now. > > The implementation is limited to the bridge operations, as the connector > operations are legacy and new users should use > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > Signed-off-by: Laurent Pinchart > Signed-off-by: Kieran Bingham > --- > Changes since v1: > > - Document the no_hpd field > - Rely on the SN_HPD_DISABLE_REG default value in the HPD case > - Add a TODO comment regarding IRQ support > [Kieran] > - Fix spelling s/assrted/asserted/ > - Only enable HPD on DisplayPort connector. > - Support IRQ based hotplug detect > > Changes since v2: [Kieran] > - Use unsigned int for values read by regmap > - Update HPD support warning message > - Only enable OP_HPD if IRQ support enabled. > - Only register IRQ handler during ti_sn_bridge_probe() > - Store IRQ in the struct ti_sn65dsi86 > - Register IRQ only when !no-hpd > - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++--- > 1 file changed, 129 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index d581c820e5d8..328a48f09f97 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -70,6 +70,7 @@ > #define BPP_18_RGBBIT(0) > #define SN_HPD_DISABLE_REG 0x5C > #define HPD_DISABLE BIT(0) > +#define HPD_DEBOUNCED_STATE BIT(4) > #define SN_GPIO_IO_REG 0x5E > #define SN_GPIO_INPUT_SHIFT 4 > #define SN_GPIO_OUTPUT_SHIFT 0 > @@ -106,10 +107,24 @@ > #define SN_PWM_EN_INV_REG 0xA5 > #define SN_PWM_INV_MASK BIT(0) > #define SN_PWM_EN_MASKBIT(1) > +#define SN_IRQ_EN_REG 0xE0 > +#define IRQ_ENBIT(0) > +#define SN_IRQ_HPD_REG 0xE6 > +#define IRQ_HPD_ENBIT(0) > +#define IRQ_HPD_INSERTION_EN BIT(1) > +#define IRQ_HPD_REMOVAL_ENBIT(2) > +#define IRQ_HPD_REPLUG_EN BIT(3) > +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5) > #define SN_AUX_CMD_STATUS_REG 0xF4 > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) > #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6) > +#define SN_IRQ_HPD_STATUS_REG 0xF5 > +#define IRQ_HPD_STATUSBIT(0) > +#define IRQ_HPD_INSERTION_STATUS BIT(1) > +#define IRQ_HPD_REMOVAL_STATUSBIT(2) > +#define IRQ_HPD_REPLUG_STATUS BIT(3) > +#define IRQ_PLL_UNLOCKBIT(5) > > #define MIN_DSI_CLK_FREQ_MHZ 40 > > @@ -168,6 +183,12 @@ > * @pwm_enabled: Used to track if the PWM signal is currently enabled. > * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM. > * @pwm_refclk_freq: Cache for the reference clock input to the PWM. > + * > + * @no_hpd: Disable hot-plug detection as instructed by device tree > (used > + *for instance for eDP panels whose HPD signal won't be > asserted > + *until the panel is turned on, and is thus not usable for > + *downstream device detection). > + * @irq: IRQ number for the device. > */ > struct ti_sn65dsi86 { > struct auxiliary_device bridge_aux; > @@ -202,6 +223,9 @@ struct ti_sn65dsi86 { > atomic_tpwm_pin_busy; > #endif > unsigned intpwm_refclk_freq; > + > + boolno_hpd; > + int irq; > }; > > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { > @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct > ti_sn65dsi86 *pdata) > ti_sn_bridge_set_refclk_freq(pdata); > > /* > -* HPD on this bridge chip is a bit useless. This is an eDP bridge > -* so the HPD is an internal signal that's only there to signal that > -* the panel is done powering up. ...but the bridge chip debounces > -* this signal by between 100 ms and 400 ms (depending on process, > -* voltage, and temperate--I measured it at about 200 ms). One > +* As this is an eDP bridge, the output will be connected to a fixed > +* panel in most systems. HPD is in that case only an internal signal > +* to signal that the pan
Re: [PATCH] drm: remove min_order BUG_ON check
On 10/03/2022 14:47, Arunpravin wrote: On 08/03/22 10:31 pm, Matthew Auld wrote: On 08/03/2022 13:59, Arunpravin wrote: On 07/03/22 10:11 pm, Matthew Auld wrote: On 07/03/2022 14:37, Arunpravin wrote: place BUG_ON(order < min_order) outside do..while loop as it fails Unigine Heaven benchmark. Unigine Heaven has buffer allocation requests for example required pages are 161 and alignment request is 128. To allocate the remaining 33 pages, continues the iteration to find the order value which is 5 and when it compares with min_order = 7, enables the BUG_ON(). To avoid this problem, placed the BUG_ON check outside of do..while loop. Signed-off-by: Arunpravin --- drivers/gpu/drm/drm_buddy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..ed94c56b720f 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, order = fls(pages) - 1; min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); + BUG_ON(order < min_order); Isn't the issue that we are allowing a size that is not aligned to the requested min_page_size? Should we not fix the caller(and throw a normal error here), or perhaps add the round_up() here instead? CASE 1: when size is not aligned to the requested min_page_size, for instance, required size = 161 pages, min_page_size = 128 pages, here we have 3 possible options, a. AFAIK,This kind of situation is common in any workload,the first allocation (i.e) 128 pages is aligned to min_page_size, Should we just allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does know the left over pages are not in min_page_size alignment? So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some arbitrary physical alignment for an object? Is that not meant to apply to every page/chunk? The above example would only have the correct physical alignment guaranteed for the first chunk, or so, is this the expected ABI behaviour? I gone through the function amdgpu_gem_create_ioctl(), it reads the physical alignment in bytes from userspace, does i915 round up the size value to the alignment or does i915 fails the allocation request if size is not aligned with min_page_size? If not, I think running unigine heaven or similar benchmark triggers BUG_ON() on current version of drm buddy i915 will always round_up the obj->base.size as per the default_page_size. But in our case the default_page_size is selected by the kernel, which is always either PAGE_SIZE, or 64K on some platforms, due to the HW having some minimum GPU page-size for mapping VRAM pages. We don't currently have anything similar to amdgpu_gem_create_in.alignment, where userspace can request some arbitrary physical alignment. Also looking at this some more, the other related bug here is the order-- == min_order check, since it now won't bail when order == 0, leading to order = -1, if we are unlucky... will add a fix Originally, if asking for min_page_size > chunk_size, then the allocation was meant to fail if it can't fill the resource request with pages of at least that size(and also alignment). Or at least that was the original meaning in i915 IIRC. we can follow the same here too, failing the allocation request if size is not aligned with min_page_size? Yeah, seems reasonable to me. I added a debug print for requested num_pages from userspace and its alignment request and executed unigine heaven, I see many such instances where min_page_size is not aligned to the size, how i915 handles such requests? b. There are many such instances in unigine heaven workload (there would be many such workloads), throwing a normal error would lower the FPS? is it possible to fix at caller application? c. adding the round_up() is possible, but in every such instances we end up allocating extra unused memory. For example, if required pages = 1028 and min_page_size = 1024 pages, we end up round up of left over 4 pages to the min_page_size, so the total size would be 2048 pages. i.e if someone does: alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags); CASE 2: I think this case should be detected (i.e) when min_page_size > size, should we return -EINVAL? This will still trigger the BUG_ON() even if we move it out of the loop, AFAICT. Should we just allow the CASE 1 proceed for the allocation and return -EINVAL for the CASE 2? + do { order = min(order, (unsigned int)fls(pages) - 1); BUG_ON(order > mm->max_order); - BUG_ON(order < min_order); do { if (flags & DRM_BUDDY_RANGE_ALLOCATION) base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
Re: [PATCH v3 1/3] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Hi Kieran, Thank you for the patch. On Thu, Mar 10, 2022 at 03:22:25PM +, Kieran Bingham wrote: > From: Laurent Pinchart > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > routed to a DisplayPort connector. Enable DisplayPort mode when the next > component in the display pipeline is detected as a DisplayPort > connector, and disable eDP features in that case. > > Signed-off-by: Laurent Pinchart > Reworked to set bridge type based on the next bridge/connector. > Signed-off-by: Kieran Bingham Reviewed-by: Laurent Pinchart (I know I'm listed as the author, but your changes look good :-)) > -- > Changes since v1/RFC: > - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to >devm_drm_of_get_bridge" > - eDP/DP mode determined from the next bridge connector type. > > Changes since v2: > - Remove setting of Standard DP Scrambler Seed. (It's read-only). > - Prevent setting DP_EDP_CONFIGURATION_SET in >ti_sn_bridge_atomic_enable() > - Use Doug's suggested text for disabling ASSR on DP mode. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 23 --- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index c892ecba91c7..93b54fcba8ba 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -62,6 +62,7 @@ > #define SN_LN_ASSIGN_REG 0x59 > #define LN_ASSIGN_WIDTH 2 > #define SN_ENH_FRAME_REG 0x5A > +#define ASSR_CONTROLBIT(0) > #define VSTREAM_ENABLE BIT(3) > #define LN_POLRS_OFFSET 4 > #define LN_POLRS_MASK 0xf0 > @@ -93,6 +94,8 @@ > #define SN_DATARATE_CONFIG_REG 0x94 > #define DP_DATARATE_MASKGENMASK(7, 5) > #define DP_DATARATE(x) ((x) << 5) > +#define SN_TRAINING_SETTING_REG 0x95 > +#define SCRAMBLE_DISABLEBIT(4) > #define SN_ML_TX_MODE_REG0x96 > #define ML_TX_MAIN_LINK_OFF 0 > #define ML_TX_NORMAL_MODE BIT(0) > @@ -982,6 +985,17 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > *pdata, int dp_rate_idx, > goto exit; > } > > + /* > + * eDP panels use an Alternate Scrambler Seed compared to displays > + * hooked up via a full DisplayPort connector. SN65DSI86 only supports > + * the alternate scrambler seed, not the normal one, so the only way we > + * can support full DisplayPort displays is by fully turning off the > + * scrambler. > + */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > +SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > + > /* >* We'll try to link train several times. As part of link training >* the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If > @@ -1046,12 +1060,13 @@ static void ti_sn_bridge_atomic_enable(struct > drm_bridge *bridge, > > /* >* The SN65DSI86 only supports ASSR Display Authentication method and > - * this method is enabled by default. An eDP panel must support this > + * this method is enabled for eDP panels. An eDP panel must support this >* authentication method. We need to enable this method in the eDP panel >* at DisplayPort address 0x0010A prior to link training. >*/ > - drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > -DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP) > + drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > +DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > /* Set the DP output format (18 bpp or 24 bpp) */ > val = (ti_sn_bridge_get_bpp(old_bridge_state) == 18) ? BPP_18_RGB : 0; > @@ -1215,6 +1230,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device > *adev, > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > pdata->bridge.of_node = np; > + pdata->bridge.type = pdata->next_bridge->type == > DRM_MODE_CONNECTOR_DisplayPort > +? DRM_MODE_CONNECTOR_DisplayPort : > DRM_MODE_CONNECTOR_eDP; > > drm_bridge_add(&pdata->bridge); > -- Regards, Laurent Pinchart
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. - Shashank BR, -R [1] One small change to allow-list the drm/msm devcore dumps was needed after a privacy review/audit of what is contained in the GPU devcored to ensure it does not contain any PII .. for CrOS on amd devices I'd be happy to walk whoever deals with amd CrOS devices through the process offline. Regards, Christian. BR, -R
Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
Hi Kieran, Thank you for the patch. On Thu, Mar 10, 2022 at 03:22:26PM +, Kieran Bingham wrote: > From: Laurent Pinchart > > Implement the bridge connector-related .get_edid() operation, and report > the related bridge capabilities and type. > > Signed-off-by: Laurent Pinchart > Signed-off-by: Kieran Bingham > --- > Changes since v1: > > - The connector .get_modes() operation doesn't rely on EDID anymore, > __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged > together > - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS > > Changes since v2: [Kieran] > - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 + > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 93b54fcba8ba..d581c820e5d8 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct > drm_bridge *bridge, > pm_runtime_put_sync(pdata->dev); > } > > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > + struct edid *edid; > + > + pm_runtime_get_sync(pdata->dev); > + edid = drm_get_edid(connector, &pdata->aux.ddc); > + pm_runtime_put_autosuspend(pdata->dev); > + > + return edid; > +} > + > static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > .attach = ti_sn_bridge_attach, > .detach = ti_sn_bridge_detach, > .mode_valid = ti_sn_bridge_mode_valid, > + .get_edid = ti_sn_bridge_get_edid, > .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, > .atomic_enable = ti_sn_bridge_atomic_enable, > .atomic_disable = ti_sn_bridge_atomic_disable, > @@ -1233,6 +1247,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device > *adev, > pdata->bridge.type = pdata->next_bridge->type == > DRM_MODE_CONNECTOR_DisplayPort > ? DRM_MODE_CONNECTOR_DisplayPort : > DRM_MODE_CONNECTOR_eDP; > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + pdata->bridge.ops = DRM_BRIDGE_OP_EDID; You could write this as if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) pdata->bridge.ops |= DRM_BRIDGE_OP_EDID; to be ready to support other ops, but that doesn't help DRM_BRIDGE_OP_DETECT in 3/3, and I don't foresee the need for other ops. In any case, Reviewed-by: Laurent Pinchart > + > drm_bridge_add(&pdata->bridge); > > ret = ti_sn_attach_host(pdata); -- Regards, Laurent Pinchart
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 2022-03-10 11:21, Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. - Shashank Another point I raised in another thread is that it looks like we might want to use devcoredump during ASIC reset to dump more HW related data which is useful for debugging. It means the use client will have to extract the pid and process name out from a bigger data set - Is that ok ? We can probably put it at the beginning for easiest parsing. Andrey BR, -R [1] One small change to allow-list the drm/msm devcore dumps was needed after a privacy review/audit of what is contained in the GPU devcored to ensure it does not contain any PII .. for CrOS on amd devices I'd be happy to walk whoever deals with amd CrOS devices through the process offline. Regards, Christian. BR, -R
Re: [PATCH 1/2] dt-bindings: drm: bridge: adi,adv7533: Document adi,disable-lanes-override property
Hi Biju, Thank you for the patch. On Wed, Mar 09, 2022 at 03:11:08PM +, Biju Das wrote: > On Renesas RZ/{G2L,V2L} platforms changing the lanes from 4 to 3 at > lower frequencies causes display instability. On such platforms, it > is better to avoid switching lanes from 4 to 3 as it needs different > set of PLL parameter constraints to make the display stable with 3 > lanes. Is this because the PLL calculation code doesn't work properly, or because the hardware can't support this ? > This patch introduces 'adi,disable-lanes-override' property to disable > lane switching at lower frequencies. > > Signed-off-by: Biju Das > Reviewed-by: Lad Prabhakar > --- > .../devicetree/bindings/display/bridge/adi,adv7533.yaml | 5 + > 1 file changed, 5 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > index f36209137c8a..2dc378039d21 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > @@ -84,6 +84,11 @@ properties: >timings for HDMI output. > type: boolean > > + adi,disable-lanes-override: > +description: > + Disables the overriding lanes at lower frequencies. > +type: boolean > + >adi,dsi-lanes: > description: Number of DSI data lanes connected to the DSI host. > $ref: /schemas/types.yaml#/definitions/uint32 -- Regards, Laurent Pinchart
Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
Hi Kieran, Thank you for the patch. On Thu, Mar 10, 2022 at 03:22:27PM +, Kieran Bingham wrote: > When the SN65DSI86 is used in DisplayPort mode, its output is likely > routed to a DisplayPort connector, which can benefit from hotplug > detection. Support it in such cases, with polling mode only for now. Don't we support IRQ mode too now ? > The implementation is limited to the bridge operations, as the connector > operations are legacy and new users should use > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > Signed-off-by: Laurent Pinchart > Signed-off-by: Kieran Bingham > --- > Changes since v1: > > - Document the no_hpd field > - Rely on the SN_HPD_DISABLE_REG default value in the HPD case > - Add a TODO comment regarding IRQ support > [Kieran] > - Fix spelling s/assrted/asserted/ > - Only enable HPD on DisplayPort connector. > - Support IRQ based hotplug detect > > Changes since v2: [Kieran] > - Use unsigned int for values read by regmap > - Update HPD support warning message > - Only enable OP_HPD if IRQ support enabled. > - Only register IRQ handler during ti_sn_bridge_probe() > - Store IRQ in the struct ti_sn65dsi86 > - Register IRQ only when !no-hpd > - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++--- > 1 file changed, 129 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index d581c820e5d8..328a48f09f97 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -70,6 +70,7 @@ > #define BPP_18_RGB BIT(0) > #define SN_HPD_DISABLE_REG 0x5C > #define HPD_DISABLE BIT(0) > +#define HPD_DEBOUNCED_STATE BIT(4) > #define SN_GPIO_IO_REG 0x5E > #define SN_GPIO_INPUT_SHIFT 4 > #define SN_GPIO_OUTPUT_SHIFT0 > @@ -106,10 +107,24 @@ > #define SN_PWM_EN_INV_REG0xA5 > #define SN_PWM_INV_MASK BIT(0) > #define SN_PWM_EN_MASK BIT(1) > +#define SN_IRQ_EN_REG0xE0 > +#define IRQ_EN BIT(0) > +#define SN_IRQ_HPD_REG 0xE6 > +#define IRQ_HPD_EN BIT(0) > +#define IRQ_HPD_INSERTION_ENBIT(1) > +#define IRQ_HPD_REMOVAL_EN BIT(2) > +#define IRQ_HPD_REPLUG_EN BIT(3) > +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5) > #define SN_AUX_CMD_STATUS_REG0xF4 > #define AUX_IRQ_STATUS_AUX_RPLY_TOUTBIT(3) > #define AUX_IRQ_STATUS_AUX_SHORTBIT(5) > #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6) > +#define SN_IRQ_HPD_STATUS_REG0xF5 > +#define IRQ_HPD_STATUS BIT(0) > +#define IRQ_HPD_INSERTION_STATUSBIT(1) > +#define IRQ_HPD_REMOVAL_STATUS BIT(2) > +#define IRQ_HPD_REPLUG_STATUS BIT(3) > +#define IRQ_PLL_UNLOCK BIT(5) > > #define MIN_DSI_CLK_FREQ_MHZ 40 > > @@ -168,6 +183,12 @@ > * @pwm_enabled: Used to track if the PWM signal is currently enabled. > * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM. > * @pwm_refclk_freq: Cache for the reference clock input to the PWM. > + * > + * @no_hpd: Disable hot-plug detection as instructed by device tree > (used > + *for instance for eDP panels whose HPD signal won't be > asserted > + *until the panel is turned on, and is thus not usable for > + *downstream device detection). > + * @irq: IRQ number for the device. > */ > struct ti_sn65dsi86 { > struct auxiliary_device bridge_aux; > @@ -202,6 +223,9 @@ struct ti_sn65dsi86 { > atomic_tpwm_pin_busy; > #endif > unsigned intpwm_refclk_freq; > + > + boolno_hpd; > + int irq; > }; > > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { > @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct > ti_sn65dsi86 *pdata) > ti_sn_bridge_set_refclk_freq(pdata); > > /* > - * HPD on this bridge chip is a bit useless. This is an eDP bridge > - * so the HPD is an internal signal that's only there to signal that > - * the panel is done powering up. ...but the bridge chip debounces > - * this signal by between 100 ms and 400 ms (depending on process, > - * voltage, and temperate--I measured it at about 200 ms). One > + * As this is an eDP bridge, the output will be connected to a fixed > + * panel in most sy
RE: [PATCH 1/2] dt-bindings: drm: bridge: adi,adv7533: Document adi,disable-lanes-override property
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH 1/2] dt-bindings: drm: bridge: adi,adv7533: Document > adi,disable-lanes-override property > > Hi Biju, > > Thank you for the patch. > > On Wed, Mar 09, 2022 at 03:11:08PM +, Biju Das wrote: > > On Renesas RZ/{G2L,V2L} platforms changing the lanes from 4 to 3 at > > lower frequencies causes display instability. On such platforms, it is > > better to avoid switching lanes from 4 to 3 as it needs different set > > of PLL parameter constraints to make the display stable with 3 lanes. > > Is this because the PLL calculation code doesn't work properly, or because > the hardware can't support this ? PLL Calculation is correct, that is the reason it works for all resolution with 4 lanes. There are 2 clocks generated by PLL5 which is connected to a mux with clock sources, namely 'FOUTPOSTDIV' and 'FOUT1PH0' This Mux is connected to DSI dividers. 'FOUTPOSTDIV' should be selected if (PLL_INPUT_FREQ/VCLK) is odd and 'FOUT1PH0' should be selected if it is even. The PLL calculation makes use of even selection('FOUT1PH0') and video works for all frequencies with 4 lanes. With 'FOUT1PH0' as clock source, if I switch to 3 lanes for lanes it doesn't work. But it work with 4lanes on all frequencies. HW can support 3 lanes, if I set parameter to make (PLL_INPUT_FREQ/VCLK) odd and select 'FOUTPOSTDIV' as clk source to DSI divider. I am not sure about the rational behind the constraint to Switch to 3 lanes for lower frequency for this ADV7535 chip, as on our platform it can work with 4lanes on all frequencies. Cheers, Biju > > > This patch introduces 'adi,disable-lanes-override' property to disable > > lane switching at lower frequencies. > > > > Signed-off-by: Biju Das > > Reviewed-by: Lad Prabhakar > > --- > > .../devicetree/bindings/display/bridge/adi,adv7533.yaml | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > > b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > > index f36209137c8a..2dc378039d21 100644 > > --- > > a/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7533.yam > > +++ l > > @@ -84,6 +84,11 @@ properties: > >timings for HDMI output. > > type: boolean > > > > + adi,disable-lanes-override: > > +description: > > + Disables the overriding lanes at lower frequencies. > > +type: boolean > > + > >adi,dsi-lanes: > > description: Number of DSI data lanes connected to the DSI host. > > $ref: /schemas/types.yaml#/definitions/uint32 > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: > > > > On 3/10/2022 4:24 PM, Rob Clark wrote: > > On Thu, Mar 10, 2022 at 1:55 AM Christian König > > wrote: > >> > >> > >> > >> Am 09.03.22 um 19:12 schrieb Rob Clark: > >>> On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > >>> wrote: > From: Shashank Sharma > > This patch adds a new sysfs event, which will indicate > the userland about a GPU reset, and can also provide > some information like: > - process ID of the process involved with the GPU reset > - process name of the involved process > - the GPU status info (using flags) > > This patch also introduces the first flag of the flags > bitmap, which can be appended as and when required. > >>> Why invent something new, rather than using the already existing > >>> devcoredump? > >> > >> Yeah, that's a really valid question. > >> > >>> I don't think we need (or should encourage/allow) something drm > >>> specific when there is already an existing solution used by both drm > >>> and non-drm drivers. Userspace should not have to learn to support > >>> yet another mechanism to do the same thing. > >> > >> Question is how is userspace notified about new available core dumps? > > > > I haven't looked into it too closely, as the CrOS userspace > > crash-reporter already had support for devcoredump, so it "just > > worked" out of the box[1]. I believe a udev event is what triggers > > the crash-reporter to go read the devcore dump out of sysfs. > > I had a quick look at the devcoredump code, and it doesn't look like > that is sending an event to the user, so we still need an event to > indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? BR, -R
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 8:27 AM Andrey Grodzovsky wrote: > > > On 2022-03-10 11:21, Sharma, Shashank wrote: > > > > > > On 3/10/2022 4:24 PM, Rob Clark wrote: > >> On Thu, Mar 10, 2022 at 1:55 AM Christian König > >> wrote: > >>> > >>> > >>> > >>> Am 09.03.22 um 19:12 schrieb Rob Clark: > On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > wrote: > > From: Shashank Sharma > > > > This patch adds a new sysfs event, which will indicate > > the userland about a GPU reset, and can also provide > > some information like: > > - process ID of the process involved with the GPU reset > > - process name of the involved process > > - the GPU status info (using flags) > > > > This patch also introduces the first flag of the flags > > bitmap, which can be appended as and when required. > Why invent something new, rather than using the already existing > devcoredump? > >>> > >>> Yeah, that's a really valid question. > >>> > I don't think we need (or should encourage/allow) something drm > specific when there is already an existing solution used by both drm > and non-drm drivers. Userspace should not have to learn to support > yet another mechanism to do the same thing. > >>> > >>> Question is how is userspace notified about new available core dumps? > >> > >> I haven't looked into it too closely, as the CrOS userspace > >> crash-reporter already had support for devcoredump, so it "just > >> worked" out of the box[1]. I believe a udev event is what triggers > >> the crash-reporter to go read the devcore dump out of sysfs. > > > > I had a quick look at the devcoredump code, and it doesn't look like > > that is sending an event to the user, so we still need an event to > > indicate a GPU reset. > > > > - Shashank > > > Another point I raised in another thread is that it looks like we might > want to use devcoredump during ASIC reset to dump more HW related data > which is useful > for debugging. It means the use client will have to extract the pid and > process name out from a bigger data set - Is that ok ? We can probably > put it at the beginning > for easiest parsing. > Yes, this is what we do for drm/msm.. the start of the devcore file has something like: kernel: 5.14.0-rc3-debug+ module: msm time: 1632763923.453207637 comm: deqp-gles3:sq0 cmdline: ./deqp-gles31 --deqp-surface-width=256 --deqp-surface-height=256 --deqp-gl-config-name=rgbad24s8ms0 --deqp-visibility=hidden --deqp-caselist-file=/home/robclark/src/deqp/build/modules/gles31/new-run/c33.r1.caselist.txt --deqp-log-filename=/home/robclark/src/deqp/build/modules/gles31/new-run/c33.r1.qpa --deqp-log-flush=disable --deqp-shadercache-filename=/home/robclark/src/deqp/build/modules/gles31/new-run/t499826814672.shader_cache --deqp-shadercache-truncate=disable revision: 618 (6.1.8.0) We capture quite a lot of state, cmdstream that triggered the hang, register/state dumps, microcontroller state, etc. But we do go out of our way to not capture textures or caches that might contain texture data by default (for privacy reasons) It has been hugely useful for debugging a few issues that happen rarely enough that they are difficult to reproduce. I guess that is crowd-sourced debugging ;-) BR, -R
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 6:10 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? Let me check that out as well, hope that is not due to a driver-private event for GPU reset, coz I think I have seen some of those in a few DRM drivers. - Shashank BR, -R
[PATCH v1 0/3] split vm_normal_pages for LRU and non-LRU handling
DEVICE_COHERENT pages introduce a subtle distinction in the way "normal" pages can be used by various callers throughout the kernel. They behave like normal pages for purposes of mapping in CPU page tables, and for COW. But they do not support LRU lists, NUMA migration or THP. Therefore we split vm_normal_page into two functions vm_normal_any_page and vm_normal_lru_page. The latter will only return pages that can be put on an LRU list and that support NUMA migration, KSM and THP. HMM tests were added to selftest to excercise these changes with device coherent pages. New test called hmm_cow_in_device, will test pages marked as COW, allocated in device zone. Also, more configurations were added into hmm_gup_test to test basic get user pages and get user pages fast paths in device zone pages. Alex Sierra (3): mm: split vm_normal_pages for LRU and non-LRU handling tools: add more gup configs to hmm_gup selftests tools: add selftests to hmm for COW in device memory fs/proc/task_mmu.c | 12 +-- include/linux/mm.h | 11 +- mm/gup.c | 10 +- mm/hmm.c | 2 +- mm/huge_memory.c | 2 +- mm/khugepaged.c| 8 +- mm/ksm.c | 4 +- mm/madvise.c | 4 +- mm/memcontrol.c| 2 +- mm/memory.c| 38 --- mm/mempolicy.c | 4 +- mm/migrate.c | 2 +- mm/migrate_device.c| 2 +- mm/mlock.c | 6 +- mm/mprotect.c | 2 +- tools/testing/selftests/vm/hmm-tests.c | 139 + 16 files changed, 185 insertions(+), 63 deletions(-) -- 2.32.0
[PATCH v1 2/3] tools: add more gup configs to hmm_gup selftests
Test device pages with get_user_pages and get_user_pages_fast. The motivation is to test device coherent type pages in the gup and gup fast paths, after vm_normal_pages was split into LRU and non-LRU handled. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling --- tools/testing/selftests/vm/hmm-tests.c | 65 +- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c index 11b83a8084fe..65e30ab6494c 100644 --- a/tools/testing/selftests/vm/hmm-tests.c +++ b/tools/testing/selftests/vm/hmm-tests.c @@ -1769,6 +1769,24 @@ TEST_F(hmm, exclusive_cow) hmm_buffer_free(buffer); } +static int gup_test_exec(int gup_fd, unsigned long addr, +int cmd, int npages, int size) +{ + struct gup_test gup = { + .nr_pages_per_call = npages, + .addr = addr, + .gup_flags = FOLL_WRITE, + .size = size, + }; + + if (ioctl(gup_fd, cmd, &gup)) { + perror("ioctl on error\n"); + return errno; + } + + return 0; +} + /* * Test get user device pages through gup_test. Setting PIN_LONGTERM flag. * This should trigger a migration back to system memory for both, private @@ -1779,7 +1797,6 @@ TEST_F(hmm, exclusive_cow) TEST_F(hmm, hmm_gup_test) { struct hmm_buffer *buffer; - struct gup_test gup; int gup_fd; unsigned long npages; unsigned long size; @@ -1792,8 +1809,7 @@ TEST_F(hmm, hmm_gup_test) if (gup_fd == -1) SKIP(return, "Skipping test, could not find gup_test driver"); - npages = 4; - ASSERT_NE(npages, 0); + npages = 3; size = npages << self->page_shift; buffer = malloc(sizeof(*buffer)); @@ -1822,28 +1838,35 @@ TEST_F(hmm, hmm_gup_test) for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i) ASSERT_EQ(ptr[i], i); - gup.nr_pages_per_call = npages; - gup.addr = (unsigned long)buffer->ptr; - gup.gup_flags = FOLL_WRITE; - gup.size = size; - /* -* Calling gup_test ioctl. It will try to PIN_LONGTERM these device pages -* causing a migration back to system memory for both, private and coherent -* type pages. -*/ - if (ioctl(gup_fd, PIN_LONGTERM_BENCHMARK, &gup)) { - perror("ioctl on PIN_LONGTERM_BENCHMARK\n"); - goto out_test; - } - - /* Take snapshot to make sure pages have been migrated to sys memory */ + ASSERT_EQ(gup_test_exec(gup_fd, + (unsigned long)buffer->ptr, + GUP_BASIC_TEST, 1, self->page_size), 0); + ASSERT_EQ(gup_test_exec(gup_fd, + (unsigned long)buffer->ptr + 1 * self->page_size, + GUP_FAST_BENCHMARK, 1, self->page_size), 0); + ASSERT_EQ(gup_test_exec(gup_fd, + (unsigned long)buffer->ptr + 2 * self->page_size, + PIN_LONGTERM_BENCHMARK, 1, self->page_size), 0); + + /* Take snapshot to CPU pagetables */ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_SNAPSHOT, buffer, npages); ASSERT_EQ(ret, 0); ASSERT_EQ(buffer->cpages, npages); m = buffer->mirror; - for (i = 0; i < npages; i++) - ASSERT_EQ(m[i], HMM_DMIRROR_PROT_WRITE); -out_test: + if (hmm_is_coherent_type(variant->device_number)) { + ASSERT_EQ(HMM_DMIRROR_PROT_DEV_COHERENT_LOCAL | HMM_DMIRROR_PROT_WRITE, m[0]); + ASSERT_EQ(HMM_DMIRROR_PROT_DEV_COHERENT_LOCAL | HMM_DMIRROR_PROT_WRITE, m[1]); + } else { + ASSERT_EQ(HMM_DMIRROR_PROT_WRITE, m[0]); + ASSERT_EQ(HMM_DMIRROR_PROT_WRITE, m[1]); + } + ASSERT_EQ(HMM_DMIRROR_PROT_WRITE, m[2]); + /* Check again the content on the pages. Make sure there's no +* corrupted data. +*/ + for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i) + ASSERT_EQ(ptr[i], i); + close(gup_fd); hmm_buffer_free(buffer); } -- 2.32.0
[PATCH v1 3/3] tools: add selftests to hmm for COW in device memory
The objective is to test device migration mechanism in pages marked as COW, for private and coherent device type. In case of writing to COW private page(s), a page fault will migrate pages back to system memory first. Then, these pages will be duplicated. In case of COW device coherent type, pages are duplicated directly from device memory. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling --- tools/testing/selftests/vm/hmm-tests.c | 80 ++ 1 file changed, 80 insertions(+) diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c index 65e30ab6494c..d70b780df877 100644 --- a/tools/testing/selftests/vm/hmm-tests.c +++ b/tools/testing/selftests/vm/hmm-tests.c @@ -1870,4 +1870,84 @@ TEST_F(hmm, hmm_gup_test) close(gup_fd); hmm_buffer_free(buffer); } + +/* + * Test copy-on-write in device pages. + * In case of writing to COW private page(s), a page fault will migrate pages + * back to system memory first. Then, these pages will be duplicated. In case + * of COW device coherent type, pages are duplicated directly from device + * memory. + */ +TEST_F(hmm, hmm_cow_in_device) +{ + struct hmm_buffer *buffer; + unsigned long npages; + unsigned long size; + unsigned long i; + int *ptr; + int ret; + unsigned char *m; + pid_t pid; + int status; + + npages = 4; + size = npages << self->page_shift; + + buffer = malloc(sizeof(*buffer)); + ASSERT_NE(buffer, NULL); + + buffer->fd = -1; + buffer->size = size; + buffer->mirror = malloc(size); + ASSERT_NE(buffer->mirror, NULL); + + buffer->ptr = mmap(NULL, size, + PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, + buffer->fd, 0); + ASSERT_NE(buffer->ptr, MAP_FAILED); + + /* Initialize buffer in system memory. */ + for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i) + ptr[i] = i; + + /* Migrate memory to device. */ + + ret = hmm_migrate_sys_to_dev(self->fd, buffer, npages); + ASSERT_EQ(ret, 0); + ASSERT_EQ(buffer->cpages, npages); + + pid = fork(); + if (pid == -1) + ASSERT_EQ(pid, 0); + if (!pid) { + /* Child process waitd for SIGTERM from the parent. */ + while (1) { + } + perror("Should not reach this\n"); + exit(0); + } + /* Parent process writes to COW pages(s) and gets a +* new copy in system. In case of device private pages, +* this write causes a migration to system mem first. +*/ + for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i) + ptr[i] = i; + + /* Terminate child and wait */ + EXPECT_EQ(0, kill(pid, SIGTERM)); + EXPECT_EQ(pid, waitpid(pid, &status, 0)); + EXPECT_NE(0, WIFSIGNALED(status)); + EXPECT_EQ(SIGTERM, WTERMSIG(status)); + + /* Take snapshot to CPU pagetables */ + ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_SNAPSHOT, buffer, npages); + ASSERT_EQ(ret, 0); + ASSERT_EQ(buffer->cpages, npages); + m = buffer->mirror; + for (i = 0; i < npages; i++) + ASSERT_EQ(HMM_DMIRROR_PROT_WRITE, m[i]); + + hmm_buffer_free(buffer); +} TEST_HARNESS_MAIN -- 2.32.0
[PATCH v1 1/3] mm: split vm_normal_pages for LRU and non-LRU handling
DEVICE_COHERENT pages introduce a subtle distinction in the way "normal" pages can be used by various callers throughout the kernel. They behave like normal pages for purposes of mapping in CPU page tables, and for COW. But they do not support LRU lists, NUMA migration or THP. Therefore we split vm_normal_page into two functions vm_normal_any_page and vm_normal_lru_page. The latter will only return pages that can be put on an LRU list and that support NUMA migration, KSM and THP. We also introduced a FOLL_LRU flag that adds the same behaviour to follow_page and related APIs, to allow callers to specify that they expect to put pages on an LRU list. Signed-off-by: Alex Sierra Acked-by: Felix Kuehling --- fs/proc/task_mmu.c | 12 ++-- include/linux/mm.h | 11 +++ mm/gup.c| 10 ++ mm/hmm.c| 2 +- mm/huge_memory.c| 2 +- mm/khugepaged.c | 8 mm/ksm.c| 4 ++-- mm/madvise.c| 4 ++-- mm/memcontrol.c | 2 +- mm/memory.c | 38 ++ mm/mempolicy.c | 4 ++-- mm/migrate.c| 2 +- mm/migrate_device.c | 2 +- mm/mlock.c | 6 +++--- mm/mprotect.c | 2 +- 15 files changed, 64 insertions(+), 45 deletions(-) diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index f46060eb91b5..cada3e702693 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -528,7 +528,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr, bool migration = false; if (pte_present(*pte)) { - page = vm_normal_page(vma, addr, *pte); + page = vm_normal_any_page(vma, addr, *pte); } else if (is_swap_pte(*pte)) { swp_entry_t swpent = pte_to_swp_entry(*pte); @@ -723,7 +723,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, struct page *page = NULL; if (pte_present(*pte)) { - page = vm_normal_page(vma, addr, *pte); + page = vm_normal_any_page(vma, addr, *pte); } else if (is_swap_pte(*pte)) { swp_entry_t swpent = pte_to_swp_entry(*pte); @@ -1077,7 +1077,7 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, return false; if (likely(!test_bit(MMF_HAS_PINNED, &vma->vm_mm->flags))) return false; - page = vm_normal_page(vma, addr, pte); + page = vm_normal_any_page(vma, addr, pte); if (!page) return false; return page_maybe_dma_pinned(page); @@ -1190,7 +1190,7 @@ static int clear_refs_pte_range(pmd_t *pmd, unsigned long addr, if (!pte_present(ptent)) continue; - page = vm_normal_page(vma, addr, ptent); + page = vm_normal_any_page(vma, addr, ptent); if (!page) continue; @@ -1402,7 +1402,7 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm, if (pm->show_pfn) frame = pte_pfn(pte); flags |= PM_PRESENT; - page = vm_normal_page(vma, addr, pte); + page = vm_normal_any_page(vma, addr, pte); if (pte_soft_dirty(pte)) flags |= PM_SOFT_DIRTY; if (pte_uffd_wp(pte)) @@ -1784,7 +1784,7 @@ static struct page *can_gather_numa_stats(pte_t pte, struct vm_area_struct *vma, if (!pte_present(pte)) return NULL; - page = vm_normal_page(vma, addr, pte); + page = vm_normal_lru_page(vma, addr, pte); if (!page) return NULL; diff --git a/include/linux/mm.h b/include/linux/mm.h index d507c32724c0..46b0bb43cef3 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -593,8 +593,8 @@ struct vm_operations_struct { unsigned long addr); #endif /* -* Called by vm_normal_page() for special PTEs to find the -* page for @addr. This is useful if the default behavior +* Called by vm_normal_*_page() for special PTEs to find the +* page for @addr. This is useful if the default behavior * (using pte_page()) would not find the correct page. */ struct page *(*find_special_page)(struct vm_area_struct *vma, @@ -1781,7 +1781,9 @@ static inline bool can_do_mlock(void) { return false; } extern int user_shm_lock(size_t, struct ucounts *); extern void user_shm_unlock(size_t, struct ucounts *); -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, +struct page *vm_normal_any_page(struct vm_area_struct *vma, unsigned long addr, +pte_t pte); +struct page *vm_normal_lru_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte); struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
Re: [PATCH v3 3/3] drm/bridge: ti-sn65dsi86: Support hotplug detection
Hi Laurent Quoting Laurent Pinchart (2022-03-10 16:42:48) > Hi Kieran, > > Thank you for the patch. > > On Thu, Mar 10, 2022 at 03:22:27PM +, Kieran Bingham wrote: > > When the SN65DSI86 is used in DisplayPort mode, its output is likely > > routed to a DisplayPort connector, which can benefit from hotplug > > detection. Support it in such cases, with polling mode only for now. > > Don't we support IRQ mode too now ? Ah yes, I missed this for an update. > > The implementation is limited to the bridge operations, as the connector > > operations are legacy and new users should use > > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > > > Signed-off-by: Laurent Pinchart > > Signed-off-by: Kieran Bingham > > --- > > Changes since v1: > > > > - Document the no_hpd field > > - Rely on the SN_HPD_DISABLE_REG default value in the HPD case > > - Add a TODO comment regarding IRQ support > > [Kieran] > > - Fix spelling s/assrted/asserted/ > > - Only enable HPD on DisplayPort connector. > > - Support IRQ based hotplug detect > > > > Changes since v2: [Kieran] > > - Use unsigned int for values read by regmap > > - Update HPD support warning message > > - Only enable OP_HPD if IRQ support enabled. > > - Only register IRQ handler during ti_sn_bridge_probe() > > - Store IRQ in the struct ti_sn65dsi86 > > - Register IRQ only when !no-hpd > > - Refactor DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD handling > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 142 +++--- > > 1 file changed, 129 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index d581c820e5d8..328a48f09f97 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -70,6 +70,7 @@ > > #define BPP_18_RGB BIT(0) > > #define SN_HPD_DISABLE_REG 0x5C > > #define HPD_DISABLE BIT(0) > > +#define HPD_DEBOUNCED_STATE BIT(4) > > #define SN_GPIO_IO_REG 0x5E > > #define SN_GPIO_INPUT_SHIFT 4 > > #define SN_GPIO_OUTPUT_SHIFT0 > > @@ -106,10 +107,24 @@ > > #define SN_PWM_EN_INV_REG0xA5 > > #define SN_PWM_INV_MASK BIT(0) > > #define SN_PWM_EN_MASK BIT(1) > > +#define SN_IRQ_EN_REG0xE0 > > +#define IRQ_EN BIT(0) > > +#define SN_IRQ_HPD_REG 0xE6 > > +#define IRQ_HPD_EN BIT(0) > > +#define IRQ_HPD_INSERTION_ENBIT(1) > > +#define IRQ_HPD_REMOVAL_EN BIT(2) > > +#define IRQ_HPD_REPLUG_EN BIT(3) > > +#define IRQ_HPD_PLL_UNLOCK_EN BIT(5) > > #define SN_AUX_CMD_STATUS_REG0xF4 > > #define AUX_IRQ_STATUS_AUX_RPLY_TOUTBIT(3) > > #define AUX_IRQ_STATUS_AUX_SHORTBIT(5) > > #define AUX_IRQ_STATUS_NAT_I2C_FAIL BIT(6) > > +#define SN_IRQ_HPD_STATUS_REG0xF5 > > +#define IRQ_HPD_STATUS BIT(0) > > +#define IRQ_HPD_INSERTION_STATUSBIT(1) > > +#define IRQ_HPD_REMOVAL_STATUS BIT(2) > > +#define IRQ_HPD_REPLUG_STATUS BIT(3) > > +#define IRQ_PLL_UNLOCK BIT(5) > > > > #define MIN_DSI_CLK_FREQ_MHZ 40 > > > > @@ -168,6 +183,12 @@ > > * @pwm_enabled: Used to track if the PWM signal is currently enabled. > > * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM. > > * @pwm_refclk_freq: Cache for the reference clock input to the PWM. > > + * > > + * @no_hpd: Disable hot-plug detection as instructed by device tree > > (used > > + *for instance for eDP panels whose HPD signal won't be > > asserted > > + *until the panel is turned on, and is thus not usable for > > + *downstream device detection). > > + * @irq: IRQ number for the device. > > */ > > struct ti_sn65dsi86 { > > struct auxiliary_device bridge_aux; > > @@ -202,6 +223,9 @@ struct ti_sn65dsi86 { > > atomic_tpwm_pin_busy; > > #endif > > unsigned intpwm_refclk_freq; > > + > > + boolno_hpd; > > + int irq; > > }; > > > > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { > > @@ -316,23 +340,25 @@ static void ti_sn65dsi86_enable_comms(struct > > ti_sn65dsi86 *pdata) > > ti_sn_bridge_set_refclk_freq(pdata); > > > > /* > > - * HPD on this bridge chip is a bit useless. This is an eDP bridge > > - * so the HPD is an internal signal that's only there to signal that > > - *
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank wrote: > > > > On 3/10/2022 6:10 PM, Rob Clark wrote: > > On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank > > wrote: > >> > >> > >> > >> On 3/10/2022 4:24 PM, Rob Clark wrote: > >>> On Thu, Mar 10, 2022 at 1:55 AM Christian König > >>> wrote: > > > > Am 09.03.22 um 19:12 schrieb Rob Clark: > > On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > > wrote: > >> From: Shashank Sharma > >> > >> This patch adds a new sysfs event, which will indicate > >> the userland about a GPU reset, and can also provide > >> some information like: > >> - process ID of the process involved with the GPU reset > >> - process name of the involved process > >> - the GPU status info (using flags) > >> > >> This patch also introduces the first flag of the flags > >> bitmap, which can be appended as and when required. > > Why invent something new, rather than using the already existing > > devcoredump? > > Yeah, that's a really valid question. > > > I don't think we need (or should encourage/allow) something drm > > specific when there is already an existing solution used by both drm > > and non-drm drivers. Userspace should not have to learn to support > > yet another mechanism to do the same thing. > > Question is how is userspace notified about new available core dumps? > >>> > >>> I haven't looked into it too closely, as the CrOS userspace > >>> crash-reporter already had support for devcoredump, so it "just > >>> worked" out of the box[1]. I believe a udev event is what triggers > >>> the crash-reporter to go read the devcore dump out of sysfs. > >> > >> I had a quick look at the devcoredump code, and it doesn't look like > >> that is sending an event to the user, so we still need an event to > >> indicate a GPU reset. > > > > There definitely is an event to userspace, I suspect somewhere down > > the device_add() path? > > > > Let me check that out as well, hope that is not due to a driver-private > event for GPU reset, coz I think I have seen some of those in a few DRM > drivers. > Definitely no driver private event for drm/msm .. I haven't dug through it all but this is the collector for devcoredump, triggered somehow via udev. Most likely from event triggered by device_add() https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/crash-reporter/udev_collector.cc BR, -R
Re: [PATCH] dt-bindings: gpu: mali-bifrost: Document RZ/V2L SoC
On Tue, 08 Mar 2022 21:15:43 +, Lad Prabhakar wrote: > The Renesas RZ/V2L SoC (a.k.a R9A07G054) has a Bifrost Mali-G31 GPU, > add a compatible string for it. > > Signed-off-by: Lad Prabhakar > Reviewed-by: Biju Das > --- > Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Applied, thanks!
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 9:40 AM, Rob Clark wrote: On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank wrote: On 3/10/2022 6:10 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? Let me check that out as well, hope that is not due to a driver-private event for GPU reset, coz I think I have seen some of those in a few DRM drivers. Definitely no driver private event for drm/msm .. I haven't dug through it all but this is the collector for devcoredump, triggered somehow via udev. Most likely from event triggered by device_add() https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/crash-reporter/udev_collector.cc Yes, that is correct. the uevent for devcoredump is from device_add() https://github.com/torvalds/linux/blob/master/drivers/base/core.c#L3340 BR, -R
Re: [PATCH] drm/bridge: anx7625: Fix not correct get property counts
Hi Xin, On Thu, Mar 10, 2022 at 05:16:53PM +0800, Xin Ji wrote: > The property length which returns from "of_get_property", divided by > sizeof(int) to get the total property counts. > > Fixes: fd0310b6fe7d ("drm/bridge: anx7625: add MIPI DPI input feature") > > Signed-off-by: Xin Ji > --- > drivers/gpu/drm/bridge/analogix/anx7625.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c > b/drivers/gpu/drm/bridge/analogix/anx7625.c > index c6a9a02ed762..87081d5b408d 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -1594,6 +1594,7 @@ static int anx7625_get_swing_setting(struct device *dev, > > if (of_get_property(dev->of_node, > "analogix,lane0-swing", &num_regs)) { > + num_regs /= sizeof(int); Since the property is an array maybe use: of_property_read_u8_array() Sam
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
Hi Am 09.03.22 um 23:25 schrieb Dmitry Osipenko: The reason for this work is to keep GEM shmem pages mapped and allocated even while the BO is neither mapped nor pinned. As it is now, GEM SHMEM creates and releases pages on each pin and unpin, and maps and unmaps memory ranges on each vmap and vunmap. It's all wasteful. Only the first pin and vmap calls should establish pages and mappings and only the purge and free functions should release them. Hm, aren't maps and pins already refcounted? They are. But even when the refcounter reaches 0 on deref, there's no need to remove the mapping or free the memory pages. We can keep them around for the next ref operation. Only the shrinker's purge or freeing the object has to do such clean-up operations. Such behavior is supported by TTM and we already use it in VRAM helpers as well. Best regards Thomas The patchset adds new helpers for BO purging to struct drm_gem_object_funcs. With this, I think it might be possible to have one global DRM shrinker and let it handle all BOs; independent of each BO's memory manager. Thank you, I'll give it a try. -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 7:33 PM, Abhinav Kumar wrote: On 3/10/2022 9:40 AM, Rob Clark wrote: On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank wrote: On 3/10/2022 6:10 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? Let me check that out as well, hope that is not due to a driver-private event for GPU reset, coz I think I have seen some of those in a few DRM drivers. Definitely no driver private event for drm/msm .. I haven't dug through it all but this is the collector for devcoredump, triggered somehow via udev. Most likely from event triggered by device_add() https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium.googlesource.com%2Fchromiumos%2Fplatform2%2F%2B%2FHEAD%2Fcrash-reporter%2Fudev_collector.cc&data=04%7C01%7Cshashank.sharma%40amd.com%7C86146416b717420501fc08da02c4785b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825340130157925%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LncI%2F5mIpeG1Avj2YXLmbZ5f1ONUfpf6TzJZH3%2Fs8%2Fw%3D&reserved=0 Yes, that is correct. the uevent for devcoredump is from device_add() Yes, I could confirm in the code that device_add() sends a uevent. kobject_uevent(&dev->kobj, KOBJ_ADD); I was trying to map the ChromiumOs's udev event rules with the event being sent from device_add(), what I could see is there is only one udev rule for any DRM subsystem events in ChromiumOs's 99-crash-reporter.rules: ACTION=="change", SUBSYSTEM=="drm", KERNEL=="card0", ENV{ERROR}=="1", \ RUN+="/sbin/crash_reporter --udev=KERNEL=card0:SUBSYSTEM=drm:ACTION=change" Can someone confirm that this is the rule which gets triggered when a devcoredump is generated ? I could not find an ERROR=1 string in the env[] while sending this event from dev_add(); - Shashank https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fbase%2Fcore.c%23L3340&data=04%7C01%7Cshashank.sharma%40amd.com%7C86146416b717420501fc08da02c4785b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825340130157925%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5HyWYZ5ZWYz4mUPWeTW51QFdoY0NlA50Nbj1dAC6os4%3D&reserved=0 BR, -R
Re: [v3,4/5] fbdev: Improve performance of cfb_imageblit()
Hi Am 09.03.22 um 11:39 schrieb Geert Uytterhoeven: Hi Marek, On Wed, Mar 9, 2022 at 10:22 AM Marek Szyprowski wrote: On 09.03.2022 09:22, Thomas Zimmermann wrote: Am 08.03.22 um 23:52 schrieb Marek Szyprowski: On 23.02.2022 20:38, Thomas Zimmermann wrote: Improve the performance of cfb_imageblit() by manually unrolling the inner blitting loop and moving some invariants out. The compiler failed to do this automatically. This change keeps cfb_imageblit() in sync with sys_imagebit(). A microbenchmark measures the average number of CPU cycles for cfb_imageblit() after a stabilizing period of a few minutes (i7-4790, FullHD, simpledrm, kernel with debugging). cfb_imageblit(), new: 15724 cycles cfb_imageblit(): old: 30566 cycles In the optimized case, cfb_imageblit() is now ~2x faster than before. v3: * fix commit description (Pekka) Signed-off-by: Thomas Zimmermann Acked-by: Sam Ravnborg Reviewed-by: Javier Martinez Canillas This patch landed recently in linux next-20220308 as commit 0d03011894d2 ("fbdev: Improve performance of cfb_imageblit()"). Sadly it causes a freeze after DRM and emulated fbdev initialization on various Samsung Exynos ARM 32bit based boards. This happens when kernel is compiled from exynos_defconfig. Surprisingly when kernel is compiled from multi_v7_defconfig all those boards boot fine, so this is a matter of one of the debugging options enabled in the exynos_defconfig. I will try to analyze this further and share the results. Reverting $subject on top of next-20220308 fixes the boot issue. Thanks for reporting. I don't have the hardware to reproduce it and there's no obvious difference to the original version. It's supposed to be the same algorithm with a different implementation. Unless you can figure out the issue, we can also revert the patch easily. I've played a bit with .config options and found that the issue is caused by the compiled-in fonts used for the framebuffer. For some reasons (so far unknown to me), exynos_defconfig has the following odd setup: CONFIG_FONT_SUPPORT=y CONFIG_FONTS=y # CONFIG_FONT_8x8 is not set # CONFIG_FONT_8x16 is not set # CONFIG_FONT_6x11 is not set CONFIG_FONT_7x14=y # CONFIG_FONT_PEARL_8x8 is not set # CONFIG_FONT_ACORN_8x8 is not set # CONFIG_FONT_MINI_4x6 is not set # CONFIG_FONT_6x10 is not set # CONFIG_FONT_10x18 is not set # CONFIG_FONT_SUN8x16 is not set # CONFIG_FONT_SUN12x22 is not set # CONFIG_FONT_TER16x32 is not set # CONFIG_FONT_6x8 is not set Such setup causes a freeze during framebuffer initialization (or just after it got registered). I've reproduced this even on Raspberry Pi 3B with multi_v7_defconfig and changed fonts configuration (this also required to disable vivid driver, which forces 8x16 font), where I got the following panic: simple-framebuffer 3eace000.framebuffer: framebuffer at 0x3eace000, 0x12c000 bytes simple-framebuffer 3eace000.framebuffer: format=a8r8g8b8, mode=640x480x32, linelength=2560 8<--- cut here --- Unable to handle kernel paging request at virtual address f0aac000 So support for images with offsets or widths that are not a multiple of 8 got broken in cfb_imageblit(). Oops... BTW, the various drawing routines used to set a bitmask indicating which alignments were supported (see blit_x), but most of them no longer do, presumably because all alignments are now supported (since ca. 20 years?). So you can (temporarily) work around this by filling in blit_x, preventing the use of the 7x14 font. How do I activate the 7x14 font? It's compiled into the kernel already (CONFIG_FONT_7x14=y). Best regards Thomas 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 -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [v3,4/5] fbdev: Improve performance of cfb_imageblit()
Hi Thomas, On Thu, Mar 10, 2022 at 8:22 PM Thomas Zimmermann wrote: > Am 09.03.22 um 11:39 schrieb Geert Uytterhoeven: > > On Wed, Mar 9, 2022 at 10:22 AM Marek Szyprowski > > wrote: > >> On 09.03.2022 09:22, Thomas Zimmermann wrote: > >>> Am 08.03.22 um 23:52 schrieb Marek Szyprowski: > On 23.02.2022 20:38, Thomas Zimmermann wrote: > > Improve the performance of cfb_imageblit() by manually unrolling > > the inner blitting loop and moving some invariants out. The compiler > > failed to do this automatically. This change keeps cfb_imageblit() > > in sync with sys_imagebit(). > > > > A microbenchmark measures the average number of CPU cycles > > for cfb_imageblit() after a stabilizing period of a few minutes > > (i7-4790, FullHD, simpledrm, kernel with debugging). > > > > cfb_imageblit(), new: 15724 cycles > > cfb_imageblit(): old: 30566 cycles > > > > In the optimized case, cfb_imageblit() is now ~2x faster than before. > > > > v3: > > * fix commit description (Pekka) > > > > Signed-off-by: Thomas Zimmermann > > Acked-by: Sam Ravnborg > > Reviewed-by: Javier Martinez Canillas > This patch landed recently in linux next-20220308 as commit 0d03011894d2 > ("fbdev: Improve performance of cfb_imageblit()"). Sadly it causes a > freeze after DRM and emulated fbdev initialization on various Samsung > Exynos ARM 32bit based boards. This happens when kernel is compiled from > exynos_defconfig. Surprisingly when kernel is compiled from > multi_v7_defconfig all those boards boot fine, so this is a matter of > one of the debugging options enabled in the exynos_defconfig. I will try > to analyze this further and share the results. Reverting $subject on top > of next-20220308 fixes the boot issue. > >>> > >>> Thanks for reporting. I don't have the hardware to reproduce it and > >>> there's no obvious difference to the original version. It's supposed > >>> to be the same algorithm with a different implementation. Unless you > >>> can figure out the issue, we can also revert the patch easily. > >> > >> I've played a bit with .config options and found that the issue is > >> caused by the compiled-in fonts used for the framebuffer. For some > >> reasons (so far unknown to me), exynos_defconfig has the following odd > >> setup: > >> > >> CONFIG_FONT_SUPPORT=y > >> CONFIG_FONTS=y > >> # CONFIG_FONT_8x8 is not set > >> # CONFIG_FONT_8x16 is not set > >> # CONFIG_FONT_6x11 is not set > >> CONFIG_FONT_7x14=y > >> # CONFIG_FONT_PEARL_8x8 is not set > >> # CONFIG_FONT_ACORN_8x8 is not set > >> # CONFIG_FONT_MINI_4x6 is not set > >> # CONFIG_FONT_6x10 is not set > >> # CONFIG_FONT_10x18 is not set > >> # CONFIG_FONT_SUN8x16 is not set > >> # CONFIG_FONT_SUN12x22 is not set > >> # CONFIG_FONT_TER16x32 is not set > >> # CONFIG_FONT_6x8 is not set > >> > >> Such setup causes a freeze during framebuffer initialization (or just > >> after it got registered). I've reproduced this even on Raspberry Pi 3B > >> with multi_v7_defconfig and changed fonts configuration (this also > >> required to disable vivid driver, which forces 8x16 font), where I got > >> the following panic: > >> > >> simple-framebuffer 3eace000.framebuffer: framebuffer at 0x3eace000, > >> 0x12c000 bytes > >> simple-framebuffer 3eace000.framebuffer: format=a8r8g8b8, > >> mode=640x480x32, linelength=2560 > >> 8<--- cut here --- > >> Unable to handle kernel paging request at virtual address f0aac000 > > > > So support for images with offsets or widths that are not a multiple > > of 8 got broken in cfb_imageblit(). Oops... > > > > BTW, the various drawing routines used to set a bitmask indicating > > which alignments were supported (see blit_x), but most of them no > > longer do, presumably because all alignments are now supported > > (since ca. 20 years?). > > So you can (temporarily) work around this by filling in blit_x, > > preventing the use of the 7x14 font. > > How do I activate the 7x14 font? It's compiled into the kernel already > (CONFIG_FONT_7x14=y). Documentation/fb/fbcon.rst:1. fbcon=font: Or just disable all other fonts. 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
Re: [PATCH v1 1/3] mm: split vm_normal_pages for LRU and non-LRU handling
On Thu, Mar 10, 2022 at 11:26:31AM -0600, Alex Sierra wrote: > @@ -606,7 +606,7 @@ static void print_bad_pte(struct vm_area_struct *vma, > unsigned long addr, > * PFNMAP mappings in order to support COWable mappings. > * > */ > -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, > +struct page *vm_normal_any_page(struct vm_area_struct *vma, unsigned long > addr, > pte_t pte) > { > unsigned long pfn = pte_pfn(pte); > @@ -620,8 +620,6 @@ struct page *vm_normal_page(struct vm_area_struct *vma, > unsigned long addr, > return NULL; > if (is_zero_pfn(pfn)) > return NULL; > - if (pte_devmap(pte)) > - return NULL; > > print_bad_pte(vma, addr, pte, NULL); > return NULL; ... what? Haven't you just made it so that a devmap page always prints a bad PTE message, and then returns NULL anyway? Surely this should be: if (pte_devmap(pte)) - return NULL; + return pfn_to_page(pfn); or maybe + goto check_pfn; But I don't know about that highest_memmap_pfn check. > @@ -661,6 +659,22 @@ struct page *vm_normal_page(struct vm_area_struct *vma, > unsigned long addr, > return pfn_to_page(pfn); > } > > +/* > + * vm_normal_lru_page -- This function gets the "struct page" associated > + * with a pte only for page cache and anon page. These pages are LRU handled. > + */ > +struct page *vm_normal_lru_page(struct vm_area_struct *vma, unsigned long > addr, > + pte_t pte) It seems a shame to add a new function without proper kernel-doc.
[PULL] drm-misc-fixes
Hi Dave and Daniel, here's the PR for drm-misc-fixes for this week. Best regards Thomas drm-misc-fixes-2022-03-10: * drm/sun4i: Fix P010 and P210 format numbers The following changes since commit 62929726ef0ec72cbbe9440c5d125d4278b99894: drm/vrr: Set VRR capable prop only if it is attached to connector (2022-03-01 11:37:21 -0800) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2022-03-10 for you to fetch changes up to 9470c29faa91c804aa04de4c10634bf02462bfa5: drm/sun4i: mixer: Fix P010 and P210 format numbers (2022-03-08 11:54:50 +0100) * drm/sun4i: Fix P010 and P210 format numbers Jernej Skrabec (1): drm/sun4i: mixer: Fix P010 and P210 format numbers drivers/gpu/drm/sun4i/sun8i_mixer.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 11:14 AM Sharma, Shashank wrote: > > > > On 3/10/2022 7:33 PM, Abhinav Kumar wrote: > > > > > > On 3/10/2022 9:40 AM, Rob Clark wrote: > >> On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank > >> wrote: > >>> > >>> > >>> > >>> On 3/10/2022 6:10 PM, Rob Clark wrote: > On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank > wrote: > > > > > > > > On 3/10/2022 4:24 PM, Rob Clark wrote: > >> On Thu, Mar 10, 2022 at 1:55 AM Christian König > >> wrote: > >>> > >>> > >>> > >>> Am 09.03.22 um 19:12 schrieb Rob Clark: > On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > wrote: > > From: Shashank Sharma > > > > This patch adds a new sysfs event, which will indicate > > the userland about a GPU reset, and can also provide > > some information like: > > - process ID of the process involved with the GPU reset > > - process name of the involved process > > - the GPU status info (using flags) > > > > This patch also introduces the first flag of the flags > > bitmap, which can be appended as and when required. > Why invent something new, rather than using the already existing > devcoredump? > >>> > >>> Yeah, that's a really valid question. > >>> > I don't think we need (or should encourage/allow) something drm > specific when there is already an existing solution used by both > drm > and non-drm drivers. Userspace should not have to learn to support > yet another mechanism to do the same thing. > >>> > >>> Question is how is userspace notified about new available core > >>> dumps? > >> > >> I haven't looked into it too closely, as the CrOS userspace > >> crash-reporter already had support for devcoredump, so it "just > >> worked" out of the box[1]. I believe a udev event is what triggers > >> the crash-reporter to go read the devcore dump out of sysfs. > > > > I had a quick look at the devcoredump code, and it doesn't look like > > that is sending an event to the user, so we still need an event to > > indicate a GPU reset. > > There definitely is an event to userspace, I suspect somewhere down > the device_add() path? > > >>> > >>> Let me check that out as well, hope that is not due to a driver-private > >>> event for GPU reset, coz I think I have seen some of those in a few DRM > >>> drivers. > >>> > >> > >> Definitely no driver private event for drm/msm .. I haven't dug > >> through it all but this is the collector for devcoredump, triggered > >> somehow via udev. Most likely from event triggered by device_add() > >> > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium.googlesource.com%2Fchromiumos%2Fplatform2%2F%2B%2FHEAD%2Fcrash-reporter%2Fudev_collector.cc&data=04%7C01%7Cshashank.sharma%40amd.com%7C86146416b717420501fc08da02c4785b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825340130157925%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LncI%2F5mIpeG1Avj2YXLmbZ5f1ONUfpf6TzJZH3%2Fs8%2Fw%3D&reserved=0 > >> > > > > Yes, that is correct. the uevent for devcoredump is from device_add() > > > Yes, I could confirm in the code that device_add() sends a uevent. > > kobject_uevent(&dev->kobj, KOBJ_ADD); > > I was trying to map the ChromiumOs's udev event rules with the event > being sent from device_add(), what I could see is there is only one udev > rule for any DRM subsystem events in ChromiumOs's 99-crash-reporter.rules: > > ACTION=="change", SUBSYSTEM=="drm", KERNEL=="card0", ENV{ERROR}=="1", \ >RUN+="/sbin/crash_reporter > --udev=KERNEL=card0:SUBSYSTEM=drm:ACTION=change" > > Can someone confirm that this is the rule which gets triggered when a > devcoredump is generated ? I could not find an ERROR=1 string in the > env[] while sending this event from dev_add(); I think it is actually this rule: ACTION=="add", SUBSYSTEM=="devcoredump", \ RUN+="/sbin/crash_reporter --udev=SUBSYSTEM=devcoredump:ACTION=add:KERNEL_NUMBER=%n" It is something non-drm specific because it supports devcore dumps from non drm drivers. I know at least some of the wifi and remoteproc drivers use it. BR, -R
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 8:35 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 11:14 AM Sharma, Shashank wrote: On 3/10/2022 7:33 PM, Abhinav Kumar wrote: On 3/10/2022 9:40 AM, Rob Clark wrote: On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank wrote: On 3/10/2022 6:10 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? Let me check that out as well, hope that is not due to a driver-private event for GPU reset, coz I think I have seen some of those in a few DRM drivers. Definitely no driver private event for drm/msm .. I haven't dug through it all but this is the collector for devcoredump, triggered somehow via udev. Most likely from event triggered by device_add() https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium.googlesource.com%2Fchromiumos%2Fplatform2%2F%2B%2FHEAD%2Fcrash-reporter%2Fudev_collector.cc&data=04%7C01%7Cshashank.sharma%40amd.com%7Cb4e920f125ae4d7de29708da02cd3112%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825377562005233%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=M4xHPErex4vn7l3lNPgniiMp%2BKb3SpOHQo2QLAndxDQ%3D&reserved=0 Yes, that is correct. the uevent for devcoredump is from device_add() Yes, I could confirm in the code that device_add() sends a uevent. kobject_uevent(&dev->kobj, KOBJ_ADD); I was trying to map the ChromiumOs's udev event rules with the event being sent from device_add(), what I could see is there is only one udev rule for any DRM subsystem events in ChromiumOs's 99-crash-reporter.rules: ACTION=="change", SUBSYSTEM=="drm", KERNEL=="card0", ENV{ERROR}=="1", \ RUN+="/sbin/crash_reporter --udev=KERNEL=card0:SUBSYSTEM=drm:ACTION=change" Can someone confirm that this is the rule which gets triggered when a devcoredump is generated ? I could not find an ERROR=1 string in the env[] while sending this event from dev_add(); I think it is actually this rule: ACTION=="add", SUBSYSTEM=="devcoredump", \ RUN+="/sbin/crash_reporter --udev=SUBSYSTEM=devcoredump:ACTION=add:KERNEL_NUMBER=%n" It is something non-drm specific because it supports devcore dumps from non drm drivers. I know at least some of the wifi and remoteproc drivers use it. Ah, this seems like a problem for me. I understand it will work for a reset/recovery app well, but if a DRM client (like a compositor), who wants to listen only to DRM events (like a GPU reset), wouldn't this create a lot of noise for it ? Like every time any subsystem produces this coredump, there will be a change in devcoresump subsystem, and the client will have to parse the core file, and then will have to decide if it wants to react to this, or ignore. Wouldn't a GPU reset event, specific to DRM subsystem server better in such case ? - Shashank BR, -R
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Thu, Mar 10, 2022 at 11:44 AM Sharma, Shashank wrote: > > > > On 3/10/2022 8:35 PM, Rob Clark wrote: > > On Thu, Mar 10, 2022 at 11:14 AM Sharma, Shashank > > wrote: > >> > >> > >> > >> On 3/10/2022 7:33 PM, Abhinav Kumar wrote: > >>> > >>> > >>> On 3/10/2022 9:40 AM, Rob Clark wrote: > On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank > wrote: > > > > > > > > On 3/10/2022 6:10 PM, Rob Clark wrote: > >> On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank > >> wrote: > >>> > >>> > >>> > >>> On 3/10/2022 4:24 PM, Rob Clark wrote: > On Thu, Mar 10, 2022 at 1:55 AM Christian König > wrote: > > > > > > > > Am 09.03.22 um 19:12 schrieb Rob Clark: > >> On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma > >> wrote: > >>> From: Shashank Sharma > >>> > >>> This patch adds a new sysfs event, which will indicate > >>> the userland about a GPU reset, and can also provide > >>> some information like: > >>> - process ID of the process involved with the GPU reset > >>> - process name of the involved process > >>> - the GPU status info (using flags) > >>> > >>> This patch also introduces the first flag of the flags > >>> bitmap, which can be appended as and when required. > >> Why invent something new, rather than using the already existing > >> devcoredump? > > > > Yeah, that's a really valid question. > > > >> I don't think we need (or should encourage/allow) something drm > >> specific when there is already an existing solution used by both > >> drm > >> and non-drm drivers. Userspace should not have to learn to support > >> yet another mechanism to do the same thing. > > > > Question is how is userspace notified about new available core > > dumps? > > I haven't looked into it too closely, as the CrOS userspace > crash-reporter already had support for devcoredump, so it "just > worked" out of the box[1]. I believe a udev event is what triggers > the crash-reporter to go read the devcore dump out of sysfs. > >>> > >>> I had a quick look at the devcoredump code, and it doesn't look like > >>> that is sending an event to the user, so we still need an event to > >>> indicate a GPU reset. > >> > >> There definitely is an event to userspace, I suspect somewhere down > >> the device_add() path? > >> > > > > Let me check that out as well, hope that is not due to a driver-private > > event for GPU reset, coz I think I have seen some of those in a few DRM > > drivers. > > > > Definitely no driver private event for drm/msm .. I haven't dug > through it all but this is the collector for devcoredump, triggered > somehow via udev. Most likely from event triggered by device_add() > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium.googlesource.com%2Fchromiumos%2Fplatform2%2F%2B%2FHEAD%2Fcrash-reporter%2Fudev_collector.cc&data=04%7C01%7Cshashank.sharma%40amd.com%7Cb4e920f125ae4d7de29708da02cd3112%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825377562005233%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=M4xHPErex4vn7l3lNPgniiMp%2BKb3SpOHQo2QLAndxDQ%3D&reserved=0 > > >>> > >>> Yes, that is correct. the uevent for devcoredump is from device_add() > >>> > >> Yes, I could confirm in the code that device_add() sends a uevent. > >> > >> kobject_uevent(&dev->kobj, KOBJ_ADD); > >> > >> I was trying to map the ChromiumOs's udev event rules with the event > >> being sent from device_add(), what I could see is there is only one udev > >> rule for any DRM subsystem events in ChromiumOs's 99-crash-reporter.rules: > >> > >> ACTION=="change", SUBSYSTEM=="drm", KERNEL=="card0", ENV{ERROR}=="1", \ > >> RUN+="/sbin/crash_reporter > >> --udev=KERNEL=card0:SUBSYSTEM=drm:ACTION=change" > >> > >> Can someone confirm that this is the rule which gets triggered when a > >> devcoredump is generated ? I could not find an ERROR=1 string in the > >> env[] while sending this event from dev_add(); > > > > I think it is actually this rule: > > > > ACTION=="add", SUBSYSTEM=="devcoredump", \ > >RUN+="/sbin/crash_reporter > > --udev=SUBSYSTEM=devcoredump:ACTION=add:KERNEL_NUMBER=%n" > > > > It is something non-drm specific because it supports devcore dumps > > from non drm drivers. I know at least some of the wifi and remoteproc > > drivers use it. > > > > Ah, this seems like a problem for me. I understand it will work for a > reset/recovery app well, but if a DRM client (like a compositor), who > wants to listen only to DRM events (like a GPU reset), wouldn't this > create a lot of noise for it ? Like every time any subsystem produces > t
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On 3/10/2022 8:56 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 11:44 AM Sharma, Shashank wrote: On 3/10/2022 8:35 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 11:14 AM Sharma, Shashank wrote: On 3/10/2022 7:33 PM, Abhinav Kumar wrote: On 3/10/2022 9:40 AM, Rob Clark wrote: On Thu, Mar 10, 2022 at 9:19 AM Sharma, Shashank wrote: On 3/10/2022 6:10 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 8:21 AM Sharma, Shashank wrote: On 3/10/2022 4:24 PM, Rob Clark wrote: On Thu, Mar 10, 2022 at 1:55 AM Christian König wrote: Am 09.03.22 um 19:12 schrieb Rob Clark: On Tue, Mar 8, 2022 at 11:40 PM Shashank Sharma wrote: From: Shashank Sharma This patch adds a new sysfs event, which will indicate the userland about a GPU reset, and can also provide some information like: - process ID of the process involved with the GPU reset - process name of the involved process - the GPU status info (using flags) This patch also introduces the first flag of the flags bitmap, which can be appended as and when required. Why invent something new, rather than using the already existing devcoredump? Yeah, that's a really valid question. I don't think we need (or should encourage/allow) something drm specific when there is already an existing solution used by both drm and non-drm drivers. Userspace should not have to learn to support yet another mechanism to do the same thing. Question is how is userspace notified about new available core dumps? I haven't looked into it too closely, as the CrOS userspace crash-reporter already had support for devcoredump, so it "just worked" out of the box[1]. I believe a udev event is what triggers the crash-reporter to go read the devcore dump out of sysfs. I had a quick look at the devcoredump code, and it doesn't look like that is sending an event to the user, so we still need an event to indicate a GPU reset. There definitely is an event to userspace, I suspect somewhere down the device_add() path? Let me check that out as well, hope that is not due to a driver-private event for GPU reset, coz I think I have seen some of those in a few DRM drivers. Definitely no driver private event for drm/msm .. I haven't dug through it all but this is the collector for devcoredump, triggered somehow via udev. Most likely from event triggered by device_add() https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchromium.googlesource.com%2Fchromiumos%2Fplatform2%2F%2B%2FHEAD%2Fcrash-reporter%2Fudev_collector.cc&data=04%7C01%7Cshashank.sharma%40amd.com%7C3b5c0e8744234962061d08da02d00248%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637825389694363926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=mWI1Z%2B8eMJApePc5ajRipbGUG9Cw9wXf2FCw6NQxVaM%3D&reserved=0 Yes, that is correct. the uevent for devcoredump is from device_add() Yes, I could confirm in the code that device_add() sends a uevent. kobject_uevent(&dev->kobj, KOBJ_ADD); I was trying to map the ChromiumOs's udev event rules with the event being sent from device_add(), what I could see is there is only one udev rule for any DRM subsystem events in ChromiumOs's 99-crash-reporter.rules: ACTION=="change", SUBSYSTEM=="drm", KERNEL=="card0", ENV{ERROR}=="1", \ RUN+="/sbin/crash_reporter --udev=KERNEL=card0:SUBSYSTEM=drm:ACTION=change" Can someone confirm that this is the rule which gets triggered when a devcoredump is generated ? I could not find an ERROR=1 string in the env[] while sending this event from dev_add(); I think it is actually this rule: ACTION=="add", SUBSYSTEM=="devcoredump", \ RUN+="/sbin/crash_reporter --udev=SUBSYSTEM=devcoredump:ACTION=add:KERNEL_NUMBER=%n" It is something non-drm specific because it supports devcore dumps from non drm drivers. I know at least some of the wifi and remoteproc drivers use it. Ah, this seems like a problem for me. I understand it will work for a reset/recovery app well, but if a DRM client (like a compositor), who wants to listen only to DRM events (like a GPU reset), wouldn't this create a lot of noise for it ? Like every time any subsystem produces this coredump, there will be a change in devcoresump subsystem, and the client will have to parse the core file, and then will have to decide if it wants to react to this, or ignore. Wouldn't a GPU reset event, specific to DRM subsystem server better in such case ? So, I suppose there are two different use-cases.. for something like distro which has generic crash telemetry (ie. when users opt in to automated crash reporting), and in general for debugging gpu crashes, you want devcoredump, preferably with plenty of information about gpu state, etc, so you actually have a chance of debugging problems you can't necessarily reproduce locally. Note also that mesa CI has some limited support for collecting devcore dumps if a CI run triggers a GPU fault. For something like just notifying a compositor that a gpu crash
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915: Improve long running OCL w/a for GuC submission
On 3/10/2022 01:27, Tvrtko Ursulin wrote: On 09/03/2022 21:16, John Harrison wrote: On 3/8/2022 01:41, Tvrtko Ursulin wrote: On 03/03/2022 22:37, john.c.harri...@intel.com wrote: From: John Harrison A workaround was added to the driver to allow OpenCL workloads to run 'forever' by disabling pre-emption on the RCS engine for Gen12. It is not totally unbound as the heartbeat will kick in eventually and cause a reset of the hung engine. However, this does not work well in GuC submission mode. In GuC mode, the pre-emption timeout is how GuC detects hung contexts and triggers a per engine reset. Thus, disabling the timeout means also losing all per engine reset ability. A full GT reset will still occur when the heartbeat finally expires, but that is a much more destructive and undesirable mechanism. The purpose of the workaround is actually to give OpenCL tasks longer to reach a pre-emption point after a pre-emption request has been issued. This is necessary because Gen12 does not support mid-thread pre-emption and OpenCL can have long running threads. So, rather than disabling the timeout completely, just set it to a 'long' value. v2: Review feedback from Tvrtko - must hard code the 'long' value instead of determining it algorithmically. So make it an extra CONFIG definition. Also, remove the execlist centric comment from the existing pre-emption timeout CONFIG option given that it applies to more than just execlists. Signed-off-by: John Harrison Reviewed-by: Daniele Ceraolo Spurio (v1) Acked-by: Michal Mrozek --- drivers/gpu/drm/i915/Kconfig.profile | 26 +++ drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 ++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile index 39328567c200..7cc38d25ee5c 100644 --- a/drivers/gpu/drm/i915/Kconfig.profile +++ b/drivers/gpu/drm/i915/Kconfig.profile @@ -57,10 +57,28 @@ config DRM_I915_PREEMPT_TIMEOUT default 640 # milliseconds help How long to wait (in milliseconds) for a preemption event to occur - when submitting a new context via execlists. If the current context - does not hit an arbitration point and yield to HW before the timer - expires, the HW will be reset to allow the more important context - to execute. + when submitting a new context. If the current context does not hit + an arbitration point and yield to HW before the timer expires, the + HW will be reset to allow the more important context to execute. + + This is adjustable via + /sys/class/drm/card?/engine/*/preempt_timeout_ms + + May be 0 to disable the timeout. + + The compiled in default may get overridden at driver probe time on + certain platforms and certain engines which will be reflected in the + sysfs control. + +config DRM_I915_PREEMPT_TIMEOUT_COMPUTE + int "Preempt timeout for compute engines (ms, jiffy granularity)" + default 7500 # milliseconds + help + How long to wait (in milliseconds) for a preemption event to occur + when submitting a new context to a compute capable engine. If the + current context does not hit an arbitration point and yield to HW + before the timer expires, the HW will be reset to allow the more + important context to execute. This is adjustable via /sys/class/drm/card?/engine/*/preempt_timeout_ms diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 4185c7338581..cc0954ad836a 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -438,9 +438,14 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id, engine->props.timeslice_duration_ms = CONFIG_DRM_I915_TIMESLICE_DURATION; - /* Override to uninterruptible for OpenCL workloads. */ + /* + * Mid-thread pre-emption is not available in Gen12. Unfortunately, + * some OpenCL workloads run quite long threads. That means they get + * reset due to not pre-empting in a timely manner. So, bump the + * pre-emption timeout value to be much higher for compute engines. + */ if (GRAPHICS_VER(i915) == 12 && (engine->flags & I915_ENGINE_HAS_RCS_REG_STATE)) - engine->props.preempt_timeout_ms = 0; + engine->props.preempt_timeout_ms = CONFIG_DRM_I915_PREEMPT_TIMEOUT_COMPUTE; I wouldn't go as far as adding a config option since as it is it only applies to Gen12 but Kconfig text says nothing about that. And I am not saying you should add a Gen12 specific config option, that would be weird. So IMO just drop it. You were the one arguing that the driver was illegally overriding the user's explicitly chosen settings, including the compile time config This is a bit out of context and illegally don't think used, so misrepresents the earlier discussion.
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
On 3/10/22 22:02, Thomas Zimmermann wrote: > Hi > > Am 09.03.22 um 23:25 schrieb Dmitry Osipenko: >>> >>> The reason for this work is to keep GEM shmem pages mapped and allocated >>> even while the BO is neither mapped nor pinned. As it is now, GEM SHMEM >>> creates and releases pages on each pin and unpin, and maps and unmaps >>> memory ranges on each vmap and vunmap. It's all wasteful. Only the >>> first pin and vmap calls should establish pages and mappings and only >>> the purge and free functions should release them. >> >> Hm, aren't maps and pins already refcounted? > > They are. But even when the refcounter reaches 0 on deref, there's no > need to remove the mapping or free the memory pages. We can keep them > around for the next ref operation. Only the shrinker's purge or freeing > the object has to do such clean-up operations. Such behavior is > supported by TTM and we already use it in VRAM helpers as well. When driver won't want to pin BO at the creation time? Looks like all shmem users do this today, and thus, pages shouldn't be freed until BO is destroyed or purged.
Re: [PATCH v1 1/3] mm: split vm_normal_pages for LRU and non-LRU handling
Am 2022-03-10 um 14:25 schrieb Matthew Wilcox: On Thu, Mar 10, 2022 at 11:26:31AM -0600, Alex Sierra wrote: @@ -606,7 +606,7 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr, * PFNMAP mappings in order to support COWable mappings. * */ -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, +struct page *vm_normal_any_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte) { unsigned long pfn = pte_pfn(pte); @@ -620,8 +620,6 @@ struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return NULL; if (is_zero_pfn(pfn)) return NULL; - if (pte_devmap(pte)) - return NULL; print_bad_pte(vma, addr, pte, NULL); return NULL; ... what? Haven't you just made it so that a devmap page always prints a bad PTE message, and then returns NULL anyway? Yeah, that was stupid. :/ I think the long-term goal was to get rid of pte_devmap. But for now, as long as we have pte_special with pte_devmap, we'll need a special case to handle that like a normal page. I only see the PFN_DEV|PFN_MAP flags set in a few places: drivers/dax/device.c, drivers/nvdimm/pmem.c, fs/fuse/virtio_fs.c. I guess we need to test at least one of them for this patch series to make sure we're not breaking them. Surely this should be: if (pte_devmap(pte)) - return NULL; + return pfn_to_page(pfn); or maybe + goto check_pfn; But I don't know about that highest_memmap_pfn check. Looks to me like it should work. highest_memmap_pfn gets updated in memremap_pages -> pagemap_range -> move_pfn_range_to_zone -> memmap_init_range. Regards, Felix @@ -661,6 +659,22 @@ struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return pfn_to_page(pfn); } +/* + * vm_normal_lru_page -- This function gets the "struct page" associated + * with a pte only for page cache and anon page. These pages are LRU handled. + */ +struct page *vm_normal_lru_page(struct vm_area_struct *vma, unsigned long addr, + pte_t pte) It seems a shame to add a new function without proper kernel-doc.
Re: [PATCH v2 4/4] dt-bindings: display/panel: Add Leadtek ltk035c5444t
On Tue, Mar 08, 2022 at 02:06:43PM +0100, Christophe Branchereau wrote: > Add binding for the leadtek ltk035c5444t, which is a 640x480 > mipi-dbi over spi / 24-bit RGB panel based on the newvision > NV03052C chipset. > > It is found in the Anbernic RG350M mips handheld. > > Signed-off-by: Christophe Branchereau > --- > .../panel/leadtek,ltk035c5444t-spi.yaml | 59 +++ > 1 file changed, 59 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/leadtek,ltk035c5444t-spi.yaml We have 18 SPI based panels already: $ git grep -i 'spi.* {' Documentation/devicetree/bindings/display/panel/ Documentation/devicetree/bindings/display/panel/abt,y030xx067a.yaml:spi { Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml:spi { Documentation/devicetree/bindings/display/panel/ilitek,ili9322.yaml:spi { Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml:spi { Documentation/devicetree/bindings/display/panel/innolux,ej030na.yaml:spi { Documentation/devicetree/bindings/display/panel/kingdisplay,kd035g6-54nt.yaml: spi { Documentation/devicetree/bindings/display/panel/lg,lg4573.yaml:spi { Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.yaml:spi { Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml:spi0 { Documentation/devicetree/bindings/display/panel/samsung,ld9040.yaml:spi { Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml:spi { Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml:spi { Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml:spi { Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.yaml:spi { Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml:spi { Documentation/devicetree/bindings/display/panel/sony,acx565akm.yaml:spi { Documentation/devicetree/bindings/display/panel/tpo,td.yaml:spi { Documentation/devicetree/bindings/display/panel/tpo,tpg110.yaml:spi { Most except for the Samsung ones look like they'd fit in our definition of 'simple panels' which primarily means 1 supply. So I think it is time for a panel-simple-spi.yaml binding to combine all these. But I'm not going to make the person adding the 19th case to do that, and this otherwise looks fine: Reviewed-by: Rob Herring With one nit fixed below: > > diff --git > a/Documentation/devicetree/bindings/display/panel/leadtek,ltk035c5444t-spi.yaml > > b/Documentation/devicetree/bindings/display/panel/leadtek,ltk035c5444t-spi.yaml > new file mode 100644 > index ..9b6f1810adab > --- /dev/null > +++ > b/Documentation/devicetree/bindings/display/panel/leadtek,ltk035c5444t-spi.yaml > @@ -0,0 +1,59 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: > http://devicetree.org/schemas/display/panel/leadtek,ltk035c5444t-spi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Leadtek ltk035c5444t 3.5" (640x480 pixels) 24-bit IPS LCD panel > + > +maintainers: > + - Paul Cercueil > + - Christophe Branchereau > + > +allOf: > + - $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > +properties: > + compatible: > +const: leadtek,ltk035c5444t-spi '-spi' is redundant, so drop. > + > + backlight: true > + port: true > + power-supply: true > + reg: true > + reset-gpios: true > + > +required: > + - compatible > + - power-supply > + - reset-gpios > + > +unevaluatedProperties: false > + > +examples: > + - | > +#include > + > +spi { > +#address-cells = <1>; > +#size-cells = <0>; > +panel@0 { > +compatible = "leadtek,ltk035c5444t-spi"; And update the example... > +reg = <0>; > + > +spi-3wire; > +spi-max-frequency = <3125000>; > + > +reset-gpios = <&gpe 2 GPIO_ACTIVE_LOW>; > + > +backlight = <&backlight>; > +power-supply = <&vcc>; > + > +port { > +panel_input: endpoint { > +remote-endpoint = <&panel_output>; > +}; > +}; > +}; > +}; > -- > 2.34.1 > >