On Thu, Feb 11, 2016 at 11:55 AM, Alexandre Demers <alexandre.f.demers@xxxxxxxxx> wrote: > > 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? > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
