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

[Pohjolainen, Topi]

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?

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