Re: Query about INTEL modifiers referred in intel_display.c

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 17, 2018 at 11:55:47AM +0100, Ayan Halder wrote:
> Hi,
> 
> I was going through drivers/gpu/drm/i915/intel_display.c to get an
> understanding about how framebuffer modifiers are used in the drm
> subsystem. 
> I could see the following in intel_framebuffer_init(),
> (added in commit 2e2adb0573)
> 
> << some code >>
>     switch (mode_cmd->modifier[0]) { 
>     case I915_FORMAT_MOD_Y_TILED_CCS:
>     case I915_FORMAT_MOD_Yf_TILED_CCS:
>         switch (mode_cmd->pixel_format) {
>         case DRM_FORMAT_XBGR8888:
>         case DRM_FORMAT_ABGR8888:
>         case DRM_FORMAT_XRGB8888:
>         case DRM_FORMAT_ARGB8888:
>             break;
>         default:
>             DRM_DEBUG_KMS("RC supported only with RGB8888 formats\n");
>             goto err;
>     }
> << some code >>
> 
> And I see the following intel_primary_plane_format_mod_supported() -->
> skl_mod_supported()
> (added in commit 714244e280)
> 
> << some code >>
>     switch (format) {                                                                                 
>     case DRM_FORMAT_XRGB8888:                                                                         
>     case DRM_FORMAT_XBGR8888:                                                                         
>     case DRM_FORMAT_ARGB8888:                                                                         
>     case DRM_FORMAT_ABGR8888:                                                                         
>         if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||                                               
>             modifier == I915_FORMAT_MOD_Y_TILED_CCS)                                                  
>             return true;
> << some code >>
> 
> Is it a case of duplicacy? If we are checking for valid
> combination of formats and modifiers in intel_framebuffer_init(), then why do we 
> need to check it again in skl_mod_supported()? 
> 
> Can we just check the valid combination only in
> skl_mod_supported() and not in intel_framebuffer_init() ?

The check in intel_framebuffer_init() is there to prevent the creation
of framebuffers that can't be scanned out by any plane. In theory we
don't have to do that, but I think it's a good idea in case userspace
just blindly probes which format+modifier combos are supported. And in
fact, before we got the IN_FORMATS property on planes blind probing
was the only way to discover the supported modifiers.

That said, I do want to eliminate that code from
intel_framebuffer_init() and replace it with a piece of generic code
that simply goes through all the planes and checks whether any of
them support the requested format+modifier combo. I've posted some
patches for that, but there were some unforseen complications due
to how some other drivers want to use modifiers. The last patch
I posted on the topic https://patchwork.freedesktop.org/patch/211306/
should help us proceed but it didn't get reviewed so we're stuck
until I can nudge it forward somehow.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx