| Message ID | 20180405195035.24722-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > From: Ville Syrjälä <ville.syrjala@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@vmware.com> > Cc: Sinclair Yeh <syeh@vmware.com> > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@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://cgit.freedesktop.org/mesa/vmwgfx/commit/?id=c54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6 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
On Thu, Apr 05, 2018 at 08:15:05PM +0000, Deepak Singh Rawat wrote: > > > > > From: Ville Syrjälä <ville.syrjala@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@vmware.com> > > Cc: Sinclair Yeh <syeh@vmware.com> > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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://cgit.freedesktop.org/mesa/vmwgfx/commit/?id=c54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6 Hmm. What's the timelike for landing that stuff? A cursory glance tells me we should just change the current code with s/cpp/4/ and it should still be fine? 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. > > 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
> > On Thu, Apr 05, 2018 at 08:15:05PM +0000, Deepak Singh Rawat wrote: > > > > > > > > From: Ville Syrjälä <ville.syrjala@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@vmware.com> > > > Cc: Sinclair Yeh <syeh@vmware.com> > > > Cc: VMware Graphics <linux-graphics-maintainer@vmware.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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
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]; requested_bb_mem += crtc->mode.hdisplay * cpp * crtc->mode.vdisplay;