Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Thu, Jun 27, 2024 at 09:18:15AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 26.06.24 um 19:59 schrieb Daniel Vetter:
> > On Wed, Jun 26, 2024 at 11:01:11AM +0200, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> > > > On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> > > > > The function drm_simple_encoder_init() is a trivial helper and
> > > > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > > > changes.
> > > > >
> > > > > Signed-off-by: Thomas Zimmermann
> > > > > ---
> > > > >drivers/gpu/drm/ast/ast_mode.c | 45
> > > > > ++
> > > > >1 file changed, 40 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > > > b/drivers/gpu/drm/ast/ast_mode.c
> > > > > index 6695af70768f..2fd9c78eab73 100644
> > > > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > > > @@ -45,7 +45,6 @@
> > > > >#include
> > > > >#include
> > > > >#include
> > > > > -#include
> > > > >#include "ast_ddc.h"
> > > > >#include "ast_drv.h"
> > > > > @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device
> > > > > *dev)
> > > > > return 0;
> > > > >}
> > > > > +/*
> > > > > + * VGA Encoder
> > > > > + */
> > > > > +
> > > > > +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> > > > > + .destroy = drm_encoder_cleanup,
> > > > > +};
> > > > > +
> > > > >/*
> > > > > * VGA Connector
> > > > > */
> > > > > @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct
> > > > > ast_device *ast)
> > > > > struct drm_connector *connector = &ast->output.vga.connector;
> > > > > int ret;
> > > > > - ret = drm_simple_encoder_init(dev, encoder,
> > > > > DRM_MODE_ENCODER_DAC);
> > > > > + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> > > > > +DRM_MODE_ENCODER_DAC, NULL);
> > > > What about using drmm_encoder_init() instead? It will call
> > > > drm_encoder_cleanup automatically.
> > > IIRC the original use case for the drmm_encoder_*() funcs was to solve
> > > problems with the clean-up order if the encoder was added dynamically. The
> > > hardware for ast is entirely static and ast uses drmm_mode_config_init()
> > > for
> > > auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> > > like a bit of wasted resources for no gain.
> > The idea of drmm_ is that you use them all. That the managed version of
> > drm_mode_config_init also happens to still work with the unmanaged
> > encoder/connector/crtc/plane cleanup is just to facilitate gradual
> > conversions.
>
> Sure, I welcome using drmm_ everywhere. I just don't like the fine-grained
> release if that's unnecessary. What most DRM drivers need is in
> drmm_mode_config_init().
Why do you think drmm_mode_config_init is better?
My take is that the point of drmm and devres is that you have really
fine-grained release hooks, so that you never have to write hand-rolled
error handling code anymore. The issue is that when you then try to
combine that with bigger hammer methods like the cleanup callbacks in
drm_mode_config_cleanup, it won't work, because it will interleave
wrongly. And then you're back to hand-rolling all the cleanup code again.
It's a bit an all-or-nothing approach unfortunately, at least for a given
area.
At least my idea was to have drmm_mode_config_init as a transational
helper only, not as the endpoint. At least for anything the driver
allocates/initializes itself, there's still a bunch of things that need to
be cleaned up separately.
Or the other counterpoint: If the global release really is best, then
drm_driver->release is all that's ever needed, and the entire drmm_
infrastructure is kinda pointless. Which yes, strictly speaking it is, at
the cost of typing a lot of error paths.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 27.06.24 um 09:47 schrieb Daniel Vetter:
On Wed, Jun 26, 2024 at 07:59:52PM +0200, Daniel Vetter wrote:
On Wed, Jun 26, 2024 at 11:01:11AM +0200, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
The idea of drmm_ is that you use them all. That the managed version of
drm_mode_config_init also happens to still work with the unmanaged
encoder/connector/crtc/plane cleanup is just to facilitate gradual
conversions.
And see my other reply, for drmm_encoder_init supporting the NULL funcs
case actually makes full sense.
Also, any driver can be hotunbound through sysfs, no hotunplug of the hw
needed at all.
I pondered this some more, I think we could embed the drmm tracking
structure into struct drm_encoder (and anywhere else we need one), which
- would mean a lot of the drmm_ versions again get a void return value,
like their non-managed counterparts.
- we could truly roll out drmm_ versions everywhere and deprecate the
unmanaged ones in a lot more cases, since drivers like ast could then
also use it.
I'm not sure this is a bright idea at scale, since devm_ doesn't do it
afaik. But maybe we should just try.
Thoughts?
I cannot say whether it's a good idea. At least it's sounds better then
kcalloc-ing drm_res for each object. The drm_res instance could directly
into struct drm_mode_object, so it's available for all the relevant
stages of the modesetting pipeline.
Best regards
Thomas
Cheers, Sima
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 25.06.24 um 16:18 schrieb Jocelyn Falempe:
On 25/06/2024 15:18, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Thanks for your patch, it looks good to me.
Reviewed-by: Jocelyn Falempe
Thanks. I've merged the patch for now. Let's see where the discussion on
managed interfaces goes.
Best regards
Thomas
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c
b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct
ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder,
DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1427,6 +1435,14 @@ static int ast_vga_output_init(struct
ast_device *ast)
return 0;
}
+/*
+ * SIL164 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_sil164_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* SIL164 Connector
*/
@@ -1480,7 +1496,8 @@ static int ast_sil164_output_init(struct
ast_device *ast)
struct drm_connector *connector = &ast->output.sil164.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder,
DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_sil164_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1496,6 +1513,14 @@ static int ast_sil164_output_init(struct
ast_device *ast)
return 0;
}
+/*
+ * DP501 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_dp501_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* DP501 Connector
*/
@@ -1578,7 +1603,8 @@ static int ast_dp501_output_init(struct
ast_device *ast)
struct drm_connector *connector = &ast->output.dp501.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder,
DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_dp501_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1594,6 +1620,14 @@ static int ast_dp501_output_init(struct
ast_device *ast)
return 0;
}
+/*
+ * ASPEED Display-Port Encoder
+ */
+
+static const struct drm_encoder_funcs ast_astdp_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* ASPEED Display-Port Connector
*/
@@ -1688,7 +1722,8 @@ static int ast_astdp_output_init(struct
ast_device *ast)
struct drm_connector *connector = &ast->output.astdp.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder,
DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_astdp_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, Jun 26, 2024 at 07:59:52PM +0200, Daniel Vetter wrote:
> On Wed, Jun 26, 2024 at 11:01:11AM +0200, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> > > On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> > > > The function drm_simple_encoder_init() is a trivial helper and
> > > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > > changes.
> > > >
> > > > Signed-off-by: Thomas Zimmermann
> > > > ---
> > > > drivers/gpu/drm/ast/ast_mode.c | 45 ++
> > > > 1 file changed, 40 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > > b/drivers/gpu/drm/ast/ast_mode.c
> > > > index 6695af70768f..2fd9c78eab73 100644
> > > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > > @@ -45,7 +45,6 @@
> > > > #include
> > > > #include
> > > > #include
> > > > -#include
> > > > #include "ast_ddc.h"
> > > > #include "ast_drv.h"
> > > > @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> > > > return 0;
> > > > }
> > > > +/*
> > > > + * VGA Encoder
> > > > + */
> > > > +
> > > > +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> > > > + .destroy = drm_encoder_cleanup,
> > > > +};
> > > > +
> > > > /*
> > > >* VGA Connector
> > > >*/
> > > > @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> > > > *ast)
> > > > struct drm_connector *connector = &ast->output.vga.connector;
> > > > int ret;
> > > > - ret = drm_simple_encoder_init(dev, encoder,
> > > > DRM_MODE_ENCODER_DAC);
> > > > + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> > > > + DRM_MODE_ENCODER_DAC, NULL);
> > > What about using drmm_encoder_init() instead? It will call
> > > drm_encoder_cleanup automatically.
> >
> > IIRC the original use case for the drmm_encoder_*() funcs was to solve
> > problems with the clean-up order if the encoder was added dynamically. The
> > hardware for ast is entirely static and ast uses drmm_mode_config_init() for
> > auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> > like a bit of wasted resources for no gain.
>
> The idea of drmm_ is that you use them all. That the managed version of
> drm_mode_config_init also happens to still work with the unmanaged
> encoder/connector/crtc/plane cleanup is just to facilitate gradual
> conversions.
>
> And see my other reply, for drmm_encoder_init supporting the NULL funcs
> case actually makes full sense.
>
> Also, any driver can be hotunbound through sysfs, no hotunplug of the hw
> needed at all.
I pondered this some more, I think we could embed the drmm tracking
structure into struct drm_encoder (and anywhere else we need one), which
- would mean a lot of the drmm_ versions again get a void return value,
like their non-managed counterparts.
- we could truly roll out drmm_ versions everywhere and deprecate the
unmanaged ones in a lot more cases, since drivers like ast could then
also use it.
I'm not sure this is a bright idea at scale, since devm_ doesn't do it
afaik. But maybe we should just try.
Thoughts?
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 19:59 schrieb Daniel Vetter:
On Wed, Jun 26, 2024 at 11:01:11AM +0200, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
The idea of drmm_ is that you use them all. That the managed version of
drm_mode_config_init also happens to still work with the unmanaged
encoder/connector/crtc/plane cleanup is just to facilitate gradual
conversions.
Sure, I welcome using drmm_ everywhere. I just don't like the
fine-grained release if that's unnecessary. What most DRM drivers need
is in drmm_mode_config_init().
Best regards
Thomas
And see my other reply, for drmm_encoder_init supporting the NULL funcs
case actually makes full sense.
Also, any driver can be hotunbound through sysfs, no hotunplug of the hw
needed at all.
-Sima
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, Jun 26, 2024 at 11:01:11AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> > On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> > >
> > > Signed-off-by: Thomas Zimmermann
> > > ---
> > > drivers/gpu/drm/ast/ast_mode.c | 45 ++
> > > 1 file changed, 40 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > b/drivers/gpu/drm/ast/ast_mode.c
> > > index 6695af70768f..2fd9c78eab73 100644
> > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > @@ -45,7 +45,6 @@
> > > #include
> > > #include
> > > #include
> > > -#include
> > > #include "ast_ddc.h"
> > > #include "ast_drv.h"
> > > @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> > > return 0;
> > > }
> > > +/*
> > > + * VGA Encoder
> > > + */
> > > +
> > > +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> > > + .destroy = drm_encoder_cleanup,
> > > +};
> > > +
> > > /*
> > >* VGA Connector
> > >*/
> > > @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> > > *ast)
> > > struct drm_connector *connector = &ast->output.vga.connector;
> > > int ret;
> > > - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> > > + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> > > +DRM_MODE_ENCODER_DAC, NULL);
> > What about using drmm_encoder_init() instead? It will call
> > drm_encoder_cleanup automatically.
>
> IIRC the original use case for the drmm_encoder_*() funcs was to solve
> problems with the clean-up order if the encoder was added dynamically. The
> hardware for ast is entirely static and ast uses drmm_mode_config_init() for
> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> like a bit of wasted resources for no gain.
The idea of drmm_ is that you use them all. That the managed version of
drm_mode_config_init also happens to still work with the unmanaged
encoder/connector/crtc/plane cleanup is just to facilitate gradual
conversions.
And see my other reply, for drmm_encoder_init supporting the NULL funcs
case actually makes full sense.
Also, any driver can be hotunbound through sysfs, no hotunplug of the hw
needed at all.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 11:19 schrieb Dmitry Baryshkov:
On Wed, 26 Jun 2024 at 12:19, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
With drm_encoder_init() there's a static-const declared struct in RO
memory. With drmm_encoder_init(), there's a kalloc for the managed
callback data. It's RW memory and the alloc can fail. Therefore I prefer
drm_encoder_init() in this case.
Ack.
One more though on this: we could discuss if we want some default
cleanup. Such as if no cleanup pointer has been set, we always call
drm_encoder_cleanup(). But IMHO that needs to be consistent among all
elements of the pipeline (planes, CRTCs, etc), and we need to document
clearly which and why the default has been chosen.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, 26 Jun 2024 at 12:19, Thomas Zimmermann wrote:
>
> Hi
>
> Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
> > On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> >>> On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> The function drm_simple_encoder_init() is a trivial helper and
> deprecated. Replace it with the regular call to drm_encoder_init().
> Resolves the dependency on drm_simple_kms_helper.h. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/ast/ast_mode.c | 45 ++
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c
> b/drivers/gpu/drm/ast/ast_mode.c
> index 6695af70768f..2fd9c78eab73 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -45,7 +45,6 @@
> #include
> #include
> #include
> -#include
> #include "ast_ddc.h"
> #include "ast_drv.h"
> @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> return 0;
> }
> +/*
> + * VGA Encoder
> + */
> +
> +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> /*
> * VGA Connector
> */
> @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> *ast)
> struct drm_connector *connector = &ast->output.vga.connector;
> int ret;
> - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> + DRM_MODE_ENCODER_DAC, NULL);
> >>> What about using drmm_encoder_init() instead? It will call
> >>> drm_encoder_cleanup automatically.
> >> IIRC the original use case for the drmm_encoder_*() funcs was to solve
> >> problems with the clean-up order if the encoder was added dynamically. The
> >> hardware for ast is entirely static and ast uses drmm_mode_config_init()
> >> for
> >> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> >> like a bit of wasted resources for no gain.
> > I'd say it's qui pro quo. We are wasting resources on drmm handling, but
> > then keep it by dropping all drm_encoder_funcs instances.
>
> With drm_encoder_init() there's a static-const declared struct in RO
> memory. With drmm_encoder_init(), there's a kalloc for the managed
> callback data. It's RW memory and the alloc can fail. Therefore I prefer
> drm_encoder_init() in this case.
Ack.
--
With best wishes
Dmitry
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 11:10 schrieb Dmitry Baryshkov:
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically. The
hardware for ast is entirely static and ast uses drmm_mode_config_init() for
auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
With drm_encoder_init() there's a static-const declared struct in RO
memory. With drmm_encoder_init(), there's a kalloc for the managed
callback data. It's RW memory and the alloc can fail. Therefore I prefer
drm_encoder_init() in this case.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Wed, Jun 26, 2024 at 11:01:11AM GMT, Thomas Zimmermann wrote:
> Hi
>
> Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
> > On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> > >
> > > Signed-off-by: Thomas Zimmermann
> > > ---
> > > drivers/gpu/drm/ast/ast_mode.c | 45 ++
> > > 1 file changed, 40 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > b/drivers/gpu/drm/ast/ast_mode.c
> > > index 6695af70768f..2fd9c78eab73 100644
> > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > @@ -45,7 +45,6 @@
> > > #include
> > > #include
> > > #include
> > > -#include
> > > #include "ast_ddc.h"
> > > #include "ast_drv.h"
> > > @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> > > return 0;
> > > }
> > > +/*
> > > + * VGA Encoder
> > > + */
> > > +
> > > +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> > > + .destroy = drm_encoder_cleanup,
> > > +};
> > > +
> > > /*
> > >* VGA Connector
> > >*/
> > > @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device
> > > *ast)
> > > struct drm_connector *connector = &ast->output.vga.connector;
> > > int ret;
> > > - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> > > + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> > > +DRM_MODE_ENCODER_DAC, NULL);
> > What about using drmm_encoder_init() instead? It will call
> > drm_encoder_cleanup automatically.
>
> IIRC the original use case for the drmm_encoder_*() funcs was to solve
> problems with the clean-up order if the encoder was added dynamically. The
> hardware for ast is entirely static and ast uses drmm_mode_config_init() for
> auto-cleaning up the modesetting pipeline. Using drmm_encoder_init() seems
> like a bit of wasted resources for no gain.
I'd say it's qui pro quo. We are wasting resources on drmm handling, but
then keep it by dropping all drm_encoder_funcs instances.
--
With best wishes
Dmitry
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 26.06.24 um 06:34 schrieb Dmitry Baryshkov:
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
IIRC the original use case for the drmm_encoder_*() funcs was to solve
problems with the clean-up order if the encoder was added dynamically.
The hardware for ast is entirely static and ast uses
drmm_mode_config_init() for auto-cleaning up the modesetting pipeline.
Using drmm_encoder_init() seems like a bit of wasted resources for no gain.
Best regards
Thomas
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Tue, Jun 25, 2024 at 04:03:21PM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 25.06.24 um 15:55 schrieb Jocelyn Falempe:
> >
> >
> > On 25/06/2024 15:18, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> >
> > Do you think it's possible to add a default to drm_encoder_init() to
> > avoid having to declare the same struct for each encoder ?
> >
> > something like:
> >
> > drm_encoder_init(...)
> > {
> >
> > if (!funcs)
> > funcs = &drm_encoder_default_funcs;
> >
> > So you can call it like this to get the default funcs:
> >
> > drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
> >
> >
> > I don't see this pattern in other drm functions, so it might not fit the
> > current coding style.
>
> Yeah, we don't do this in other places. And it's not guaranteed that
> drm_encoder_cleanup() is the correct helper in all cases; even the common
> ones. I would prefer to not add such tweaks. As for
> drm_simple_encoder_init(), it was an attempt to solve exactly this problem,
> but the function is so trivial that it's not actually worth the dependency.
For drmm_encoder_init/alloc this is doable, because it makes sure that
there really is only one correct encoder cleanup hook for simple encoders.
Without drmm_ there's the entire "who calls kfree() and how buggy is it"
mess :-/
So I'd very much welcome this kind of handling in drmm_encoder_alloc/init,
but in the unmanaged drm_encoder_init I agree it sounds like a mistake.
Cheers,
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On Tue, Jun 25, 2024 at 03:18:09PM GMT, Thomas Zimmermann wrote:
> The function drm_simple_encoder_init() is a trivial helper and
> deprecated. Replace it with the regular call to drm_encoder_init().
> Resolves the dependency on drm_simple_kms_helper.h. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/ast/ast_mode.c | 45 ++
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 6695af70768f..2fd9c78eab73 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -45,7 +45,6 @@
> #include
> #include
> #include
> -#include
>
> #include "ast_ddc.h"
> #include "ast_drv.h"
> @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
> return 0;
> }
>
> +/*
> + * VGA Encoder
> + */
> +
> +static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> /*
> * VGA Connector
> */
> @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
> struct drm_connector *connector = &ast->output.vga.connector;
> int ret;
>
> - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> + ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
> +DRM_MODE_ENCODER_DAC, NULL);
What about using drmm_encoder_init() instead? It will call
drm_encoder_cleanup automatically.
--
With best wishes
Dmitry
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On 25/06/2024 15:18, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Thanks for your patch, it looks good to me.
Reviewed-by: Jocelyn Falempe
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1427,6 +1435,14 @@ static int ast_vga_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * SIL164 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_sil164_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* SIL164 Connector
*/
@@ -1480,7 +1496,8 @@ static int ast_sil164_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.sil164.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_sil164_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1496,6 +1513,14 @@ static int ast_sil164_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * DP501 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_dp501_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* DP501 Connector
*/
@@ -1578,7 +1603,8 @@ static int ast_dp501_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.dp501.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_dp501_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1594,6 +1620,14 @@ static int ast_dp501_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * ASPEED Display-Port Encoder
+ */
+
+static const struct drm_encoder_funcs ast_astdp_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* ASPEED Display-Port Connector
*/
@@ -1688,7 +1722,8 @@ static int ast_astdp_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.astdp.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_astdp_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
Hi
Am 25.06.24 um 15:55 schrieb Jocelyn Falempe:
On 25/06/2024 15:18, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Do you think it's possible to add a default to drm_encoder_init() to
avoid having to declare the same struct for each encoder ?
something like:
drm_encoder_init(...)
{
if (!funcs)
funcs = &drm_encoder_default_funcs;
So you can call it like this to get the default funcs:
drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
I don't see this pattern in other drm functions, so it might not fit
the current coding style.
Yeah, we don't do this in other places. And it's not guaranteed that
drm_encoder_cleanup() is the correct helper in all cases; even the
common ones. I would prefer to not add such tweaks. As for
drm_simple_encoder_init(), it was an attempt to solve exactly this
problem, but the function is so trivial that it's not actually worth the
dependency.
Best regards
Thomas
Best regards,
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On 25/06/2024 15:18, Thomas Zimmermann wrote:
The function drm_simple_encoder_init() is a trivial helper and
deprecated. Replace it with the regular call to drm_encoder_init().
Resolves the dependency on drm_simple_kms_helper.h. No functional
changes.
Do you think it's possible to add a default to drm_encoder_init() to
avoid having to declare the same struct for each encoder ?
something like:
drm_encoder_init(...)
{
if (!funcs)
funcs = &drm_encoder_default_funcs;
So you can call it like this to get the default funcs:
drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
I don't see this pattern in other drm functions, so it might not fit the
current coding style.
Best regards,
--
Jocelyn
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_mode.c | 45 ++
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 6695af70768f..2fd9c78eab73 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -45,7 +45,6 @@
#include
#include
#include
-#include
#include "ast_ddc.h"
#include "ast_drv.h"
@@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev)
return 0;
}
+/*
+ * VGA Encoder
+ */
+
+static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* VGA Connector
*/
@@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.vga.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, &ast_vga_encoder_funcs,
+ DRM_MODE_ENCODER_DAC, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1427,6 +1435,14 @@ static int ast_vga_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * SIL164 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_sil164_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* SIL164 Connector
*/
@@ -1480,7 +1496,8 @@ static int ast_sil164_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.sil164.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_sil164_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1496,6 +1513,14 @@ static int ast_sil164_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * DP501 Encoder
+ */
+
+static const struct drm_encoder_funcs ast_dp501_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* DP501 Connector
*/
@@ -1578,7 +1603,8 @@ static int ast_dp501_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.dp501.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_dp501_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
@@ -1594,6 +1620,14 @@ static int ast_dp501_output_init(struct ast_device *ast)
return 0;
}
+/*
+ * ASPEED Display-Port Encoder
+ */
+
+static const struct drm_encoder_funcs ast_astdp_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
/*
* ASPEED Display-Port Connector
*/
@@ -1688,7 +1722,8 @@ static int ast_astdp_output_init(struct ast_device *ast)
struct drm_connector *connector = &ast->output.astdp.connector;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &ast_astdp_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
if (ret)
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
