On Wed, Jun 09, 2021 at 07:46:28AM -0400, Rodrigo Vivi wrote:
> On Wed, Jun 09, 2021 at 11:56:31AM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Sort out the mess with the local variables in
> > intel_fbdev_init_bios(). Get rid of all aliasing pointers,
> > use standard naming/types, and introduc a few more locals
>
> ^ typo
>
> > in the loops to avoid the hard to read long struct walks.
> >
> > While at we also polish the debugs a bit to use the
> > canonical [CRTC:%d:%s] style.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_fbdev.c | 94 +++++++++++++---------
> > 1 file changed, 56 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 4af40229f5ec..df05d285f0bd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -335,32 +335,43 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> > * fbcon), so we just find the biggest and use that.
> > */
> > static bool intel_fbdev_init_bios(struct drm_device *dev,
> > - struct intel_fbdev *ifbdev)
> > + struct intel_fbdev *ifbdev)
> > {
> > struct drm_i915_private *i915 = to_i915(dev);
> > struct intel_framebuffer *fb = NULL;
> > - struct drm_crtc *crtc;
> > - struct intel_crtc *intel_crtc;
> > + struct intel_crtc *crtc;
> > unsigned int max_size = 0;
> >
> > /* Find the largest fb */
> > - for_each_crtc(dev, crtc) {
> > + for_each_intel_crtc(dev, crtc) {
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > + struct intel_plane *plane =
> > + to_intel_plane(crtc->base.primary);
> > + struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> > struct drm_i915_gem_object *obj =
> > - intel_fb_obj(crtc->primary->state->fb);
> > - intel_crtc = to_intel_crtc(crtc);
> > + intel_fb_obj(plane_state->uapi.fb);
>
> oh, here we have again that plane_state uapi change that I don't understand
> and I'm not seeing any mention in any commit msg..
sorry...
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> >
> > - if (!crtc->state->active || !obj) {
> > + if (!crtc_state->uapi.active) {
> > drm_dbg_kms(&i915->drm,
> > - "pipe %c not active or no fb, skipping\n",
> > - pipe_name(intel_crtc->pipe));
> > + "[CRTC:%d:%s] not active, skipping\n",
> > + crtc->base.base.id, crtc->base.name);
> > + continue;
> > + }
> > +
> > + if (!obj) {
> > + drm_dbg_kms(&i915->drm,
> > + "[PLANE:%d:%s] no fb, skipping\n",
> > + plane->base.base.id, plane->base.name);
> > continue;
> > }
> >
> > if (obj->base.size > max_size) {
> > drm_dbg_kms(&i915->drm,
> > - "found possible fb from plane %c\n",
> > - pipe_name(intel_crtc->pipe));
> > - fb = to_intel_framebuffer(crtc->primary->state->fb);
> > + "found possible fb from [PLANE:%d:%s]\n",
> > + plane->base.base.id, plane->base.name);
> > + fb = to_intel_framebuffer(plane_state->uapi.fb);
> > max_size = obj->base.size;
> > }
> > }
> > @@ -372,60 +383,62 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> > }
> >
> > /* Now make sure all the pipes will fit into it */
> > - for_each_crtc(dev, crtc) {
> > + for_each_intel_crtc(dev, crtc) {
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > + struct intel_plane *plane =
> > + to_intel_plane(crtc->base.primary);
> > unsigned int cur_size;
> >
> > - intel_crtc = to_intel_crtc(crtc);
> > -
> > - if (!crtc->state->active) {
> > + if (!crtc_state->uapi.active) {
> > drm_dbg_kms(&i915->drm,
> > - "pipe %c not active, skipping\n",
> > - pipe_name(intel_crtc->pipe));
> > + "[CRTC:%d:%s] not active, skipping\n",
> > + crtc->base.base.id, crtc->base.name);
> > continue;
> > }
> >
> > - drm_dbg_kms(&i915->drm, "checking plane %c for BIOS fb\n",
> > - pipe_name(intel_crtc->pipe));
> > + drm_dbg_kms(&i915->drm, "checking [PLANE:%d:%s] for BIOS fb\n",
> > + plane->base.base.id, plane->base.name);
> >
> > /*
> > * See if the plane fb we found above will fit on this
> > * pipe. Note we need to use the selected fb's pitch and bpp
> > * rather than the current pipe's, since they differ.
> > */
> > - cur_size = crtc->state->adjusted_mode.crtc_hdisplay;
> > + cur_size = crtc_state->uapi.adjusted_mode.crtc_hdisplay;
> > cur_size = cur_size * fb->base.format->cpp[0];
> > if (fb->base.pitches[0] < cur_size) {
> > drm_dbg_kms(&i915->drm,
> > - "fb not wide enough for plane %c (%d vs %d)\n",
> > - pipe_name(intel_crtc->pipe),
> > + "fb not wide enough for [PLANE:%d:%s] (%d vs %d)\n",
> > + plane->base.base.id, plane->base.name,
> > cur_size, fb->base.pitches[0]);
> > fb = NULL;
> > break;
> > }
> >
> > - cur_size = crtc->state->adjusted_mode.crtc_vdisplay;
> > + cur_size = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
> > cur_size = intel_fb_align_height(&fb->base, 0, cur_size);
> > cur_size *= fb->base.pitches[0];
> > drm_dbg_kms(&i915->drm,
> > - "pipe %c area: %dx%d, bpp: %d, size: %d\n",
> > - pipe_name(intel_crtc->pipe),
> > - crtc->state->adjusted_mode.crtc_hdisplay,
> > - crtc->state->adjusted_mode.crtc_vdisplay,
> > + "[CRTC:%d:%s] area: %dx%d, bpp: %d, size: %d\n",
> > + crtc->base.base.id, crtc->base.name,
> > + crtc_state->uapi.adjusted_mode.crtc_hdisplay,
> > + crtc_state->uapi.adjusted_mode.crtc_vdisplay,
> > fb->base.format->cpp[0] * 8,
> > cur_size);
> >
> > if (cur_size > max_size) {
> > drm_dbg_kms(&i915->drm,
> > - "fb not big enough for plane %c (%d vs %d)\n",
> > - pipe_name(intel_crtc->pipe),
> > + "fb not big enough for [PLANE:%d:%s] (%d vs %d)\n",
> > + plane->base.base.id, plane->base.name,
> > cur_size, max_size);
> > fb = NULL;
> > break;
> > }
> >
> > drm_dbg_kms(&i915->drm,
> > - "fb big enough for plane %c (%d >= %d)\n",
> > - pipe_name(intel_crtc->pipe),
> > + "fb big enough [PLANE:%d:%s] (%d >= %d)\n",
> > + plane->base.base.id, plane->base.name,
> > max_size, cur_size);
> > }
> >
> > @@ -441,15 +454,20 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> > drm_framebuffer_get(&ifbdev->fb->base);
> >
> > /* Final pass to check if any active pipes don't have fbs */
> > - for_each_crtc(dev, crtc) {
> > - intel_crtc = to_intel_crtc(crtc);
> > + for_each_intel_crtc(dev, crtc) {
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > + struct intel_plane *plane =
> > + to_intel_plane(crtc->base.primary);
> > + struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> >
> > - if (!crtc->state->active)
> > + if (!crtc_state->uapi.active)
> > continue;
> >
> > - drm_WARN(dev, !crtc->primary->state->fb,
> > - "re-used BIOS config but lost an fb on crtc %d\n",
> > - crtc->base.id);
> > + drm_WARN(dev, !plane_state->uapi.fb,
> > + "re-used BIOS config but lost an fb on [PLANE:%d:%s]\n",
> > + plane->base.base.id, plane->base.name);
> > }
> >
> >
> > --
> > 2.31.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
