Re: [PATCH] RFC: drm/amd/display: enable ABGR and XBGR formats (v2)

[Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Tue, Jul 17, 2018 at 2:38 PM, Mauro Rossi <issor.oruam@gmail.com> wrote:
> Hi Alex,
>
> Il giorno mar 17 lug 2018 alle ore 15:43 Alex Deucher
> <alexdeucher@gmail.com> ha scritto:
>>
>> On Sun, Jul 15, 2018 at 10:03 PM, Mauro Rossi <issor.oruam@gmail.com>
>> wrote:
>> > From: Mauro Rossi <issor.oruam@gmail.com>
>> >
>> > (v1) {A,X}BGR8888 code paths are added in amdgpu_dm, by using an
>> > fb_format
>> >      already listed in dc/dc_hw_types.h
>> > (SURFACE_PIXEL_FORMAT_GRPH_ABGR8888),
>> >      and in dce 8.0, 10.0 and 11.0, i.e. Bonaire and later.
>> >      GRPH_FORMAT_ARGB8888 is used due to lack of specific
>> > GRPH_FORMAT_ABGR8888
>> >
>> > (v2) support for {A,X}BGR8888 in atombios_crtc (now in dce4 path, to be
>> > refined)
>> >      to initialize frame buffer device and avoid following dmesg error:
>> >      "[drm] Cannot find any crtc or sizes"
>> >
>> > Tested with oreo-x86 (hwcomposer.drm + gralloc.gbm + mesa-dev/radv)
>> > SurfaceFlinger can now select RGBA_8888 format for
>> > HWC_FRAMEBUFFER_TARGET
>> > No major regression or crash observed so far, but some android 2D
>> > overlay
>> > may be affected by color artifacts. Kind feedback requested.
>> >
>> > Signed-off-by: Mauro Rossi <issor.oruam@gmail.com>
>>
>> Please split the patch in three (one for radeon and one for amdgpu dc
>> and one for amdgpu non-dc).  Also the GRPH_SWAP_CONTROL register has a
>> crossbar where you can change the channel routing.  You may need that
>> for the channel routing to work correctly.
>>
>> Alex
>
>
> Thanks for your suggestion and guidance! :-)
>
> I may need some time to assimilate the suggestions and some confirmations,
> as I am an amateur in AMD GPU coding, to be honest, I should have mentioned
> that before.
>
> Regarding the radeon scope of changes,
> do you recommend to keep the enablement of {A,X}BGR8888  for dce4 and later,
> or to extend the enablement of  {A,X}BGR8888 to older families of radeon
> gpus/chipsets?

If you are motivated to enable it on older asics, go for it.

>
> What is the lower radeon family where {A,X}BGR8888  can be natively
> supported by HW,
> by means of  swap control registers for channel routing configuration?
>

Back to the AVIVO family (DCE1, r5xx).


> Based on the scope of  {A,X}BGR8888 support in final patches,
> I may need to add handling in other dce code and maybe other modules,
> could you please provide information in terms of necessary changes/high
> level steps to follow?
>
> Do you have some pointer to documentation on  swap control registers for the
> families
> that may be considered as 'safe to be kept in scope' for  {A,X}BGR8888
> support?

For DCE1 (r5xx chips), there was a swap bit in the
D1GRPH_CONTROL/D2GRPH_CONTROL registers.  Bit 16 (D1GRPH_SWAP_RB), if
set, swaps the R and B components.  See r500_reg.h.  For DCE2 (r6xx
chips) and newer, they use the RGB crossbars GRPH_SWAP_CONTROL (see
r600_reg.h and evergreen_reg.h)

DCE1:
http://developer.amd.com/wordpress/media/2012/10/RRG-216M56-03oOEM.pdf
DCE2+:
http://developer.amd.com/wordpress/media/2012/10/42590_m76_rrg_1.01o.pdf


>
> Last but not least I would like to ask you about how to test no-regression,
> even if this will come later,
> when patches will be in good shape for further evaluation, do you have tools
> and samples for conformance/no-regression testing?
> I am asking because I don't have samples for all families.

I have samples for most families.

Alex

>
> Kind regards
>
> Mauro
>
>
>
>>
>>
>>
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c            | 9 +++++++++
>> >  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c            | 9 +++++++++
>> >  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c             | 8 ++++++++
>> >  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 ++++++
>> >  drivers/gpu/drm/radeon/atombios_crtc.c            | 8 ++++++++
>> >  5 files changed, 40 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> > index 022f303463fc..d4280d2e7737 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> > @@ -2005,6 +2005,15 @@ static int dce_v10_0_crtc_do_set_base(struct
>> > drm_crtc *crtc,
>> >                 /* Greater 8 bpc fb needs to bypass hw-lut to retain
>> > precision */
>> >                 bypass_lut = true;
>> >                 break;
>> > +       case DRM_FORMAT_XBGR8888:
>> > +       case DRM_FORMAT_ABGR8888:
>> > +               fb_format = REG_SET_FIELD(0, GRPH_CONTROL, GRPH_DEPTH,
>> > 2);
>> > +               fb_format = REG_SET_FIELD(fb_format, GRPH_CONTROL,
>> > GRPH_FORMAT, 0); /* Hack */
>> > +#ifdef __BIG_ENDIAN
>> > +               fb_swap = REG_SET_FIELD(fb_swap, GRPH_SWAP_CNTL,
>> > GRPH_ENDIAN_SWAP,
>> > +                                       ENDIAN_8IN32);
>> > +#endif
>> > +               break;
>> >         default:
>> >                 DRM_ERROR("Unsupported screen format %s\n",
>> >                           drm_get_format_name(target_fb->format->format,
>> > &format_name));
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> > b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> > index 800a9f36ab4f..d48ee8f2e192 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> > @@ -2044,6 +2044,15 @@ static int dce_v11_0_crtc_do_set_base(struct
>> > drm_crtc *crtc,
>> >                 /* Greater 8 bpc fb needs to bypass hw-lut to retain
>> > precision */
>> >                 bypass_lut = true;
>> >                 break;
>> > +       case DRM_FORMAT_XBGR8888:
>> > +       case DRM_FORMAT_ABGR8888:
>> > +               fb_format = REG_SET_FIELD(0, GRPH_CONTROL, GRPH_DEPTH,
>> > 2);
>> > +               fb_format = REG_SET_FIELD(fb_format, GRPH_CONTROL,
>> > GRPH_FORMAT, 0); /* Hack */
>> > +#ifdef __BIG_ENDIAN
>> > +               fb_swap = REG_SET_FIELD(fb_swap, GRPH_SWAP_CNTL,
>> > GRPH_ENDIAN_SWAP,
>> > +                                       ENDIAN_8IN32);
>> > +#endif
>> > +               break;
>> >         default:
>> >                 DRM_ERROR("Unsupported screen format %s\n",
>> >                           drm_get_format_name(target_fb->format->format,
>> > &format_name));
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> > b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> > index 012e0a9ae0ff..0e2fc1ac475f 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> > @@ -1929,6 +1929,14 @@ static int dce_v8_0_crtc_do_set_base(struct
>> > drm_crtc *crtc,
>> >                 /* Greater 8 bpc fb needs to bypass hw-lut to retain
>> > precision */
>> >                 bypass_lut = true;
>> >                 break;
>> > +       case DRM_FORMAT_XBGR8888:
>> > +       case DRM_FORMAT_ABGR8888:
>> > +               fb_format = ((GRPH_DEPTH_32BPP <<
>> > GRPH_CONTROL__GRPH_DEPTH__SHIFT) |
>> > +                            (GRPH_FORMAT_ARGB8888 <<
>> > GRPH_CONTROL__GRPH_FORMAT__SHIFT)); /* Hack */
>> > +#ifdef __BIG_ENDIAN
>> > +               fb_swap = (GRPH_ENDIAN_8IN32 <<
>> > GRPH_SWAP_CNTL__GRPH_ENDIAN_SWAP__SHIFT);
>> > +#endif
>> > +               break;
>> >         default:
>> >                 DRM_ERROR("Unsupported screen format %s\n",
>> >                           drm_get_format_name(target_fb->format->format,
>> > &format_name));
>> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > index 63c67346d316..6c10fa291150 100644
>> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > @@ -1824,6 +1824,10 @@ static int fill_plane_attributes_from_fb(struct
>> > amdgpu_device *adev,
>> >         case DRM_FORMAT_ABGR2101010:
>> >                 plane_state->format =
>> > SURFACE_PIXEL_FORMAT_GRPH_ABGR2101010;
>> >                 break;
>> > +       case DRM_FORMAT_XBGR8888:
>> > +       case DRM_FORMAT_ABGR8888:
>> > +               plane_state->format =
>> > SURFACE_PIXEL_FORMAT_GRPH_ABGR8888;
>> > +               break;
>> >         case DRM_FORMAT_NV21:
>> >                 plane_state->format =
>> > SURFACE_PIXEL_FORMAT_VIDEO_420_YCbCr;
>> >                 break;
>> > @@ -3115,6 +3119,8 @@ static const uint32_t rgb_formats[] = {
>> >         DRM_FORMAT_XBGR2101010,
>> >         DRM_FORMAT_ARGB2101010,
>> >         DRM_FORMAT_ABGR2101010,
>> > +       DRM_FORMAT_XBGR8888,
>> > +       DRM_FORMAT_ABGR8888,
>> >  };
>> >
>> >  static const uint32_t yuv_formats[] = {
>> > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c
>> > b/drivers/gpu/drm/radeon/atombios_crtc.c
>> > index 02baaaf20e9d..b954b3658a33 100644
>> > --- a/drivers/gpu/drm/radeon/atombios_crtc.c
>> > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
>> > @@ -1259,6 +1259,14 @@ static int dce4_crtc_do_set_base(struct drm_crtc
>> > *crtc,
>> >                 /* Greater 8 bpc fb needs to bypass hw-lut to retain
>> > precision */
>> >                 bypass_lut = true;
>> >                 break;
>> > +       case DRM_FORMAT_XBGR8888:
>> > +       case DRM_FORMAT_ABGR8888:
>> > +               fb_format =
>> > (EVERGREEN_GRPH_DEPTH(EVERGREEN_GRPH_DEPTH_32BPP) |
>> > +
>> > EVERGREEN_GRPH_FORMAT(EVERGREEN_GRPH_FORMAT_ARGB8888));
>> > +#ifdef __BIG_ENDIAN
>> > +               fb_swap =
>> > EVERGREEN_GRPH_ENDIAN_SWAP(EVERGREEN_GRPH_ENDIAN_8IN32);
>> > +#endif
>> > +               break;
>> >         default:
>> >                 DRM_ERROR("Unsupported screen format %s\n",
>> >                           drm_get_format_name(target_fb->format->format,
>> > &format_name));
>> > --
>> > 2.17.1
>> >
>> > _______________________________________________
>> > amd-gfx mailing list
>> > amd-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx