[PATCH v2 02/10] drm/bochs: Use helpers for struct drm_edid

[Dmitry Baryshkov]

On Wed, Sep 25, 2024 at 10:15:49AM GMT, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.09.24 um 06:35 schrieb Dmitry Baryshkov:
> > On Mon, Sep 02, 2024 at 12:53:40PM GMT, Thomas Zimmermann wrote:
> > > Implement a read function for struct drm_edid and read the EDID data
> > > with drm_edit_read_custom(). Update the connector data accordingly.
> > > 
> > > The EDID data comes from the emulator itself and the connector stores
> > > a copy in its EDID property. The drm_edid field in struct bochs_device
> > > is therefore not required. Remove it.
> > > 
> > > If qemu provides no EDID data, install default display modes as before.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> > > Acked-by: Gerd Hoffmann <kraxel at redhat.com>
> > > ---
> > >   drivers/gpu/drm/tiny/bochs.c | 48 +++++++++++++++++-------------------
> > >   1 file changed, 22 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> > > index 47a45a14306c..197fc00b373f 100644
> > > --- a/drivers/gpu/drm/tiny/bochs.c
> > > +++ b/drivers/gpu/drm/tiny/bochs.c
> > > @@ -85,7 +85,6 @@ struct bochs_device {
> > >   	u16 yres_virtual;
> > >   	u32 stride;
> > >   	u32 bpp;
> > > -	const struct drm_edid *drm_edid;
> > >   	/* drm */
> > >   	struct drm_device *dev;
> > > @@ -172,12 +171,14 @@ static void bochs_hw_set_little_endian(struct bochs_device *bochs)
> > >   #define bochs_hw_set_native_endian(_b) bochs_hw_set_little_endian(_b)
> > >   #endif
> > > -static int bochs_get_edid_block(void *data, u8 *buf,
> > > -				unsigned int block, size_t len)
> > > +static int bochs_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> > >   {
> > >   	struct bochs_device *bochs = data;
> > >   	size_t i, start = block * EDID_LENGTH;
> > > +	if (!bochs->mmio)
> > > +		return -1;
> > > +
> > >   	if (start + len > 0x400 /* vga register offset */)
> > >   		return -1;
> > > @@ -187,25 +188,20 @@ static int bochs_get_edid_block(void *data, u8 *buf,
> > >   	return 0;
> > >   }
> > > -static int bochs_hw_load_edid(struct bochs_device *bochs)
> > > +static const struct drm_edid *bochs_hw_read_edid(struct drm_connector *connector)
> > >   {
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct bochs_device *bochs = dev->dev_private;
> > >   	u8 header[8];
> > > -	if (!bochs->mmio)
> > > -		return -1;
> > > -
> > >   	/* check header to detect whenever edid support is enabled in qemu */
> > >   	bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header));
> > >   	if (drm_edid_header_is_valid(header) != 8)
> > I understand that you probably don't want to change the behaviour of the
> > driver, but maybe it's better to check drm_edid_read_custom return
> > value? Bochs, amdgpu and radeon are the only drivers that call
> > drm_edid_header_is_valid().
> 
> The nearby comment indicates that EDID support might not be present in qemu.
> But drm_edid_read_custom() warns in this case, even though it's a legal
> state. Hence the test.

Ack, thanks for the explanation.

> 
> Best regards
> Thomas
> 
> > 
> > > -		return -1;
> > > +		return NULL;
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
With best wishes
Dmitry