During iteration process one of the proposed mechanism for not breaking existing userspace was to report writeback connectors as disconnected, however the final version used DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/drm_writeback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 8273950..e7b6e5e 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -23,8 +23,8 @@ * from a CRTC to a memory buffer. They are used and act similarly to other * types of connectors, with some important differences: * - Writeback connectors don't provide a way to output visually to the user. - * - Writeback connectors should always report as "disconnected" (so that - * clients which don't understand them will ignore them). + * - Writeback connectors are visible to userspace only when the client sets + * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS. * - Writeback connectors don't have EDID. * * A framebuffer may only be attached to a writeback connector when the
Older version of this patch series reported writeback as disconnected to avoid confusing userspace not aware of writeback connectors. However, the version that got merged uses a special cap (DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.
This helps us avoid some special handling of writeback connector in drm_helper_probe_single_connector_modes, see [1].
https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/arm/malidp_mw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index cfd718e..ba6ae66 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector) static enum drm_connector_status malidp_mw_connector_detect(struct drm_connector *connector, bool force) { - return connector_status_disconnected; + return connector_status_connected; }
static void malidp_mw_connector_destroy(struct drm_connector *connector)
On Fri, Jul 13, 2018 at 04:10:59PM +0100, Alexandru Gheorghe wrote:
Older version of this patch series reported writeback as disconnected to avoid confusing userspace not aware of writeback connectors. However, the version that got merged uses a special cap (DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.
This helps us avoid some special handling of writeback connector in drm_helper_probe_single_connector_modes, see [1].
https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Reviewed-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/arm/malidp_mw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index cfd718e..ba6ae66 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector) static enum drm_connector_status malidp_mw_connector_detect(struct drm_connector *connector, bool force) {
- return connector_status_disconnected;
- return connector_status_connected;
}
static void malidp_mw_connector_destroy(struct drm_connector *connector)
2.7.4
On Fri, Jul 13, 2018 at 04:10:59PM +0100, Alexandru Gheorghe wrote:
Older version of this patch series reported writeback as disconnected to avoid confusing userspace not aware of writeback connectors. However, the version that got merged uses a special cap (DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.
This helps us avoid some special handling of writeback connector in drm_helper_probe_single_connector_modes, see [1].
https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Acked-by: Liviu Dudau liviu.dudau@arm.com
drivers/gpu/drm/arm/malidp_mw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index cfd718e..ba6ae66 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector) static enum drm_connector_status malidp_mw_connector_detect(struct drm_connector *connector, bool force) {
- return connector_status_disconnected;
- return connector_status_connected;
}
static void malidp_mw_connector_destroy(struct drm_connector *connector)
2.7.4
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..08b5bb2 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id; + struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i; @@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
+ /* We expect to have a maximum of two encoders one for the actual + * display and a virtual one for the writeback connector + */ + WARN_ON(drm->mode_config.num_encoder > 2); + list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) { + encoder->possible_clones = + (1 << drm->mode_config.num_encoder) - 1; + } + ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
In future, please include a "Changes in vX" section so it's easier to tell what's changed.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..08b5bb2 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id;
- struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i;
@@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
- /* We expect to have a maximum of two encoders one for the actual
* display and a virtual one for the writeback connector*/- WARN_ON(drm->mode_config.num_encoder > 2);
- list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
encoder->possible_clones =(1 << drm->mode_config.num_encoder) - 1;- }
This loop isn't necessary, you can just do the assignment once instead of num_encoder times :-)
Sean
- ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
-- 2.7.4
On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
In future, please include a "Changes in vX" section so it's easier to tell what's changed.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..08b5bb2 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id;
- struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i;
@@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
- /* We expect to have a maximum of two encoders one for the actual
Also, while I'm here, drop the comment contents a line. ie:
/* * We expect... */
Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
Sean
* display and a virtual one for the writeback connector*/- WARN_ON(drm->mode_config.num_encoder > 2);
- list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
encoder->possible_clones =(1 << drm->mode_config.num_encoder) - 1;- }
This loop isn't necessary, you can just do the assignment once instead of num_encoder times :-)
Sean
- ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
-- 2.7.4
-- Sean Paul, Software Engineer, Google / Chromium OS
On Fri, Jul 13, 2018 at 11:47:33AM -0400, Sean Paul wrote:
On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
In future, please include a "Changes in vX" section so it's easier to tell what's changed.
Yeah, sorry about that, the patch was so small that it never crossed my mind.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..08b5bb2 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id;
- struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i;
@@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
- /* We expect to have a maximum of two encoders one for the actual
Also, while I'm here, drop the comment contents a line. ie:
/*
- We expect...
*/
Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
I blame checkpatch, it should've complain about it.
Sean
* display and a virtual one for the writeback connector*/- WARN_ON(drm->mode_config.num_encoder > 2);
- list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
encoder->possible_clones =(1 << drm->mode_config.num_encoder) - 1;- }
This loop isn't necessary, you can just do the assignment once instead of num_encoder times :-)
Sean
Not sure I get what you are suggesting, there are two encoders, so at least two assignments and the loop does just that.
- ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
-- 2.7.4
-- Sean Paul, Software Engineer, Google / Chromium OS
-- Sean Paul, Software Engineer, Google / Chromium OS
On Fri, Jul 13, 2018 at 04:55:41PM +0100, Alexandru-Cosmin Gheorghe wrote:
On Fri, Jul 13, 2018 at 11:47:33AM -0400, Sean Paul wrote:
On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
In future, please include a "Changes in vX" section so it's easier to tell what's changed.
Yeah, sorry about that, the patch was so small that it never crossed my mind.
Anything you can do to help reviewers is most appreciated.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..08b5bb2 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id;
- struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i;
@@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
- /* We expect to have a maximum of two encoders one for the actual
Also, while I'm here, drop the comment contents a line. ie:
/*
- We expect...
*/
Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
I blame checkpatch, it should've complain about it.
Sean
* display and a virtual one for the writeback connector*/- WARN_ON(drm->mode_config.num_encoder > 2);
- list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
encoder->possible_clones =(1 << drm->mode_config.num_encoder) - 1;- }
This loop isn't necessary, you can just do the assignment once instead of num_encoder times :-)
Sean
Not sure I get what you are suggesting, there are two encoders, so at least two assignments and the loop does just that.
Yeah, temporarily (I hope) lapse on my part. You're right :-)
Sean
- ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
-- 2.7.4
-- Sean Paul, Software Engineer, Google / Chromium OS
-- Sean Paul, Software Engineer, Google / Chromium OS
-- Cheers, Alex G
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Changes since v2: - Use proper style for multi-line comments. --- drivers/gpu/drm/arm/malidp_drv.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..4169a72 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id; + struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i; @@ -737,6 +738,16 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
+ /* + * We expect to have a maximum of two encoders one for the actual + * display and a virtual one for the writeback connector + */ + WARN_ON(drm->mode_config.num_encoder > 2); + list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) { + encoder->possible_clones = + (1 << drm->mode_config.num_encoder) - 1; + } + ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
On Mon, Jul 16, 2018 at 10:56:13AM +0100, Alexandru Gheorghe wrote:
Set possible_clones field to report that the writeback connector and the one driving the display could be enabled at the same time.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Acked-by: Liviu Dudau liviu.dudau@arm.com
Changes since v2:
- Use proper style for multi-line comments.
drivers/gpu/drm/arm/malidp_drv.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 5b72605..4169a72 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev) struct malidp_hw_device *hwdev; struct platform_device *pdev = to_platform_device(dev); struct of_device_id const *dev_id;
- struct drm_encoder *encoder; /* number of lines for the R, G and B output */ u8 output_width[MAX_OUTPUT_CHANNELS]; int ret = 0, i;
@@ -737,6 +738,16 @@ static int malidp_bind(struct device *dev) goto bind_fail; }
- /*
* We expect to have a maximum of two encoders one for the actual* display and a virtual one for the writeback connector*/- WARN_ON(drm->mode_config.num_encoder > 2);
- list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
encoder->possible_clones =(1 << drm->mode_config.num_encoder) - 1;- }
- ret = malidp_irq_init(pdev); if (ret < 0) goto irq_init_fail;
-- 2.7.4
Drivers that subclass drm_plane need to copy the logic for linking the drm_plane with its state and to initialize core properties to their default values. E.g (alpha and rotation)
Having a helper to reset the plane_state makes sense because of multiple reasons: 1. Eliminate code duplication. 2. Add a single place for initializing core properties to their default values, no need for driver to do it if what the helper sets makes sense for them. 3. No need to debug the driver when you enable a core property and observe it doesn't have a proper default value.
Tested with mali-dp the other drivers are just built-tested.
Alexandru Gheorghe (10): drm/atomic: Add __drm_atomic_helper_plane_reset drm/amd/display: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/exynos: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: rcar-du: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/sun4i: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++-- drivers/gpu/drm/arm/malidp_planes.c | 7 ++-- .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +-- drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++------ drivers/gpu/drm/exynos/exynos_drm_plane.c | 3 +- drivers/gpu/drm/imx/ipuv3-plane.c | 8 ++--- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +-- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +-- drivers/gpu/drm/sun4i/sun4i_layer.c | 4 +-- drivers/gpu/drm/vc4/vc4_plane.c | 4 +-- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +-- include/drm/drm_atomic_helper.h | 2 ++ 12 files changed, 39 insertions(+), 45 deletions(-)
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/** + * __drm_atomic_helper_plane_reset - resets planes state to default values + * @plane: plane object + * @new_state: atomic plane state + * + * Initializes plane state to default. This is useful for drivers that subclass + * the plane state. + */ +void __drm_atomic_helper_plane_reset(struct drm_plane *plane, + struct drm_plane_state *state) +{ + if (state) { + state->plane = plane; + state->rotation = DRM_MODE_ROTATE_0; + /* Reset the alpha value to fully opaque if it matters */ + if (plane->alpha_property) + state->alpha = plane->alpha_property->values[1]; + } + plane->state = state; +} +EXPORT_SYMBOL(__drm_atomic_helper_plane_reset); + /** * drm_atomic_helper_plane_reset - default &drm_plane_funcs.reset hook for planes * @plane: drm plane @@ -3521,15 +3543,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
kfree(plane->state); plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL); - - if (plane->state) { - plane->state->plane = plane; - plane->state->rotation = DRM_MODE_ROTATE_0; - - /* Reset the alpha value to fully opaque if it matters */ - if (plane->alpha_property) - plane->state->alpha = plane->alpha_property->values[1]; - } + __drm_atomic_helper_plane_reset(plane, plane->state); } EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..2dd40c761dfd 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -155,6 +155,8 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state); void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state);
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane, + struct drm_plane_state *state); void drm_atomic_helper_plane_reset(struct drm_plane *plane); void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, struct drm_plane_state *state);
On Fri, Jul 20, 2018 at 10:15:00PM +0100, Alexandru Gheorghe wrote:
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- __drm_atomic_helper_plane_reset - resets planes state to default values
- @plane: plane object
- @new_state: atomic plane state
- Initializes plane state to default. This is useful for drivers that subclass
- the plane state.
- */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state)+{
- if (state) {
state->plane = plane;state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)state->alpha = plane->alpha_property->values[1];- }
- plane->state = state;
+} +EXPORT_SYMBOL(__drm_atomic_helper_plane_reset);
/**
- drm_atomic_helper_plane_reset - default &drm_plane_funcs.reset hook for planes
- @plane: drm plane
@@ -3521,15 +3543,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
kfree(plane->state); plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
- if (plane->state) {
plane->state->plane = plane;plane->state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)plane->state->alpha = plane->alpha_property->values[1];- }
- __drm_atomic_helper_plane_reset(plane, plane->state);
} EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..2dd40c761dfd 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -155,6 +155,8 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state); void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state);
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state);void drm_atomic_helper_plane_reset(struct drm_plane *plane); void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, struct drm_plane_state *state); -- 2.18.0
With my limited knowledge on DRM, this looks OK to me:- Reviewed-by :- Ayan Kumar halder ayan.halder@arm.com
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- __drm_atomic_helper_plane_reset - resets planes state to default values
- @plane: plane object
- @new_state: atomic plane state
- Initializes plane state to default. This is useful for drivers that subclass
- the plane state.
- */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state)+{
- if (state) {
state->plane = plane;state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)state->alpha = plane->alpha_property->values[1];- }
Is this function supposed to be callable with state == NULL ?
- plane->state = state;
If so, the comment could mention that this sets plane->state to NULL if state == NULL, and a few of the call sites could be simplified.
If not, I would remove the conditional if (state) {} part and also mention this in the comment.
regards Philipp
On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- __drm_atomic_helper_plane_reset - resets planes state to default values
- @plane: plane object
- @new_state: atomic plane state
- Initializes plane state to default. This is useful for drivers that subclass
- the plane state.
- */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state)+{
- if (state) {
state->plane = plane;state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)state->alpha = plane->alpha_property->values[1];- }
Is this function supposed to be callable with state == NULL ?
- plane->state = state;
If so, the comment could mention that this sets plane->state to NULL if state == NULL, and a few of the call sites could be simplified.
If not, I would remove the conditional if (state) {} part and also mention this in the comment.
Yes, It's intended to be callable with null. I will update the comment.
regards Philipp
On Tue, 24 Jul 2018 09:14:02 +0100 Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@arm.com wrote:
On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- __drm_atomic_helper_plane_reset - resets planes state to default values
- @plane: plane object
- @new_state: atomic plane state
- Initializes plane state to default. This is useful for drivers that subclass
- the plane state.
- */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state)+{
- if (state) {
state->plane = plane;state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)state->alpha = plane->alpha_property->values[1];- }
Is this function supposed to be callable with state == NULL ?
- plane->state = state;
If so, the comment could mention that this sets plane->state to NULL if state == NULL, and a few of the call sites could be simplified.
If not, I would remove the conditional if (state) {} part and also mention this in the comment.
Yes, It's intended to be callable with null.
May I ask why? I'd assume drivers that call this function to pass a non-NULL plane state. What's the use case for passing a NULL state here?
On Tue, Jul 24, 2018 at 10:16:56AM +0200, Boris Brezillon wrote:
On Tue, 24 Jul 2018 09:14:02 +0100 Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@arm.com wrote:
On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
There are a lot of drivers that subclass drm_plane_state, all of them duplicate the code that links toghether the plane with plane_state.
On top of that, drivers that enable core properties also have to duplicate the code for initializing the properties to their default values, which in all cases are the same as the defaults from core.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++-------- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8008a7de2e10..e1c6f101652e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- __drm_atomic_helper_plane_reset - resets planes state to default values
- @plane: plane object
- @new_state: atomic plane state
- Initializes plane state to default. This is useful for drivers that subclass
- the plane state.
- */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
struct drm_plane_state *state)+{
- if (state) {
state->plane = plane;state->rotation = DRM_MODE_ROTATE_0;/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)state->alpha = plane->alpha_property->values[1];- }
Is this function supposed to be callable with state == NULL ?
- plane->state = state;
If so, the comment could mention that this sets plane->state to NULL if state == NULL, and a few of the call sites could be simplified.
If not, I would remove the conditional if (state) {} part and also mention this in the comment.
Yes, It's intended to be callable with null.
May I ask why? I'd assume drivers that call this function to pass a non-NULL plane state. What's the use case for passing a NULL state here?
Drivers check it, drm_atomic_helper_plane_reset doesn't. Looking at the existing __drm_atomic_helper_plane_* it seems they all assume state not null, so I think it probably makes more sense to do that for this function as well.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index ca017c1dd4da..c08157686782 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3013,11 +3013,8 @@ static void dm_drm_plane_reset(struct drm_plane *plane) amdgpu_state = kzalloc(sizeof(*amdgpu_state), GFP_KERNEL); WARN_ON(amdgpu_state == NULL); - if (amdgpu_state) { - plane->state = &amdgpu_state->base; - plane->state->plane = plane; - plane->state->rotation = DRM_MODE_ROTATE_0; - } + if (amdgpu_state) + __drm_atomic_helper_plane_reset(plane, &amdgpu_state->base); }
static struct drm_plane_state *
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/arm/malidp_planes.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index 29409a65d864..49c37f6dd63e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane) kfree(state); plane->state = NULL; state = kzalloc(sizeof(*state), GFP_KERNEL); - if (state) { - state->base.plane = plane; - state->base.rotation = DRM_MODE_ROTATE_0; - plane->state = &state->base; - } + if (state) + __drm_atomic_helper_plane_reset(plane, &state->base); }
static struct
On Fri, Jul 20, 2018 at 10:15:02PM +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index 29409a65d864..49c37f6dd63e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane) kfree(state); plane->state = NULL; state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state) {
state->base.plane = plane;state->base.rotation = DRM_MODE_ROTATE_0;plane->state = &state->base;- }
- if (state)
__drm_atomic_helper_plane_reset(plane, &state->base);}
static struct
2.18.0
A commit description like the following might be useful (though the patch is a part of series) :- Invoke a newly added drm atomic helper function (__drm_atomic_helper_plane_reset) to reset the 'drm_plane_state' to its default values for rotation, alpha and plane.
Otherwise, with my limited knowledge on DRM, this looks OK to me:- Reviewed-by :- Ayan Kumar halder ayan.halder@arm.com
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jul 20, 2018 at 10:15:02PM +0100, Alexandru Gheorghe wrote:
With a more detailed commit message:
Acked-by: Liviu Dudau liviu.dudau@arm.com
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index 29409a65d864..49c37f6dd63e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane) kfree(state); plane->state = NULL; state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state) {
state->base.plane = plane;state->base.rotation = DRM_MODE_ROTATE_0;plane->state = &state->base;- }
- if (state)
__drm_atomic_helper_plane_reset(plane, &state->base);}
static struct
2.18.0
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 04440064b9b7..9330a076e15a 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -942,10 +942,7 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p) "Failed to allocate initial plane state\n"); return; } - - p->state = &state->base; - p->state->alpha = DRM_BLEND_ALPHA_OPAQUE; - p->state->plane = p; + __drm_atomic_helper_plane_reset(p, &state->base); } }
Hi Alexandru,
On Fri, 20 Jul 2018 22:15:03 +0100 Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com wrote:
Please add a commit message here (even if it's just repeating what the subject says).
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
With this addressed:
Acked-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index 04440064b9b7..9330a076e15a 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -942,10 +942,7 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p) "Failed to allocate initial plane state\n"); return; }
p->state = &state->base;p->state->alpha = DRM_BLEND_ALPHA_OPAQUE;p->state->plane = p;
__drm_atomic_helper_plane_reset(p, &state->base);}
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/exynos/exynos_drm_plane.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index eb9915da7dec..681328fbe7de 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -139,8 +139,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); if (exynos_state) { - plane->state = &exynos_state->base; - plane->state->plane = plane; + __drm_atomic_helper_plane_reset(plane, &exynos_state->base); plane->state->zpos = exynos_plane->config->zpos; } }
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 203f247d4854..1bd4de03ce9e 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane) ipu_state = to_ipu_plane_state(plane->state); __drm_atomic_helper_plane_destroy_state(plane->state); kfree(ipu_state); + plane->state = NULL; }
ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
- if (ipu_state) { - ipu_state->base.plane = plane; - ipu_state->base.rotation = DRM_MODE_ROTATE_0; - } + if (ipu_state) + __drm_atomic_helper_plane_reset(plane, &ipu_state->base);
- plane->state = &ipu_state->base; }
static struct drm_plane_state *
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 203f247d4854..1bd4de03ce9e 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane) ipu_state = to_ipu_plane_state(plane->state); __drm_atomic_helper_plane_destroy_state(plane->state); kfree(ipu_state);
plane->state = NULL;
With __drm_atomic_helper_plane_reset as-is, this could be dropped ...
}
ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
- if (ipu_state) {
ipu_state->base.plane = plane;ipu_state->base.rotation = DRM_MODE_ROTATE_0;- }
- if (ipu_state)
__drm_atomic_helper_plane_reset(plane, &ipu_state->base);
... if this is change to just
__drm_atomic_helper_plane_reset(plane, &ipu_state->base);
- plane->state = &ipu_state->base;
}
static struct drm_plane_state *
regards Philipp
On Tue, 2018-07-24 at 09:49 +0200, Philipp Zabel wrote:
Hi Alexandru,
On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 203f247d4854..1bd4de03ce9e 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane) ipu_state = to_ipu_plane_state(plane->state); __drm_atomic_helper_plane_destroy_state(plane->state); kfree(ipu_state);
plane->state = NULL;With __drm_atomic_helper_plane_reset as-is, this could be dropped ...
}
ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
- if (ipu_state) {
ipu_state->base.plane = plane;ipu_state->base.rotation = DRM_MODE_ROTATE_0;- }
- if (ipu_state)
__drm_atomic_helper_plane_reset(plane, &ipu_state->base);... if this is change to just
__drm_atomic_helper_plane_reset(plane, &ipu_state->base);
Euh, obviously that won't be possible, sorry for the noise. I blame the heat.
regards Philipp
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +--- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index c20f7ed48c8d..19a9d5f6db1c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -689,15 +689,13 @@ static void rcar_du_plane_reset(struct drm_plane *plane) state = kzalloc(sizeof(*state), GFP_KERNEL); if (state == NULL) return; + __drm_atomic_helper_plane_reset(plane, &state->state);
state->hwindex = -1; state->source = RCAR_DU_PLANE_MEMORY; state->colorkey = RCAR_DU_COLORKEY_NONE; state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
- plane->state = &state->state; - plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE; - plane->state->plane = plane; }
static int rcar_du_plane_atomic_set_property(struct drm_plane *plane, diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 72eebeda518e..0a0aa490f805 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -346,11 +346,9 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) if (state == NULL) return;
- state->state.alpha = DRM_BLEND_ALPHA_OPAQUE; + __drm_atomic_helper_plane_reset(plane, &state->state); state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
- plane->state = &state->state; - plane->state->plane = plane; }
static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/sun4i/sun4i_layer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c index 750ad24de1d7..78f77af8805a 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.c +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c @@ -35,9 +35,7 @@ static void sun4i_backend_layer_reset(struct drm_plane *plane)
state = kzalloc(sizeof(*state), GFP_KERNEL); if (state) { - plane->state = &state->state; - plane->state->plane = plane; - plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE; + __drm_atomic_helper_plane_reset(plane, &state->state); plane->state->zpos = layer->id; } }
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/vc4/vc4_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 9d7a36f148cf..688ad9bb0f08 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane) if (!vc4_state) return;
- plane->state = &vc4_state->base; - plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE; - vc4_state->base.plane = plane; + __drm_atomic_helper_plane_reset(plane, &vc4_state->base); }
static void vc4_dlist_write(struct vc4_plane_state *vc4_state, u32 val)
On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/vc4/vc4_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 9d7a36f148cf..688ad9bb0f08 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane) if (!vc4_state) return;
- plane->state = &vc4_state->base;
- plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
- vc4_state->base.plane = plane;
- __drm_atomic_helper_plane_reset(plane, &vc4_state->base);
For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset value of alpha from DRM_BLEND_ALPHA_OPAQUE to plane->alpha_property->values[1].
This might or might not be a good idea, but you should definitely explain why.
Maxime
Hi Maxime,
On Tue, Jul 24, 2018 at 08:57:20AM +0200, Maxime Ripard wrote:
On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/vc4/vc4_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 9d7a36f148cf..688ad9bb0f08 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane) if (!vc4_state) return;
- plane->state = &vc4_state->base;
- plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
- vc4_state->base.plane = plane;
- __drm_atomic_helper_plane_reset(plane, &vc4_state->base);
For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset value of alpha from DRM_BLEND_ALPHA_OPAQUE to plane->alpha_property->values[1].
plane->alpha_property->values[1] holds the max value for alpha, and it's initialized by the core to DRM_BLEND_ALPHA_OPAQUE.
This might or might not be a good idea, but you should definitely explain why.
Would a commit message suffice? Or do you have other ideas?
Maxime
-- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jul 24, 2018 at 09:11:21AM +0100, Alexandru-Cosmin Gheorghe wrote:
Hi Maxime,
On Tue, Jul 24, 2018 at 08:57:20AM +0200, Maxime Ripard wrote:
On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/vc4/vc4_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 9d7a36f148cf..688ad9bb0f08 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane) if (!vc4_state) return;
- plane->state = &vc4_state->base;
- plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
- vc4_state->base.plane = plane;
- __drm_atomic_helper_plane_reset(plane, &vc4_state->base);
For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset value of alpha from DRM_BLEND_ALPHA_OPAQUE to plane->alpha_property->values[1].
plane->alpha_property->values[1] holds the max value for alpha, and it's initialized by the core to DRM_BLEND_ALPHA_OPAQUE.
This might or might not be a good idea, but you should definitely explain why.
Would a commit message suffice? Or do you have other ideas?
Yes, that would be enough.
Maxime
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 466336b34fff..1e0fb3c79b50 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane) return; }
- plane->state = &vps->base; - plane->state->plane = plane; - plane->state->rotation = DRM_MODE_ROTATE_0; + __drm_atomic_helper_plane_reset(plane, &vps->base); }
Hi Alexandru,
Thanks for the patch, for the vmwgfx part:
Reviewed-by: Deepak Rawat drawat@vmware.com
Signed-off-by: Alexandru Gheorghe <alexandru- cosmin.gheorghe@arm.com>
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 466336b34fff..1e0fb3c79b50 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane) return; }
- plane->state = &vps->base;
- plane->state->plane = plane;
- plane->state->rotation = DRM_MODE_ROTATE_0;
- __drm_atomic_helper_plane_reset(plane, &vps->base);
}
-- 2.18.0
Hello Alex,
other than adding a brief commit message to this patch,
Reviewed-by: Sinclair Yeh syeh@vmware.com
On Fri, Jul 20, 2018 at 10:15:09PM +0100, Alexandru Gheorghe wrote:
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 466336b34fff..1e0fb3c79b50 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane) return; }
- plane->state = &vps->base;
- plane->state->plane = plane;
- plane->state->rotation = DRM_MODE_ROTATE_0;
- __drm_atomic_helper_plane_reset(plane, &vps->base);
}
-- 2.18.0
On 2018-07-20 05:14 PM, Alexandru Gheorghe wrote:
Drivers that subclass drm_plane need to copy the logic for linking the drm_plane with its state and to initialize core properties to their default values. E.g (alpha and rotation)
Having a helper to reset the plane_state makes sense because of multiple reasons:
- Eliminate code duplication.
- Add a single place for initializing core properties to their
default values, no need for driver to do it if what the helper sets makes sense for them. 3. No need to debug the driver when you enable a core property and observe it doesn't have a proper default value.
Tested with mali-dp the other drivers are just built-tested.
For some reason I lost 02/10 hence I'm replying to the cover letter.
Patches 1 & 2 are
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
Alexandru Gheorghe (10): drm/atomic: Add __drm_atomic_helper_plane_reset drm/amd/display: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/exynos: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the logic drm: rcar-du: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/sun4i: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++-- drivers/gpu/drm/arm/malidp_planes.c | 7 ++-- .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +-- drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++------ drivers/gpu/drm/exynos/exynos_drm_plane.c | 3 +- drivers/gpu/drm/imx/ipuv3-plane.c | 8 ++--- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +-- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 +-- drivers/gpu/drm/sun4i/sun4i_layer.c | 4 +-- drivers/gpu/drm/vc4/vc4_plane.c | 4 +-- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +-- include/drm/drm_atomic_helper.h | 2 ++ 12 files changed, 39 insertions(+), 45 deletions(-)
On Fri, Jul 13, 2018 at 04:10:58PM +0100, Alexandru Gheorghe wrote:
During iteration process one of the proposed mechanism for not breaking existing userspace was to report writeback connectors as disconnected, however the final version used DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Reviewed-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/drm_writeback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 8273950..e7b6e5e 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -23,8 +23,8 @@
- from a CRTC to a memory buffer. They are used and act similarly to other
- types of connectors, with some important differences:
- Writeback connectors don't provide a way to output visually to the user.
- Writeback connectors should always report as "disconnected" (so that
- clients which don't understand them will ignore them).
- Writeback connectors are visible to userspace only when the client sets
- DRM_CLIENT_CAP_WRITEBACK_CONNECTORS.
- Writeback connectors don't have EDID.
- A framebuffer may only be attached to a writeback connector when the
-- 2.7.4
On Fri, Jul 13, 2018 at 04:10:58PM +0100, Alexandru Gheorghe wrote:
During iteration process one of the proposed mechanism for not breaking existing userspace was to report writeback connectors as disconnected, however the final version used DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
Acked-by: Liviu Dudau liviu.dudau@arm.com
drivers/gpu/drm/drm_writeback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 8273950..e7b6e5e 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -23,8 +23,8 @@
- from a CRTC to a memory buffer. They are used and act similarly to other
- types of connectors, with some important differences:
- Writeback connectors don't provide a way to output visually to the user.
- Writeback connectors should always report as "disconnected" (so that
- clients which don't understand them will ignore them).
- Writeback connectors are visible to userspace only when the client sets
- DRM_CLIENT_CAP_WRITEBACK_CONNECTORS.
- Writeback connectors don't have EDID.
- A framebuffer may only be attached to a writeback connector when the
-- 2.7.4
dri-devel@lists.freedesktop.org
-
Alexandru Gheorghe -
Alexandru-Cosmin Gheorghe
-
Ayan Halder -
Boris Brezillon -
Deepak Singh Rawat -
Harry Wentland -
Liviu Dudau -
Maxime Ripard -
Philipp Zabel -
Sean Paul -
Sinclair Yeh