diff mbox series

[3/7] drm/i915: Enable 10bpc + CCS on TGL+

Message ID 20240918144445.5716-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: 10bpc/fp16 + CCS support | expand

Commit Message

Ville Syrjälä Sept. 18, 2024, 2:44 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

TGL+ support 10bpc compressed scanout. Enable it.

v2: Set .depth=30 for all variants to match drm_fourcc.c
    Set clear color block size to 0x0

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
 .../drm/i915/display/skl_universal_plane.c    |  8 ++---
 2 files changed, 40 insertions(+), 4 deletions(-)

Comments

Juha-Pekka Heikkila Oct. 4, 2024, 1:35 p.m. UTC | #1 
On 18.9.2024 17.44, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> TGL+ support 10bpc compressed scanout. Enable it.
> 
> v2: Set .depth=30 for all variants to match drm_fourcc.c
>      Set clear color block size to 0x0
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
>   .../drm/i915/display/skl_universal_plane.c    |  8 ++---
>   2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index bcf0d016f499..9b9da4f71f73 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
>   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
>   	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,

Is that comment about depth=30 for all variants because of these alpha 
formats? Why is that? Here on other formats alpha is taken as part of 
depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"


> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>   	{ .format = DRM_FORMAT_YUYV, .num_planes = 2,
>   	  .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>   	  .hsub = 2, .vsub = 1, .is_yuv = true },
> @@ -113,6 +125,18 @@ static const struct drm_format_info gen12_ccs_cc_formats[] = {
>   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
>   	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 3,
> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 3,
> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 3,
> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 3,
> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>   };
>   
>   static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
> @@ -128,6 +152,18 @@ static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
>   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
>   	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> +	  .hsub = 1, .vsub = 1, },
> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>   };
>   
>   struct intel_modifier_desc {
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 17d4c880ecc4..9f34df60b112 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2315,6 +2315,10 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>   	case DRM_FORMAT_XBGR8888:
>   	case DRM_FORMAT_ARGB8888:
>   	case DRM_FORMAT_ABGR8888:
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +	case DRM_FORMAT_ABGR2101010:
>   		if (intel_fb_is_ccs_modifier(modifier))
>   			return true;
>   		fallthrough;
> @@ -2331,10 +2335,6 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>   			return true;
>   		fallthrough;
>   	case DRM_FORMAT_RGB565:
> -	case DRM_FORMAT_XRGB2101010:
> -	case DRM_FORMAT_XBGR2101010:
> -	case DRM_FORMAT_ARGB2101010:
> -	case DRM_FORMAT_ABGR2101010:
>   	case DRM_FORMAT_XVYU2101010:
>   	case DRM_FORMAT_C8:
>   	case DRM_FORMAT_XBGR16161616F:
Ville Syrjälä Oct. 4, 2024, 6:03 p.m. UTC | #2 
On Fri, Oct 04, 2024 at 04:35:17PM +0300, Juha-Pekka Heikkila wrote:
> On 18.9.2024 17.44, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > TGL+ support 10bpc compressed scanout. Enable it.
> > 
> > v2: Set .depth=30 for all variants to match drm_fourcc.c
> >      Set clear color block size to 0x0
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
> >   .../drm/i915/display/skl_universal_plane.c    |  8 ++---
> >   2 files changed, 40 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index bcf0d016f499..9b9da4f71f73 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
> >   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> >   	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> >   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> 
> Is that comment about depth=30 for all variants because of these alpha 
> formats? Why is that? Here on other formats alpha is taken as part of 
> depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"

That stuff is just legacy compatibility stuff, and back in
the day peope decided that depth==32 simply means ARGB8888.
I'm not sure we should even state depth=30 on ARGB2101010
at all, or would it be better to leave it at 0.

Another option might be to just set .depth=0 on absolutely
all compressed formats. Using these with some legacy uapi
which only talks in terms of bpp and depth doesn't seem
feasible anyway.

But for now I think we just want to match drm_fourcc.c since
that's what we did for the other compressed formats.

> > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> >   	{ .format = DRM_FORMAT_YUYV, .num_planes = 2,
> >   	  .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> >   	  .hsub = 2, .vsub = 1, .is_yuv = true },
> > @@ -113,6 +125,18 @@ static const struct drm_format_info gen12_ccs_cc_formats[] = {
> >   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> >   	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> >   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 3,
> > +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 3,
> > +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 3,
> > +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 3,
> > +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> >   };
> >   
> >   static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
> > @@ -128,6 +152,18 @@ static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
> >   	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> >   	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> >   	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> > +	  .hsub = 1, .vsub = 1, },
> > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
> > +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
> > +	  .hsub = 1, .vsub = 1, .has_alpha = true },
> >   };
> >   
> >   struct intel_modifier_desc {
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 17d4c880ecc4..9f34df60b112 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -2315,6 +2315,10 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
> >   	case DRM_FORMAT_XBGR8888:
> >   	case DRM_FORMAT_ARGB8888:
> >   	case DRM_FORMAT_ABGR8888:
> > +	case DRM_FORMAT_XRGB2101010:
> > +	case DRM_FORMAT_XBGR2101010:
> > +	case DRM_FORMAT_ARGB2101010:
> > +	case DRM_FORMAT_ABGR2101010:
> >   		if (intel_fb_is_ccs_modifier(modifier))
> >   			return true;
> >   		fallthrough;
> > @@ -2331,10 +2335,6 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
> >   			return true;
> >   		fallthrough;
> >   	case DRM_FORMAT_RGB565:
> > -	case DRM_FORMAT_XRGB2101010:
> > -	case DRM_FORMAT_XBGR2101010:
> > -	case DRM_FORMAT_ARGB2101010:
> > -	case DRM_FORMAT_ABGR2101010:
> >   	case DRM_FORMAT_XVYU2101010:
> >   	case DRM_FORMAT_C8:
> >   	case DRM_FORMAT_XBGR16161616F:
Juha-Pekka Heikkila Oct. 8, 2024, 9:01 a.m. UTC | #3 
On 4.10.2024 21.03, Ville Syrjälä wrote:
> On Fri, Oct 04, 2024 at 04:35:17PM +0300, Juha-Pekka Heikkila wrote:
>> On 18.9.2024 17.44, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> TGL+ support 10bpc compressed scanout. Enable it.
>>>
>>> v2: Set .depth=30 for all variants to match drm_fourcc.c
>>>       Set clear color block size to 0x0
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
>>>    .../drm/i915/display/skl_universal_plane.c    |  8 ++---
>>>    2 files changed, 40 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
>>> index bcf0d016f499..9b9da4f71f73 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
>>> @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
>>>    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
>>>    	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>>    	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
>>
>> Is that comment about depth=30 for all variants because of these alpha
>> formats? Why is that? Here on other formats alpha is taken as part of
>> depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"
> 
> That stuff is just legacy compatibility stuff, and back in
> the day peope decided that depth==32 simply means ARGB8888.
> I'm not sure we should even state depth=30 on ARGB2101010
> at all, or would it be better to leave it at 0.
> 
> Another option might be to just set .depth=0 on absolutely
> all compressed formats. Using these with some legacy uapi
> which only talks in terms of bpp and depth doesn't seem
> feasible anyway.
> 
> But for now I think we just want to match drm_fourcc.c since
> that's what we did for the other compressed formats.

ack. patch set is

Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

> 
>>> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>>    	{ .format = DRM_FORMAT_YUYV, .num_planes = 2,
>>>    	  .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
>>>    	  .hsub = 2, .vsub = 1, .is_yuv = true },
>>> @@ -113,6 +125,18 @@ static const struct drm_format_info gen12_ccs_cc_formats[] = {
>>>    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
>>>    	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>>>    	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 3,
>>> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 3,
>>> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 3,
>>> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 3,
>>> +	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>>    };
>>>    
>>>    static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
>>> @@ -128,6 +152,18 @@ static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
>>>    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
>>>    	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>>>    	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, },
>>> +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>> +	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
>>> +	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
>>> +	  .hsub = 1, .vsub = 1, .has_alpha = true },
>>>    };
>>>    
>>>    struct intel_modifier_desc {
>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> index 17d4c880ecc4..9f34df60b112 100644
>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> @@ -2315,6 +2315,10 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>>>    	case DRM_FORMAT_XBGR8888:
>>>    	case DRM_FORMAT_ARGB8888:
>>>    	case DRM_FORMAT_ABGR8888:
>>> +	case DRM_FORMAT_XRGB2101010:
>>> +	case DRM_FORMAT_XBGR2101010:
>>> +	case DRM_FORMAT_ARGB2101010:
>>> +	case DRM_FORMAT_ABGR2101010:
>>>    		if (intel_fb_is_ccs_modifier(modifier))
>>>    			return true;
>>>    		fallthrough;
>>> @@ -2331,10 +2335,6 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>>>    			return true;
>>>    		fallthrough;
>>>    	case DRM_FORMAT_RGB565:
>>> -	case DRM_FORMAT_XRGB2101010:
>>> -	case DRM_FORMAT_XBGR2101010:
>>> -	case DRM_FORMAT_ARGB2101010:
>>> -	case DRM_FORMAT_ABGR2101010:
>>>    	case DRM_FORMAT_XVYU2101010:
>>>    	case DRM_FORMAT_C8:
>>>    	case DRM_FORMAT_XBGR16161616F:
>
Xi Ruoyao Nov. 25, 2024, 6:55 a.m. UTC | #4 
On Tue, 2024-10-08 at 12:01 +0300, Juha-Pekka Heikkila wrote:
> On 4.10.2024 21.03, Ville Syrjälä wrote:
> > On Fri, Oct 04, 2024 at 04:35:17PM +0300, Juha-Pekka Heikkila wrote:
> > > On 18.9.2024 17.44, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > TGL+ support 10bpc compressed scanout. Enable it.
> > > > 
> > > > v2: Set .depth=30 for all variants to match drm_fourcc.c
> > > >       Set clear color block size to 0x0
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >    drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
> > > >    .../drm/i915/display/skl_universal_plane.c    |  8 ++---
> > > >    2 files changed, 40 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > index bcf0d016f499..9b9da4f71f73 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
> > > >    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > > >    	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > >    	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > > > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > +	  .hsub = 1, .vsub = 1, },
> > > > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > +	  .hsub = 1, .vsub = 1, },
> > > > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> > > 
> > > Is that comment about depth=30 for all variants because of these alpha
> > > formats? Why is that? Here on other formats alpha is taken as part of
> > > depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"
> > 
> > That stuff is just legacy compatibility stuff, and back in
> > the day peope decided that depth==32 simply means ARGB8888.
> > I'm not sure we should even state depth=30 on ARGB2101010
> > at all, or would it be better to leave it at 0.
> > 
> > Another option might be to just set .depth=0 on absolutely
> > all compressed formats. Using these with some legacy uapi
> > which only talks in terms of bpp and depth doesn't seem
> > feasible anyway.
> > 
> > But for now I think we just want to match drm_fourcc.c since
> > that's what we did for the other compressed formats.
> 
> ack. patch set is
> 
> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

Hi Ville and Juha-Pekka,

Unfortunately this commit has caused gnome-shell to display nothing on
my system.  Its log contains error messages:

    Failed to ensure KMS FB ID on /dev/dri/card1: drmModeAddFB2WithModifiers failed: Invalid argument
    meta_frame_native_release: assertion '!frame_native->kms_update' failed

Reverting commits 7c35015fab5d ("drm/i915: Enable fp16 + CCS on TGL+")
and c315fbfa44f4 (this one) "fixes" the issue for me.

The system does have a TGL (i5-11300H) but I don't think my monitor
(it's just the display panel of a budget laptop) supports 10bpc.

Any pointer on debugging this further?
Ville Syrjälä Nov. 27, 2024, 5:57 a.m. UTC | #5 
On Mon, Nov 25, 2024 at 02:55:34PM +0800, Xi Ruoyao wrote:
> On Tue, 2024-10-08 at 12:01 +0300, Juha-Pekka Heikkila wrote:
> > On 4.10.2024 21.03, Ville Syrjälä wrote:
> > > On Fri, Oct 04, 2024 at 04:35:17PM +0300, Juha-Pekka Heikkila wrote:
> > > > On 18.9.2024 17.44, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > TGL+ support 10bpc compressed scanout. Enable it.
> > > > > 
> > > > > v2: Set .depth=30 for all variants to match drm_fourcc.c
> > > > >       Set clear color block size to 0x0
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
> > > > >    .../drm/i915/display/skl_universal_plane.c    |  8 ++---
> > > > >    2 files changed, 40 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > index bcf0d016f499..9b9da4f71f73 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
> > > > >    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > > > >    	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > >    	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > > > > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> > > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > > +	  .hsub = 1, .vsub = 1, },
> > > > > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> > > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > > +	  .hsub = 1, .vsub = 1, },
> > > > > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> > > > 
> > > > Is that comment about depth=30 for all variants because of these alpha
> > > > formats? Why is that? Here on other formats alpha is taken as part of
> > > > depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"
> > > 
> > > That stuff is just legacy compatibility stuff, and back in
> > > the day peope decided that depth==32 simply means ARGB8888.
> > > I'm not sure we should even state depth=30 on ARGB2101010
> > > at all, or would it be better to leave it at 0.
> > > 
> > > Another option might be to just set .depth=0 on absolutely
> > > all compressed formats. Using these with some legacy uapi
> > > which only talks in terms of bpp and depth doesn't seem
> > > feasible anyway.
> > > 
> > > But for now I think we just want to match drm_fourcc.c since
> > > that's what we did for the other compressed formats.
> > 
> > ack. patch set is
> > 
> > Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> 
> Hi Ville and Juha-Pekka,
> 
> Unfortunately this commit has caused gnome-shell to display nothing on
> my system.  Its log contains error messages:
> 
>     Failed to ensure KMS FB ID on /dev/dri/card1: drmModeAddFB2WithModifiers failed: Invalid argument
>     meta_frame_native_release: assertion '!frame_native->kms_update' failed
> 
> Reverting commits 7c35015fab5d ("drm/i915: Enable fp16 + CCS on TGL+")
> and c315fbfa44f4 (this one) "fixes" the issue for me.
> 
> The system does have a TGL (i5-11300H) but I don't think my monitor
> (it's just the display panel of a budget laptop) supports 10bpc.
> 
> Any pointer on debugging this further?

Please file a new bug at
https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/new

Boot with 'drm.debug=0x1e log_buf_len=4M' added to the kernel cmdline
and attach the resulting dmesg to the bug.
Xi Ruoyao Nov. 27, 2024, 6:58 a.m. UTC | #6 
On Wed, 2024-11-27 at 07:57 +0200, Ville Syrjälä wrote:
> On Mon, Nov 25, 2024 at 02:55:34PM +0800, Xi Ruoyao wrote:
> > On Tue, 2024-10-08 at 12:01 +0300, Juha-Pekka Heikkila wrote:
> > > On 4.10.2024 21.03, Ville Syrjälä wrote:
> > > > On Fri, Oct 04, 2024 at 04:35:17PM +0300, Juha-Pekka Heikkila wrote:
> > > > > On 18.9.2024 17.44, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > TGL+ support 10bpc compressed scanout. Enable it.
> > > > > > 
> > > > > > v2: Set .depth=30 for all variants to match drm_fourcc.c
> > > > > >       Set clear color block size to 0x0
> > > > > > 
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/display/intel_fb.c       | 36 +++++++++++++++++++
> > > > > >    .../drm/i915/display/skl_universal_plane.c    |  8 ++---
> > > > > >    2 files changed, 40 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > > index bcf0d016f499..9b9da4f71f73 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > > > @@ -67,6 +67,18 @@ static const struct drm_format_info gen12_ccs_formats[] = {
> > > > > >    	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > > > > >    	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > > >    	  .hsub = 1, .vsub = 1, .has_alpha = true },
> > > > > > +	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
> > > > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > > > +	  .hsub = 1, .vsub = 1, },
> > > > > > +	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
> > > > > > +	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > > > > > +	  .hsub = 1, .vsub = 1, },
> > > > > > +	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
> > > > > 
> > > > > Is that comment about depth=30 for all variants because of these alpha
> > > > > formats? Why is that? Here on other formats alpha is taken as part of
> > > > > depth, like in above "DRM_FORMAT_ABGR8888, .depth = 32"
> > > > 
> > > > That stuff is just legacy compatibility stuff, and back in
> > > > the day peope decided that depth==32 simply means ARGB8888.
> > > > I'm not sure we should even state depth=30 on ARGB2101010
> > > > at all, or would it be better to leave it at 0.
> > > > 
> > > > Another option might be to just set .depth=0 on absolutely
> > > > all compressed formats. Using these with some legacy uapi
> > > > which only talks in terms of bpp and depth doesn't seem
> > > > feasible anyway.
> > > > 
> > > > But for now I think we just want to match drm_fourcc.c since
> > > > that's what we did for the other compressed formats.
> > > 
> > > ack. patch set is
> > > 
> > > Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > 
> > Hi Ville and Juha-Pekka,
> > 
> > Unfortunately this commit has caused gnome-shell to display nothing on
> > my system.  Its log contains error messages:
> > 
> >     Failed to ensure KMS FB ID on /dev/dri/card1: drmModeAddFB2WithModifiers failed: Invalid argument
> >     meta_frame_native_release: assertion '!frame_native->kms_update' failed
> > 
> > Reverting commits 7c35015fab5d ("drm/i915: Enable fp16 + CCS on TGL+")
> > and c315fbfa44f4 (this one) "fixes" the issue for me.
> > 
> > The system does have a TGL (i5-11300H) but I don't think my monitor
> > (it's just the display panel of a budget laptop) supports 10bpc.
> > 
> > Any pointer on debugging this further?
> 
> Please file a new bug at
> https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/new
> 
> Boot with 'drm.debug=0x1e log_buf_len=4M' added to the kernel cmdline
> and attach the resulting dmesg to the bug.

https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13057
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index bcf0d016f499..9b9da4f71f73 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -67,6 +67,18 @@  static const struct drm_format_info gen12_ccs_formats[] = {
 	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
 	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
 	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
 	{ .format = DRM_FORMAT_YUYV, .num_planes = 2,
 	  .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
 	  .hsub = 2, .vsub = 1, .is_yuv = true },
@@ -113,6 +125,18 @@  static const struct drm_format_info gen12_ccs_cc_formats[] = {
 	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
 	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
 	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 3,
+	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 3,
+	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 3,
+	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 3,
+	  .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 0 }, .block_h = { 1, 1, 0 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
 };
 
 static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
@@ -128,6 +152,18 @@  static const struct drm_format_info gen12_flat_ccs_cc_formats[] = {
 	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
 	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
 	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_XRGB2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_XBGR2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
+	  .hsub = 1, .vsub = 1, },
+	{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
+	{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .num_planes = 2,
+	  .char_per_block = { 4, 0 }, .block_w = { 1, 0 }, .block_h = { 1, 0 },
+	  .hsub = 1, .vsub = 1, .has_alpha = true },
 };
 
 struct intel_modifier_desc {
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 17d4c880ecc4..9f34df60b112 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -2315,6 +2315,10 @@  static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ARGB8888:
 	case DRM_FORMAT_ABGR8888:
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ARGB2101010:
+	case DRM_FORMAT_ABGR2101010:
 		if (intel_fb_is_ccs_modifier(modifier))
 			return true;
 		fallthrough;
@@ -2331,10 +2335,6 @@  static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
 			return true;
 		fallthrough;
 	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-	case DRM_FORMAT_ARGB2101010:
-	case DRM_FORMAT_ABGR2101010:
 	case DRM_FORMAT_XVYU2101010:
 	case DRM_FORMAT_C8:
 	case DRM_FORMAT_XBGR16161616F: