Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 15 Apr 2019 10:20:45 +0100
From:   Liviu Dudau <Liviu.Dudau@arm.com>
To:     "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>
Cc:     Ben Davis <Ben.Davis@arm.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "airlied@linux.ie" <airlied@linux.ie>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "maxime.ripard@bootlin.com" <maxime.ripard@bootlin.com>,
        nd <nd@arm.com>, "sean@poorly.run" <sean@poorly.run>
Subject: Re: [v2,1/2] drm: Add writeback_w,h properties
Message-ID: <20190415092045.GO15144@e110455-lin.cambridge.arm.com>
References: <1555078094-6587-2-git-send-email-ben.davis@arm.com>
 <20190415075922.GA24596@james-ThinkStation-P300>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190415075922.GA24596@james-ThinkStation-P300>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 15, 2019 at 08:59:30AM +0100, james qian wang (Arm Technology China) wrote:
> On Fri, Apr 12, 2019 at 02:08:28PM +0000, Ben Davis wrote:
> > Add new properties to specify width and height for writeback.
> > 
> > Signed-off-by: Ben Davis <ben.davis@arm.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++++++
> >  drivers/gpu/drm/drm_writeback.c   | 28 ++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       |  4 ++++
> >  include/drm/drm_mode_config.h     | 10 ++++++++++
> >  4 files changed, 50 insertions(+)
> > 
> > -- 
> > 2.7.4
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d520a04..1f68dce 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -765,6 +765,10 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  			return -EINVAL;
> >  		}
> >  		state->content_protection = val;
> > +	} else if (property == config->prop_writeback_w) {
> > +		state->writeback_w = val;
> > +	} else if (property == config->prop_writeback_h) {
> > +		state->writeback_h = val;
> >  	} else if (property == config->writeback_fb_id_property) {
> >  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
> >  		int ret = drm_atomic_set_writeback_fb_for_connector(state, fb);
> > @@ -837,6 +841,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->scaling_mode;
> >  	} else if (property == connector->content_protection_property) {
> >  		*val = state->content_protection;
> > +	} else if (property == config->prop_writeback_w) {
> > +		*val = state->writeback_w;
> > +	} else if (property == config->prop_writeback_h) {
> > +		*val = state->writeback_h;
> >  	} else if (property == config->writeback_fb_id_property) {
> >  		/* Writeback framebuffer is one-shot, write and forget */
> >  		*val = 0;
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index c20e6fe..3d0453e 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -74,6 +74,12 @@
> >   *	applications making use of writeback connectors *always* retrieve an
> >   *	out-fence for the commit and use it appropriately.
> >   *	From userspace, this property will always read as zero.
> > + *
> > + *  "WRITEBACK_W":
> > + *	The width of the writeback buffer to write back. 0 acts as default.
> > + *
> > + *  "WRITEBACK_H":
> > + *	The height of the writeback buffer to write back. 0 acts as default.
> >   */
> >  
> >  #define fence_to_wb_connector(x) container_of(x->lock, \
> > @@ -141,6 +147,22 @@ static int create_writeback_properties(struct drm_device *dev)
> >  		dev->mode_config.writeback_out_fence_ptr_property = prop;
> >  	}
> >  
> > +	if (!dev->mode_config.prop_writeback_w) {
> > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +						 "WRITEBACK_W", 1, UINT_MAX);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		dev->mode_config.prop_writeback_w = prop;
> > +	}
> > +
> > +	if (!dev->mode_config.prop_writeback_h) {
> > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +						 "WRITEBACK_H", 1, UINT_MAX);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +		dev->mode_config.prop_writeback_h = prop;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -225,6 +247,12 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >  	drm_object_attach_property(&connector->base,
> >  				   config->writeback_pixel_formats_property,
> >  				   blob->base.id);
> > +
> > +	drm_object_attach_property(&connector->base,
> > +				   config->prop_writeback_w, 0);
> > +	drm_object_attach_property(&connector->base,
> > +				   config->prop_writeback_h, 0);
> 
> Hi Ben:
> Do we real need these two individual properties for specifing the writeback
> w/h, can we use fb->w/h ?
> And since these two properties are added as common and standard properties,
> it influnce all existing write-back implementation which all assumed
> writeback size as fb->w/h.

The idea of having these additional properties was to maintain backwards
compatibility (of some sort). If you don't set writeback_w/h then the
assumption is that they are the same as fb->w/h, but I can see how it's
going to be confusing to have fb->w/h different from crtc->w/h and
different from writeback_w/h.


> To compatible with existing writeback support, I suggest to keep to
> use fb->w/h or add these properties as malidp private.

We don't need to make them malidp private, there is nothing malidp
specific in them. I'll talk with Ben, we should probably drop this patch
entirely and just enable malidp scaling when fb->w/h differ from
crtc->w/h.

Best regards,
Liviu


> 
> Thanks
> James
> 
> 
> >  	wb_connector->pixel_formats_blob_ptr = blob;
> >  
> >  	return 0;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 8fe22ab..51c4cb2 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -515,6 +515,10 @@ struct drm_connector_state {
> >  	 */
> >  	struct drm_writeback_job *writeback_job;
> >  
> > +	/** @writeback_w: width of plane to write to wb buffer */
> > +	/** @writeback_h: height of plane to write to wb buffer */
> > +	uint32_t writeback_w, writeback_h;
> > +
> >  	/**
> >  	 * @max_requested_bpc: Connector property to limit the maximum bit
> >  	 * depth of the pixels.
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index 7f60e8e..a0c2133 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -622,6 +622,16 @@ struct drm_mode_config {
> >  	 */
> >  	struct drm_property *prop_crtc_h;
> >  	/**
> > +	 * @prop_writeback_w: Writeback connector property for the plane
> > +	 * destination position in the writeback buffer.
> > +	 */
> > +	struct drm_property *prop_writeback_w;
> > +	/**
> > +	 * @prop_writeback_h: Writeback connector property for the plane
> > +	 * destination position in the writeback buffer.
> > +	 */
> > +	struct drm_property *prop_writeback_h;
> > +	/**
> >  	 * @prop_fb_id: Default atomic plane property to specify the
> >  	 * &drm_framebuffer.
> >  	 */
> 
> Pls del this window style line ending 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯