Re: [PATCH 5/5] drm/vkms: Use drm_crtc_vblank_crtc()
On 4/8/24 16:06, Ville Syrjala wrote:
From: Ville Syrjälä
Replace the open coded drm_crtc_vblank_crtc() with the real
thing.
Cc: Rodrigo Siqueira
Cc: Melissa Wen
Cc: "Maíra Canal"
Cc: Haneen Mohammed
Cc: Daniel Vetter
Signed-off-by: Ville Syrjälä
Reviewed-by: Maíra Canal
Best Regards,
- Maíra
---
drivers/gpu/drm/vkms/vkms_crtc.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 61e500b8c9da..40b4d084e3ce 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -61,9 +61,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
hrtimer *timer)
static int vkms_enable_vblank(struct drm_crtc *crtc)
{
- struct drm_device *dev = crtc->dev;
- unsigned int pipe = drm_crtc_index(crtc);
- struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+ struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
drm_calc_timestamping_constants(crtc, &crtc->mode);
@@ -88,10 +86,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
bool in_vblank_irq)
{
struct drm_device *dev = crtc->dev;
- unsigned int pipe = crtc->index;
struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
struct vkms_output *output = &vkmsdev->output;
- struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+ struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
if (!READ_ONCE(vblank->enabled)) {
*vblank_time = ktime_get();
Re: [Intel-gfx] [PATCH 3/5] drm/i915: Remove redundant framebuffer format check
Hi,
On 1/12/23 09:43, Ville Syrjälä wrote:
On Mon, Jan 09, 2023 at 07:58:06AM -0300, Maíra Canal wrote:
Now that framebuffer_check() verifies that the format is properly
supported, there is no need to check it again on i915's inside
helpers.
Therefore, remove the redundant framebuffer format check from the
intel_framebuffer_init() function, letting framebuffer_check()
perform the framebuffer validation.
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/i915/display/intel_fb.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
b/drivers/gpu/drm/i915/display/intel_fb.c
index 63137ae5ab21..230b729e42d6 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -1914,15 +1914,6 @@ int intel_framebuffer_init(struct intel_framebuffer
*intel_fb,
}
}
- if (!drm_any_plane_has_format(&dev_priv->drm,
- mode_cmd->pixel_format,
- mode_cmd->modifier[0])) {
- drm_dbg_kms(&dev_priv->drm,
- "unsupported pixel format %p4cc / modifier
0x%llx\n",
- &mode_cmd->pixel_format, mode_cmd->modifier[0]);
- goto err;
- }
-
This doesn't work for the legacy tiling->modifier path.
Do you have any idea on how we could remove drm_any_plane_has_format() from
i915? Or is it strictly necessary to validate the modifier in the legacy
path?
Best Regards,
- Maíra Canal
/*
* gen2/3 display engine uses the fence if present,
* so the tiling mode must match the fb modifier exactly.
--
2.39.0
[Intel-gfx] [PATCH 5/5] drm/plane: Unexport drm_any_plane_has_format()
As the format validation is being dealt with exclusively inside framebuffer_check(), there is no need to export the drm_any_plane_has_format() symbol. Therefore, unexport the drm_any_plane_has_format() symbol, reinforcing that format validation is being dealt with by the DRM API. Signed-off-by: Maíra Canal --- drivers/gpu/drm/drm_plane.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 24e7998d1731..67c0ab60c7b6 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -837,7 +837,6 @@ bool drm_any_plane_has_format(struct drm_device *dev, return false; } -EXPORT_SYMBOL(drm_any_plane_has_format); /* * __setplane_internal - setplane handler for internal callers -- 2.39.0
[Intel-gfx] [PATCH 4/5] drm/vmwgfx: Remove redundant framebuffer format check
Now that framebuffer_check() verifies that the format is properly
supported, there is no need to check it again on vmwgfx's inside
helpers.
Therefore, remove the redundant framebuffer format check from the
vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_bo()
functions, letting framebuffer_check() perform the framebuffer
validation.
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 18 --
1 file changed, 18 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 257f090071f1..05b8d8f912bf 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1317,15 +1317,6 @@ static int vmw_kms_new_framebuffer_surface(struct
vmw_private *dev_priv,
* Sanity checks.
*/
- if (!drm_any_plane_has_format(&dev_priv->drm,
- mode_cmd->pixel_format,
- mode_cmd->modifier[0])) {
- drm_dbg(&dev_priv->drm,
- "unsupported pixel format %p4cc / modifier 0x%llx\n",
- &mode_cmd->pixel_format, mode_cmd->modifier[0]);
- return -EINVAL;
- }
-
/* Surface must be marked as a scanout. */
if (unlikely(!surface->metadata.scanout))
return -EINVAL;
@@ -1648,15 +1639,6 @@ static int vmw_kms_new_framebuffer_bo(struct vmw_private
*dev_priv,
return -EINVAL;
}
- if (!drm_any_plane_has_format(&dev_priv->drm,
- mode_cmd->pixel_format,
- mode_cmd->modifier[0])) {
- drm_dbg(&dev_priv->drm,
- "unsupported pixel format %p4cc / modifier 0x%llx\n",
- &mode_cmd->pixel_format, mode_cmd->modifier[0]);
- return -EINVAL;
- }
-
vfbd = kzalloc(sizeof(*vfbd), GFP_KERNEL);
if (!vfbd) {
ret = -ENOMEM;
--
2.39.0
[Intel-gfx] [PATCH 3/5] drm/i915: Remove redundant framebuffer format check
Now that framebuffer_check() verifies that the format is properly
supported, there is no need to check it again on i915's inside
helpers.
Therefore, remove the redundant framebuffer format check from the
intel_framebuffer_init() function, letting framebuffer_check()
perform the framebuffer validation.
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/i915/display/intel_fb.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
b/drivers/gpu/drm/i915/display/intel_fb.c
index 63137ae5ab21..230b729e42d6 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -1914,15 +1914,6 @@ int intel_framebuffer_init(struct intel_framebuffer
*intel_fb,
}
}
- if (!drm_any_plane_has_format(&dev_priv->drm,
- mode_cmd->pixel_format,
- mode_cmd->modifier[0])) {
- drm_dbg_kms(&dev_priv->drm,
- "unsupported pixel format %p4cc / modifier
0x%llx\n",
- &mode_cmd->pixel_format, mode_cmd->modifier[0]);
- goto err;
- }
-
/*
* gen2/3 display engine uses the fence if present,
* so the tiling mode must match the fb modifier exactly.
--
2.39.0
[Intel-gfx] [PATCH 2/5] drm/amdgpu: Remove redundant framebuffer format check
Now that framebuffer_check() verifies that the format is properly
supported, there is no need to check it again on amdgpu's inside
helpers.
Therefore, remove the redundant framebuffer format check from the
amdgpu_display_gem_fb_verify_and_init() function, letting
framebuffer_check() perform the framebuffer validation.
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index b22471b3bd63..611b7a4b086c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1120,16 +1120,6 @@ static int amdgpu_display_gem_fb_verify_and_init(struct
drm_device *dev,
rfb->base.obj[0] = obj;
drm_helper_mode_fill_fb_struct(dev, &rfb->base, mode_cmd);
- /* Verify that the modifier is supported. */
- if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
- mode_cmd->modifier[0])) {
- drm_dbg_kms(dev,
- "unsupported pixel format %p4cc / modifier
0x%llx\n",
- &mode_cmd->pixel_format, mode_cmd->modifier[0]);
-
- ret = -EINVAL;
- goto err;
- }
ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
if (ret)
--
2.39.0
[Intel-gfx] [PATCH 0/5] Check for valid framebuffer's format
This series is a follow-up of [1] in which I introduced a check for valid formats on drm_gem_fb_create(). During the discussion, I realized that would be a better idea to put the check inside framebuffer_check() so that it wouldn't be needed to hit any driver-specific code path when the check fails. Thanks to Daniel, Simon, Rob and Thomas for the insightful discussion! Therefore, add the valid format check inside framebuffer_check() and remove the same check from the drivers. Adding the check to framebuffer_check() will guarantee that the igt@kms_addfb_basic@addfb25-bad-modifier IGT test passes, showing the correct behavior of the check. This patchset was tested on i915, amdgpu, and vc4 with the IGT tests. [1] https://lore.kernel.org/dri-devel/20230103125322.855089-1-mca...@igalia.com/T/ Best Regards, - Maíra Canal Maíra Canal (5): drm/framebuffer: Check for valid formats drm/amdgpu: Remove redundant framebuffer format check drm/i915: Remove redundant framebuffer format check drm/vmwgfx: Remove redundant framebuffer format check drm/plane: Unexport drm_any_plane_has_format() Documentation/gpu/todo.rst | 9 - drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 10 -- drivers/gpu/drm/drm_framebuffer.c | 8 drivers/gpu/drm/drm_plane.c | 1 - drivers/gpu/drm/i915/display/intel_fb.c | 9 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 18 -- 6 files changed, 12 insertions(+), 43 deletions(-) -- 2.39.0
[Intel-gfx] [PATCH 1/5] drm/framebuffer: Check for valid formats
Currently, framebuffer_check() doesn't check if the pixel format is
supported, which can lead to the acceptance of invalid pixel formats
e.g. the acceptance of invalid modifiers. Therefore, add a check for
valid formats on framebuffer_check(), so that the ADDFB2 IOCTL rejects
calls with invalid formats.
Moreover, note that this check is only valid for atomic drivers,
because, for non-atomic drivers, checking drm_any_plane_has_format() is
not possible since the format list for the primary plane is fake, and
we'd therefore reject valid formats.
Signed-off-by: Maíra Canal
---
Documentation/gpu/todo.rst| 9 -
drivers/gpu/drm/drm_framebuffer.c | 8
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 1f8a5ebe188e..3a79c26c5cc7 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -276,11 +276,10 @@ Various hold-ups:
- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
setup code can't be deleted.
-- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
- atomic drivers we could check for valid formats by calling
- drm_plane_check_pixel_format() against all planes, and pass if any plane
- supports the format. For non-atomic that's not possible since like the format
- list for the primary plane is fake and we'd therefor reject valid formats.
+- Need to switch to drm_gem_fb_create(), as now framebuffer_check() checks for
+ valid formats for atomic drivers.
+
+- Add an addfb format validation for non-atomic drivers.
- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
version of the varios drm_gem_fb_create functions. Maybe called
diff --git a/drivers/gpu/drm/drm_framebuffer.c
b/drivers/gpu/drm/drm_framebuffer.c
index aff3746dedfb..605642bf3650 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -280,6 +280,14 @@ static int framebuffer_check(struct drm_device *dev,
}
}
+ /* Verify that the modifier is supported. */
+ if (drm_drv_uses_atomic_modeset(dev) &&
+ !drm_any_plane_has_format(dev, r->pixel_format, r->modifier[0])) {
+ drm_dbg_kms(dev, "Unsupported pixel format %p4cc / modifier
0x%llx\n",
+ &r->pixel_format, r->modifier[0]);
+ return -EINVAL;
+ }
+
return 0;
}
--
2.39.0
Re: [Intel-gfx] [PATCH v8 17/24] drm/probe-helper: Provide a TV get_modes helper
PAL Default, with NTSC on command-line",
> + BIT(DRM_MODE_TV_MODE_NTSC) |
> BIT(DRM_MODE_TV_MODE_PAL),
> + DRM_MODE_TV_MODE_PAL,
> + DRM_MODE_TV_MODE_NTSC,
> + &ntsc_mode, &pal_mode),
> +};
> +
> +static void
> +drm_connector_helper_tv_get_modes_desc(const struct
> drm_connector_helper_tv_get_modes_test *t,
> + char *desc)
> +{
> + sprintf(desc, "%s", t->name);
> +}
> +KUNIT_ARRAY_PARAM(drm_connector_helper_tv_get_modes,
> + drm_connector_helper_tv_get_modes_tests,
> + drm_connector_helper_tv_get_modes_desc);
> +
> +static struct kunit_case drm_test_connector_helper_tv_get_modes_tests[] = {
> + KUNIT_CASE_PARAM(drm_test_connector_helper_tv_get_modes_check,
> + drm_connector_helper_tv_get_modes_gen_params),
> + { }
> +};
> +
> +static struct kunit_suite drm_test_connector_helper_tv_get_modes_suite = {
> + .name = "drm_test_connector_helper_tv_get_modes",
> + .init = drm_probe_helper_test_init,
> + .test_cases = drm_test_connector_helper_tv_get_modes_tests,
> +};
> +
> +kunit_test_suites(
> + &drm_test_connector_helper_tv_get_modes_suite
> +);
Considering that there is only one suite, you could use the
kunit_test_suite macro instead.
Moreover, it would be nice to run the checkpatch script on this test, as
there are a couple of problems with the code style.
Best Regards,
- Maíra Canal
Re: [Intel-gfx] [PATCH v8 16/24] drm/modes: Introduce more named modes
_1920_1080_60),
> KUNIT_CASE(drm_test_pick_cmdline_named_ntsc),
> + KUNIT_CASE(drm_test_pick_cmdline_named_ntsc_j),
> KUNIT_CASE(drm_test_pick_cmdline_named_pal),
> + KUNIT_CASE(drm_test_pick_cmdline_named_pal_m),
As the tests drm_test_pick_cmdline_named_ntsc,
drm_test_pick_cmdline_named_ntsc_j, drm_test_pick_cmdline_named_pal and
drm_test_pick_cmdline_named_pal_m follow a pretty similar structure, it
would be nice to parametrize those tests.
Best Regards,
- Maíra Canal
> {}
> };
>
>
Re: [Intel-gfx] [PATCH v8 12/24] drm/connector: Add a function to lookup a TV mode by its name
Hi Maxime,
On 11/10/22 08:07, Maxime Ripard wrote:
> As part of the command line parsing rework coming in the next patches,
> we'll need to lookup drm_connector_tv_mode values by their name, already
> defined in drm_tv_mode_enum_list.
>
> In order to avoid any code duplication, let's do a function that will
> perform a lookup of a TV mode name and return its value.
>
> Reviewed-by: Noralf Trønnes
> Tested-by: Mateusz Kwiatkowski
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v7:
> - Add kunit tests
> ---
> drivers/gpu/drm/drm_connector.c| 24
> drivers/gpu/drm/tests/Makefile | 1 +
> drivers/gpu/drm/tests/drm_connector_test.c | 90
> ++
> include/drm/drm_connector.h| 2 +
> 4 files changed, 117 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_connector_test.c
> b/drivers/gpu/drm/tests/drm_connector_test.c
> new file mode 100644
> index ..f2272b9df211
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_connector_test.c
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for drm_modes functions
> + */
> +
> +#include
> +
> +#include
> +
> +struct drm_get_tv_mode_from_name_test {
> + const char *name;
> + enum drm_connector_tv_mode expected_mode;
> +};
> +
> +#define TV_MODE_NAME(_name, _mode) \
> + { \
> + .name = _name, \
> + .expected_mode = _mode, \
> + }
> +
> +static void drm_test_get_tv_mode_from_name_valid(struct kunit *test)
> +{
> + const struct drm_get_tv_mode_from_name_test *params = test->param_value;
> +
> + KUNIT_EXPECT_EQ(test,
> + drm_get_tv_mode_from_name(params->name,
> strlen(params->name)),
> + params->expected_mode);
> +}
> +
> +static const
> +struct drm_get_tv_mode_from_name_test
> drm_get_tv_mode_from_name_valid_tests[] = {
> + TV_MODE_NAME("NTSC", DRM_MODE_TV_MODE_NTSC),
> + TV_MODE_NAME("NTSC-443", DRM_MODE_TV_MODE_NTSC_443),
> + TV_MODE_NAME("NTSC-J", DRM_MODE_TV_MODE_NTSC_J),
> + TV_MODE_NAME("PAL", DRM_MODE_TV_MODE_PAL),
> + TV_MODE_NAME("PAL-M", DRM_MODE_TV_MODE_PAL_M),
> + TV_MODE_NAME("PAL-N", DRM_MODE_TV_MODE_PAL_N),
> + TV_MODE_NAME("SECAM", DRM_MODE_TV_MODE_SECAM),
> +};
> +
> +static void
> +drm_get_tv_mode_from_name_valid_desc(const struct
> drm_get_tv_mode_from_name_test *t,
> + char *desc)
> +{
> + sprintf(desc, "%s", t->name);
> +}
I believe that it should be a blank line here for code style.
> +KUNIT_ARRAY_PARAM(drm_get_tv_mode_from_name_valid,
> + drm_get_tv_mode_from_name_valid_tests,
> + drm_get_tv_mode_from_name_valid_desc);
> +
> +static void drm_test_get_tv_mode_from_name_invalid(struct kunit *test)
> +{
> + const char *name = *(const char **)test->param_value;
> +
> + KUNIT_EXPECT_LT(test,
> + drm_get_tv_mode_from_name(name, strlen(name)),
> + 0);
> +}
> +
> +static const
> +char *drm_get_tv_mode_from_name_invalid_tests[] = {
> + /* Truncated */
> + "NTS",
> +};
Considering that there is only one invalid test, is there a particular
reason to parametrize this test?
> +
> +static void
> +drm_get_tv_mode_from_name_invalid_desc(const char **name, char *desc)
> +{
> + sprintf(desc, "%s", *name);
> +}
> +KUNIT_ARRAY_PARAM(drm_get_tv_mode_from_name_invalid,
> + drm_get_tv_mode_from_name_invalid_tests,
> + drm_get_tv_mode_from_name_invalid_desc);
> +
> +static struct kunit_case drm_get_tv_mode_from_name_tests[] = {
> + KUNIT_CASE_PARAM(drm_test_get_tv_mode_from_name_valid,
> + drm_get_tv_mode_from_name_valid_gen_params),
> + KUNIT_CASE_PARAM(drm_test_get_tv_mode_from_name_invalid,
> + drm_get_tv_mode_from_name_invalid_gen_params),
> + { }
> +};
> +
> +static struct kunit_suite drm_get_tv_mode_from_name_test_suite = {
> + .name = "drm_get_tv_mode_from_name",
> + .test_cases = drm_get_tv_mode_from_name_tests,
> +};
> +
> +kunit_test_suites(
> + &drm_get_tv_mode_from_name_test_suite
> +);
Considering that there is only one suite, you could use the
kunit_test_suite macro instead.
Best Regards,
- Maíra Canal
>
Re: [Intel-gfx] [PATCH v8 06/24] drm/modes: Add a function to generate analog display modes
de), 50);
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> + /* BT.601 defines hsync_start at 732 for 576i */
> + KUNIT_EXPECT_EQ(test, mode->hsync_start, 732);
> +
> + /*
> + * The PAL standard expects a line to take 64us. With a pixel
> + * clock of 13.5 MHz, a pixel takes around 74ns, so we need to
> + * have 64000ns / 74ns = 864.
> + *
> + * This is also mandated by BT.601.
> + */
> + KUNIT_EXPECT_EQ(test, mode->htotal, 864);
> +
> + KUNIT_EXPECT_EQ(test, mode->vdisplay, 576);
> + KUNIT_EXPECT_EQ(test, mode->vtotal, 625);
> +}
> +
> +static void drm_modes_analog_tv_pal_576i_inlined(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *expected, *mode;
> +
> + expected = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_PAL,
> + 13500 * HZ_PER_KHZ, 720, 576,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, expected);
> +
> + mode = drm_mode_analog_pal_576i(priv->drm);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
> +}
> +
> +static struct kunit_case drm_modes_analog_tv_tests[] = {
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i),
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i_inlined),
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i),
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i_inlined),
> + { }
> +};
> +
> +static struct kunit_suite drm_modes_analog_tv_test_suite = {
> + .name = "drm_modes_analog_tv",
> + .init = drm_modes_test_init,
> + .test_cases = drm_modes_analog_tv_tests,
> +};
> +
> +kunit_test_suites(
> + &drm_modes_analog_tv_test_suite
> +);
Considering that there is only one suite, you could use the
kunit_test_suite macro instead.
Best Regards,
- Maíra Canal
> +MODULE_LICENSE("GPL v2");
Re: [Intel-gfx] [PATCH v4 14/30] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/29/22 13:31, Maxime Ripard wrote:
> drm_connector_pick_cmdline_mode() is in charge of finding a proper
> drm_display_mode from the definition we got in the video= command line
> argument.
>
> Let's add some unit tests to make sure we're not getting any regressions
> there.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v4:
> - Removed MODULE macros
> ---
> drivers/gpu/drm/drm_client_modeset.c| 4 +
> drivers/gpu/drm/tests/drm_client_modeset_test.c | 105
>
> 2 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> b/drivers/gpu/drm/drm_client_modeset.c
> index bbc535cc50dd..d553e793e673 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev
> *client, int mode)
> return ret;
> }
> EXPORT_SYMBOL(drm_client_modeset_dpms);
> +
> +#ifdef CONFIG_DRM_KUNIT_TEST
> +#include "tests/drm_client_modeset_test.c"
> +#endif
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> new file mode 100644
> index ..09dc5acbbc42
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Maxime Ripard
> + */
> +
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "drm_kunit_helpers.h"
> +
> +struct drm_client_modeset_test_priv {
> + struct drm_device *drm;
> + struct drm_connector connector;
> +};
> +
> +static int drm_client_modeset_connector_get_modes(struct drm_connector
> *connector)
> +{
> + struct drm_display_mode *mode;
Small nit: I guess this should be added on patch 21/30, as it is not
being currently used.
> + int count;
> +
> + count = drm_add_modes_noedid(connector, 1920, 1200);
> +
> + return count;
> +}
> +
> +static const struct drm_connector_helper_funcs
> drm_client_modeset_connector_helper_funcs = {
> + .get_modes = drm_client_modeset_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs drm_client_modeset_connector_funcs =
> {
> +};
> +
> +static int drm_client_modeset_test_init(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv;
> + int ret;
> +
> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
I believe it would be nicer to use KUNIT_ASSERT_NOT_NULL here, instead
of returning a error.
> + test->priv = priv;
> +
> + priv->drm = drm_kunit_device_init(test, "drm-client-modeset-test");
> + if (IS_ERR(priv->drm))
> + return PTR_ERR(priv->drm);
Here you could use KUNIT_ASSERT_NOT_ERR_OR_NULL.
> +
> + ret = drmm_connector_init(priv->drm, &priv->connector,
> + &drm_client_modeset_connector_funcs,
> + DRM_MODE_CONNECTOR_Unknown,
> + NULL);
> + if (ret)
> + return ret;
Same idea here. This would make it more compliant to the KUnit API.
> + drm_connector_helper_add(&priv->connector,
> &drm_client_modeset_connector_helper_funcs);
> +
> + return 0;
> +
> +}
> +
> +static void drm_pick_cmdline_res_1920_1080_60(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv = test->priv;
> + struct drm_device *drm = priv->drm;
> + struct drm_connector *connector = &priv->connector;
> + struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
> + struct drm_display_mode *expected_mode, *mode;
> + const char *cmdline = "1920x1080@60";
> + int ret;
> +
> + expected_mode = drm_mode_find_dmt(priv->drm, 1920, 1080, 60, false);
> + KUNIT_ASSERT_PTR_NE(test, expected_mode, NULL);
You could use KUNIT_ASSERT_NOT_NULL here.
> +
> + KUNIT_ASSERT_TRUE(test,
> + drm_mode_parse_command_line_for_connector(cmdline,
> + connector,
> +
> cmdline_mode));
> +
> + mutex_lock(&drm->mode_config.mutex);
> + ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> + mutex_unlock(&drm->mode_config.mutex);
> + KUNIT_ASSERT_GT(test, ret, 0);
> +
> + mode = drm_c
Re: [Intel-gfx] [PATCH v2 4/4] drm/tests: Set also mock plane src_x, src_y, src_w and src_h
On 8/23/22 08:29, Jouni Högander wrote:
> We need to set also src_x, src_y, src_w and src_h for the mock plane.
> After fix for drm_atomic_helper_damage_iter_init we are using these
> when iterating damage_clips.
>
> Signed-off-by: Jouni Högander
Tested-by: Maíra Canal
Best Regards,
- Maíra Canal
> ---
> drivers/gpu/drm/tests/drm_damage_helper_test.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/tests/drm_damage_helper_test.c
> b/drivers/gpu/drm/tests/drm_damage_helper_test.c
> index bf250bd08d7e..c608ae06f0e3 100644
> --- a/drivers/gpu/drm/tests/drm_damage_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_damage_helper_test.c
> @@ -59,6 +59,11 @@ static int drm_damage_helper_init(struct kunit *test)
> static void set_plane_src(struct drm_plane_state *state, int x1, int y1, int
> x2,
> int y2)
> {
> + state->src_x = x1;
> + state->src_y = y1;
> + state->src_w = x2 - x1;
> + state->src_h = y2 - y1;
> +
> state->src.x1 = x1;
> state->src.y1 = y1;
> state->src.x2 = x2;
Re: [Intel-gfx] [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
unsigned int hdisplay,
> + unsigned int vdisplay,
> + bool interlace)
> +{
> + struct drm_display_mode *mode;
> + enum drm_mode_analog analog;
> + int ret;
> +
> + switch (tv_mode) {
> + case DRM_MODE_TV_MODE_NTSC_443:
> + fallthrough;
> + case DRM_MODE_TV_MODE_NTSC_J:
> + fallthrough;
> + case DRM_MODE_TV_MODE_NTSC_M:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_60:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_M:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_60:
> + analog = DRM_MODE_ANALOG_NTSC;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL_B:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_D:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_G:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_H:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_I:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_N:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_NC:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_B:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_D:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_G:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_K:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_K1:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_L:
> + analog = DRM_MODE_ANALOG_PAL;
> + break;
> +
> + default:
> + return NULL;
> + }
> +
> + mode = drm_mode_create(dev);
> + if (!mode)
> + return NULL;
> +
> + ret = fill_analog_mode(mode,
> +&tv_modes_parameters[analog],
> +pixel_clock_hz, hdisplay, vdisplay, interlace);
> + if (ret)
> + goto err_free_mode;
> +
> + return mode;
> +
> +err_free_mode:
> + drm_mode_destroy(dev, mode);
> + return NULL;
> +}
> +EXPORT_SYMBOL(drm_analog_tv_mode);
> +
> /**
> * drm_cvt_mode -create a modeline based on the CVT algorithm
> * @dev: drm device
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index b29ef1085cad..b22ac96fdd65 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> drm_framebuffer_test.o \
> drm_kunit_helpers.o \
> drm_mm_test.o \
> + drm_modes_test.o \
> drm_plane_helper_test.o \
> drm_rect_test.o
> diff --git a/drivers/gpu/drm/tests/drm_modes_test.c
> b/drivers/gpu/drm/tests/drm_modes_test.c
> new file mode 100644
> index ..87d398fcb99e
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_modes_test.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for drm_modes functions
> + */
> +
> +#include
> +
> +#include
> +
> +#include "drm_kunit_helpers.h"
> +
> +struct drm_modes_test_priv {
> + struct drm_device *drm;
> +};
> +
> +static int drm_modes_test_init(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv;
> +
> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
I believe it would be nicer to use KUNIT_ASSERT_NOT_NULL here, instead
of returning a error.
> + test->priv = priv;
> +
> + priv->drm = drm_kunit_device_init("drm-modes-test");
> + if (IS_ERR(priv->drm))
> + return PTR_ERR(priv->drm);
Here you could use KUNIT_ASSERT_NOT_ERR_OR_NULL.
> +
> + return 0;
> +}
> +
> +static void drm_modes_test_exit(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> +
> + drm_kunit_device_exit(priv->drm);
> +}
> +
> +static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *mode;
> +
> + mode = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_NTSC_M,
> + 13500 * 1000, 720, 480,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> + /* 63.556us * 13.5MHz = 858 pixels */
> +
Re: [Intel-gfx] [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
DRM_MODE_TV_MODE_PAL_NC);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_60(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-60",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_60);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_b(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-B",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_B);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_d(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-D",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_D);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_g(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-G",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_G);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_k(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-K",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_K);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_k1(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-K1",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_K1);
> +}
> +
> +static void drm_cmdline_test_tv_option_secam_l(struct kunit *test)
> +{
> + drm_cmdline_test_tv_options(test,
> + "720x576i,tv_mode=SECAM-L",
> + drm_mode_analog_pal_576i(NULL),
> + DRM_MODE_TV_MODE_SECAM_L);
> +}
Instead of creating a function to each drm_cmdline_test_tv_options test,
you can create a parameterized test [1] for this function. This will
help the readability of the tests.
[1] https://docs.kernel.org/dev-tools/kunit/usage.html#parameterized-testing
> +
> +static void drm_cmdline_test_tv_option_invalid(struct kunit *test)
> +{
> + struct drm_cmdline_mode mode = { };
> + const char *cmdline = "720x480i,tv_mode=invalid";
> +
> + KUNIT_EXPECT_FALSE(test,
> drm_mode_parse_command_line_for_connector(cmdline,
> +
> &no_connector, &mode));
> +}
> +
> +static void drm_cmdline_test_tv_option_truncated(struct kunit *test)
> +{
> + struct drm_cmdline_mode mode = { };
> + const char *cmdline = "720x480i,tv_mode=NTSC";
> +
> + KUNIT_EXPECT_FALSE(test,
> drm_mode_parse_command_line_for_connector(cmdline,
> +
> &no_connector, &mode));
> +}
> +
I guess this tests can be incorporated in the future to the negative
tests from Michał Winiarski [2]. I don't know which one will be merged
first.
[2]
https://lore.kernel.org/dri-devel/20220817211236.252091-1-michal.winiar...@intel.com/
Best Regards,
- Maíra Canal
> static void drm_cmdline_test_invalid_option(struct kunit *test)
> {
> struct drm_cmdline_mode mode = { };
> @@ -1047,6 +1242,27 @@ static struct kunit_case drm_cmdline_parser_tests[] = {
> KUNIT_CASE(drm_cmdline_test_name_refresh_invalid_mode),
> KUNIT_CASE(drm_cmdline_test_name_option),
> KUNIT_CASE(drm_cmdline_test_name_bpp_option),
> + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_443),
> + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_j),
> + KUNIT_CASE(drm_cmdline_test_tv_option_ntsc_m),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_60),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_b),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_d),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_g),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_h),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_i),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_m),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_n),
> + KUNIT_CASE(drm_cmdline_test_tv_option_pal_nc),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_60),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_b),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_d),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_g),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_k1),
> + KUNIT_CASE(drm_cmdline_test_tv_option_secam_l),
> + KUNIT_CASE(drm_cmdline_test_tv_option_invalid),
> + KUNIT_CASE(drm_cmdline_test_tv_option_truncated),
> KUNIT_CASE(drm_cmdline_test_rotate_0),
> KUNIT_CASE(drm_cmdline_test_rotate_90),
> KUNIT_CASE(drm_cmdline_test_rotate_180),
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 49d261977d4e..9589108ba202 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1447,6 +1447,11 @@ struct drm_cmdline_mode {
>* @tv_margins: TV margins to apply to the mode.
>*/
> struct drm_connector_tv_margins tv_margins;
> +
> + /**
> + * @tv_mode: TV mode standard. See DRM_MODE_TV_MODE_*.
> + */
> + enum drm_connector_tv_mode tv_mode;
> };
>
> /**
>
Re: [Intel-gfx] [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime,
On 8/29/22 10:11, Maxime Ripard wrote:
> Our new tv mode option allows to specify the TV mode from a property.
> However, it can still be useful, for example to avoid any boot time
> artifact, to set that property directly from the kernel command line.
>
> Let's add some code to allow it, and some unit tests to exercise that code.
>
> Signed-off-by: Maxime Ripard
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 73d01e755496..a759a4ba0036 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char
> *delim,
> return 0;
> }
>
> +static int drm_mode_parse_tv_mode(const char *delim,
> + struct drm_cmdline_mode *mode)
> +{
> + const char *value;
> + unsigned int len;
Looks like this variable len is not being used and is producing the
following warning:
../drivers/gpu/drm/drm_modes.c:2122:15: warning: unused variable 'len'
[-Wunused-variable]
unsigned int len;
^
Best Regards,
- Maíra Canal
> + int ret;
> +
> + if (*delim != '=')
> + return -EINVAL;
> +
> + value = delim + 1;
> + delim = strchr(value, ',');
> + if (!delim)
> + delim = value + strlen(value);
> +
> + ret = drm_get_tv_mode_from_name(value, delim - value);
> + if (ret < 0)
> + return ret;
> +
> + mode->tv_mode = ret;
> +
> + return 0;
> +}
> +
> static int drm_mode_parse_cmdline_options(const char *str,
> bool freestanding,
> const struct drm_connector *connector,
> @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char
> *str,
> } else if (!strncmp(option, "panel_orientation", delim -
> option)) {
> if (drm_mode_parse_panel_orientation(delim, mode))
> return -EINVAL;
> + } else if (!strncmp(option, "tv_mode", delim - option)) {
> + if (drm_mode_parse_tv_mode(delim, mode))
> + return -EINVAL;
> } else {
> return -EINVAL;
> }
> @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> unsigned int xres;
> unsigned int yres;
> unsigned int flags;
> + unsigned int tv_mode;
> };
>
> -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
> +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \
> { \
> .name = _name, \
> .pixel_clock_khz = _pclk, \
> .xres = _x, \
> .yres = _y, \
> .flags = _flags,\
> + .tv_mode = _mode, \
> }
>
> static const struct drm_named_mode drm_named_modes[] = {
> - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
> - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC_M),
> + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL_B),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
> *name,
> cmdline_mode->xres = mode->xres;
> cmdline_mode->yres = mode->yres;
> cmdline_mode->interlace = !!(mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> + cmdline_mode->tv_mode = mode->tv_mode;
> cmdline_mode->specified = true;
>
> return 1;
> diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> index 59b29cdfdd35..f1e73ed65be0 100644
> --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct
> kunit *test)
> KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED);
> }
>
> +static void drm_cmdline_test_tv_options(struct kunit *test,
> + const char *cmdline,
> + const struct drm_display_mode
> *expected_mode,
Re: [Intel-gfx] [PATCH 1/3] drm: Use original src rect while initializing damage iterator
Hi Jouni,
On 7/15/22 10:49, Jouni Högander wrote:
> drm_plane_state->src might be modified by the driver. This is done
> e.g. in i915 driver when there is bigger framebuffer than the plane
> and there is some offset within framebuffer. I915 driver calculates
> separate offset and adjusts src rect coords to be relative to this
> offset. Damage clips are still relative to original src coords
> provided by user-space.
>
> This patch ensures original coordinates provided by user-space are
> used when initiliazing damage iterator.
>
drm_damage_helper has some KUnit tests on drivers/gpu/drm/tests, and by
applying this patch the drm_damage_helper tests started to fail. Could
you also refactor the drm_damage_helper tests?
To run the tests, you can run:
$ ./tools/testing/kunit/kunit.py run \
--kunitconfig=drivers/gpu/drm/tests \
--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y \
--kconfig_add CONFIG_VIRTIO_UML=y
There is also some documentation on the DRM KUnit Tests on [1].
[1] https://docs.kernel.org/gpu/drm-internals.html#unit-testing
Best Regards,
- Maíra Canal
> Signed-off-by: Jouni Högander > ---
> drivers/gpu/drm/drm_damage_helper.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> index 937b699ac2a8..d8b2955e88fd 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -224,6 +224,7 @@ drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> const struct drm_plane_state *old_state,
> const struct drm_plane_state *state)
> {
> + struct drm_rect src;
> memset(iter, 0, sizeof(*iter));
>
> if (!state || !state->crtc || !state->fb || !state->visible)
> @@ -233,10 +234,12 @@ drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> iter->num_clips = drm_plane_get_damage_clips_count(state);
>
> /* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
> - iter->plane_src.x1 = state->src.x1 >> 16;
> - iter->plane_src.y1 = state->src.y1 >> 16;
> - iter->plane_src.x2 = (state->src.x2 >> 16) + !!(state->src.x2 & 0x);
> - iter->plane_src.y2 = (state->src.y2 >> 16) + !!(state->src.y2 & 0x);
> + src = drm_plane_state_src(state);
> +
> + iter->plane_src.x1 = src.x1 >> 16;
> + iter->plane_src.y1 = src.y1 >> 16;
> + iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0x);
> + iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0x);
>
> if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
> iter->clips = NULL;
