Re: [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct page refcount
On Sat, Oct 16, 2021 at 8:45 AM Jason Gunthorpe wrote: > > On Thu, Oct 14, 2021 at 06:37:35PM -0700, Dan Williams wrote: > > On Thu, Oct 14, 2021 at 4:06 PM Jason Gunthorpe wrote: > > > > > > On Thu, Oct 14, 2021 at 12:01:14PM -0700, Dan Williams wrote: > > > > > > Does anyone know why devmap is pte_special anyhow? > > > > > > > > It does not need to be special as mentioned here: > > > > > > > > https://lore.kernel.org/all/CAPcyv4iFeVDVPn6uc=aksyuvkiu3-fk-n16ijvzq3n8ot00...@mail.gmail.com/ > > > > > > I added a remark there > > > > > > Not special means more to me, it means devmap should do the refcounts > > > properly like normal memory pages. > > > > > > It means vm_normal_page should return !NULL and it means insert_page, > > > not insert_pfn should be used to install them in the PTE. VMAs should > > > not be MIXED MAP, but normal struct page maps. > > > > > > I think this change alone would fix all the refcount problems > > > everwhere in DAX and devmap. > > > > > > > The refcount dependencies also go away after this... > > > > > > > > https://lore.kernel.org/all/161604050866.1463742.7759521510383551055.st...@dwillia2-desk3.amr.corp.intel.com/ > > > > > > > > ...but you can see that patches 1 and 2 in that series depend on being > > > > able to guarantee that all mappings are invalidated when the undelying > > > > device that owns the pgmap goes away. > > > > > > If I have put everything together right this is because of what I > > > pointed to here. FS-DAX is installing 0 refcount pages into PTEs and > > > expecting that to work sanely. > > > > > > This means the page map cannot be removed until all the PTEs are fully > > > flushed, which buggily doesn't happen because of the missing unplug. > > > > > > However, this is all because nobody incrd a refcount to represent the > > > reference in the PTE and since this ment that 0 refcount pages were > > > wrongly stuffed into PTEs then devmap used the refcount == 1 hack to > > > unbreak GUP? > > > > > > So.. Is there some reason why devmap pages are trying so hard to avoid > > > sane refcounting??? > > > > I wouldn't put it that way. It's more that the original sin of > > ZONE_DEVICE that sought to reuse the lru field space, by never having > > a zero recount, then got layered upon and calcified in malignant ways. > > In the meantime surrounding infrastructure got decrustified. Work like > > the 'struct page' cleanup among other things, made it clearer and > > clearer over time that the original design choice needed to be fixed. > > So, there used to be some reason, but with the current code > arrangement it is not the case? This is why it looks so strange when > reading it.. > > AFIACT we are good on the LRU stuff now? Eg release_pages() does not > use page->lru for is_zone_device_page()? Yes, the lru collision was fixed by the 'struct page' cleanup. > > > The MIXED_MAP and insert_pfn were a holdover from page-less DAX, but > > now that we have page-available DAX, yes, we can skip the FS > > notification and just rely on typical refcounting and hanging until > > the FS has a chance to uninstall the PTEs. You're right, the FS > > notification is an improvement to the conversion, not a requirement. > > It all sounds so simple. I looked at this for a good long time and the > first major roadblock is huge pages. > > The mm side is designed around THP which puts a single high order page > into the PUD/PMD such that the mm only has to juggle one page. This a > very sane and reasonable thing to do. > > DAX is stuffing arrays of 4k pages into the PUD/PMDs. Aligning with > THP would make using normal refconting much simpler. I looked at > teaching the mm core to deal with page arrays - it is certainly > doable, but it is quite inefficient and ugly mm code. THP does not support PUD, and neither does FSDAX, so it's only PMDs we need to worry about. > > So, can we fix DAX and TTM - the only uses of PUD/PMDs I could find? > > Joao has a series that does this to device-dax: > > https://lore.kernel.org/all/20210827145819.16471-1-joao.m.mart...@oracle.com/ That assumes there's never any need to fracture a huge page which FSDAX could not support unless the filesystem was built with 2MB block size. > TTM is kind of broken already but does have a struct page, and it is > probably already a high order one. Maybe it is OK? I will ask Thomas > > That leaves FSDAX. Can this be fixed? I know nothing of filesystems or > fsdax to guess. Sounds like folios to me .. My thought here is to assemble a compound page on the fly when establishing the FSDAX PMD mapping. > Assuming changing FSDAX is hard.. How would DAX people feel about just > deleting the PUD/PMD support until it can be done with compound pages? There are end users that would notice the PMD regression, and I think FSDAX PMDs with proper compound page metadata is on the same order of work as fixing the refcount. > Doing so would allow fixing the lifecycle, cleaning up gup and > basically delete a huge wack of
Re: [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct page refcount
On Sat, Oct 16, 2021 at 9:39 AM Matthew Wilcox wrote: > > On Sat, Oct 16, 2021 at 12:44:50PM -0300, Jason Gunthorpe wrote: > > Assuming changing FSDAX is hard.. How would DAX people feel about just > > deleting the PUD/PMD support until it can be done with compound pages? > > I think there are customers who would find that an unacceptable answer :-) No, not given the number of end users that ask for help debugging PMD support.
[PATCH 0/3] drm/amdgpu replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
>From the TODO list Documentation/gpu/todo.rst --- Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Many drivers still call drm_detect_hdmi_monitor() to retrieve the same information, which is less efficient. Audit each individual driver calling drm_detect_hdmi_monitor() and switch to drm_display_info.is_hdmi if applicable. --- The task is divided in three small patches. The last patch depends on the first one. Claudio Suarez (3): drm/amdgpu: update drm_display_info correctly when the edid is read drm/amdgpu: use drm_edid_get_monitor_name() instead of duplicating the code drm/amdgpu: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi .../gpu/drm/amd/amdgpu/amdgpu_connectors.c| 17 + drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 4 +- .../gpu/drm/amd/amdgpu/atombios_encoders.c| 6 +-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 37 +-- drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +- drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- 8 files changed, 29 insertions(+), 44 deletions(-) -- 2.33.0
[PATCH 3/3] drm/amdgpu: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
Once EDID is parsed, the monitor HDMI support information is available
through drm_display_info.is_hdmi. The amdgpu driver still calls
drm_detect_hdmi_monitor() to retrieve the same information, which
is less efficient. Change to drm_display_info.is_hdmi
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez
---
.../gpu/drm/amd/amdgpu/amdgpu_connectors.c| 12 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 4 ++--
.../gpu/drm/amd/amdgpu/atombios_encoders.c| 6 +++---
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 19 +++
drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +-
drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +-
7 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 647aecee1185..0710c19d5e2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -108,7 +108,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector
*connector)
case DRM_MODE_CONNECTOR_DVII:
case DRM_MODE_CONNECTOR_HDMIB:
if (amdgpu_connector->use_digital) {
- if
(drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ if (connector->display_info.is_hdmi) {
if (connector->display_info.bpc)
bpc = connector->display_info.bpc;
}
@@ -116,7 +116,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector
*connector)
break;
case DRM_MODE_CONNECTOR_DVID:
case DRM_MODE_CONNECTOR_HDMIA:
- if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ if (connector->display_info.is_hdmi) {
if (connector->display_info.bpc)
bpc = connector->display_info.bpc;
}
@@ -125,7 +125,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector
*connector)
dig_connector = amdgpu_connector->con_priv;
if ((dig_connector->dp_sink_type ==
CONNECTOR_OBJECT_ID_DISPLAYPORT) ||
(dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) ||
- drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ connector->display_info.is_hdmi) {
if (connector->display_info.bpc)
bpc = connector->display_info.bpc;
}
@@ -149,7 +149,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector
*connector)
break;
}
- if (drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ if (connector->display_info.is_hdmi) {
/*
* Pre DCE-8 hw can't handle > 12 bpc, and more than 12 bpc
doesn't make
* much sense without support for > 12 bpc framebuffers. RGB
4:4:4 at
@@ -1173,7 +1173,7 @@ static enum drm_mode_status
amdgpu_connector_dvi_mode_valid(struct drm_connector
(amdgpu_connector->connector_object_id ==
CONNECTOR_OBJECT_ID_DUAL_LINK_DVI_D) ||
(amdgpu_connector->connector_object_id ==
CONNECTOR_OBJECT_ID_HDMI_TYPE_B)) {
return MODE_OK;
- } else if
(drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ } else if (connector->display_info.is_hdmi) {
/* HDMI 1.3+ supports max clock of 340 Mhz */
if (mode->clock > 34)
return MODE_CLOCK_HIGH;
@@ -1465,7 +1465,7 @@ static enum drm_mode_status
amdgpu_connector_dp_mode_valid(struct drm_connector
(amdgpu_dig_connector->dp_sink_type ==
CONNECTOR_OBJECT_ID_eDP)) {
return amdgpu_atombios_dp_mode_valid_helper(connector,
mode);
} else {
- if
(drm_detect_hdmi_monitor(amdgpu_connector_edid(connector))) {
+ if (connector->display_info.is_hdmi) {
/* HDMI 1.3+ supports max clock of 340 Mhz */
if (mode->clock > 34)
return MODE_CLOCK_HIGH;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index dc50c05f23fc..0bd9d7a4d76f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1364,7 +1364,7 @@ bool amdgpu_display_crtc_scaling_mode_fixup(struct
drm_crtc *crtc,
if ((!(mode->flags & DRM_MODE_FLAG_INTERLACE)) &&
((amdgpu_encoder->underscan_type == UNDERSCAN_ON) ||
((amdgpu_encoder->underscan_type == UNDERSCAN_AUTO) &&
-
[PATCH 1/3] drm/amdgpu: update drm_display_info correctly when the edid is read
drm_display_info is updated by drm_get_edid() or
drm_connector_update_edid_property(). In the amdgpu driver it is almost
always updated when the edid is read in amdgpu_connector_get_edid(),
but not always. Change amdgpu_connector_get_edid() and
amdgpu_connector_free_edid() to keep drm_display_info updated.
Signed-off-by: Claudio Suarez
---
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c| 5 -
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index b9c11c2b2885..647aecee1185 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -315,8 +315,10 @@ static void amdgpu_connector_get_edid(struct drm_connector
*connector)
if (!amdgpu_connector->edid) {
/* some laptops provide a hardcoded edid in rom for LCDs */
if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||
-(connector->connector_type == DRM_MODE_CONNECTOR_eDP)))
+(connector->connector_type == DRM_MODE_CONNECTOR_eDP))) {
amdgpu_connector->edid =
amdgpu_connector_get_hardcoded_edid(adev);
+ drm_connector_update_edid_property(connector,
amdgpu_connector->edid);
+ }
}
}
@@ -326,6 +328,7 @@ static void amdgpu_connector_free_edid(struct drm_connector
*connector)
kfree(amdgpu_connector->edid);
amdgpu_connector->edid = NULL;
+ drm_connector_update_edid_property(connector, NULL);
}
static int amdgpu_connector_ddc_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1ea31dcc7a8b..02ecd216a556 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2583,13 +2583,12 @@ void amdgpu_dm_update_connector_after_detect(
aconnector->edid =
(struct edid *)sink->dc_edid.raw_edid;
- drm_connector_update_edid_property(connector,
- aconnector->edid);
if (aconnector->dc_link->aux_mode)
drm_dp_cec_set_edid(>dm_dp_aux.aux,
aconnector->edid);
}
+ drm_connector_update_edid_property(connector, aconnector->edid);
amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
update_connector_ext_caps(aconnector);
} else {
--
2.33.0
[PATCH 2/3] drm/amdgpu: use drm_edid_get_monitor_name() instead of duplicating the code
Use drm_edid_get_monitor_name() instead of duplicating the code that
parses the EDID in dm_helpers_parse_edid_caps()
Signed-off-by: Claudio Suarez
---
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 18 +++---
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6fee12c91ef5..bc58ee29306a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -59,7 +59,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
int sad_count = -1;
int sadb_count = -1;
int i = 0;
- int j = 0;
uint8_t *sadb = NULL;
enum dc_edid_status result = EDID_OK;
@@ -78,20 +77,9 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
edid_caps->manufacture_week = edid_buf->mfg_week;
edid_caps->manufacture_year = edid_buf->mfg_year;
- /* One of the four detailed_timings stores the monitor name. It's
-* stored in an array of length 13. */
- for (i = 0; i < 4; i++) {
- if (edid_buf->detailed_timings[i].data.other_data.type == 0xfc)
{
- while (j < 13 &&
edid_buf->detailed_timings[i].data.other_data.data.str.str[j]) {
- if
(edid_buf->detailed_timings[i].data.other_data.data.str.str[j] == '\n')
- break;
-
- edid_caps->display_name[j] =
-
edid_buf->detailed_timings[i].data.other_data.data.str.str[j];
- j++;
- }
- }
- }
+ drm_edid_get_monitor_name(edid_buf,
+ edid_caps->display_name,
+ AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
edid_caps->edid_hdmi = drm_detect_hdmi_monitor(
(struct edid *) edid->raw_edid);
--
2.33.0
