On Thu, 11 Feb 2016 at 10:30 Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
On Thu, Feb 11, 2016 at 12:23 AM, Alexandre Demers
<alexandre.f.demers@xxxxxxxxx> wrote:
> I was looking at /drivers/gpu/drm/radeon/atombios_crtc.c and at radeon_drm.h
> (both under kernel and libdrm). I noticed that there seems to be a missing
> TILE_MODE definition: under atombios_crtc, line 1289
> (/drivers/gpu/drm/radeon/atombios_crtc.c#L1289), an unexplained value is
> being used (index = 10;) compared to the rest of the code around where
> defined variables are being used.
>
> Looking at the defined variables under radeon_drm.h, there is a missing
> value in the tile index. Index 10 is missing. If it was defined, it could be
> used in place of the numerical value at line 1289 under atombios_crtc.c.
> According to the other names and usages, shouldn't there be a #define
> SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP 10? In fact, the equivalent of
> SI_TILE_MODE_COLORD_2D_8BPP is CIK_TILE_MODE_COLOR_2D under CIK; under CIK,
> there is a variable defined for index 10, which is
> CIK_TILE_MODE_COLOR_2D_SCANOUT. Thus, I'd be inclined to think there should
> really be a SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP variable defined.
>
> I've been searching in the SI 3D register documentation and I couldn't find
> a tile index table to relate to.
>
> Lastly, based on how the other "X_2D_SCANOUT_YBPP" variables are covered
> under si_surface_sanity() (libdrm's radeon_surface.c), is it expected that
> this index value (10) is not covered specifically. Should there be a "case
> 1: *tile_mode = SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP; break;" at line 1362,
> before "case 2: ..."? Would it make sense?
>
I don't think it's a particularly useful case. In practice I doubt it
would ever be hit. I guess it's for 8bpp greyscale. The 3D engine
can't render to indexed color or 332 surfaces. If you aren't using
the 3D engine, there's not much point in using tiling to begin with.
Alex
Thanks for the answer.
At least, I'd like to add a "#define SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP 10;" (if that variable name makes sense, I'm open to any other more meaningful name) just so we don't have a "index = 10" right there in the middle of the code while other cases use defined variables: it is mostly about consistency and readability of the code. Any objection?
Your comment is more about if this case/index value should be covered under si_surface_sanity(). I'll trust you on the fact that it shouldn't be encountered.
Cheers
Alexandre Demers
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
