On 27 April 2017 at 11:52, Constantine Kharlamov <hi-an...@yandex.ru> wrote:
> 27.04.2017, 13:26, "Emil Velikov" <emil.l.veli...@gmail.com>:
>> Hi Constantine,
>>
>> I'm not familiar with r600, so mostly a drive-by idea/nit.
>
> I'm not familiar with graphics :P Well, perhaps a little now.
>
>> On 25 April 2017 at 12:59, Constantine Kharlamov <hi-an...@yandex.ru> wrote:
>>
>>> @@ -1746,45 +1747,47 @@ static void
>>> evergreen_emit_framebuffer_state(struct r600_context *rctx, struct r
>>> radeon_set_context_reg(cs, R_028E50_CB_COLOR8_INFO + (i -
>>> 8) * 0x1C, 0);
>>>
>>> /* ZS buffer. */
>>> - if (state->zsbuf) {
>>
>>> + if (rctx->framebuffer.dirty_zsbuf) {
>>> + if (state->zsbuf) {
>>
>> You can fold both if statements in a single one, leaving the
>> indentation level as-is.
>> As a bonus the resulting patch will be a lot smaller ;-)
>>
>> Same can be done across the patch.
>>
>> -Emil
>
> Btw it's how it's done in radeonsi, but I can't do the same in r600g because
> of the drm_minor check. The original code goes like:
>
> if (state->zsbuf) {
> /* … */
> } else if (rctx->screen->b.info.drm_minor >= 18) {
> /* … */
> }
>
I've missed the drm_version check, thanks for the wake-up call.
Thinking out loud:
Last year we bumped to the kernel requirement to 3.2 (released in 2011).
Worth doing a similar one this year, say 3.10 (2013)? It should drop
~1/3 of the drm_minor checks.
> The only way to keep indentation is to change it like:
>
> if (rctx->framebuffer.dirty_zsbuf && state->zsbuf) {
> /* … */
> } else if (rctx->framebuffer.dirty_zsbuf &&
> rctx->screen->b.info.drm_minor >= 18) {
> /* … */
> }
>
> I think, rather than duplicate the check, it's better to add an indentation
> level.
Personally I would opt for the latter, but I am not going through the
r600 codebase so feel free to ignore me.
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
