<div dir="ltr"><br><div class="gmail_quote"><div dir="ltr">On Thu, 11 Feb 2016 at 10:30 Alex Deucher <<a href="mailto:alexdeucher@gmail.com">alexdeucher@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, Feb 11, 2016 at 12:23 AM, Alexandre Demers<br>
<<a href="mailto:alexandre.f.demers@gmail.com" target="_blank">alexandre.f.demers@gmail.com</a>> wrote:<br>
> I was looking at /drivers/gpu/drm/radeon/atombios_crtc.c and at radeon_drm.h<br>
> (both under kernel and libdrm). I noticed that there seems to be a missing<br>
> TILE_MODE definition: under atombios_crtc, line 1289<br>
> (/drivers/gpu/drm/radeon/atombios_crtc.c#L1289), an unexplained value is<br>
> being used (index = 10;) compared to the rest of the code around where<br>
> defined variables are being used.<br>
><br>
> Looking at the defined variables under radeon_drm.h, there is a missing<br>
> value in the tile index. Index 10 is missing. If it was defined, it could be<br>
> used in place of the numerical value at line 1289 under atombios_crtc.c.<br>
> According to the other names and usages, shouldn't there be a #define<br>
> SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP 10? In fact, the equivalent of<br>
> SI_TILE_MODE_COLORD_2D_8BPP is CIK_TILE_MODE_COLOR_2D under CIK; under CIK,<br>
> there is a variable defined for index 10, which is<br>
> CIK_TILE_MODE_COLOR_2D_SCANOUT. Thus, I'd be inclined to think there should<br>
> really be a SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP variable defined.<br>
><br>
> I've been searching in the SI 3D register documentation and I couldn't find<br>
> a tile index table to relate to.<br>
><br>
> Lastly, based on how the other "X_2D_SCANOUT_YBPP" variables are covered<br>
> under si_surface_sanity() (libdrm's radeon_surface.c), is it expected that<br>
> this index value (10) is not covered specifically. Should there be a "case<br>
> 1: *tile_mode = SI_TILE_MODE_COLOR_2D_SCANOUT_8BPP; break;" at line 1362,<br>
> before "case 2: ..."? Would it make sense?<br>
><br>
<br>
I don't think it's a particularly useful case.  In practice I doubt it<br>
would ever be hit.  I guess it's for 8bpp greyscale.  The 3D engine<br>
can't render to indexed color or 332 surfaces.  If you aren't using<br>
the 3D engine, there's not much point in using tiling to begin with.<br>
<br>
Alex<br></blockquote><div><br></div><div>Thanks for the answer.</div><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><div>Cheers</div><div>Alexandre Demers</div></div></div>