[20/37] drm/vmwgfx: Populate fb->format correctly
diff mbox

Message ID 1479498793-31021-21-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä Nov. 18, 2016, 7:52 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Populate the fb->format to point at the correct format information
structure.

Cc: linux-graphics-maintainer@vmware.com
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Daniel Vetter Nov. 30, 2016, 3:40 p.m. UTC | #1
On Fri, Nov 18, 2016 at 09:52:56PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Populate the fb->format to point at the correct format information
> structure.
> 
> Cc: linux-graphics-maintainer@vmware.com
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5788913ca8f9..77f936e3056e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -589,6 +589,7 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
>  
>  	vfbs->base.base.dev = dev;
>  	/* XXX get the first 4 from the surface info */
> +	vfbs->base.base.format = drm_format_info(pixel_format);
>  	vfbs->base.base.pixel_format = pixel_format;
>  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
> @@ -897,6 +898,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
>  	}
>  
>  	vfbd->base.base.dev = dev;
> +	vfbd->base.base.format = drm_format_info(pixel_format);

Why not use drm_mode_fill_fb_struct here? This just seems to be bad
copypasta in both cases ...
-Daniel

>  	vfbd->base.base.pixel_format = pixel_format;
>  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Nov. 30, 2016, 3:52 p.m. UTC | #2
On Fri, Nov 18, 2016 at 09:52:56PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Populate the fb->format to point at the correct format information
> structure.
> 
> Cc: linux-graphics-maintainer@vmware.com
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5788913ca8f9..77f936e3056e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -589,6 +589,7 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
>  
>  	vfbs->base.base.dev = dev;
>  	/* XXX get the first 4 from the surface info */
> +	vfbs->base.base.format = drm_format_info(pixel_format);
>  	vfbs->base.base.pixel_format = pixel_format;
>  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbs->base.base.pitches[0] = mode_cmd->pitch;

If you change the earlier patch to just use fill_fb_struct this one here
isn't needed.
-Daniel

> @@ -897,6 +898,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
>  	}
>  
>  	vfbd->base.base.dev = dev;
> +	vfbd->base.base.format = drm_format_info(pixel_format);
>  	vfbd->base.base.pixel_format = pixel_format;
>  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä Nov. 30, 2016, 4:03 p.m. UTC | #3
On Wed, Nov 30, 2016 at 04:40:27PM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 09:52:56PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Populate the fb->format to point at the correct format information
> > structure.
> > 
> > Cc: linux-graphics-maintainer@vmware.com
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > index 5788913ca8f9..77f936e3056e 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -589,6 +589,7 @@ static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
> >  
> >  	vfbs->base.base.dev = dev;
> >  	/* XXX get the first 4 from the surface info */
> > +	vfbs->base.base.format = drm_format_info(pixel_format);
> >  	vfbs->base.base.pixel_format = pixel_format;
> >  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
> >  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
> > @@ -897,6 +898,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
> >  	}
> >  
> >  	vfbd->base.base.dev = dev;
> > +	vfbd->base.base.format = drm_format_info(pixel_format);
> 
> Why not use drm_mode_fill_fb_struct here? This just seems to be bad
> copypasta in both cases ...

That it is. I was hoping I'd trick the vmware folks into fixing it by
posting this series. One issue is that this code still used the old
mode_cmd struct rather than the new mode_cmd2 struct. So the fix may
not be entirely trivial, and hence I was hoping people who care
about the driver would do it.

> -Daniel
> 
> >  	vfbd->base.base.pixel_format = pixel_format;
> >  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
> >  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Laurent Pinchart Nov. 30, 2016, 4:09 p.m. UTC | #4
Hello,

(Thomas, there's a question for you below)

On Wednesday 30 Nov 2016 18:03:30 Ville Syrjälä wrote:
> On Wed, Nov 30, 2016 at 04:40:27PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 18, 2016 at 09:52:56PM +0200, ville.syrjala@linux.intel.com 
wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> 
> >> Populate the fb->format to point at the correct format information
> >> structure.
> >> 
> >> Cc: linux-graphics-maintainer@vmware.com
> >> Cc: Sinclair Yeh <syeh@vmware.com>
> >> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> >> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 5788913ca8f9..77f936e3056e
> >> 100644
> >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> >> @@ -589,6 +589,7 @@ static int vmw_kms_new_framebuffer_surface(struct
> >> vmw_private *dev_priv,
> >>  	vfbs->base.base.dev = dev;
> >>  	/* XXX get the first 4 from the surface info */
> >> 
> >> +	vfbs->base.base.format = drm_format_info(pixel_format);
> >>  	vfbs->base.base.pixel_format = pixel_format;
> >>  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
> >>  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
> >> @@ -897,6 +898,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> >> vmw_private *dev_priv,
> >>  	}
> >>  	
> >>  	vfbd->base.base.dev = dev;
> >> +	vfbd->base.base.format = drm_format_info(pixel_format);
> > 
> > Why not use drm_mode_fill_fb_struct here? This just seems to be bad
> > copypasta in both cases ...
> 
> That it is. I was hoping I'd trick the vmware folks into fixing it by
> posting this series. One issue is that this code still used the old
> mode_cmd struct rather than the new mode_cmd2 struct. So the fix may
> not be entirely trivial, and hence I was hoping people who care
> about the driver would do it.

I gave it a try and it required touching too much code for my limited (or 
rather non-existing) knowledge of the driver. Pretty please to the vmwgfx 
maintainers, could you fix this ? vmwgfx is the last driver still relying on 
the old mode_cmd structure.

> >>  	vfbd->base.base.pixel_format = pixel_format;
> >>  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
> >>  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
Daniel Vetter Nov. 30, 2016, 5:22 p.m. UTC | #5
On Wed, Nov 30, 2016 at 06:09:02PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> (Thomas, there's a question for you below)
> 
> On Wednesday 30 Nov 2016 18:03:30 Ville Syrjälä wrote:
> > On Wed, Nov 30, 2016 at 04:40:27PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 18, 2016 at 09:52:56PM +0200, ville.syrjala@linux.intel.com 
> wrote:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> 
> > >> Populate the fb->format to point at the correct format information
> > >> structure.
> > >> 
> > >> Cc: linux-graphics-maintainer@vmware.com
> > >> Cc: Sinclair Yeh <syeh@vmware.com>
> > >> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> ---
> > >> 
> > >>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >> 
> > >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > >> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 5788913ca8f9..77f936e3056e
> > >> 100644
> > >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > >> @@ -589,6 +589,7 @@ static int vmw_kms_new_framebuffer_surface(struct
> > >> vmw_private *dev_priv,
> > >>  	vfbs->base.base.dev = dev;
> > >>  	/* XXX get the first 4 from the surface info */
> > >> 
> > >> +	vfbs->base.base.format = drm_format_info(pixel_format);
> > >>  	vfbs->base.base.pixel_format = pixel_format;
> > >>  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
> > >>  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
> > >> @@ -897,6 +898,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> > >> vmw_private *dev_priv,
> > >>  	}
> > >>  	
> > >>  	vfbd->base.base.dev = dev;
> > >> +	vfbd->base.base.format = drm_format_info(pixel_format);
> > > 
> > > Why not use drm_mode_fill_fb_struct here? This just seems to be bad
> > > copypasta in both cases ...
> > 
> > That it is. I was hoping I'd trick the vmware folks into fixing it by
> > posting this series. One issue is that this code still used the old
> > mode_cmd struct rather than the new mode_cmd2 struct. So the fix may
> > not be entirely trivial, and hence I was hoping people who care
> > about the driver would do it.
> 
> I gave it a try and it required touching too much code for my limited (or 
> rather non-existing) knowledge of the driver. Pretty please to the vmwgfx 
> maintainers, could you fix this ? vmwgfx is the last driver still relying on 
> the old mode_cmd structure.

Well you all managed to trick me into typing it, and it didn't seem too
bad. Pretty much just mechanically switching everything from mode_cmd to
mode_cmd2. Maybe I've touched too many drm drivers for big refactorings
and I have too low standards for what's involved ;-)

Patch submitted, review very much appreciated. And I'll try to ping
Sinclair on irc once he shows up for testing.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 5788913ca8f9..77f936e3056e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -589,6 +589,7 @@  static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
 
 	vfbs->base.base.dev = dev;
 	/* XXX get the first 4 from the surface info */
+	vfbs->base.base.format = drm_format_info(pixel_format);
 	vfbs->base.base.pixel_format = pixel_format;
 	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
 	vfbs->base.base.pitches[0] = mode_cmd->pitch;
@@ -897,6 +898,7 @@  static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
 	}
 
 	vfbd->base.base.dev = dev;
+	vfbd->base.base.format = drm_format_info(pixel_format);
 	vfbd->base.base.pixel_format = pixel_format;
 	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
 	vfbd->base.base.pitches[0] = mode_cmd->pitch;