Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote: On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote: This patch converts intel_plane_state-src rect from 16.16 values into regular ints. This approach aligns with sprite_plane_state-src rects which are already in regular ints. Signed-off-by: Chandra Konduru chandra.kond...@intel.com You're not touching cursor state here, so I guess it stays in 16.16 form always? Does it need to be updated to behave the same way? Looking at our sprite code today, it treats intel_state-src as 16.16 for most of the check function, then re-writes it as integer pixels near the end, which I guess matches the type of change you're doing here. I still find this pretty confusing that our structure is sometimes interpreted in one way and other times interpreted a different way. Personally I think it would be less error-prone if we just treated src as 16.16 always, but if you to keep the current logic which changes the meaning at the end of the check() stage, can you add some comments to struct intel_plane_state clarifying that? Rewriting intel_plane_state won't work since on duplicate_state we'd need to undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just use 16.16 everywhere. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
-Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 07, 2015 1:44 AM To: Roper, Matthew D Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote: On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote: This patch converts intel_plane_state-src rect from 16.16 values into regular ints. This approach aligns with sprite_plane_state-src rects which are already in regular ints. Signed-off-by: Chandra Konduru chandra.kond...@intel.com You're not touching cursor state here, so I guess it stays in 16.16 form always? Does it need to be updated to behave the same way? Looking at our sprite code today, it treats intel_state-src as 16.16 for most of the check function, then re-writes it as integer pixels near the end, which I guess matches the type of change you're doing here. I still find this pretty confusing that our structure is sometimes interpreted in one way and other times interpreted a different way. Personally I think it would be less error-prone if we just treated src as 16.16 always, but if you to keep the current logic which changes the meaning at the end of the check() stage, can you add some comments to struct intel_plane_state clarifying that? Rewriting intel_plane_state won't work since on duplicate_state we'd need to undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just use 16.16 everywhere. -Daniel Recently in upstream, sprite plane src rect changed to regular int at end of check before entering commit but left primary src rect as 16.16. This is causing issue for having any common helper function to handle sprites and primary. So my patch is aligning both primary and sprite's src rect as regular int after checks are done. Earlier Matt commented that it is upto i915's implementation how to keep its internal src rect values in its state which is his response to my earlier change to fix a bug when passing src rect keeping them in 16.16 format. I am fine whichever format you want. Can you or Matt make it clear which format to keep them? -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
On Tue, Apr 07, 2015 at 11:29:06AM -0700, Konduru, Chandra wrote: -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 07, 2015 1:44 AM To: Roper, Matthew D Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote: On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote: This patch converts intel_plane_state-src rect from 16.16 values into regular ints. This approach aligns with sprite_plane_state-src rects which are already in regular ints. Signed-off-by: Chandra Konduru chandra.kond...@intel.com You're not touching cursor state here, so I guess it stays in 16.16 form always? Does it need to be updated to behave the same way? Looking at our sprite code today, it treats intel_state-src as 16.16 for most of the check function, then re-writes it as integer pixels near the end, which I guess matches the type of change you're doing here. I still find this pretty confusing that our structure is sometimes interpreted in one way and other times interpreted a different way. Personally I think it would be less error-prone if we just treated src as 16.16 always, but if you to keep the current logic which changes the meaning at the end of the check() stage, can you add some comments to struct intel_plane_state clarifying that? Rewriting intel_plane_state won't work since on duplicate_state we'd need to undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just use 16.16 everywhere. -Daniel Recently in upstream, sprite plane src rect changed to regular int at end of check before entering commit but left primary src rect as 16.16. This is causing issue for having any common helper function to handle sprites and primary. So my patch is aligning both primary and sprite's src rect as regular int after checks are done. Earlier Matt commented that it is upto i915's implementation how to keep its internal src rect values in its state which is his response to my earlier change to fix a bug when passing src rect keeping them in 16.16 format. I am fine whichever format you want. Can you or Matt make it clear which format to keep them? The internal src/dest rect are derived state that the driver tracks for its own purposes, so yeah, it's up to us how we decide to store it. The confusing (and error-prone) part is that our sprite check code treats it as 16.16 whereas our commit code treats it as integer; to make matters worse, we aren't even consistent with this scheme across the various plane types (primary, sprite, cursor). Even though the state gets copied in duplicate_state(), we don't have any immediate problems with the existing sprite code today because it does wind up getting blown away and recomputed before we have a chance to mix up the meaning of the values. Your patch here looks like it would work without problems today too for the same reason. But simply the inconsistency of what the value means at various points in the process bothers me because it is likely to cause mistakes as people write code in the future. Since the check phase is the more complex logic here, and since it depends on the 16.16 storage, it seems natural to me to just preserve that 16.16 format throughout and just shift as necessary in the commit step. From what I can see, the mid-operation switch between 16.16 and integer format in the sprite code originated with commit commit 96d61a7f267ff355a401ca23a732810027d10ba2 Author: Gustavo Padovan gustavo.pado...@collabora.co.uk Date: Fri Sep 5 17:04:47 2014 -0300 drm/i915: split intel_update_plane into check() and commit() when the check and commit steps were first split apart. The change isn't directly referenced in the commit message, so I think it just sort of snuck in under the radar. Matt -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- Matt Roper Graphics Software Engineer IoTG Platform Enabling Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
-Original Message- From: Roper, Matthew D Sent: Tuesday, April 07, 2015 11:46 AM To: Konduru, Chandra Cc: Daniel Vetter; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints On Tue, Apr 07, 2015 at 11:29:06AM -0700, Konduru, Chandra wrote: -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 07, 2015 1:44 AM To: Roper, Matthew D Cc: Konduru, Chandra; Vetter, Daniel; intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander Subject: Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints On Thu, Apr 02, 2015 at 04:03:22PM -0700, Matt Roper wrote: On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote: This patch converts intel_plane_state-src rect from 16.16 values into regular ints. This approach aligns with sprite_plane_state-src rects which are already in regular ints. Signed-off-by: Chandra Konduru chandra.kond...@intel.com You're not touching cursor state here, so I guess it stays in 16.16 form always? Does it need to be updated to behave the same way? Looking at our sprite code today, it treats intel_state-src as 16.16 for most of the check function, then re-writes it as integer pixels near the end, which I guess matches the type of change you're doing here. I still find this pretty confusing that our structure is sometimes interpreted in one way and other times interpreted a different way. Personally I think it would be less error-prone if we just treated src as 16.16 always, but if you to keep the current logic which changes the meaning at the end of the check() stage, can you add some comments to struct intel_plane_state clarifying that? Rewriting intel_plane_state won't work since on duplicate_state we'd need to undo that again. That's a bit too brittle imo. I'd go with Matt's suggestion to just use 16.16 everywhere. -Daniel Recently in upstream, sprite plane src rect changed to regular int at end of check before entering commit but left primary src rect as 16.16. This is causing issue for having any common helper function to handle sprites and primary. So my patch is aligning both primary and sprite's src rect as regular int after checks are done. Earlier Matt commented that it is upto i915's implementation how to keep its internal src rect values in its state which is his response to my earlier change to fix a bug when passing src rect keeping them in 16.16 format. I am fine whichever format you want. Can you or Matt make it clear which format to keep them? The internal src/dest rect are derived state that the driver tracks for its own purposes, so yeah, it's up to us how we decide to store it. The confusing (and error-prone) part is that our sprite check code treats it as 16.16 whereas our commit code treats it as integer; to make matters worse, we aren't even consistent with this scheme across the various plane types (primary, sprite, cursor). Even though the state gets copied in duplicate_state(), we don't have any immediate problems with the existing sprite code today because it does wind up getting blown away and recomputed before we have a chance to mix up the meaning of the values. Your patch here looks like it would work without problems today too for the same reason. But simply the inconsistency of what the value means at various points in the process bothers me because it is likely to cause mistakes as people write code in the future. Since the check phase is the more complex logic here, and since it depends on the 16.16 storage, it seems natural to me to just preserve that 16.16 format throughout and just shift as necessary in the commit step. From what I can see, the mid-operation switch between 16.16 and integer format in the sprite code originated with commit commit 96d61a7f267ff355a401ca23a732810027d10ba2 Author: Gustavo Padovan gustavo.pado...@collabora.co.uk Date: Fri Sep 5 17:04:47 2014 -0300 drm/i915: split intel_update_plane into check() and commit() when the check and commit steps were first split apart. The change isn't directly referenced in the commit message, so I think it just sort of snuck in under the radar. Today platform_update_plane(x, y, src_w, src_h) and platform_update_primary_plane(x, y) parameters are in regular integer format. keeping them as is. Will bring src rect back into 16.16 format. And any consumer of src will do 16 on src values as needed. Matt -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- Matt Roper Graphics Software Engineer IoTG Platform Enabling
Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
This patch converts intel_plane_state-src rect from 16.16
values into regular ints.
This approach aligns with sprite_plane_state-src rects
which are already in regular ints.
Signed-off-by: Chandra Konduru chandra.kond...@intel.com
You're not touching cursor state here, so I guess it stays in 16.16 form
always? Does it need to be updated to behave the same way?
Looking at our sprite code today, it treats intel_state-src as 16.16
for most of the check function, then re-writes it as integer pixels near
the end, which I guess matches the type of change you're doing here. I
still find this pretty confusing that our structure is sometimes
interpreted in one way and other times interpreted a different way.
Personally I think it would be less error-prone if we just treated src
as 16.16 always, but if you to keep the current logic which changes the
meaning at the end of the check() stage, can you add some comments to
struct intel_plane_state clarifying that?
With some extra comments to avoid future confusion, you can consider this
Reviewed-by: Matt Roper matthew.d.ro...@intel.com
Matt
---
drivers/gpu/drm/i915/intel_display.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index ee71bde..dddbe11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane,
intel_crtc-atomic.update_wm = true;
}
+ /*
+ * Hardware doesn't handle subpixel coordinates.
+ * Adjust to (macro)pixel boundary.
+ */
+ src-x1 = 16;
+ src-x2 = 16;
+ src-y1 = 16;
+ src-y2 = 16;
+
return 0;
}
@@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
intel_crtc = to_intel_crtc(crtc);
plane-fb = fb;
- crtc-x = src-x1 16;
- crtc-y = src-y1 16;
+ crtc-x = src-x1;
+ crtc-y = src-y1;
if (intel_crtc-active) {
if (state-visible) {
--
1.7.9.5
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
This patch converts intel_plane_state-src rect from 16.16
values into regular ints.
This approach aligns with sprite_plane_state-src rects
which are already in regular ints.
Signed-off-by: Chandra Konduru chandra.kond...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index ee71bde..dddbe11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane,
intel_crtc-atomic.update_wm = true;
}
+ /*
+* Hardware doesn't handle subpixel coordinates.
+* Adjust to (macro)pixel boundary.
+*/
+ src-x1 = 16;
+ src-x2 = 16;
+ src-y1 = 16;
+ src-y2 = 16;
+
return 0;
}
@@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
intel_crtc = to_intel_crtc(crtc);
plane-fb = fb;
- crtc-x = src-x1 16;
- crtc-y = src-y1 16;
+ crtc-x = src-x1;
+ crtc-y = src-y1;
if (intel_crtc-active) {
if (state-visible) {
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
