[Intel-gfx] [PATCH 2/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()

[Deepak Singh Rawat]

> 
> On Thu, Apr 05, 2018 at 08:15:05PM +0000, Deepak Singh Rawat wrote:
> >
> > >
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Instead of looking at plane->fb let's look at the proper new
> > > plane state.
> > >
> > > Not that the code makes a ton of sense. It's only going through the
> > > crtcs in the atomic state, so assuming not all of them are included
> > > we're not even calculating the total bandwidth here. Also we're
> > > not considering whether each crtc is actually enabled or not.
> > >
> > > Cc: Thomas Hellstrom <thellstrom at vmware.com>
> > > Cc: Sinclair Yeh <syeh at vmware.com>
> > > Cc: VMware Graphics <linux-graphics-maintainer at vmware.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > index 6728c6247b4b..a2a796b4cc23 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > @@ -1536,9 +1536,13 @@ vmw_kms_atomic_check_modeset(struct
> > > drm_device *dev,
> > >  		unsigned long requested_bb_mem = 0;
> > >
> > >  		if (dev_priv->active_display_unit ==
> > > vmw_du_screen_target) {
> > > -			if (crtc->primary->fb) {
> > > -				int cpp = crtc->primary->fb->pitches[0] /
> > > -					  crtc->primary->fb->width;
> > > +			struct drm_plane *plane = crtc->primary;
> > > +			struct drm_plane_state *plane_state;
> > > +
> > > +			plane_state =
> > > drm_atomic_get_new_plane_state(state, plane);
> > > +
> > > +			if (plane_state && plane_state->fb) {
> > > +				int cpp = plane_state->fb->format->cpp[0];
> >
> > Hi Ville,
> >
> > Thanks for the patch, recently I have done some refactoring of this code
> area
> > which is no yet sent to dri-devel. But the refactored code eliminated the
> need
> > to look the fb
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__cgit.freedesktop.org_mesa_vmwgfx_commit_-3Fid-
> 3Dc54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6&d=DwIDAw&c=uilaK90D4T
> OVoH58JNXRgQ&r=zOOG28inJK0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=lrHQqnjNpBdFNzU0f4b3rLTtaYp0VRzCgZztJ
> ew0kz0&s=mz1Kt2NO_HIpgVtQ9xHvKRQLGXx2HSqY8xt0oiEpGWg&e=
> 
> Hmm. What's the timelike for landing that stuff?

I am not sure, I still think there is more work here to clean this up. I guess for now
we can have your patch to take care of avoiding plane->fb.

> 
> A cursory glance tells me we should just change the current code with
> s/cpp/4/ and it should still be fine?

Yes replacing cpp with 4 is fine as that is what virtual hardware always look for.

> 
> BTW the drm_for_each_crtc(crtc, dev) loop in there doesn't look entirely
> kosher. It's potentially going to access crtc->state w/o holding the lock.

Thanks for the suggestion, I will look into this.

> 
> >
> > There is still some future work to be done in this area.
> >
> > >
> > >  				requested_bb_mem += crtc->mode.hdisplay
> > > * cpp *
> > >  						    crtc->mode.vdisplay;
> > > --
> > > 2.16.1
> 
> --
> Ville Syrjälä
> Intel OTC