Re: [Mesa-dev] [PATCH 10/12] intel/blorp: Handle clearing compressed surfaces

[Jason Ekstrand]

On Mon, Oct 2, 2017 at 10:49 AM, Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:

> On 29/09/17 04:23, Matt Turner wrote:
>
>> On Fri, Sep 15, 2017 at 9:01 AM, Jason Ekstrand <ja...@jlekstrand.net>
>> wrote:
>>
>>> ---
>>>   src/intel/blorp/blorp_clear.c | 24 +++++++++++++++++-------
>>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/intel/blorp/blorp_clear.c
>>> b/src/intel/blorp/blorp_clear.c
>>> index 0feebef..e8b1e32 100644
>>> --- a/src/intel/blorp/blorp_clear.c
>>> +++ b/src/intel/blorp/blorp_clear.c
>>> @@ -442,14 +442,24 @@ blorp_clear(struct blorp_batch *batch,
>>>         if (batch->blorp->isl_dev->info->gen == 4 &&
>>>             (params.dst.surf.usage & ISL_SURF_USAGE_CUBE_BIT)) {
>>>            blorp_surf_convert_to_single_slice(batch->blorp->isl_dev,
>>> &params.dst);
>>> +      }
>>> +
>>> +      if (isl_format_is_compressed(params.dst.surf.format)) {
>>> +         blorp_surf_convert_to_uncompressed(batch->blorp->isl_dev,
>>> &params.dst,
>>> +                                            NULL, NULL, NULL, NULL);
>>> +                                            //&dst_x, &dst_y, &dst_w,
>>> &dst_h);
>>>
>> Did you mean to leave this as is?
>>
>> The previous patch (commit f395d0abc) caused a Coverity warning
>> because you began checking if x and y are non-NULL in one place after
>> dereferencing them under different conditions earlier. This code being
>> commented out makes me wonder what was really intended.
>>
>>
> That commented line should be removed but all parameters to NULL is the
> intended behavior.
>

That's correct.  More to Matt's point, yes, we dereference them under
different conditions.  The implicit assumption (which is currently always
valid) is that if you specify width and height then you also specified X
and Y.  Prior to this patch, every caller specified x and y but only some
of them specified width/height.  As of this patch, there is now a caller
which does not specify x and y (and also leaves width/height NULL).  I
don't think there's any point in (nor would it be possible to) supporting
width/height without x/y.  I'd be happy if someone added some asserts to
that effect.

--Jason

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev