Re: [PATCH 10/37] drm/nouveau: Fix crtc->primary->fb vs. drm_fb fail

From: Daniel Vetter <daniel@ffwll.ch>
To: ville.syrjala@linux.intel.com
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	dri-devel@lists.freedesktop.org, Ben Skeggs <bskeggs@redhat.com>
Subject: Re: [PATCH 10/37] drm/nouveau: Fix crtc->primary->fb vs. drm_fb fail
Date: Wed, 30 Nov 2016 15:56:14 +0100	[thread overview]
Message-ID: <20161130145614.5ftu63m5w5kr26oa@phenom.ffwll.local> (raw)
In-Reply-To: <1479498793-31021-11-git-send-email-ville.syrjala@linux.intel.com>

On Fri, Nov 18, 2016 at 09:52:46PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> So it looks like the code is trying to pick between the passed in fb and
> crtc->primary->fb based on that funky 'bool atomic'. But later it will
> mix uses of both drm_fb (which was picked by the aforementioned logic)
> and crtc->primary->fb. So looks like a bug to me. Let's make it use
> drm_fb only.
> 
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Oh dear, set_base_atomic indeed expects that the passed-in fb is used, and
that drivers do _not_ update crtc->primary->fb. And drm core expects them
to do that, kinda, or at least allows it. I guess time to nuke all that
kgdb stuff as dead code and unused since obviously all broken.

But that's another story, this here seems to make some sense.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> index 59d1d1c5de5f..7c6c66f177df 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> @@ -854,9 +854,9 @@ nv04_crtc_do_mode_set_base(struct drm_crtc *crtc,
>  
>  	/* Update the framebuffer format. */
>  	regp->CRTC[NV_CIO_CRE_PIXEL_INDEX] &= ~3;
> -	regp->CRTC[NV_CIO_CRE_PIXEL_INDEX] |= (crtc->primary->fb->depth + 1) / 8;
> +	regp->CRTC[NV_CIO_CRE_PIXEL_INDEX] |= (drm_fb->depth + 1) / 8;
>  	regp->ramdac_gen_ctrl &= ~NV_PRAMDAC_GENERAL_CONTROL_ALT_MODE_SEL;
> -	if (crtc->primary->fb->depth == 16)
> +	if (drm_fb->depth == 16)
>  		regp->ramdac_gen_ctrl |= NV_PRAMDAC_GENERAL_CONTROL_ALT_MODE_SEL;
>  	crtc_wr_cio_state(crtc, regp, NV_CIO_CRE_PIXEL_INDEX);
>  	NVWriteRAMDAC(dev, nv_crtc->index, NV_PRAMDAC_GENERAL_CONTROL,
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel