Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6304 bytes --]

On Tue, Oct 18, 2022 at 01:58:53PM +0200, Thomas Zimmermann wrote:
> Hi Thierry
> 
> Am 17.10.22 um 16:54 schrieb Thierry Reding:
> > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> [...]
> > > 
> > > That whole 'Memory Manangement' block is will be unmaintainable. Before I go
> > > into a detailed review, please see my questions about the reservedmem code
> > > at the end of the patch.
> > 
> > It looks reasonably maintainable to me. Given that we only have __iomem
> > and non-__iomem cases, this is about the extent of the complexity that
> > could ever be added.
> 
> I think we should split the detection and usage, as the driver does with
> other properties. It would fit better into the driver's overall design. I'll
> send out another email with a review to illustrate what I have in mind.

Okay, great.

> > > >    /*
> > > >     * Modesetting
> > > >     */
> > > > @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> > > >    	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> > > >    	drm_atomic_for_each_plane_damage(&iter, &damage) {
> > > > -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
> > > >    		struct drm_rect dst_clip = plane_state->dst;
> > > >    		if (!drm_rect_intersect(&dst_clip, &damage))
> > > >    			continue;
> > > > -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> > > > -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> > > > -			    &damage);
> > > > +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> > > > +								      &dst_clip));
> > > 
> > > You'll modify screen_base and it'll eventually blow up. You have to keep
> > > initializing the dst variable within the loop.
> > > 
> > > > +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> > > > +			    shadow_plane_state->data, fb, &damage);
> > > >    	}
> > > >    	drm_dev_exit(idx);
> > > > @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> > > >    		return;
> > > >    	/* Clear screen to black if disabled */
> > > > -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> > > > +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
> > > >    	drm_dev_exit(idx);
> > > >    }
> > > > @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	struct drm_device *dev;
> > > >    	int width, height, stride;
> > > >    	const struct drm_format_info *format;
> > > > -	struct resource *res, *mem;
> > > > -	void __iomem *screen_base;
> > > >    	struct drm_plane *primary_plane;
> > > >    	struct drm_crtc *crtc;
> > > >    	struct drm_encoder *encoder;
> > > > @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
> > > >    		&format->format, width, height, stride);
> > > > -	/*
> > > > -	 * Memory management
> > > > -	 */
> > > > -
> > > > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > -	if (!res)
> > > > -		return ERR_PTR(-EINVAL);
> > > > -
> > > > -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> > > > -	if (ret) {
> > > > -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> > > > +	ret = simpledrm_device_init_mm(sdev);
> > > > +	if (ret)
> > > >    		return ERR_PTR(ret);
> > > > -	}
> > > > -
> > > > -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> > > > -	if (!mem) {
> > > > -		/*
> > > > -		 * We cannot make this fatal. Sometimes this comes from magic
> > > > -		 * spaces our resource handlers simply don't know about. Use
> > > > -		 * the I/O-memory resource as-is and try to map that instead.
> > > > -		 */
> > > > -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> > > > -		mem = res;
> > > > -	}
> > > > -
> > > > -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> > > > -	if (!screen_base)
> > > > -		return ERR_PTR(-ENOMEM);
> > > > -	sdev->screen_base = screen_base;
> > > >    	/*
> > > >    	 * Modesetting
> > > > @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
> > > >    module_platform_driver(simpledrm_platform_driver);
> > > > +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> > > > +{
> > > > +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> > > > +
> > > > +	sdev->sysmem_start = rmem->base;
> > > > +	sdev->sysmem_size = rmem->size;
> > > 
> > >  From what I understand, you use the sysmem_ variables in the same code that
> > > allocates the simpledrm_device, which creates a chicken-egg problem. When
> > > does this code run?
> > 
> > This will run as a result of the of_reserved_mem_device_init_by_idx()
> > call earlier. It might be possible to push more code from the sysmem
> > initialization code path above into this function. That may also make
> > the somewhat clunky sysmem_start/size fields unnecessary.
> > 
> > Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
> > bits here and directly resolve the memory-region property and read its
> > "reg" property. However it seemed more appropriate to use the existing
> > infrastructure for this, even if it's rather minimal.
> 
> I agree. It would still be nicer if there was a version of
> of_reserved_mem_device_init_by_idx that returns the instance of reserved_mem
> instead of setting it in the device structure behind our back.

There's of_reserved_mem_lookup() which does that, or at least something
close to that. Ultimately, as Rob mentioned, we may not need any of this
infrastructure and can just directly parse the node from the driver.
This should allow us to avoid any of this infrastructure (i.e. the extra
indirection) and encapsulate the handling of this better.

I have a couple of ideas, but I'll wait for your feedback to work that
in as well.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]