This is the final patchset for converting ast to managed initialization.
Patches #1 to #4 address I2C helpers. The structures are being stored in struct ast_connector. The initialization and cleanups is being converted to managed release helpers.
Patches #5 to #10 address modesetting and device structures. All are being embedded into struct ast_private. With struct ast_private being a subclass of struct drm_device, patch #10 switches ast to DRM's managed- allocation helpers.
Patches #11 and #12 address firmware memory that ast allocates internally.
Finally, patch #13 removes ast's destroy function in favor of managed release helpers.
Tested on AST 2100 HW.
Thomas Zimmermann (13): drm/ast: Move I2C code within ast_mode.c drm/ast: Test if I2C support has been initialized drm/ast: Embed I2C fields in struct ast_connector drm/ast: Managed release of I2C adapter drm/ast: Embed CRTC and connector in struct ast_private drm/ast: Separate DRM driver from PCI code drm/ast: Replace driver load/unload functions with device create/destroy drm/ast: Replace struct_drm_device.dev_private with to_ast_private() drm/ast: Don't use ast->dev if dev is available drm/ast: Embed struct drm_device in struct ast_private drm/ast: Managed release of ast firmware drm/ast: Manage release of firmware backup memory drm/ast: Managed device release
drivers/gpu/drm/ast/ast_cursor.c | 8 +- drivers/gpu/drm/ast/ast_dp501.c | 23 ++- drivers/gpu/drm/ast/ast_drv.c | 82 ++++---- drivers/gpu/drm/ast/ast_drv.h | 43 +++-- drivers/gpu/drm/ast/ast_main.c | 74 ++++---- drivers/gpu/drm/ast/ast_mm.c | 2 +- drivers/gpu/drm/ast/ast_mode.c | 310 ++++++++++++++----------------- drivers/gpu/drm/ast/ast_post.c | 6 +- 8 files changed, 263 insertions(+), 285 deletions(-)
-- 2.27.0
The I2C support feels slammed down to the end of ast_mode.c. Improve this by moving the code before it's callers, remove the declarations, rename the callbacks to match I2C's get/set sda/scl convention, and prefix all functions with ast_. No functional changes have been made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++---------------- 1 file changed, 125 insertions(+), 124 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 154cd877d9d1..19f1dfc8e9e0 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -46,9 +46,6 @@ #include "ast_drv.h" #include "ast_tables.h"
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c); - static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, u8 blue) @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
}
+/* + * I2C + */ + +static int ast_i2c_getscl(void *i2c_priv) +{ + struct ast_i2c_chan *i2c = i2c_priv; + struct ast_private *ast = to_ast_private(i2c->dev); + uint32_t val, val2, count, pass; + + count = 0; + pass = 0; + val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; + do { + val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; + if (val == val2) { + pass++; + } else { + pass = 0; + val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; + } + } while ((pass < 5) && (count++ < 0x10000)); + + return val & 1 ? 1 : 0; +} + +static int ast_i2c_getsda(void *i2c_priv) +{ + struct ast_i2c_chan *i2c = i2c_priv; + struct ast_private *ast = to_ast_private(i2c->dev); + uint32_t val, val2, count, pass; + + count = 0; + pass = 0; + val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; + do { + val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; + if (val == val2) { + pass++; + } else { + pass = 0; + val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; + } + } while ((pass < 5) && (count++ < 0x10000)); + + return val & 1 ? 1 : 0; +} + +static void ast_i2c_setscl(void *i2c_priv, int clock) +{ + struct ast_i2c_chan *i2c = i2c_priv; + struct ast_private *ast = to_ast_private(i2c->dev); + int i; + u8 ujcrb7, jtemp; + + for (i = 0; i < 0x10000; i++) { + ujcrb7 = ((clock & 0x01) ? 0 : 1); + ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7); + jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01); + if (ujcrb7 == jtemp) + break; + } +} + +static void ast_i2c_setsda(void *i2c_priv, int data) +{ + struct ast_i2c_chan *i2c = i2c_priv; + struct ast_private *ast = to_ast_private(i2c->dev); + int i; + u8 ujcrb7, jtemp; + + for (i = 0; i < 0x10000; i++) { + ujcrb7 = ((data & 0x01) ? 0 : 1) << 2; + ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7); + jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04); + if (ujcrb7 == jtemp) + break; + } +} + +static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) +{ + struct ast_i2c_chan *i2c; + int ret; + + i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL); + if (!i2c) + return NULL; + + i2c->adapter.owner = THIS_MODULE; + i2c->adapter.class = I2C_CLASS_DDC; + i2c->adapter.dev.parent = &dev->pdev->dev; + i2c->dev = dev; + i2c_set_adapdata(&i2c->adapter, i2c); + snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), + "AST i2c bit bus"); + i2c->adapter.algo_data = &i2c->bit; + + i2c->bit.udelay = 20; + i2c->bit.timeout = 2; + i2c->bit.data = i2c; + i2c->bit.setsda = ast_i2c_setsda; + i2c->bit.setscl = ast_i2c_setscl; + i2c->bit.getsda = ast_i2c_getsda; + i2c->bit.getscl = ast_i2c_getscl; + ret = i2c_bit_add_bus(&i2c->adapter); + if (ret) { + drm_err(dev, "Failed to register bit i2c\n"); + goto out_free; + } + + return i2c; +out_free: + kfree(i2c); + return NULL; +} + +static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{ + if (!i2c) + return; + i2c_del_adapter(&i2c->adapter); + kfree(i2c); +} + /* * Primary plane */ @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
return 0; } - -static int get_clock(void *i2c_priv) -{ - struct ast_i2c_chan *i2c = i2c_priv; - struct ast_private *ast = to_ast_private(i2c->dev); - uint32_t val, val2, count, pass; - - count = 0; - pass = 0; - val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; - do { - val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; - if (val == val2) { - pass++; - } else { - pass = 0; - val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01; - } - } while ((pass < 5) && (count++ < 0x10000)); - - return val & 1 ? 1 : 0; -} - -static int get_data(void *i2c_priv) -{ - struct ast_i2c_chan *i2c = i2c_priv; - struct ast_private *ast = to_ast_private(i2c->dev); - uint32_t val, val2, count, pass; - - count = 0; - pass = 0; - val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; - do { - val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; - if (val == val2) { - pass++; - } else { - pass = 0; - val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01; - } - } while ((pass < 5) && (count++ < 0x10000)); - - return val & 1 ? 1 : 0; -} - -static void set_clock(void *i2c_priv, int clock) -{ - struct ast_i2c_chan *i2c = i2c_priv; - struct ast_private *ast = to_ast_private(i2c->dev); - int i; - u8 ujcrb7, jtemp; - - for (i = 0; i < 0x10000; i++) { - ujcrb7 = ((clock & 0x01) ? 0 : 1); - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7); - jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01); - if (ujcrb7 == jtemp) - break; - } -} - -static void set_data(void *i2c_priv, int data) -{ - struct ast_i2c_chan *i2c = i2c_priv; - struct ast_private *ast = to_ast_private(i2c->dev); - int i; - u8 ujcrb7, jtemp; - - for (i = 0; i < 0x10000; i++) { - ujcrb7 = ((data & 0x01) ? 0 : 1) << 2; - ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7); - jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04); - if (ujcrb7 == jtemp) - break; - } -} - -static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) -{ - struct ast_i2c_chan *i2c; - int ret; - - i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL); - if (!i2c) - return NULL; - - i2c->adapter.owner = THIS_MODULE; - i2c->adapter.class = I2C_CLASS_DDC; - i2c->adapter.dev.parent = &dev->pdev->dev; - i2c->dev = dev; - i2c_set_adapdata(&i2c->adapter, i2c); - snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), - "AST i2c bit bus"); - i2c->adapter.algo_data = &i2c->bit; - - i2c->bit.udelay = 20; - i2c->bit.timeout = 2; - i2c->bit.data = i2c; - i2c->bit.setsda = set_data; - i2c->bit.setscl = set_clock; - i2c->bit.getsda = get_data; - i2c->bit.getscl = get_clock; - ret = i2c_bit_add_bus(&i2c->adapter); - if (ret) { - drm_err(dev, "Failed to register bit i2c\n"); - goto out_free; - } - - return i2c; -out_free: - kfree(i2c); - return NULL; -} - -static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{ - if (!i2c) - return; - i2c_del_adapter(&i2c->adapter); - kfree(i2c); -}
Hi Thomas.
A few comments related to the code - but not to this patch and not to this patch-set. But I noticed so here goes.
Sam
On Tue, Jul 28, 2020 at 09:44:13AM +0200, Thomas Zimmermann wrote:
The I2C support feels slammed down to the end of ast_mode.c. Improve this by moving the code before it's callers, remove the declarations, rename the callbacks to match I2C's get/set sda/scl convention, and prefix all functions with ast_. No functional changes have been made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++---------------- 1 file changed, 125 insertions(+), 124 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 154cd877d9d1..19f1dfc8e9e0 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -46,9 +46,6 @@ #include "ast_drv.h" #include "ast_tables.h"
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, u8 blue) @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
}
+/*
- I2C
- */
+static int ast_i2c_getscl(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
s/uint32_t/u32
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
So val is a bool - but anyway an int is used.
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
Likewise for val2.
if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
bool to int conversion could do the trick here.
+}
+static int ast_i2c_getsda(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
+}
+static void ast_i2c_setscl(void *i2c_priv, int clock) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
And now u8 is used for registers. Maybe because ast_get_index_reg_mask() returns uint8_t.
So for consistentcy do the uint8_t => u8 etc. conversion first.
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
+}
+static void ast_i2c_setsda(void *i2c_priv, int data) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
+}
+static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) +{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
If ast_private * was passed here and not i2c. Then the implementation of ast_i2c_* could be a tad simpler - no upclassing needed. And then you could drop the drm_device field. (And would need to invent another way to check if i2c is initialized).
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = ast_i2c_setsda;
- i2c->bit.setscl = ast_i2c_setscl;
- i2c->bit.getsda = ast_i2c_getsda;
- i2c->bit.getscl = ast_i2c_getscl;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
+out_free:
- kfree(i2c);
- return NULL;
+}
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
+}
/*
- Primary plane
*/ @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
return 0; }
-static int get_clock(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static int get_data(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static void set_clock(void *i2c_priv, int clock) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
-}
-static void set_data(void *i2c_priv, int data) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
-}
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) -{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = set_data;
- i2c->bit.setscl = set_clock;
- i2c->bit.getsda = get_data;
- i2c->bit.getscl = get_clock;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
-out_free:
- kfree(i2c);
- return NULL;
-}
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
-}
2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
Am 28.07.20 um 20:04 schrieb Sam Ravnborg:
Hi Thomas.
A few comments related to the code - but not to this patch and not to this patch-set. But I noticed so here goes.
You're right on these points. But it's worth a separate patchset. I really just move code around here.
Best regards Thomas
Sam
On Tue, Jul 28, 2020 at 09:44:13AM +0200, Thomas Zimmermann wrote:
The I2C support feels slammed down to the end of ast_mode.c. Improve this by moving the code before it's callers, remove the declarations, rename the callbacks to match I2C's get/set sda/scl convention, and prefix all functions with ast_. No functional changes have been made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++---------------- 1 file changed, 125 insertions(+), 124 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 154cd877d9d1..19f1dfc8e9e0 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -46,9 +46,6 @@ #include "ast_drv.h" #include "ast_tables.h"
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, u8 blue) @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
}
+/*
- I2C
- */
+static int ast_i2c_getscl(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
s/uint32_t/u32
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
So val is a bool - but anyway an int is used.
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;Likewise for val2.
if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
bool to int conversion could do the trick here.
+}
+static int ast_i2c_getsda(void *i2c_priv) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
+}
+static void ast_i2c_setscl(void *i2c_priv, int clock) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
And now u8 is used for registers. Maybe because ast_get_index_reg_mask() returns uint8_t.
So for consistentcy do the uint8_t => u8 etc. conversion first.
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
+}
+static void ast_i2c_setsda(void *i2c_priv, int data) +{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
+}
+static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) +{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
If ast_private * was passed here and not i2c. Then the implementation of ast_i2c_* could be a tad simpler - no upclassing needed. And then you could drop the drm_device field. (And would need to invent another way to check if i2c is initialized).
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = ast_i2c_setsda;
- i2c->bit.setscl = ast_i2c_setscl;
- i2c->bit.getsda = ast_i2c_getsda;
- i2c->bit.getscl = ast_i2c_getscl;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
+out_free:
- kfree(i2c);
- return NULL;
+}
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
+}
/*
- Primary plane
*/ @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
return 0; }
-static int get_clock(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static int get_data(void *i2c_priv) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- uint32_t val, val2, count, pass;
- count = 0;
- pass = 0;
- val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
- do {
val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;if (val == val2) {pass++;} else {pass = 0;val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;}- } while ((pass < 5) && (count++ < 0x10000));
- return val & 1 ? 1 : 0;
-}
-static void set_clock(void *i2c_priv, int clock) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((clock & 0x01) ? 0 : 1);ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);if (ujcrb7 == jtemp)break;- }
-}
-static void set_data(void *i2c_priv, int data) -{
- struct ast_i2c_chan *i2c = i2c_priv;
- struct ast_private *ast = to_ast_private(i2c->dev);
- int i;
- u8 ujcrb7, jtemp;
- for (i = 0; i < 0x10000; i++) {
ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);if (ujcrb7 == jtemp)break;- }
-}
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) -{
- struct ast_i2c_chan *i2c;
- int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
- if (!i2c)
return NULL;- i2c->adapter.owner = THIS_MODULE;
- i2c->adapter.class = I2C_CLASS_DDC;
- i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev;
- i2c_set_adapdata(&i2c->adapter, i2c);
- snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
"AST i2c bit bus");- i2c->adapter.algo_data = &i2c->bit;
- i2c->bit.udelay = 20;
- i2c->bit.timeout = 2;
- i2c->bit.data = i2c;
- i2c->bit.setsda = set_data;
- i2c->bit.setscl = set_clock;
- i2c->bit.getsda = get_data;
- i2c->bit.getscl = get_clock;
- ret = i2c_bit_add_bus(&i2c->adapter);
- if (ret) {
drm_err(dev, "Failed to register bit i2c\n");goto out_free;- }
- return i2c;
-out_free:
- kfree(i2c);
- return NULL;
-}
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{
- if (!i2c)
return;- i2c_del_adapter(&i2c->adapter);
- kfree(i2c);
-}
2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
The ast driver is supposed to work without I2C support. This is tested by looking at the connector's i2c field being non-NULL.
After embedding the I2C structure in the connector, the i2c field will not be a pointer. So change the test to look at the dev field in struct ast_i2c_chan.
ast_get_modes() did not really test if I2C has been initialized, so the patch adds this test as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 19f1dfc8e9e0..45be020afcad 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -603,7 +603,6 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) i2c->adapter.owner = THIS_MODULE; i2c->adapter.class = I2C_CLASS_DDC; i2c->adapter.dev.parent = &dev->pdev->dev; - i2c->dev = dev; i2c_set_adapdata(&i2c->adapter, i2c); snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST i2c bit bus"); @@ -622,17 +621,30 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) goto out_free; }
+ i2c->dev = dev; /* signals presence of I2C support */ + return i2c; out_free: kfree(i2c); return NULL; }
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) { - if (!i2c) + return i2c && !!i2c->dev; +} + +static void ast_i2c_fini(struct ast_i2c_chan *i2c) +{ + if (!ast_i2c_is_initialized(i2c)) return; i2c_del_adapter(&i2c->adapter); + i2c->dev = NULL; /* clear to signal absence of I2C support */ +} + +static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{ + ast_i2c_fini(i2c); kfree(i2c); }
@@ -1054,6 +1066,7 @@ static int ast_get_modes(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); struct ast_private *ast = to_ast_private(connector->dev); + struct ast_i2c_chan *i2c = ast_connector->i2c; struct edid *edid; int ret; bool flags = false; @@ -1069,8 +1082,8 @@ static int ast_get_modes(struct drm_connector *connector) else kfree(edid); } - if (!flags) - edid = drm_get_edid(connector, &ast_connector->i2c->adapter); + if (!flags && ast_i2c_is_initialized(i2c)) + edid = drm_get_edid(connector, &i2c->adapter); if (edid) { drm_connector_update_edid_property(&ast_connector->base, edid); ret = drm_add_edid_modes(connector, edid);
Hi Thomas.
On Tue, Jul 28, 2020 at 09:44:14AM +0200, Thomas Zimmermann wrote:
The ast driver is supposed to work without I2C support. This is tested by looking at the connector's i2c field being non-NULL.
After embedding the I2C structure in the connector, the i2c field will not be a pointer. So change the test to look at the dev field in struct ast_i2c_chan.
ast_get_modes() did not really test if I2C has been initialized, so the patch adds this test as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 19f1dfc8e9e0..45be020afcad 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -603,7 +603,6 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) i2c->adapter.owner = THIS_MODULE; i2c->adapter.class = I2C_CLASS_DDC; i2c->adapter.dev.parent = &dev->pdev->dev;
- i2c->dev = dev; i2c_set_adapdata(&i2c->adapter, i2c); snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST i2c bit bus");
@@ -622,17 +621,30 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) goto out_free; }
- i2c->dev = dev; /* signals presence of I2C support */
- return i2c;
out_free: kfree(i2c); return NULL; }
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) {
- if (!i2c)
- return i2c && !!i2c->dev;
+}
It seems pointless to convert the pointer to bool here ("!!").
+static void ast_i2c_fini(struct ast_i2c_chan *i2c) +{
- if (!ast_i2c_is_initialized(i2c)) return;
Empty line after return?
i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c) +{
- ast_i2c_fini(i2c); kfree(i2c);
}
@@ -1054,6 +1066,7 @@ static int ast_get_modes(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); struct ast_private *ast = to_ast_private(connector->dev);
- struct ast_i2c_chan *i2c = ast_connector->i2c; struct edid *edid; int ret; bool flags = false;
@@ -1069,8 +1082,8 @@ static int ast_get_modes(struct drm_connector *connector) else kfree(edid); }
- if (!flags)
edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
- if (!flags && ast_i2c_is_initialized(i2c))
edid = drm_get_edid(connector, &i2c->adapter);-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
With the I2C fields embedded in struct ast_connector, the related call to kzalloc() can be removed.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.h | 3 +-- drivers/gpu/drm/ast/ast_mode.c | 33 ++++++++++----------------------- 2 files changed, 11 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index e3a264ac7ee2..d303df568099 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -98,7 +98,6 @@ enum ast_tx_chip { #define AST_HWC_SIGNATURE_HOTSPOTX 0x14 #define AST_HWC_SIGNATURE_HOTSPOTY 0x18
- struct ast_private { struct drm_device *dev;
@@ -234,7 +233,7 @@ struct ast_i2c_chan {
struct ast_connector { struct drm_connector base; - struct ast_i2c_chan *i2c; + struct ast_i2c_chan i2c; };
#define to_ast_connector(x) container_of(x, struct ast_connector, base) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 45be020afcad..f421a60d8a96 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -591,15 +591,10 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) +static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { - struct ast_i2c_chan *i2c; int ret;
- i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL); - if (!i2c) - return NULL; - i2c->adapter.owner = THIS_MODULE; i2c->adapter.class = I2C_CLASS_DDC; i2c->adapter.dev.parent = &dev->pdev->dev; @@ -618,20 +613,17 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) ret = i2c_bit_add_bus(&i2c->adapter); if (ret) { drm_err(dev, "Failed to register bit i2c\n"); - goto out_free; + return ret; }
i2c->dev = dev; /* signals presence of I2C support */
- return i2c; -out_free: - kfree(i2c); - return NULL; + return 0; }
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) { - return i2c && !!i2c->dev; + return !!i2c->dev; }
static void ast_i2c_fini(struct ast_i2c_chan *i2c) @@ -642,12 +634,6 @@ static void ast_i2c_fini(struct ast_i2c_chan *i2c) i2c->dev = NULL; /* clear to signal absence of I2C support */ }
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{ - ast_i2c_fini(i2c); - kfree(i2c); -} - /* * Primary plane */ @@ -1066,7 +1052,7 @@ static int ast_get_modes(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); struct ast_private *ast = to_ast_private(connector->dev); - struct ast_i2c_chan *i2c = ast_connector->i2c; + struct ast_i2c_chan *i2c = &ast_connector->i2c; struct edid *edid; int ret; bool flags = false; @@ -1154,7 +1140,7 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, static void ast_connector_destroy(struct drm_connector *connector) { struct ast_connector *ast_connector = to_ast_connector(connector); - ast_i2c_destroy(ast_connector->i2c); + ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector); } @@ -1177,20 +1163,21 @@ static int ast_connector_init(struct drm_device *dev) struct ast_connector *ast_connector; struct drm_connector *connector; struct drm_encoder *encoder; + int ret;
ast_connector = kzalloc(sizeof(struct ast_connector), GFP_KERNEL); if (!ast_connector) return -ENOMEM;
connector = &ast_connector->base; - ast_connector->i2c = ast_i2c_create(dev); - if (!ast_connector->i2c) + ret = ast_i2c_init(&ast_connector->i2c, dev); + if (ret) drm_err(dev, "failed to add ddc bus for connector\n");
drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c->adapter); + &ast_connector->i2c.adapter);
drm_connector_helper_add(connector, &ast_connector_helper_funcs);
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{ + struct ast_i2c_chan *i2c = data; + + i2c_del_adapter(&i2c->adapter); + i2c->dev = NULL; /* clear to signal absence of I2C support */ +} + static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0; + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); }
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{ - if (!ast_i2c_is_initialized(i2c)) - return; - i2c_del_adapter(&i2c->adapter); - i2c->dev = NULL; /* clear to signal absence of I2C support */ -} - /* * Primary plane */ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) { - struct ast_connector *ast_connector = to_ast_connector(connector); - ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector); }
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I think this breaks bisect, since at this point the release callback is called when the connector is already gone. At the end of the series it's fine again though.
I've done a very cursory reading of your series to look for high-level issues, I think overall reasonable. On the series:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
But maybe you want to polish a bit more, up to you. -Daniel
drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{
- struct ast_i2c_chan *i2c = data;
- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0;
- return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
}
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{
- if (!ast_i2c_is_initialized(i2c))
return;- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
/*
- Primary plane
*/ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) {
- struct ast_connector *ast_connector = to_ast_connector(connector);
- ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector);
}
2.27.0
Hi
Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch:
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I think this breaks bisect, since at this point the release callback is called when the connector is already gone. At the end of the series it's fine again though.
I've done a very cursory reading of your series to look for high-level issues, I think overall reasonable. On the series:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
But maybe you want to polish a bit more, up to you.
Thanks. I'll address your points and wait a bit longer. Usually Sam has a number of good comments as well.
Best regards Thomas
-Daniel
drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{
- struct ast_i2c_chan *i2c = data;
- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0;
- return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
}
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{
- if (!ast_i2c_is_initialized(i2c))
return;- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
/*
- Primary plane
*/ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) {
- struct ast_connector *ast_connector = to_ast_connector(connector);
- ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector);
}
2.27.0
Hi
Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch:
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
I think this breaks bisect, since at this point the release callback is called when the connector is already gone. At the end of the series it's fine again though.
I'll move the auto-release of I2C to the end of the series. It should work then.
Best regards Thomas
I've done a very cursory reading of your series to look for high-level issues, I think overall reasonable. On the series:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
But maybe you want to polish a bit more, up to you. -Daniel
drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{
- struct ast_i2c_chan *i2c = data;
- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0;
- return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
}
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{
- if (!ast_i2c_is_initialized(i2c))
return;- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
/*
- Primary plane
*/ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) {
- struct ast_connector *ast_connector = to_ast_connector(connector);
- ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector);
}
2.27.0
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{
- struct ast_i2c_chan *i2c = data;
- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0;
- return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
}
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{
- if (!ast_i2c_is_initialized(i2c))
return;- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
The intro of ast_i2c_fini() and then removal again confuses me a little. But end result looks simple so I guess thats what counts.
Sam
/*
- Primary plane
*/ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) {
- struct ast_connector *ast_connector = to_ast_connector(connector);
- ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector);
}
2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi
Am 28.07.20 um 20:06 schrieb Sam Ravnborg:
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
Managed releases of the device's I2C adapter simplify the connector's release.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *data) +{
- struct ast_i2c_chan *i2c = data;
- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
i2c->dev = dev; /* signals presence of I2C support */
- return 0;
- return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
}
static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; }
-static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{
- if (!ast_i2c_is_initialized(i2c))
return;- i2c_del_adapter(&i2c->adapter);
- i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
The intro of ast_i2c_fini() and then removal again confuses me a little. But end result looks simple so I guess thats what counts.
The intention was to separate _fini from _destroy and make the patch series more readable. But looking at it now, this idea did not really work. I guess, I'll drop _fini.
Best regards Thomas
Sam
/*
- Primary plane
*/ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
static void ast_connector_destroy(struct drm_connector *connector) {
- struct ast_connector *ast_connector = to_ast_connector(connector);
- ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector);
}
2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Only single instances of CRTC and connector are supported per device. Embed both in ast's structure and remove the individual memory allocations. DRM's CRTC/connector cleanup helpers replace the rsp. destroy functions in ast.
While at it, also convert to_ast_connector() to a function.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.h | 34 ++++++++++++++----------- drivers/gpu/drm/ast/ast_mode.c | 46 ++++++++-------------------------- 2 files changed, 31 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index d303df568099..b401560e4e8f 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -98,6 +98,23 @@ enum ast_tx_chip { #define AST_HWC_SIGNATURE_HOTSPOTX 0x14 #define AST_HWC_SIGNATURE_HOTSPOTY 0x18
+struct ast_i2c_chan { + struct i2c_adapter adapter; + struct drm_device *dev; + struct i2c_algo_bit_data bit; +}; + +struct ast_connector { + struct drm_connector base; + struct ast_i2c_chan i2c; +}; + +static inline struct ast_connector * +to_ast_connector(struct drm_connector *connector) +{ + return container_of(connector, struct ast_connector, base); +} + struct ast_private { struct drm_device *dev;
@@ -118,9 +135,11 @@ struct ast_private { unsigned int next_index; } cursor;
- struct drm_encoder encoder; struct drm_plane primary_plane; struct drm_plane cursor_plane; + struct drm_crtc crtc; + struct drm_encoder encoder; + struct ast_connector connector;
bool support_wide_screen; enum { @@ -225,19 +244,6 @@ static inline void ast_open_key(struct ast_private *ast)
#define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
-struct ast_i2c_chan { - struct i2c_adapter adapter; - struct drm_device *dev; - struct i2c_algo_bit_data bit; -}; - -struct ast_connector { - struct drm_connector base; - struct ast_i2c_chan i2c; -}; - -#define to_ast_connector(x) container_of(x, struct ast_connector, base) - struct ast_vbios_stdtable { u8 misc; u8 seq[4]; diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 27eb49bd12b3..201313ab3e71 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -952,12 +952,6 @@ static void ast_crtc_reset(struct drm_crtc *crtc) __drm_atomic_helper_crtc_reset(crtc, &ast_state->base); }
-static void ast_crtc_destroy(struct drm_crtc *crtc) -{ - drm_crtc_cleanup(crtc); - kfree(crtc); -} - static struct drm_crtc_state * ast_crtc_atomic_duplicate_state(struct drm_crtc *crtc) { @@ -993,7 +987,7 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc, static const struct drm_crtc_funcs ast_crtc_funcs = { .reset = ast_crtc_reset, .gamma_set = drm_atomic_helper_legacy_gamma_set, - .destroy = ast_crtc_destroy, + .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, .atomic_duplicate_state = ast_crtc_atomic_duplicate_state, @@ -1003,27 +997,19 @@ static const struct drm_crtc_funcs ast_crtc_funcs = { static int ast_crtc_init(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev); - struct drm_crtc *crtc; + struct drm_crtc *crtc = &ast->crtc; int ret;
- crtc = kzalloc(sizeof(*crtc), GFP_KERNEL); - if (!crtc) - return -ENOMEM; - ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane, &ast->cursor_plane, &ast_crtc_funcs, NULL); if (ret) - goto err_kfree; + return ret;
drm_mode_crtc_set_gamma_size(crtc, 256); drm_crtc_helper_add(crtc, &ast_crtc_helper_funcs);
return 0; - -err_kfree: - kfree(crtc); - return ret; }
/* @@ -1138,12 +1124,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, return flags; }
-static void ast_connector_destroy(struct drm_connector *connector) -{ - drm_connector_cleanup(connector); - kfree(connector); -} - static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { .get_modes = ast_get_modes, .mode_valid = ast_mode_valid, @@ -1152,31 +1132,28 @@ static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { static const struct drm_connector_funcs ast_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = ast_connector_destroy, + .destroy = drm_connector_cleanup, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
static int ast_connector_init(struct drm_device *dev) { - struct ast_connector *ast_connector; - struct drm_connector *connector; - struct drm_encoder *encoder; + struct ast_private *ast = to_ast_private(dev); + struct ast_connector *ast_connector = &ast->connector; + struct drm_connector *connector = &ast_connector->base; + struct drm_encoder *encoder = &ast->encoder; + struct ast_i2c_chan *i2c = &ast_connector->i2c; int ret;
- ast_connector = kzalloc(sizeof(struct ast_connector), GFP_KERNEL); - if (!ast_connector) - return -ENOMEM; - - connector = &ast_connector->base; - ret = ast_i2c_init(&ast_connector->i2c, dev); + ret = ast_i2c_init(i2c, dev); if (ret) drm_err(dev, "failed to add ddc bus for connector\n");
drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c.adapter); + &i2c->adapter);
drm_connector_helper_add(connector, &ast_connector_helper_funcs);
@@ -1185,7 +1162,6 @@ static int ast_connector_init(struct drm_device *dev)
connector->polled = DRM_CONNECTOR_POLL_CONNECT;
- encoder = list_first_entry(&dev->mode_config.encoder_list, struct drm_encoder, head); drm_connector_attach_encoder(connector, encoder);
return 0;
Putting the DRM driver to the top of the file and the PCI code to the bottom makes ast_drv.c more readable. While at it, the patch prefixes file-scope variables with ast_.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.c | 59 ++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index 0b58f7aee6b0..9d04f2b5225c 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -43,9 +43,33 @@ int ast_modeset = -1; MODULE_PARM_DESC(modeset, "Disable/Enable modesetting"); module_param_named(modeset, ast_modeset, int, 0400);
-#define PCI_VENDOR_ASPEED 0x1a03 +/* + * DRM driver + */ + +DEFINE_DRM_GEM_FOPS(ast_fops); + +static struct drm_driver ast_driver = { + .driver_features = DRIVER_ATOMIC | + DRIVER_GEM | + DRIVER_MODESET, + + .fops = &ast_fops, + .name = DRIVER_NAME, + .desc = DRIVER_DESC, + .date = DRIVER_DATE, + .major = DRIVER_MAJOR, + .minor = DRIVER_MINOR, + .patchlevel = DRIVER_PATCHLEVEL,
-static struct drm_driver driver; + DRM_GEM_VRAM_DRIVER +}; + +/* + * PCI driver + */ + +#define PCI_VENDOR_ASPEED 0x1a03
#define AST_VGA_DEVICE(id, info) { \ .class = PCI_BASE_CLASS_DISPLAY << 16, \ @@ -56,13 +80,13 @@ static struct drm_driver driver; .subdevice = PCI_ANY_ID, \ .driver_data = (unsigned long) info }
-static const struct pci_device_id pciidlist[] = { +static const struct pci_device_id ast_pciidlist[] = { AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL), AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL), {0, 0, 0}, };
-MODULE_DEVICE_TABLE(pci, pciidlist); +MODULE_DEVICE_TABLE(pci, ast_pciidlist);
static void ast_kick_out_firmware_fb(struct pci_dev *pdev) { @@ -94,7 +118,7 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) return ret;
- dev = drm_dev_alloc(&driver, &pdev->dev); + dev = drm_dev_alloc(&ast_driver, &pdev->dev); if (IS_ERR(dev)) return PTR_ERR(dev);
@@ -118,11 +142,9 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_drm_dev_put: drm_dev_put(dev); return ret; - }
-static void -ast_pci_remove(struct pci_dev *pdev) +static void ast_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev);
@@ -217,30 +239,12 @@ static const struct dev_pm_ops ast_pm_ops = {
static struct pci_driver ast_pci_driver = { .name = DRIVER_NAME, - .id_table = pciidlist, + .id_table = ast_pciidlist, .probe = ast_pci_probe, .remove = ast_pci_remove, .driver.pm = &ast_pm_ops, };
-DEFINE_DRM_GEM_FOPS(ast_fops); - -static struct drm_driver driver = { - .driver_features = DRIVER_ATOMIC | - DRIVER_GEM | - DRIVER_MODESET, - - .fops = &ast_fops, - .name = DRIVER_NAME, - .desc = DRIVER_DESC, - .date = DRIVER_DATE, - .major = DRIVER_MAJOR, - .minor = DRIVER_MINOR, - .patchlevel = DRIVER_PATCHLEVEL, - - DRM_GEM_VRAM_DRIVER -}; - static int __init ast_init(void) { if (vgacon_text_force() && ast_modeset == -1) @@ -261,4 +265,3 @@ module_exit(ast_exit); MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL and additional rights"); -
The ast driver's load and unload functions are left-overs from when struct drm_driver.load/unload was still in use. The PCI probe helper allocated the DRM device and ran load to initialize it.
This patch replaces this code with device create and destroy. The main difference is that the device's create function allocates the DRM device and ast structures in the same place. This will be required for switching ast to managed allocations.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.c | 24 +++++++++++------------- drivers/gpu/drm/ast/ast_drv.h | 6 ++++-- drivers/gpu/drm/ast/ast_main.c | 24 ++++++++++++++++++------ 3 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index 9d04f2b5225c..ad93c35b4cf7 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -109,6 +109,7 @@ static void ast_kick_out_firmware_fb(struct pci_dev *pdev)
static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { + struct ast_private *ast; struct drm_device *dev; int ret;
@@ -118,27 +119,23 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) return ret;
- dev = drm_dev_alloc(&ast_driver, &pdev->dev); - if (IS_ERR(dev)) - return PTR_ERR(dev); - - dev->pdev = pdev; - pci_set_drvdata(pdev, dev); - - ret = ast_driver_load(dev, ent->driver_data); - if (ret) + ast = ast_device_create(&ast_driver, pdev, ent->driver_data); + if (IS_ERR(ast)) { + ret = PTR_ERR(ast); goto err_drm_dev_put; + } + dev = ast->dev;
ret = drm_dev_register(dev, ent->driver_data); if (ret) - goto err_ast_driver_unload; + goto err_ast_device_destroy;
drm_fbdev_generic_setup(dev, 32);
return 0;
-err_ast_driver_unload: - ast_driver_unload(dev); +err_ast_device_destroy: + ast_device_destroy(ast); err_drm_dev_put: drm_dev_put(dev); return ret; @@ -147,9 +144,10 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) static void ast_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); + struct ast_private *ast = to_ast_private(dev);
drm_dev_unregister(dev); - ast_driver_unload(dev); + ast_device_destroy(ast); drm_dev_put(dev); }
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index b401560e4e8f..f1aebc719d9e 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -159,8 +159,10 @@ static inline struct ast_private *to_ast_private(struct drm_device *dev) return dev->dev_private; }
-int ast_driver_load(struct drm_device *dev, unsigned long flags); -void ast_driver_unload(struct drm_device *dev); +struct ast_private *ast_device_create(struct drm_driver *drv, + struct pci_dev *pdev, + unsigned long flags); +void ast_device_destroy(struct ast_private *ast);
#define AST_IO_AR_PORT_WRITE (0x40) #define AST_IO_MISC_PORT_WRITE (0x42) diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index dd12b55d57a2..8d46166f8462 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -30,6 +30,7 @@
#include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_drv.h> #include <drm/drm_gem.h> #include <drm/drm_gem_vram_helper.h>
@@ -378,15 +379,25 @@ static int ast_get_dram_info(struct drm_device *dev) return 0; }
-int ast_driver_load(struct drm_device *dev, unsigned long flags) +struct ast_private *ast_device_create(struct drm_driver *drv, + struct pci_dev *pdev, + unsigned long flags) { + struct drm_device *dev; struct ast_private *ast; bool need_post; int ret = 0;
+ dev = drm_dev_alloc(drv, &pdev->dev); + if (IS_ERR(dev)) + return ERR_CAST(dev); + + dev->pdev = pdev; + pci_set_drvdata(pdev, dev); + ast = kzalloc(sizeof(struct ast_private), GFP_KERNEL); if (!ast) - return -ENOMEM; + return ERR_PTR(-ENOMEM);
dev->dev_private = ast; ast->dev = dev; @@ -435,16 +446,17 @@ int ast_driver_load(struct drm_device *dev, unsigned long flags) if (ret) goto out_free;
- return 0; + return ast; + out_free: kfree(ast); dev->dev_private = NULL; - return ret; + return ERR_PTR(ret); }
-void ast_driver_unload(struct drm_device *dev) +void ast_device_destroy(struct ast_private *ast) { - struct ast_private *ast = to_ast_private(dev); + struct drm_device *dev = ast->dev;
/* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
The ast code still references dev_private in several place when looking up the ast device structure. Convert the remaining locations to use to_ast_private().
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_cursor.c | 2 +- drivers/gpu/drm/ast/ast_mode.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c index acf0d23514e8..8d693c8b346f 100644 --- a/drivers/gpu/drm/ast/ast_cursor.c +++ b/drivers/gpu/drm/ast/ast_cursor.c @@ -47,7 +47,7 @@ static void ast_cursor_fini(struct ast_private *ast)
static void ast_cursor_release(struct drm_device *dev, void *ptr) { - struct ast_private *ast = dev->dev_private; + struct ast_private *ast = to_ast_private(dev);
ast_cursor_fini(ast); } diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 201313ab3e71..01340b0e40f8 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -784,7 +784,7 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane, { struct drm_plane_state *state = plane->state; struct drm_framebuffer *fb = state->fb; - struct ast_private *ast = plane->dev->dev_private; + struct ast_private *ast = to_ast_private(plane->dev); unsigned int offset_x, offset_y;
offset_x = AST_MAX_HWC_WIDTH - fb->width;
Several places in ast use ast->dev, when a dev pointer is already available within the function. Remove the extra indirection. No functional changes made.
This is just a small cleanup before embedding the DRM device instance in struct ast_private.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 2 +- drivers/gpu/drm/ast/ast_post.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 01340b0e40f8..dceb11a320b2 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1196,7 +1196,7 @@ int ast_mode_config_init(struct ast_private *ast) dev->mode_config.min_height = 0; dev->mode_config.preferred_depth = 24; dev->mode_config.prefer_shadow = 1; - dev->mode_config.fb_base = pci_resource_start(ast->dev->pdev, 0); + dev->mode_config.fb_base = pci_resource_start(dev->pdev, 0);
if (ast->chip == AST2100 || ast->chip == AST2200 || diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index c043fe717553..b1d42a639ece 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -368,9 +368,9 @@ void ast_post_gpu(struct drm_device *dev) u32 reg; struct ast_private *ast = to_ast_private(dev);
- pci_read_config_dword(ast->dev->pdev, 0x04, ®); + pci_read_config_dword(dev->pdev, 0x04, ®); reg |= 0x3; - pci_write_config_dword(ast->dev->pdev, 0x04, reg); + pci_write_config_dword(dev->pdev, 0x04, reg);
ast_enable_vga(dev); ast_open_key(ast);
Turns struct ast_private into a subclass of struct drm_device by embedding the latter. This allows for using DRM's managed device allocation.
The use of struct drm_device.dev_private is deprecated. The patch converts the last remaining users to to_ast_private().
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_cursor.c | 6 ++--- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.h | 4 +-- drivers/gpu/drm/ast/ast_main.c | 42 ++++++++++---------------------- drivers/gpu/drm/ast/ast_mm.c | 2 +- drivers/gpu/drm/ast/ast_mode.c | 2 +- drivers/gpu/drm/ast/ast_post.c | 2 +- 7 files changed, 22 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c index 8d693c8b346f..6c96f74cdb9e 100644 --- a/drivers/gpu/drm/ast/ast_cursor.c +++ b/drivers/gpu/drm/ast/ast_cursor.c @@ -57,7 +57,7 @@ static void ast_cursor_release(struct drm_device *dev, void *ptr) */ int ast_cursor_init(struct ast_private *ast) { - struct drm_device *dev = ast->dev; + struct drm_device *dev = &ast->base; size_t size, i; struct drm_gem_vram_object *gbo; void __iomem *vaddr; @@ -168,7 +168,7 @@ static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int h
int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb) { - struct drm_device *dev = ast->dev; + struct drm_device *dev = &ast->base; struct drm_gem_vram_object *gbo; int ret; void *src; @@ -217,7 +217,7 @@ static void ast_cursor_set_base(struct ast_private *ast, u64 address)
void ast_cursor_page_flip(struct ast_private *ast) { - struct drm_device *dev = ast->dev; + struct drm_device *dev = &ast->base; struct drm_gem_vram_object *gbo; s64 off;
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index ad93c35b4cf7..c394383a7979 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -124,7 +124,7 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) ret = PTR_ERR(ast); goto err_drm_dev_put; } - dev = ast->dev; + dev = &ast->base;
ret = drm_dev_register(dev, ent->driver_data); if (ret) diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index f1aebc719d9e..86c9a7ac712b 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -116,7 +116,7 @@ to_ast_connector(struct drm_connector *connector) }
struct ast_private { - struct drm_device *dev; + struct drm_device base;
void __iomem *regs; void __iomem *ioregs; @@ -156,7 +156,7 @@ struct ast_private {
static inline struct ast_private *to_ast_private(struct drm_device *dev) { - return dev->dev_private; + return container_of(dev, struct ast_private, base); }
struct ast_private *ast_device_create(struct drm_driver *drv, diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 8d46166f8462..792fb7f616ec 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -388,25 +388,17 @@ struct ast_private *ast_device_create(struct drm_driver *drv, bool need_post; int ret = 0;
- dev = drm_dev_alloc(drv, &pdev->dev); - if (IS_ERR(dev)) - return ERR_CAST(dev); + ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_private, base); + if (IS_ERR(ast)) + return ast; + dev = &ast->base;
dev->pdev = pdev; pci_set_drvdata(pdev, dev);
- ast = kzalloc(sizeof(struct ast_private), GFP_KERNEL); - if (!ast) - return ERR_PTR(-ENOMEM); - - dev->dev_private = ast; - ast->dev = dev; - ast->regs = pci_iomap(dev->pdev, 1, 0); - if (!ast->regs) { - ret = -EIO; - goto out_free; - } + if (!ast->regs) + return ERR_PTR(-EIO);
/* * If we don't have IO space at all, use MMIO now and @@ -421,17 +413,16 @@ struct ast_private *ast_device_create(struct drm_driver *drv, /* "map" IO regs if the above hasn't done so already */ if (!ast->ioregs) { ast->ioregs = pci_iomap(dev->pdev, 2, 0); - if (!ast->ioregs) { - ret = -EIO; - goto out_free; - } + if (!ast->ioregs) + return ERR_PTR(-EIO); }
ast_detect_chip(dev, &need_post);
ret = ast_get_dram_info(dev); if (ret) - goto out_free; + return ERR_PTR(ret); + drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d\n", ast->mclk, ast->dram_type, ast->dram_bus_width);
@@ -440,29 +431,22 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
ret = ast_mm_init(ast); if (ret) - goto out_free; + return ERR_PTR(ret);
ret = ast_mode_config_init(ast); if (ret) - goto out_free; + return ERR_PTR(ret);
return ast; - -out_free: - kfree(ast); - dev->dev_private = NULL; - return ERR_PTR(ret); }
void ast_device_destroy(struct ast_private *ast) { - struct drm_device *dev = ast->dev; + struct drm_device *dev = &ast->base;
/* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
ast_release_firmware(dev); kfree(ast->dp501_fw_addr); - - kfree(ast); } diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c index 9186ec3ebbe0..8392ebde504b 100644 --- a/drivers/gpu/drm/ast/ast_mm.c +++ b/drivers/gpu/drm/ast/ast_mm.c @@ -85,9 +85,9 @@ static void ast_mm_release(struct drm_device *dev, void *ptr)
int ast_mm_init(struct ast_private *ast) { + struct drm_device *dev = &ast->base; u32 vram_size; int ret; - struct drm_device *dev = ast->dev;
vram_size = ast_get_vram_size(ast);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index dceb11a320b2..4a4010fdfd2d 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1180,7 +1180,7 @@ static const struct drm_mode_config_funcs ast_mode_config_funcs = {
int ast_mode_config_init(struct ast_private *ast) { - struct drm_device *dev = ast->dev; + struct drm_device *dev = &ast->base; int ret;
ret = ast_cursor_init(ast); diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index b1d42a639ece..8902c2f84bf9 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -365,8 +365,8 @@ static void ast_init_dram_reg(struct drm_device *dev)
void ast_post_gpu(struct drm_device *dev) { - u32 reg; struct ast_private *ast = to_ast_private(dev); + u32 reg;
pci_read_config_dword(dev->pdev, 0x04, ®); reg |= 0x3;
The ast driver loads firmware for the DP501 display encoder. The patch replaces the removal code with a managed release function.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 3 --- 3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4b85a504825a..88121c0e0d05 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -8,11 +8,24 @@
MODULE_FIRMWARE("ast_dp501_fw.bin");
+static void ast_release_firmware(void *data) +{ + struct ast_private *ast = data; + + release_firmware(ast->dp501_fw); + ast->dp501_fw = NULL; +} + static int ast_load_dp501_microcode(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev); + int ret; + + ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); + if (ret) + return ret;
- return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); + return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); }
static void send_ack(struct ast_private *ast) @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) } } } - -void ast_release_firmware(struct drm_device *dev) -{ - struct ast_private *ast = to_ast_private(dev); - - release_firmware(ast->dp501_fw); - ast->dp501_fw = NULL; -} diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 86c9a7ac712b..02908d005b99 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); -void ast_release_firmware(struct drm_device *dev);
/* ast_cursor.c */ int ast_cursor_init(struct ast_private *ast); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 792fb7f616ec..e3b7748335a3 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
void ast_device_destroy(struct ast_private *ast) { - struct drm_device *dev = &ast->base; - /* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
- ast_release_firmware(dev); kfree(ast->dp501_fw_addr); }
On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
The ast driver loads firmware for the DP501 display encoder. The patch replaces the removal code with a managed release function.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Hm a devm_request_firmware which does exactly this would be nice I think. Maybe as a follow-up refactor? -Daniel
drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 3 --- 3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4b85a504825a..88121c0e0d05 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -8,11 +8,24 @@
MODULE_FIRMWARE("ast_dp501_fw.bin");
+static void ast_release_firmware(void *data) +{
- struct ast_private *ast = data;
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
+}
static int ast_load_dp501_microcode(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev);
- int ret;
- ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- if (ret)
return ret;
- return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
}
static void send_ack(struct ast_private *ast) @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) } } }
-void ast_release_firmware(struct drm_device *dev) -{
- struct ast_private *ast = to_ast_private(dev);
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
-} diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 86c9a7ac712b..02908d005b99 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); -void ast_release_firmware(struct drm_device *dev);
/* ast_cursor.c */ int ast_cursor_init(struct ast_private *ast); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 792fb7f616ec..e3b7748335a3 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
void ast_device_destroy(struct ast_private *ast) {
struct drm_device *dev = &ast->base;
/* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
ast_release_firmware(dev); kfree(ast->dp501_fw_addr);
}
2.27.0
Hi
Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch:
On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
The ast driver loads firmware for the DP501 display encoder. The patch replaces the removal code with a managed release function.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Hm a devm_request_firmware which does exactly this would be nice I think. Maybe as a follow-up refactor?
There are so many ideas for follow-up patches wrt. devres and drmres, we should add a todo item to collect them. Especially, devres is much more over head in terms of reviews and kernel building/testing tha tit makes sense to collect ideas and address them in larger chunks.
Best regards Thomas
-Daniel
drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 3 --- 3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4b85a504825a..88121c0e0d05 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -8,11 +8,24 @@
MODULE_FIRMWARE("ast_dp501_fw.bin");
+static void ast_release_firmware(void *data) +{
- struct ast_private *ast = data;
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
+}
static int ast_load_dp501_microcode(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev);
- int ret;
- ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- if (ret)
return ret;
- return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
}
static void send_ack(struct ast_private *ast) @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) } } }
-void ast_release_firmware(struct drm_device *dev) -{
- struct ast_private *ast = to_ast_private(dev);
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
-} diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 86c9a7ac712b..02908d005b99 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); -void ast_release_firmware(struct drm_device *dev);
/* ast_cursor.c */ int ast_cursor_init(struct ast_private *ast); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 792fb7f616ec..e3b7748335a3 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
void ast_device_destroy(struct ast_private *ast) {
struct drm_device *dev = &ast->base;
/* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
ast_release_firmware(dev); kfree(ast->dp501_fw_addr);
}
2.27.0
On Tue, Jul 28, 2020 at 11:32:04AM +0200, Thomas Zimmermann wrote:
Hi
Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch:
On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
The ast driver loads firmware for the DP501 display encoder. The patch replaces the removal code with a managed release function.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Hm a devm_request_firmware which does exactly this would be nice I think. Maybe as a follow-up refactor?
There are so many ideas for follow-up patches wrt. devres and drmres, we should add a todo item to collect them. Especially, devres is much more over head in terms of reviews and kernel building/testing tha tit makes sense to collect ideas and address them in larger chunks.
Yeah maybe a section with wanted devres functions in todo.rst makes sense. For devres it depends which subsystem you're dealing with I guess, and how much they want to see before it lands. -Daniel
Best regards Thomas
-Daniel
drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 3 --- 3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4b85a504825a..88121c0e0d05 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -8,11 +8,24 @@
MODULE_FIRMWARE("ast_dp501_fw.bin");
+static void ast_release_firmware(void *data) +{
- struct ast_private *ast = data;
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
+}
static int ast_load_dp501_microcode(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev);
- int ret;
- ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- if (ret)
return ret;
- return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
- return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
}
static void send_ack(struct ast_private *ast) @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) } } }
-void ast_release_firmware(struct drm_device *dev) -{
- struct ast_private *ast = to_ast_private(dev);
- release_firmware(ast->dp501_fw);
- ast->dp501_fw = NULL;
-} diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 86c9a7ac712b..02908d005b99 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); -void ast_release_firmware(struct drm_device *dev);
/* ast_cursor.c */ int ast_cursor_init(struct ast_private *ast); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 792fb7f616ec..e3b7748335a3 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
void ast_device_destroy(struct ast_private *ast) {
struct drm_device *dev = &ast->base;
/* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
ast_release_firmware(dev); kfree(ast->dp501_fw_addr);
}
2.27.0
-- 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
The ast driver keeps a backup copy of the DP501 encoder's firmware. This patch adds managed release of the allocated memory.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index e3b7748335a3..67e20727d82d 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -33,6 +33,7 @@ #include <drm/drm_drv.h> #include <drm/drm_gem.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h>
#include "ast_drv.h"
@@ -231,11 +232,11 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post) ast->tx_chip_type = AST_TX_SIL164; break; case 0x08: - ast->dp501_fw_addr = kzalloc(32*1024, GFP_KERNEL); + ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL); if (ast->dp501_fw_addr) { /* backup firmware */ if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) { - kfree(ast->dp501_fw_addr); + drmm_kfree(dev, ast->dp501_fw_addr); ast->dp501_fw_addr = NULL; } } @@ -444,6 +445,4 @@ void ast_device_destroy(struct ast_private *ast) { /* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); - - kfree(ast->dp501_fw_addr); }
On Tue, Jul 28, 2020 at 09:44:24AM +0200, Thomas Zimmermann wrote:
The ast driver keeps a backup copy of the DP501 encoder's firmware. This patch adds managed release of the allocated memory.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
We can't really do anything with the firmware after the device is gone, so I think this is one of the very rare exceptions where devm_kzalloc is the right thing to do! Totally minor nit though, since either way it gets cleaned up. But I think conceptually cleaner. -Daniel
drivers/gpu/drm/ast/ast_main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index e3b7748335a3..67e20727d82d 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -33,6 +33,7 @@ #include <drm/drm_drv.h> #include <drm/drm_gem.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h>
#include "ast_drv.h"
@@ -231,11 +232,11 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post) ast->tx_chip_type = AST_TX_SIL164; break; case 0x08:
ast->dp501_fw_addr = kzalloc(32*1024, GFP_KERNEL);
ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL); if (ast->dp501_fw_addr) { /* backup firmware */ if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
kfree(ast->dp501_fw_addr);
drmm_kfree(dev, ast->dp501_fw_addr); ast->dp501_fw_addr = NULL; } }@@ -444,6 +445,4 @@ void ast_device_destroy(struct ast_private *ast) { /* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
- kfree(ast->dp501_fw_addr);
}
2.27.0
This turns the ast's device cleanup code into a managed release helper function. Note that the code uses devres helpers. The release function switches the device back to VGA mode and therefore runs during HW device cleanup; not at DRM device cleanup.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_drv.c | 17 +++-------------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++------ 3 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index c394383a7979..f0b4af1c390a 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -120,35 +120,24 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return ret;
ast = ast_device_create(&ast_driver, pdev, ent->driver_data); - if (IS_ERR(ast)) { - ret = PTR_ERR(ast); - goto err_drm_dev_put; - } + if (IS_ERR(ast)) + return PTR_ERR(ast); dev = &ast->base;
ret = drm_dev_register(dev, ent->driver_data); if (ret) - goto err_ast_device_destroy; + return ret;
drm_fbdev_generic_setup(dev, 32);
return 0; - -err_ast_device_destroy: - ast_device_destroy(ast); -err_drm_dev_put: - drm_dev_put(dev); - return ret; }
static void ast_pci_remove(struct pci_dev *pdev) { struct drm_device *dev = pci_get_drvdata(pdev); - struct ast_private *ast = to_ast_private(dev);
drm_dev_unregister(dev); - ast_device_destroy(ast); - drm_dev_put(dev); }
static int ast_drm_freeze(struct drm_device *dev) diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 02908d005b99..0911136e0842 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -162,7 +162,6 @@ static inline struct ast_private *to_ast_private(struct drm_device *dev) struct ast_private *ast_device_create(struct drm_driver *drv, struct pci_dev *pdev, unsigned long flags); -void ast_device_destroy(struct ast_private *ast);
#define AST_IO_AR_PORT_WRITE (0x40) #define AST_IO_MISC_PORT_WRITE (0x42) diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 67e20727d82d..d62749a10cdd 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -380,6 +380,18 @@ static int ast_get_dram_info(struct drm_device *dev) return 0; }
+/* + * Run this function as part of the HW device cleanup; not + * when the DRM device gets released. + */ +static void ast_device_release(void *data) +{ + struct ast_private *ast = data; + + /* enable standard VGA decode */ + ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); +} + struct ast_private *ast_device_create(struct drm_driver *drv, struct pci_dev *pdev, unsigned long flags) @@ -438,11 +450,9 @@ struct ast_private *ast_device_create(struct drm_driver *drv, if (ret) return ERR_PTR(ret);
- return ast; -} + ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast); + if (ret) + return ERR_PTR(ret);
-void ast_device_destroy(struct ast_private *ast) -{ - /* enable standard VGA decode */ - ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); + return ast; }
Hi Thomas.
On Tue, Jul 28, 2020 at 09:44:12AM +0200, Thomas Zimmermann wrote:
This is the final patchset for converting ast to managed initialization.
Patches #1 to #4 address I2C helpers. The structures are being stored in struct ast_connector. The initialization and cleanups is being converted to managed release helpers.
Patches #5 to #10 address modesetting and device structures. All are being embedded into struct ast_private. With struct ast_private being a subclass of struct drm_device, patch #10 switches ast to DRM's managed- allocation helpers.
Patches #11 and #12 address firmware memory that ast allocates internally.
Finally, patch #13 removes ast's destroy function in favor of managed release helpers.
Tested on AST 2100 HW.
Thomas Zimmermann (13): drm/ast: Move I2C code within ast_mode.c drm/ast: Test if I2C support has been initialized drm/ast: Embed I2C fields in struct ast_connector drm/ast: Managed release of I2C adapter drm/ast: Embed CRTC and connector in struct ast_private drm/ast: Separate DRM driver from PCI code drm/ast: Replace driver load/unload functions with device create/destroy drm/ast: Replace struct_drm_device.dev_private with to_ast_private() drm/ast: Don't use ast->dev if dev is available drm/ast: Embed struct drm_device in struct ast_private drm/ast: Managed release of ast firmware drm/ast: Manage release of firmware backup memory drm/ast: Managed device release
A few nits posted to a few patches. Patch 1-11 are all: Acked-by: Sam Ravnborg sam@ravnborg.org
I did not look at 12 and did not follow all the changes in 13. Not that I found 13 faulty - just lost track and -ENOTIME
Sam
drivers/gpu/drm/ast/ast_cursor.c | 8 +- drivers/gpu/drm/ast/ast_dp501.c | 23 ++- drivers/gpu/drm/ast/ast_drv.c | 82 ++++---- drivers/gpu/drm/ast/ast_drv.h | 43 +++-- drivers/gpu/drm/ast/ast_main.c | 74 ++++---- drivers/gpu/drm/ast/ast_mm.c | 2 +- drivers/gpu/drm/ast/ast_mode.c | 310 ++++++++++++++----------------- drivers/gpu/drm/ast/ast_post.c | 6 +- 8 files changed, 263 insertions(+), 285 deletions(-)
-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
daniel＠ffwll.ch -
Sam Ravnborg -
Thomas Zimmermann