| Message ID | 20240806125601.78650-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Transparently handle BMC in outputs | expand |
On 06/08/2024 14:52, Thomas Zimmermann wrote: > Convert DP501 support to struct drm_edid and its helpers. Simplifies > and modernizes the EDID handling. > > The driver reads 4 bytes at once, but the overall read length is now > variable. Therefore update the EDID read loop to never return more than > the requested bytes. Thanks for this patch. I don't have a hardware to test, but I think it may break the EDID reading. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/ast/ast_dp501.c | 92 ++++++++++++++++++++------------- > 1 file changed, 55 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > index 478efa226170..1c5a4902d4c2 100644 > --- a/drivers/gpu/drm/ast/ast_dp501.c > +++ b/drivers/gpu/drm/ast/ast_dp501.c > @@ -318,32 +318,63 @@ static bool ast_dp501_is_connected(struct ast_device *ast) > return true; > } > > -static bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > +static int ast_dp512_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len) > { > - struct ast_device *ast = to_ast_device(dev); > - u32 i, boot_address, offset, data; > - u32 *pEDIDidx; > + struct ast_device *ast = data; > + u32 i, boot_address, offset, ediddata; > > - if (!ast_dp501_is_connected(ast)) > - return false; > + if (block > (512 / EDID_LENGTH)) > + return -EIO; > + > + offset = AST_DP501_EDID_DATA + block * EDID_LENGTH; > > if (ast->config_mode == ast_use_p2a) { > boot_address = get_fw_base(ast); > > - /* Read EDID */ > - offset = AST_DP501_EDID_DATA; > - for (i = 0; i < 128; i += 4) { > - data = ast_mindwm(ast, boot_address + offset + i); > - pEDIDidx = (u32 *)(ediddata + i); > - *pEDIDidx = data; > + for (i = 0; i < len; i += 4) { > + ediddata = ast_mindwm(ast, boot_address + offset + i); I think "i" is always a multiple of 4, so the switch case is broken, and will always go to case 0, so reading only 1 byte every 4 bytes ? > + > + switch (i % 4) { > + case 3: > + *buf = (ediddata >> 24) & 0xff; > + ++buf; > + fallthrough; > + case 2: > + *buf = (ediddata >> 16) & 0xff; > + ++buf; > + fallthrough; > + case 1: > + *buf = (ediddata >> 8) & 0xff; > + ++buf; > + fallthrough; > + case 0: > + *buf = (ediddata) & 0xff; > + ++buf; > + break; > + } > } > } else { > - /* Read EDID */ > - offset = AST_DP501_EDID_DATA; > - for (i = 0; i < 128; i += 4) { > - data = readl(ast->dp501_fw_buf + offset + i); > - pEDIDidx = (u32 *)(ediddata + i); > - *pEDIDidx = data; > + for (i = 0; i < len; i += 4) { > + ediddata = readl(ast->dp501_fw_buf + offset + i); > + > + switch (i % 4) { > + case 3: > + *buf = (ediddata >> 24) & 0xff; > + ++buf; > + fallthrough; > + case 2: > + *buf = (ediddata >> 16) & 0xff; > + ++buf; > + fallthrough; > + case 1: > + *buf = (ediddata >> 8) & 0xff; > + ++buf; > + fallthrough; > + case 0: > + *buf = (ediddata) & 0xff; > + ++buf; > + break; > + } > } > } > > @@ -511,29 +542,16 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = { > > static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector) > { > - void *edid; > - bool succ; > + struct ast_device *ast = to_ast_device(connector->dev); > + const struct drm_edid *drm_edid; > int count; > > - edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > - if (!edid) > - goto err_drm_connector_update_edid_property; > - > - succ = ast_dp501_read_edid(connector->dev, edid); > - if (!succ) > - goto err_kfree; > - > - drm_connector_update_edid_property(connector, edid); > - count = drm_add_edid_modes(connector, edid); > - kfree(edid); > + drm_edid = drm_edid_read_custom(connector, ast_dp512_read_edid_block, ast); > + drm_edid_connector_update(connector, drm_edid); > + count = drm_edid_connector_add_modes(connector); > + drm_edid_free(drm_edid); > > return count; > - > -err_kfree: > - kfree(edid); > -err_drm_connector_update_edid_property: > - drm_connector_update_edid_property(connector, NULL); > - return 0; > } > > static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector,
Hi Am 09.08.24 um 13:36 schrieb Jocelyn Falempe: > > > On 06/08/2024 14:52, Thomas Zimmermann wrote: >> Convert DP501 support to struct drm_edid and its helpers. Simplifies >> and modernizes the EDID handling. >> >> The driver reads 4 bytes at once, but the overall read length is now >> variable. Therefore update the EDID read loop to never return more than >> the requested bytes. > > Thanks for this patch. > I don't have a hardware to test, but I think it may break the EDID > reading. Unfortunately, I don't have the hardware either and I've not even found something that would use this code. > >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/ast/ast_dp501.c | 92 ++++++++++++++++++++------------- >> 1 file changed, 55 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c >> b/drivers/gpu/drm/ast/ast_dp501.c >> index 478efa226170..1c5a4902d4c2 100644 >> --- a/drivers/gpu/drm/ast/ast_dp501.c >> +++ b/drivers/gpu/drm/ast/ast_dp501.c >> @@ -318,32 +318,63 @@ static bool ast_dp501_is_connected(struct >> ast_device *ast) >> return true; >> } >> -static bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) >> +static int ast_dp512_read_edid_block(void *data, u8 *buf, unsigned >> int block, size_t len) >> { >> - struct ast_device *ast = to_ast_device(dev); >> - u32 i, boot_address, offset, data; >> - u32 *pEDIDidx; >> + struct ast_device *ast = data; >> + u32 i, boot_address, offset, ediddata; >> - if (!ast_dp501_is_connected(ast)) >> - return false; >> + if (block > (512 / EDID_LENGTH)) >> + return -EIO; >> + >> + offset = AST_DP501_EDID_DATA + block * EDID_LENGTH; >> if (ast->config_mode == ast_use_p2a) { >> boot_address = get_fw_base(ast); >> - /* Read EDID */ >> - offset = AST_DP501_EDID_DATA; >> - for (i = 0; i < 128; i += 4) { >> - data = ast_mindwm(ast, boot_address + offset + i); >> - pEDIDidx = (u32 *)(ediddata + i); >> - *pEDIDidx = data; >> + for (i = 0; i < len; i += 4) { Here, i need to round up len to a multiple of 4. >> + ediddata = ast_mindwm(ast, boot_address + offset + i); > > I think "i" is always a multiple of 4, so the switch case is broken, > and will always go to case 0, so reading only 1 byte every 4 bytes ? Right, what I want is ((len - i) % 4), so that i get the remaining bytes to read. Best regards Thomas > >> + >> + switch (i % 4) { >> + case 3: >> + *buf = (ediddata >> 24) & 0xff; >> + ++buf; >> + fallthrough; >> + case 2: >> + *buf = (ediddata >> 16) & 0xff; >> + ++buf; >> + fallthrough; >> + case 1: >> + *buf = (ediddata >> 8) & 0xff; >> + ++buf; >> + fallthrough; >> + case 0: >> + *buf = (ediddata) & 0xff; >> + ++buf; >> + break; >> + } >> } >> } else { >> - /* Read EDID */ >> - offset = AST_DP501_EDID_DATA; >> - for (i = 0; i < 128; i += 4) { >> - data = readl(ast->dp501_fw_buf + offset + i); >> - pEDIDidx = (u32 *)(ediddata + i); >> - *pEDIDidx = data; >> + for (i = 0; i < len; i += 4) { >> + ediddata = readl(ast->dp501_fw_buf + offset + i); >> + >> + switch (i % 4) { >> + case 3: >> + *buf = (ediddata >> 24) & 0xff; >> + ++buf; >> + fallthrough; >> + case 2: >> + *buf = (ediddata >> 16) & 0xff; >> + ++buf; >> + fallthrough; >> + case 1: >> + *buf = (ediddata >> 8) & 0xff; >> + ++buf; >> + fallthrough; >> + case 0: >> + *buf = (ediddata) & 0xff; >> + ++buf; >> + break; >> + } >> } >> } >> @@ -511,29 +542,16 @@ static const struct drm_encoder_helper_funcs >> ast_dp501_encoder_helper_funcs = { >> static int ast_dp501_connector_helper_get_modes(struct >> drm_connector *connector) >> { >> - void *edid; >> - bool succ; >> + struct ast_device *ast = to_ast_device(connector->dev); >> + const struct drm_edid *drm_edid; >> int count; >> - edid = kmalloc(EDID_LENGTH, GFP_KERNEL); >> - if (!edid) >> - goto err_drm_connector_update_edid_property; >> - >> - succ = ast_dp501_read_edid(connector->dev, edid); >> - if (!succ) >> - goto err_kfree; >> - >> - drm_connector_update_edid_property(connector, edid); >> - count = drm_add_edid_modes(connector, edid); >> - kfree(edid); >> + drm_edid = drm_edid_read_custom(connector, >> ast_dp512_read_edid_block, ast); >> + drm_edid_connector_update(connector, drm_edid); >> + count = drm_edid_connector_add_modes(connector); >> + drm_edid_free(drm_edid); >> return count; >> - >> -err_kfree: >> - kfree(edid); >> -err_drm_connector_update_edid_property: >> - drm_connector_update_edid_property(connector, NULL); >> - return 0; >> } >> static int ast_dp501_connector_helper_detect_ctx(struct >> drm_connector *connector, >
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 478efa226170..1c5a4902d4c2 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -318,32 +318,63 @@ static bool ast_dp501_is_connected(struct ast_device *ast) return true; } -static bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) +static int ast_dp512_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len) { - struct ast_device *ast = to_ast_device(dev); - u32 i, boot_address, offset, data; - u32 *pEDIDidx; + struct ast_device *ast = data; + u32 i, boot_address, offset, ediddata; - if (!ast_dp501_is_connected(ast)) - return false; + if (block > (512 / EDID_LENGTH)) + return -EIO; + + offset = AST_DP501_EDID_DATA + block * EDID_LENGTH; if (ast->config_mode == ast_use_p2a) { boot_address = get_fw_base(ast); - /* Read EDID */ - offset = AST_DP501_EDID_DATA; - for (i = 0; i < 128; i += 4) { - data = ast_mindwm(ast, boot_address + offset + i); - pEDIDidx = (u32 *)(ediddata + i); - *pEDIDidx = data; + for (i = 0; i < len; i += 4) { + ediddata = ast_mindwm(ast, boot_address + offset + i); + + switch (i % 4) { + case 3: + *buf = (ediddata >> 24) & 0xff; + ++buf; + fallthrough; + case 2: + *buf = (ediddata >> 16) & 0xff; + ++buf; + fallthrough; + case 1: + *buf = (ediddata >> 8) & 0xff; + ++buf; + fallthrough; + case 0: + *buf = (ediddata) & 0xff; + ++buf; + break; + } } } else { - /* Read EDID */ - offset = AST_DP501_EDID_DATA; - for (i = 0; i < 128; i += 4) { - data = readl(ast->dp501_fw_buf + offset + i); - pEDIDidx = (u32 *)(ediddata + i); - *pEDIDidx = data; + for (i = 0; i < len; i += 4) { + ediddata = readl(ast->dp501_fw_buf + offset + i); + + switch (i % 4) { + case 3: + *buf = (ediddata >> 24) & 0xff; + ++buf; + fallthrough; + case 2: + *buf = (ediddata >> 16) & 0xff; + ++buf; + fallthrough; + case 1: + *buf = (ediddata >> 8) & 0xff; + ++buf; + fallthrough; + case 0: + *buf = (ediddata) & 0xff; + ++buf; + break; + } } } @@ -511,29 +542,16 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = { static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector) { - void *edid; - bool succ; + struct ast_device *ast = to_ast_device(connector->dev); + const struct drm_edid *drm_edid; int count; - edid = kmalloc(EDID_LENGTH, GFP_KERNEL); - if (!edid) - goto err_drm_connector_update_edid_property; - - succ = ast_dp501_read_edid(connector->dev, edid); - if (!succ) - goto err_kfree; - - drm_connector_update_edid_property(connector, edid); - count = drm_add_edid_modes(connector, edid); - kfree(edid); + drm_edid = drm_edid_read_custom(connector, ast_dp512_read_edid_block, ast); + drm_edid_connector_update(connector, drm_edid); + count = drm_edid_connector_add_modes(connector); + drm_edid_free(drm_edid); return count; - -err_kfree: - kfree(edid); -err_drm_connector_update_edid_property: - drm_connector_update_edid_property(connector, NULL); - return 0; } static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector,
Convert DP501 support to struct drm_edid and its helpers. Simplifies and modernizes the EDID handling. The driver reads 4 bytes at once, but the overall read length is now variable. Therefore update the EDID read loop to never return more than the requested bytes. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_dp501.c | 92 ++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 37 deletions(-)