Re: [PATCH 2/6] drm/msm: gpu: Use the zap shader on 5XX if we can

[Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Mon 17 Apr 14:51 PDT 2017, Jordan Crouse wrote:

> On Mon, Apr 17, 2017 at 12:58:07PM -0700, Bjorn Andersson wrote:
> > On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:
> > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > index 6c55d24..0e2b00a 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > @@ -470,6 +470,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu)
> > >  	return 0;
> > >  }
> > >  
> > > +static int a5xx_zap_shader_resume(struct msm_gpu *gpu)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = qcom_scm_set_remote_state(0, 13);
> > 
> > Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can
> > we have a define for this?
> 
> I think it is officially a coincidence but probably an informed one. Now that I
> think about it I seem to remember Andy asking me to add a define for the 13.

So far I haven't pushed the pas-id defines to the public scm header, but
we could do that and until we see a discrepancy just use that define
here.

> The state is more confusing - because video is the other consumer of this call
> and they call 0 to disable and 1 to enable, and GPU calls 0 to enable.  I can
> add a #define but it will be just for GPU - maybe something like
> REMOTE_STATE_GPU_RESUME?
> 

As the states aren't globally defined I think it makes sense to just
keep them opaque in the scm-world and have the two(?) states defined
locally here in the adreno driver.

> 
> > > +	if (ret)
> > > +		DRM_ERROR("%s: zap-shader resume failed: %d\n",
> > > +			gpu->name, ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int a5xx_zap_shader_init(struct msm_gpu *gpu)
> > > +{
> > > +	static bool loaded;
> > > +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > > +	struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu);
> > > +	struct platform_device *pdev = a5xx_gpu->pdev;
> > > +	struct device_node *node;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * If the zap shader is already loaded into memory we just need to kick
> > > +	 * the remote processor to reinitialize it
> > > +	 */
> > > +	if (loaded)
> > > +		return a5xx_zap_shader_resume(gpu);
> > > +
> > > +	/* Populate the sub-nodes if they haven't already been done */
> > > +	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> > 
> > As this is new code, there's little (no) value in doing this stepwise,
> > just squash this with the later commits replacing this.
> 
> Yeah, I'm thinking I'll do a squash on the next go around unless Rob objects.
> 

Sounds good.

Regards,
Bjorn
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno