<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 11:34 AM, Eduardo Lima Mitev <span dir="ltr"><<a href="mailto:elima@igalia.com" target="_blank">elima@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the review! Please see some comments inline:<br>
<span class=""><br>
On 03/23/2015 06:53 PM, Laura Ekstrand wrote:<br>
> I'm glad you found this target checking problem.  I have come across the<br>
> same problem in other texture functions, but I have been too busy to<br>
> find all instances of this.  It's kind of a natural, negative byproduct<br>
> of adding DSA.<br>
><br>
</span>> [..]<br>
<span class="">><br>
> I recommend getting rid of the legal_texsubimage_target check in<br>
> texsubimage_error_check and moving it up into texturesubimage, right<br>
> after the texObj has been retrieved. Otherwise in the texsubimage case,<br>
> the target will get checked twice, once in texsubimage and once in<br>
> texsubimage-error-check.  In general, checking hurts driver performance,<br>
> so we should aim to get rid of redundancies.<br>
><br>
><br>
<br>
</span>Yes, will do that.<br>
<span class=""><br>
><br>
> This is not related to target checking and should be moved to a separate<br>
> commit (with a name like gles 3 support for texsubimage_error_check).<br>
><br>
<br>
</span>Ok, makes sense.<br>
<span class=""><br>
><br>
> I would also move this block up to about line 2508 so it can sit next to<br>
> the other format and type checks there.  It would be nice to have all of<br>
> the format_and_type_checks formatted into one if, else if, else block so<br>
> it's really obvious what's going on, too.<br>
><br>
>     +   if (_mesa_is_gles3(ctx)) {<br>
>     +      /* Validation of format and type for ES3 has to be done here<br>
>     +       * after the texture image is resolved, because the internal<br>
>     +       * format is needed for the verification<br>
>     +       */<br>
>     +      err = _mesa_es3_error_check_format_and_type(ctx, format, type,<br>
>     +<br>
>     texImage->InternalFormat);<br>
>     +      if (err != GL_NO_ERROR) {<br>
>     +         _mesa_error(ctx, err,<br>
>     +                     "%s(incompatible format = %s, type = %s, "<br>
>     +                     "internalformat = %s)",<br>
>     +                     callerName, _mesa_lookup_enum_by_nr(format),<br>
>     +                     _mesa_lookup_enum_by_nr(type),<br>
>     +<br>
>      _mesa_lookup_enum_by_nr(texImage->InternalFormat));<br>
>     +         return GL_TRUE;<br>
>     +      }<br>
>     +   }<br>
>     +<br>
<br>
</span>Note that this error check has to be performed after the texture image<br>
is resolved, because the internal format is not known before.<br>
<br>
If we want to merge it with the above format and type error checks, the<br>
only choice is to move all down after:<br>
<br>
texImage = _mesa_select_tex_image(texObj, target, level);<br>
<br>
But then we have a chicken-egg situation, since that function requires<br>
target to be validated before.<br>
<br>
WDYT?<br>
<br>
cheers,<br>
Eduardo<br>
<br>
</blockquote></div>What does the (es)_error_check_format_and_type have to do with validating the target?  I thought legal_texsubimage_target did all of the target checking?  So if we move up _mesa_select_tex_image above the format and type checks, it shouldn't be a problem because the target is already checked.  (That's my understanding of the problem.)<br></div></div>