[Mesa-dev] [v4 03/10] intel: replace single region with a vector of regions

[Chad Versace]

On 05/21/2013 11:17 PM, Pohjolainen, Topi wrote:
> On Tue, May 21, 2013 at 10:11:17PM -0700, Chad Versace wrote:
>> On 05/02/2013 12:08 AM, Topi Pohjolainen wrote:
>>> No functional change in preparation for supporting multiple planes
>>> per image each having its own region.
>>>
>>> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>>> ---
>>>   src/mesa/drivers/dri/intel/intel_fbo.c       |  6 +--
>>>   src/mesa/drivers/dri/intel/intel_regions.h   |  7 ++-
>>>   src/mesa/drivers/dri/intel/intel_screen.c    | 69 ++++++++++++++--------------
>>>   src/mesa/drivers/dri/intel/intel_tex_image.c |  2 +-
>>>   4 files changed, 45 insertions(+), 39 deletions(-)
>>
>> [snip]
>>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_regions.h b/src/mesa/drivers/dri/intel/intel_regions.h
>>> index 1fb6b27..e610f6b 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_regions.h
>>> +++ b/src/mesa/drivers/dri/intel/intel_regions.h
>>> @@ -129,8 +129,13 @@ struct intel_image_format {
>>>      } planes[3];
>>>   };
>>>
>>> +/**
>>> + * An image representing multiple planes may come in two flavours:
>>> + *  - all planes in single region but in different offsets or
>>> + *  - each plane in its own region.
>>> + */
>>
>>
>> In case (1), does image->regions contain a single image, or does
>> it contain 3 pointers to the same image?
>>
>> More importantly, I don't understand a key point here. By examining a given instance
>> of __DRIimageRec, how can I determine if the image falls under case (1)
>> or case (2)?
>>
>> Here is a concrete instance of the problem I don't know how to solve.
>> Suppose that I have an instance of __DRIimageRec and that I've determined
>> so far that its contents are as below, where image->planar_format was obtained
>> by a lookup in the intel_screen.c:intel_image_formats table.
>>
>>    image = {
>>      // ...
>>      .planar_format = {
>>         .fourcc = __DRI_IMAGE_FOURCC_YUV422,
>>         .components = __DRI_IMAGE_COMPONENTS_Y_U_V,
>>         .nplanes = 3,
>>         . planes = {
>>           [0] = {
>>             .buffer_index = 0,
>>              .dri_format = __DRI_IMAGE_FORMAT_R8,
>>              // ...
>>            },
>>           [1] = {
>>             .buffer_index = 1,
>>             .dri_format = __DRI_IMAGE_FORMAT_R8,
>>             // ...
>>           },
>>           [2] = {
>>             .buffer_index = 2,
>>             .dri_format = __DRI_IMAGE_FORMAT_R8,
>>             // ...
>>            },
>>      };
>>
>> With this information, how can I know if dereferencing image->regions[1]
>> will segfault or not? That is, how can I know if this image have one or
>> three images?
>
> All the entities that produce instances of 'struct __DRIimageRec' make certain
> that 'regions' contain as many valid pointers as there are planes. Hence the
> controlling counter is 'planar_format.nplanes'. I originally thought adding
> a counter along with 'regions', say 'nregions', but then decided against as it
> would be duplicating 'planar_format.nplanes' - they would always agree anyway.
> If 'planar_format' itself is missing (zero pointer) the image is implicitly
> always packed having only one region.
>
> In this patch I have simply extended the number of pointers, but no logic is
> yet trying to access elements of 'regions' beyond the first. The following
> patches introduce planar cases where other elements are accessed the
> 'planar_format' controlling how many.
>
> In addition, patch number six takes advantage of the fact that all the unused
> region pointers in the array are fixed to zero (for now all instances of
> 'struct __DRIimageRec' are allocated from the heap and all members are
> initialised to zero). It is also the patch that introduces the creation of
> images having more than one region guaranteeing that the planar format is also
> set accordingly.
>
>
> I think I understand where you are getting at - these rules are not obvious and
> should probably be enforced and/or made clearer. Would you have any preference,
> introducing the 'nregions' perhaps?

I'd prefer to not add 'nregions', since that is duplicate information. I think
a fuller explanation in the comments here would suffice.

>
>>
>>>   struct __DRIimageRec {
>>> -   struct intel_region *region;
>>> +   struct intel_region *regions[3];
>>>      GLenum internal_format;
>>>      uint32_t dri_format;
>>>      GLuint format;
>>
>> [snip]
>>
>
>