diff mbox

[3/7] drm/i915/skl: Adjust intel_fb_align_height() for Yb/Yf tiling

Message ID 1424706961-27519-4-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 23, 2015, 3:55 p.m. UTC 
From: Damien Lespiau <damien.lespiau@intel.com>

We now need the bpp of the fb as Yf tiling has different tile widths
depending on it.

v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
v3: Rebased for fb modifier changes. (Tvrtko Ursulin)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

Comments

Lespiau, Damien Feb. 24, 2015, 4:54 p.m. UTC | #1 
On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> We now need the bpp of the fb as Yf tiling has different tile widths
> depending on it.
> 
> v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Might want to add a MISSING_CASE() here as well. For the record, the
vertical alignments for Yf are taken from a dodgy looking document found
by chance.

We don't have a 128bpp format supported by display

Now that I think about it, for the horizontal stride, BSpec says:

  - 8 bpp -> 64 bytes
  - * bpp -> 128 bytes

Given that a tile is a page size this would give for the vertical
alignment:

  - 8 bpp -> 64 byes (that's what we have)
  - * -> 32 bytes

So the 64bpp cases look a bit suspicious. Given that one the goals for
this new tiling format is to have tiles as square as possible (in terms
of pixels, not byte size), it'd make sense to have different strides
constraints for the 64bpp format, so it could be BSpec being wrong on
the horizontal stride constraint for 64bpp?
Tvrtko Ursulin Feb. 25, 2015, 10:54 a.m. UTC | #2 
On 02/24/2015 04:54 PM, Damien Lespiau wrote:
> On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
>> From: Damien Lespiau <damien.lespiau@intel.com>
>>
>> We now need the bpp of the fb as Yf tiling has different tile widths
>> depending on it.
>>
>> v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
>> v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Might want to add a MISSING_CASE() here as well. For the record, the

Will do.

> vertical alignments for Yf are taken from a dodgy looking document found
> by chance.
>
> We don't have a 128bpp format supported by display

Ok but we can't really error out from this helper so what do you suggest?

> Now that I think about it, for the horizontal stride, BSpec says:
>
>    - 8 bpp -> 64 bytes
>    - * bpp -> 128 bytes
>
> Given that a tile is a page size this would give for the vertical
> alignment:
>
>    - 8 bpp -> 64 byes (that's what we have)
>    - * -> 32 bytes
>
> So the 64bpp cases look a bit suspicious. Given that one the goals for
> this new tiling format is to have tiles as square as possible (in terms
> of pixels, not byte size), it'd make sense to have different strides
> constraints for the 64bpp format, so it could be BSpec being wrong on
> the horizontal stride constraint for 64bpp?

It's either square or 2:1 rectangular. I'll try to double check the 
numbers for 64bpp then.

Regards,

Tvrtko
Lespiau, Damien Feb. 25, 2015, 2 p.m. UTC | #3 
On Wed, Feb 25, 2015 at 10:54:31AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/24/2015 04:54 PM, Damien Lespiau wrote:
> >On Mon, Feb 23, 2015 at 03:55:57PM +0000, Tvrtko Ursulin wrote:
> >>From: Damien Lespiau <damien.lespiau@intel.com>
> >>
> >>We now need the bpp of the fb as Yf tiling has different tile widths
> >>depending on it.
> >>
> >>v2: Rebased for the new addfb2 interface. (Tvrtko Ursulin)
> >>v3: Rebased for fb modifier changes. (Tvrtko Ursulin)
> >>
> >>Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >Might want to add a MISSING_CASE() here as well. For the record, the
> 
> Will do.
> 
> >vertical alignments for Yf are taken from a dodgy looking document found
> >by chance.
> >
> >We don't have a 128bpp format supported by display
> 
> Ok but we can't really error out from this helper so what do you suggest?

We could just remove the 128 case, we can't reach it.

> >Now that I think about it, for the horizontal stride, BSpec says:
> >
> >   - 8 bpp -> 64 bytes
> >   - * bpp -> 128 bytes
> >
> >Given that a tile is a page size this would give for the vertical
> >alignment:
> >
> >   - 8 bpp -> 64 byes (that's what we have)
> >   - * -> 32 bytes
> >
> >So the 64bpp cases look a bit suspicious. Given that one the goals for
> >this new tiling format is to have tiles as square as possible (in terms
> >of pixels, not byte size), it'd make sense to have different strides
> >constraints for the 64bpp format, so it could be BSpec being wrong on
> >the horizontal stride constraint for 64bpp?
> 
> It's either square or 2:1 rectangular. I'll try to double check the numbers
> for 64bpp then.

Thanks. Thinking about it a bit more, it could just be that the display
engine has a slightly stricter constraint than the 3D pipeline for the
alignment of 64bpc fbs. And so the code would be fine. It's all
assumptions though.
Lespiau, Damien Feb. 25, 2015, 3:20 p.m. UTC | #4 
On Wed, Feb 25, 2015 at 02:00:18PM +0000, Damien Lespiau wrote:
> > It's either square or 2:1 rectangular. I'll try to double check the numbers
> > for 64bpp then.
> 
> Thanks. Thinking about it a bit more, it could just be that the display
> engine has a slightly stricter constraint than the 3D pipeline for the
> alignment of 64bpc fbs. And so the code would be fine. It's all
> assumptions though.

Given that it could be just that and we're doing things as they are
documented, you can add my:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a523d84..4f0033a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2195,9 +2195,36 @@  intel_fb_align_height(struct drm_device *dev, int height,
 		      uint64_t fb_format_modifier)
 {
 	int tile_height;
+	uint32_t bits_per_pixel;
 
-	tile_height = fb_format_modifier == I915_FORMAT_MOD_X_TILED ?
-		(IS_GEN2(dev) ? 16 : 8) : 1;
+	switch (fb_format_modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		tile_height = IS_GEN2(dev) ? 16 : 8;
+		break;
+	case I915_FORMAT_MOD_Y_TILED:
+		tile_height = 32;
+		break;
+	case I915_FORMAT_MOD_Yf_TILED:
+		bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8;
+		switch (bits_per_pixel) {
+		default:
+		case 8:
+			tile_height = 64;
+			break;
+		case 16:
+		case 32:
+			tile_height = 32;
+			break;
+		case 64:
+		case 128:
+			tile_height = 16;
+			break;
+		}
+		break;
+	default:
+		tile_height = 1;
+		break;
+	}
 
 	return ALIGN(height, tile_height);
 }