radeon_drm.h: missing TILE_MODE definition?

[Alex Deucher]

On Thu, Feb 11, 2016 at 11:55 AM, Alexandre Demers
<alexandre.f.demers at gmail.com> wrote:
>
> On Thu, 11 Feb 2016 at 10:30 Alex Deucher <alexdeucher at gmail.com> wrote:
>>
>> On Thu, Feb 11, 2016 at 12:23 AM, Alexandre Demers
>> <alexandre.f.demers at gmail.com> 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?
>

Go ahead.

Alex

> 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