[Mesa-dev] [PATCH 2/2] gbm: Add flags to enable creation of rotated scanout buffers (v3)

[Vivek Kasireddy]

Hi Michel,
Do you have any further comments/suggestions on this patch?


Thanks and Regards,
Vivek

On Fri, 6 Nov 2015 18:56:13 +0900
Michel Dänzer <michel at daenzer.net> wrote:

> On 06.11.2015 12:08, Vivek Kasireddy wrote:
> > For certain platforms that support rotated scanout buffers,
> > currently, there is no way to create them with the GBM DRI
> > interface. These flags will instruct the DRI driver to create the
> > buffer by setting additional requirements such as tiling mode.
> > 
> > v2: Reserve a bit per angle. (Ville and Michel)
> > 
> > v3:
> > - Combine all GBM_BO_USE_SCANOUT_ROTATION_* flags into
> >   GBM_BO_USE_SCANOUT_ANY macro (Michel)
> > - Pull the code that updates dri_use based on the rotation flag
> >   into a separate function.
> 
> [...]
> 
> > diff --git a/src/gbm/backends/dri/gbm_dri.c
> > b/src/gbm/backends/dri/gbm_dri.c index 57cdeac..741e509 100644
> > --- a/src/gbm/backends/dri/gbm_dri.c
> > +++ b/src/gbm/backends/dri/gbm_dri.c
> > @@ -124,6 +124,20 @@ image_get_buffers(__DRIdrawable *driDrawable,
> >  }
> >  
> >  static void
> > +gbm_to_dri_flag(uint32_t usage,
> > +                unsigned *dri_use)
> > +{
> > +   if (usage & GBM_BO_USE_SCANOUT)
> > +      *dri_use |= __DRI_IMAGE_USE_SCANOUT;
> > +   if (usage & GBM_BO_USE_SCANOUT_ROTATION_90)
> > +      *dri_use |= __DRI_IMAGE_USE_SCANOUT_ROTATION_90;
> > +   if (usage & GBM_BO_USE_SCANOUT_ROTATION_180)
> > +      *dri_use |= __DRI_IMAGE_USE_SCANOUT_ROTATION_180;
> > +   if (usage & GBM_BO_USE_SCANOUT_ROTATION_270)
> > +      *dri_use |= __DRI_IMAGE_USE_SCANOUT_ROTATION_270;
> > +}
> 
> I like the idea of this helper function, but it could handle
> GBM_BO_USE_CURSOR and GBM_BO_USE_LINEAR as well. Ideally, there would
> be a separate preparatory change which creates the helper function and
> makes gbm_dri_bo_import and gbm_dri_bo_create use it; then this change
> can just add the new flags in the helper function. If that's too much
> trouble, the handling of the other flags can be moved into the helper
> function in a followup change.
> 
> 
> > diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
> > index 8db2153..4bda089 100644
> > --- a/src/gbm/main/gbm.h
> > +++ b/src/gbm/main/gbm.h
> > @@ -214,6 +214,13 @@ enum gbm_bo_flags {
> >      * Buffer is linear, i.e. not tiled.
> >      */
> >     GBM_BO_USE_LINEAR = (1 << 4),
> > +   /**
> > +    * Buffer would be rotated and some platforms have additional
> > tiling
> > +    * requirements for rotated scanout buffers.
> > +    */
> > +   GBM_BO_USE_SCANOUT_ROTATION_90 = (1 << 5),
> > +   GBM_BO_USE_SCANOUT_ROTATION_180 = (1 << 6),
> > +   GBM_BO_USE_SCANOUT_ROTATION_270 = (1 << 7),
> >  };
> 
> Hmm, we should probably explicitly specify the orientation of the 90
> and 270 degree rotations. Clockwise? (Same in patch 1)
> 
> 
> > @@ -240,6 +247,10 @@ gbm_bo_create(struct gbm_device *gbm,
> >  #define GBM_BO_IMPORT_WL_BUFFER         0x5501
> >  #define GBM_BO_IMPORT_EGL_IMAGE         0x5502
> >  #define GBM_BO_IMPORT_FD                0x5503
> > +#define GBM_BO_USE_SCANOUT_ANY (GBM_BO_USE_SCANOUT
> > |		  \
> > +				GBM_BO_USE_SCANOUT_ROTATION_90 |  \
> > +				GBM_BO_USE_SCANOUT_ROTATION_180 | \
> > +				GBM_BO_USE_SCANOUT_ROTATION_170)
> >  
> >  struct gbm_import_fd_data {
> >     int fd;
> > 
> 
> Please add the define after the definition of enum gbm_bo_flags
> instead of here.
> 
> 
> Other than that, these are starting to look pretty good to me.
> 
>